add fixed private key for exchange

[stevenwarejones]

No description provided.

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jonmolle and @stevenwarejones)

---

src/main/kotlin/org/wfanet/panelmatch/client/deploy/ExchangeWorkflowFlags.kt line 134 at r2 (raw file):

    required = true,
  )
  lateinit var privateKeyPath: String

Is this a file system path or a blob key? If the former, use File instead of String. If the latter, call this a blob key rather than a path to avoid confusion.

[stevenwarejones]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jonmolle and @SanjayVas)

---

src/main/kotlin/org/wfanet/panelmatch/client/deploy/ExchangeWorkflowFlags.kt line 134 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Is this a file system path or a blob key? If the former, use File instead of String. If the latter, call this a blob key rather than a path to avoid confusion.

its a blob key path. Updating.

[SanjayVas]

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

[stevenwarejones]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jonmolle)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/V2AlphaCertificateManager.kt line 84 at r3 (raw file):

      if (privateKeyBlobKey !== null && privateKeyBlobKey.isNotEmpty()) privateKeyBlobKey
      else exchange.path
    val signingKeys = requireNotNull(getSigningKeys(keyPath)) { "Missing keys for $keyPath" }

I was thinking about changing the behavior so that even if privateKeyBlogKey is present, it will still check for a key at the exchange.path and only if that is not present, then it will use privateKeyBlobKey. Thoughts? @SanjayVas @jonmolle ?

[SanjayVas]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jonmolle)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/V2AlphaCertificateManager.kt line 84 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I was thinking about changing the behavior so that even if privateKeyBlogKey is present, it will still check for a key at the exchange.path and only if that is not present, then it will use privateKeyBlobKey. Thoughts? @SanjayVas @jonmolle ?

No strong opinion other than to make sure the behavior is clearly documented if you do that. Maybe call it "fallbackPrivateKeyBlobKey" to be extra clear?

[stevenwarejones]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jonmolle and @SanjayVas)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/V2AlphaCertificateManager.kt line 84 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

No strong opinion other than to make sure the behavior is clearly documented if you do that. Maybe call it "fallbackPrivateKeyBlobKey" to be extra clear?

sgtm

[SanjayVas]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jonmolle and @stevenwarejones)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/V2AlphaCertificateManager.kt line 83 at r4 (raw file):

    val keyFromPrimaryPath = getSigningKeys(exchange.path)
    val signingKeys = if (keyFromPrimaryPath == null) {
      requireNotNull(getSigningKeys(fallbackPrivateKeyBlobKey!!))

nit: checkNotNull rather than non-null assertion. The latter is for cases where we know it's not null, but the compiler can't figure it out.

Suggestion:

checkNotNull(fallbackPrivateKeyBlobKey)

[SanjayVas]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jonmolle)

---

src/main/kotlin/org/wfanet/panelmatch/common/certificates/V2AlphaCertificateManager.kt line 83 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: checkNotNull rather than non-null assertion. The latter is for cases where we know it's not null, but the compiler can't figure it out.

I think you misunderstood. The concern is around the non-null assertion (!! operator), not requireNotNull vs. checkNotNull (though that itself can be an issue depending on whether getSigningKeys here returns null based specifically on exchange).

No code guarantees that fallbackPrivateKeyBlobKey is not null here, therefore a non-null assertion is incorrect. It's instead illegal state - it is an error to call this method unless fallbackPrivateBlobKey is correct. It is the caller's responsibility to ensure this in order to avoid the error.