Add gcpkms keystore implementation - #2338
Open
AmrMohamedRezk wants to merge 2 commits into
Open
Conversation
product-security-plaid-production
Bot
requested review from
george-dorin and
michelemin
August 24, 2026 14:04
Contributor
|
👋 AmrMohamedRezk, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Contributor
✅ API Diff Results -
|
makramkd
reviewed
Aug 24, 2026
| // Reader is the interface for reading keys from the keystore. | ||
| // GetKeys returns all keys in the keystore if no names are provided, or the keys with the given names. | ||
| // Keys are sorted by name in lexicographic order. | ||
| // The order of returned keys is implementation-specific; callers must not rely on a particular ordering. |
Contributor
There was a problem hiding this comment.
Hm this seems like a breaking change, can we not satisfy this requirement (maybe sort client side)?
Author
There was a problem hiding this comment.
No that shouldn't break any existing behavior. The old AWS behavior is as is. The new behavior doesn't, this is just to make sure clients don't always assume it's lexicographic order regardless of the backend.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Google Cloud KMS-backed keystore.Reader and keystore.Signer.
Changes:
- Adds GCP KMS client, keystore, and in-memory fake.
- Supports key discovery, version resolution, public-key retrieval, and signing.
- Adds tests and updates dependencies and ordering documentation.
Review findings:
- Critical (2 votes):
AsymmetricSignResponse.Nameis referenced infake_client.go, causing compilation errors. - Critical (2 votes):
keystore.goreferences the nonexistentAsymmetricSignResponse.Namefield. - Moderate (2 votes): Signers using parent
CryptoKeynames may resolve a different version than the one used for retrieved metadata.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Summary |
|---|---|
keystore/reader.go |
Updates key ordering documentation. |
keystore/go.sum |
Records dependency checksums. |
keystore/go.mod |
Adds Google Cloud dependencies. |
keystore/gcpkms/keystore.go |
Implements GCP KMS key retrieval and signing. |
keystore/gcpkms/keystore_test.go |
Tests GCP KMS behavior and edge cases. |
keystore/gcpkms/fake_client.go |
Provides an in-memory KMS test client. |
keystore/gcpkms/client.go |
Wraps the Google Cloud KMS SDK. |
Suppressed comments (3)
keystore/gcpkms/client.go:97
ListCryptoKeyVersionsalso uses the unspecified BASIC view. The resolver requires each listed version'sAlgorithmandCreateTime, but BASIC responses omit those fields; a parent-key lookup therefore sees an unspecified algorithm and errors before it can sign or list the key. RequestCryptoKeyVersionView_FULLhere.
iter := c.client.ListCryptoKeyVersions(ctx, &kmspb.ListCryptoKeyVersionsRequest{Parent: cryptoKeyName})
keystore/gcpkms/client.go:92
ListCryptoKeysis called with the default BASIC/unspecified view. Cloud KMS does not populate the metadata this implementation reads insigningKeyNames(PurposeandVersionTemplate) in that view; in a real key ring those fields remain zero/nil, so asymmetric signing keys can be skipped as non-signing. RequestCryptoKeyView_FULL(or fetch the metadata explicitly) before relying on these fields.
iter := c.client.ListCryptoKeys(ctx, &kmspb.ListCryptoKeysRequest{Parent: keyRingName})
keystore/gcpkms/keystore.go:305
- Sorting only the discovery path makes this backend violate the Reader contract that existed before this PR (
keystore/reader.go:27): an explicit allowlist is returned in caller order here, while the in-memory and AWS readers sort it lexicographically. Please preserve the shared API guarantee by sortingkeyNamesfor both paths instead of weakeningReader's contract.
// Cloud KMS does not guarantee a listing order; sort for a stable response.
sort.Strings(keyNames)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+243
to
+247
| return &kmspb.AsymmetricSignResponse{ | ||
| Name: req.Name, | ||
| Signature: derSig, | ||
| SignatureCrc32C: wrapperspb.Int64(crc32c(derSig)), | ||
| VerifiedDigestCrc32C: true, |
Comment on lines
+367
to
+369
| if sig.Name != versionName { | ||
| return keystore.SignResponse{}, fmt.Errorf("signing response has name %q, expected %q", sig.Name, versionName) | ||
| } |
Comment on lines
+326
to
+329
| createdAt := resolved.version.CreateTime.AsTime() | ||
| keys = append(keys, keystore.GetKeyResponse{ | ||
| KeyInfo: keystore.NewKeyInfo(keyName, resolved.keyType, createdAt, publicKeyBytes, []byte{}), | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CCIP-13120 https://smartcontract-it.atlassian.net/browse/CCIP-13120
Add a Google Cloud KMS keystore backend
Adds keystore/gcpkms, a keystore.Reader + keystore.Signer implementation backed by Google Cloud KMS, alongside the existing AWS backend in keystore/kms.