Skip to content

fix(gcp-kms): persist KMS keys and key material across serve --persist restarts - #1435

Merged
NitinKumar004 merged 1 commit into
developmentfrom
fix/gcp-kms-persist
Oct 4, 2026
Merged

NitinKumar004 merged 1 commit into
developmentfrom
fix/gcp-kms-persist

Conversation

@NitinKumar004

Copy link
Copy Markdown
Collaborator

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.Discover never saw it, so a serve --persist restart lost every key, and ciphertexts stored by apps became undecryptable.

Real behaviour: KMS keys are durable. A ciphertext from cryptoKeys.encrypt decrypts 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):

  • New providers/gcp/kms mock, backed by memstore. It has three value-typed stores (key rings, crypto keys, versions) keyed by full resource name.
    • Versions carry their key material: AES/HMAC Secret, and RSA/EC/Ed25519 PrivateKey as PKCS#8 DER.
    • It implements snapshot.Snapshottable with a compile-time assert, so TestSnapshotCompleteness now covers it.
    • Exported Mock methods use the native op names (UpdateCryptoKey, CreateCryptoKeyVersion, AsymmetricSign, MacVerify, GenerateRandomBytes and so on). The control-plane, state-machine and crypto logic moved over from the server package unchanged; crypto.go is a git mv.
  • Provider.KMS is wired through DriversFrom (from_provider.go). server/gcp falls back to a fresh mock when Drivers.KMS is nil, so the handler is still always registered.
  • Key ring and crypto key IAM policies move to the shared persisted resourceiam store through gcpiam.Serve, after an existence check. Etag semantics are the shared ones. This drops the handler-local iam.go.
  • The server package is now only the wire layer: JSON shapes, enum normalization, CRC32C checks and routing.
  • Old snapshots: persist.Restore only visits services present in the file, so a snapshot without a kms entry restores cleanly. Mock.Restore also skips missing sections.
  • Coverage: regenerated. The GCP KMS page now lists the data plane (Encrypt, Decrypt, AsymmetricSign, AsymmetricDecrypt, GetPublicKey, MacSign, MacVerify, GenerateRandomBytes). IAM verbs come from the shared policy store, as for privateca. I removed the dead gcp/kms entry from internal/coveragegen/wireops.go; that fallback only applies to wire-only handlers. go generate is 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.go runs through cloudemu.NewGCP() + gcpserver.NewFromProvider + persist.ExportAll/RestoreAll, the same path serve takes. It covers:
    • Encrypt with AAD before the snapshot, decrypt after the restore.
    • Asymmetric sign (EC P-256) before; after the restore, the same public key PEM, the old signature verifies, and a new signature verifies with the original key.
    • Key ring IAM, list and get reads are identical after the restore.
    • Red on origin/development: 404 "KeyRing r1 not found" after restore.
  • providers/gcp/kms/snapshot_test.go checks 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.
  • Existing server/gcp/kms SDK tests pass unchanged: the gapic data plane, the cloudkms/v1 lifecycle and the IAM etag rules.

Gates: go build ./... passes. go test -race passes on providers/gcp/kms, server/gcp/kms and server/gcp, and plain go test on persist, providers/gcp, internal/coveragegen and the cmd/cloudemu lean-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:

  • Apply google_kms_key_ring + google_kms_crypto_key (rotation_period) + google_kms_secret_ciphertext: 3 added.
  • SIGTERM: "state saved", and the file has a kms section.
  • Restart serve from the same state file. Apply with data "google_kms_secret" enabled: 0 changed, and the output is survives-restart. plan -detailed-exitcode exits 0.
  • destroy: 3 destroyed.

Docker: pending Gate 2.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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/kms entry 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.
  2. 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.
  3. 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
NitinKumar004 marked this pull request as ready for review October 4, 2026 08:45
@NitinKumar004
NitinKumar004 merged commit 392593d into development Oct 4, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant