fix(gcp-kms): persist KMS keys and key material across serve --persist restarts - #1435
Merged
Merged
Conversation
…al in a Snapshottable provider mock
NitinKumar004
commented
Oct 4, 2026
NitinKumar004
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review of c7690bb (GCP KMS persistence)
Verdict: MERGE (CI still pending on Test 1-4, CodeQL, gosec and Contrib terraform at review time; Lint, Race, Tidy, Structure, Format, govulncheck and the other contrib jobs are green).
Verified on a clean build of the pushed SHA with serve --persist (binary, ports 14066/14068/14069):
- Control plane over HTTP: create key ring, create symmetric and EC_SIGN_P256 keys, get version, destroy then restore (DESTROY_SCHEDULED, then DISABLED, primary cleared), getIamPolicy and setIamPolicy on a key, get public key. Response shapes match the previous wire output.
- Terraform: key ring, key and google_kms_secret_ciphertext applied. After stopping serve (state saved) and restarting it, a ciphertext produced before the restart decrypts through google_kms_secret to the original plaintext. Resources show no drift on plan, and destroy removes both resources.
- Snapshot file is written 0600. It carries key material only in the snapshot. No API response carries it: every version read goes through readCopy, which nils Secret and PrivateKey (Get, List, Update, Destroy, Restore). Encrypt, Decrypt and the sign/MAC calls hold m.mu while they touch the material and return only Result.Out.
- Restore tolerates missing sections: empty keyRings/cryptoKeys/cryptoKeyVersions are skipped, and a snapshot with no KMS entry leaves the mock empty. Lazily generated material is written back with versions.Set, so it is included in the next snapshot.
- Persist completeness is reflection-based over the provider struct; Provider.KMS implements Snapshottable, so it is covered, plus the new TestKMSKeysSurviveRestore (ciphertext, signature and public key survive).
- internal/coveragegen/wireops.go: the diff is exactly the removal of the "gcp/kms" map entry (8 lines). Nothing else in the file changed, so there is no conflict surface with the AWS work.
Non-blocking findings:
- Coverage docs regression (docs/coverage/gcp/kms.md). GetIamPolicy, SetIamPolicy and TestIamPermissions dropped out of the KMS list (17 to 22 ops, net of the data plane additions). They are still served: getIamPolicy/setIamPolicy on a crypto key returned the expected policy and etag above. The privateca page also omits its IAM verbs, so the omission is consistent with how resourceiam-backed services are documented today. But KMS listed them before, so this is a docs loss for users. Recommend keeping them listed: re-add a
gcp/kmsentry in wireops.go containing only those three verbs (not the old 17-op list), then run go generate. If the generator cannot merge wire-derived and hand-listed ops, leave as is and track it as a docs follow-up. - Key material is held as exported fields on Version (Secret, PrivateKey). They are safe today because every outward path uses readCopy, but any future getter that returns a Version without readCopy would leak. Consider making them unexported with explicit snapshot marshalling, or add a test that asserts no returned Version carries material.
- Version values returned from the store share the backing []byte of Secret/PrivateKey with the stored copy. This is race-safe now because all reads happen under m.mu and nothing mutates the slices, but it is not a deep copy; a bytes.Clone in the store read path would make that invariant structural.
Not run: local go test (rely on CI), Docker image parity (excluded by scope).
NitinKumar004
marked this pull request as ready for review
October 4, 2026 08:45
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.
Plan (fast mode)
Row: GKMS-N1. Cloud KMS state (key rings, crypto keys, versions, key material and IAM policies) lived in a wire-only store in
server/gcp/kms/store.go.snapshot.Discovernever saw it, so aserve --persistrestart lost every key, and ciphertexts stored by apps became undecryptable.Real behaviour: KMS keys are durable. A ciphertext from
cryptoKeys.encryptdecrypts for as long as its version is ENABLED, and an asymmetric version keeps the same key pair (and public key) for its whole life.Fix (privateca pattern):
providers/gcp/kmsmock, backed by memstore. It has three value-typed stores (key rings, crypto keys, versions) keyed by full resource name.Secret, and RSA/EC/Ed25519PrivateKeyas PKCS#8 DER.snapshot.Snapshottablewith a compile-time assert, soTestSnapshotCompletenessnow covers it.crypto.gois agit mv.Provider.KMSis wired throughDriversFrom(from_provider.go).server/gcpfalls back to a fresh mock whenDrivers.KMSis nil, so the handler is still always registered.resourceiamstore throughgcpiam.Serve, after an existence check. Etag semantics are the shared ones. This drops the handler-localiam.go.persist.Restoreonly visits services present in the file, so a snapshot without akmsentry restores cleanly.Mock.Restorealso skips missing sections.gcp/kmsentry frominternal/coveragegen/wireops.go; that fallback only applies to wire-only handlers.go generateis idempotent.Size: 17 non-test files, +1247/-1111. Most of it is a move, so the net change is about +136 lines.
Tests
persist/kms_persist_test.goruns throughcloudemu.NewGCP()+gcpserver.NewFromProvider+persist.ExportAll/RestoreAll, the same path serve takes. It covers:providers/gcp/kms/snapshot_test.gochecks key material for RSA, Ed25519, OAEP, HMAC and AES byte-for-byte across a snapshot, then signing and decrypt after the restore. An empty{}snapshot restores without error.server/gcp/kmsSDK tests pass unchanged: the gapic data plane, the cloudkms/v1 lifecycle and the IAM etag rules.Gates:
go build ./...passes.go test -racepasses onproviders/gcp/kms,server/gcp/kmsandserver/gcp, and plaingo testonpersist,providers/gcp,internal/coveragegenand thecmd/cloudemulean-deps guard. golangci-lint--new-from-rev=origin/development: 0 issues. gofmt is clean.E2E
Binary,
serve --persist --persist-strategy on-shutdown --state-file X(GCP :16069), OpenTofu 1.10 google provider:google_kms_key_ring+google_kms_crypto_key(rotation_period) +google_kms_secret_ciphertext: 3 added.kmssection.data "google_kms_secret"enabled: 0 changed, and the output issurvives-restart.plan -detailed-exitcodeexits 0.Docker: pending Gate 2.