Implement CertificateManager for Kingdom-less panel match protocol.

[robinsons]

No description provided.

[wfa-reviewable]

This change is 

[stevenwarejones]

Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @robinsons)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 64 at r1 (raw file):

        }
      val x509 = readCertificate(certificate.x509Der)
      verifyCertificate(x509, certificate.ownerId)

the ownerId shouldn't be derived from the certificate. The ownerId should already be known from the ExchangeDateKey ie a map should already exist, right? And, instead of an owner Id, I think using the AKID is preferable.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 124 at r1 (raw file):

    ownerId: String,
  ): X509Certificate {
    val rootCert = getRootCertificate(ownerId)

see my other comment... you should just look up the root by the AKID on the leaf certificate imo.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 162 at r1 (raw file):

    private const val BLOB_KEY_PREFIX = "certificates"
    private const val DATA_PROVIDER_CERT_PREFIX = "edp-certificate"
    private const val MODEL_PROVIDER_CERT_PREFIX = "mp-certificate"

in the init cannot you verify that the model provider and data provider prefixes differ?

---

src/main/proto/wfa/panelmatch/client/internal/certificate.proto line 27 at r1 (raw file):

  // ID of the entity that created this certificate. This is the ID of either a
  // data provider or model provider participating in an exchange.
  string owner_id = 1;

see my other comment, I don't think we need this field.

---

src/main/proto/wfa/panelmatch/common/certificates/signing_keys.proto line 25 at r1 (raw file):

  // TODO(@robinsons): Rename field to cert_name since not all panel match
  // protocols use resources.
  string cert_resource_name = 1;

why not go ahead and rename it?

[robinsons]

Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 64 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

the ownerId shouldn't be derived from the certificate. The ownerId should already be known from the ExchangeDateKey ie a map should already exist, right? And, instead of an owner Id, I think using the AKID is preferable.

One of my goals was to keep this close to the existing V2AlphaCertificateManager for the sake of simplicity. With that in mind, I've updated it to remove ownerId from the certificate and instead derive it from certName.

---

As for switching to AKID, the main reason I hesitate is that the daemon is currently set up such that it doesn't use AKID anywhere. For example, the rootCerts map is just a StorageClientSecretMap whose blob keys are the names of the entity to whom the cert belongs, i.e. the ownerId. So the system already seems designed to work using owner IDs.

IIUC, switching to AKID would entail passing an additional map from AKID to owner ID, so that getCertificate() could look up the owner ID for the cert, and then the remaining flow would remain as is. It seems the main win from this would be that we don't have to rely on a particular certificate naming scheme in order to derive ownerId - is that right?

I guess my main question is how strongly you feel about this. If it's something you definitely want, would you be OK if I filed an issue for this and we tackled it for both certificate manager impls at once sometime after the Kingdomless work is done?

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 124 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

see my other comment... you should just look up the root by the AKID on the leaf certificate imo.

Resolving so that we can continue the AKID conversation in a single thread in the above comment.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 162 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

in the init cannot you verify that the model provider and data provider prefixes differ?

I've changed this to build the blob key using the party's ID and removed these fields.

---

src/main/proto/wfa/panelmatch/client/internal/certificate.proto line 27 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

see my other comment, I don't think we need this field.

Done.

---

src/main/proto/wfa/panelmatch/common/certificates/signing_keys.proto line 25 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

why not go ahead and rename it?

Done.

[stevenwarejones]

Reviewed 2 of 6 files at r2, all commit messages.
Reviewable status: 6 of 10 files reviewed, 3 unresolved discussions (waiting on @robinsons)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 64 at r1 (raw file):

Previously, robinsons wrote…

One of my goals was to keep this close to the existing V2AlphaCertificateManager for the sake of simplicity. With that in mind, I've updated it to remove ownerId from the certificate and instead derive it from certName.

---

As for switching to AKID, the main reason I hesitate is that the daemon is currently set up such that it doesn't use AKID anywhere. For example, the rootCerts map is just a StorageClientSecretMap whose blob keys are the names of the entity to whom the cert belongs, i.e. the ownerId. So the system already seems designed to work using owner IDs.

IIUC, switching to AKID would entail passing an additional map from AKID to owner ID, so that getCertificate() could look up the owner ID for the cert, and then the remaining flow would remain as is. It seems the main win from this would be that we don't have to rely on a particular certificate naming scheme in order to derive ownerId - is that right?

I guess my main question is how strongly you feel about this. If it's something you definitely want, would you be OK if I filed an issue for this and we tackled it for both certificate manager impls at once sometime after the Kingdomless work is done?

my main concern was removing the field from the certificate proto. The rest is lower priority.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 58 at r2 (raw file):

  ): X509Certificate {
    val certKey =
      requireNotNull(CertificateKey.fromCertName(certName)) { "Invalid certName: $certName" }

can you also add something verifying that the ownerId maps to this exchange. we should have that mapping, right?

---

src/main/proto/wfa/panelmatch/client/internal/certificate.proto line 27 at r1 (raw file):

Previously, robinsons wrote…

Done.

my main concern was removing this field.

[stevenwarejones]

Reviewable status: 6 of 10 files reviewed, 3 unresolved discussions (waiting on @robinsons)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 165 at r2 (raw file):

        val match = CERT_NAME_PATTERN.find(certName) ?: return null
        val ownerId = match.groupValues[OWNER_ID_INDEX]
        val createTime = Instant.ofEpochSecond(match.groupValues[CREATE_TIME_INDEX].toLong())

you should probably incorporate a UUID in case this is done in parallel with multiple clients.

[stevenwarejones]

Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @robinsons)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 162 at r2 (raw file):

      private val CERT_NAME_PATTERN = Regex("(.+)$SEPARATOR([0-9]+)")

      fun fromCertName(certName: String): CertificateKey? {

can you add a test for this function?

[stevenwarejones]

Reviewed 2 of 6 files at r2, 1 of 2 files at r3.
Reviewable status: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @robinsons)

[robinsons]

Reviewable status: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 64 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

my main concern was removing the field from the certificate proto. The rest is lower priority.

Got it, thank you for clarifying.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 58 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

can you also add something verifying that the ownerId maps to this exchange. we should have that mapping, right?

We can get it from the exchange workflow, yes. Added this.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 162 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

can you add a test for this function?

Done.

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/KingomlessCertificateManager.kt line 165 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you should probably incorporate a UUID in case this is done in parallel with multiple clients.

Replaced the timestamp with a UUID. The timestamp was supposed to accomplish this, but UUID is a better bet for uniqueness.

[stevenwarejones]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @robinsons)