Delete CNG private keys when CleanupCredentials removes expired certs by darkfronza · Pull Request #1030 · smallstep/crypto

darkfronza · 2026-05-18T22:28:31Z

CleanupCredentials previously deleted the certificate from the Windows store but left the paired CNG private key in the provider, accumulating orphan .PCPKSP blobs on disk as certificates were renewed.

Add a delete-key URI flag on CAPIKMS.DeleteCertificate. When set, the cert's CNG key is removed via nCryptDeleteKey before the cert context is deleted, so a partial failure leaves the cert in place for retry rather than orphaning the key.
Rewrite CAPIKMS.CleanupCredentials to walk the store directly and delete each expired cert together with its CNG key. Enumeration is restarted after every delete because CertDeleteCertificateFromStore frees the context the next find call would chain from.
TPMKMS.deleteCertificateFromWindowsCertificateStore now sets delete-key=true on its URI, since TPMKMS-managed certs have a 1:1 CNG key with no independent use.
Update TestKMS_CleanupCredentials_capi to assert that both the certificate and the CNG private key are removed.

EFF-232

Change-Type: feature
Release-Note: yes
Audience: developer
Impact: low
Breaking: false

maraino

Capi already supports the DeleteKey method, why don't use just that in TPMKMS?

CleanupCredentials previously deleted the certificate from the Windows store but left the paired CNG private key in the provider, accumulating orphan .PCPKSP blobs on disk as certificates were renewed. - Add a `delete-key` URI flag on CAPIKMS.DeleteCertificate. When set, the cert's CNG key is removed via nCryptDeleteKey before the cert context is deleted, so a partial failure leaves the cert in place for retry rather than orphaning the key. - Rewrite CAPIKMS.CleanupCredentials to walk the store directly and delete each expired cert together with its CNG key. Enumeration is restarted after every delete because CertDeleteCertificateFromStore frees the context the next find call would chain from. - TPMKMS.deleteCertificateFromWindowsCertificateStore now sets delete-key=true on its URI, since TPMKMS-managed certs have a 1:1 CNG key with no independent use. - Update TestKMS_CleanupCredentials_capi to assert that both the certificate and the CNG private key are removed. EFF-232 Change-Type: feature Release-Note: yes Audience: developer Impact: low Breaking: false Co-Authored-By: Claude <noreply@anthropic.com>

hslatman · 2026-06-18T17:30:10Z

+	// Also remove the CNG private key paired with the certificate; TPMKMS-managed
+	// certs have a 1:1 CNG key with no independent use, so leaving the .PCPKSP
+	// blob behind would orphan it on disk.
+	uv.Set("delete-key", "true")



Is it clear that all call sites of DeleteCertificate are OK with also removing the key at all times?

Doing this by default, in DeleteCertificate, could be surprising. A key without a certificate is OK in general; maybe not that useful, but generally not an issue. The non-Windows certificate store path keeps the key stored on disk, so this makes it behave different in certain cases.

Agree, when we renew a certificate we might want to reuse the key.

Good call — addressed in 0e5a1a8.

DeleteCertificate no longer removes the key by default. The key is only deleted when the caller explicitly sets delete-key=true on the request URI; otherwise the certificate is removed and the CNG/PCP key is left in place, matching the non-Windows certificate-store behavior. So a renewal that wants to reuse the key just deletes the old cert without the flag.

Key deletion is now driven through CleanupCredentials instead (also opt-in via delete-key on its URI). When requested there, it first tries a full teardown via TPMKMS.DeleteKey — which removes both the TPM storage entry and the CNG/NCrypt container — and only falls back to deleting the container directly when DeleteKey cannot act on the key (storage not loadable, key not found, or TPM not openable).

…ey-containers-not-deleted-for-superseded-certs # Conflicts: # kms/capi/capi.go

Reworks how the CNG private key paired with a certificate is removed, addressing review feedback that deleting the key by default in DeleteCertificate is surprising and diverges from the non-Windows path. - CleanupCredentialsRequest now uses a URI "Name" (issuer, store-location, store, delete-key) like the other KMS requests; RawSubject stays a separate field as it doesn't encode well into a URI. - TPMKMS.CleanupCredentials honors "delete-key" from the request. When set it tries a full teardown via TPMKMS.DeleteKey (removing both the TPM storage entry and the CNG/NCrypt container), falling back to a direct container delete only when DeleteKey can't act on the key (storage not loadable, key not found, or TPM not openable). - TPMKMS.DeleteCertificate no longer deletes the key by default; it only does so when "delete-key=true" is set on the request URI. - CAPIKMS.CleanupCredentials parses the same URI and honors "delete-key". EFF-232 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

maraino

Some questions and some suggestions

- platform: KMS.CleanupCredentials now translates the request URI scheme via transformToURI like the other methods, so callers use the generic "kms" scheme. Returns NotImplementedError when the backend doesn't implement CredentialsCleaner. - tpmkms: CleanupCredentials returns apiv1.NotImplementedError instead of silently succeeding when not configured with the Windows certificate store, for consistency with the platform KMS. - tpmkms: shouldFallbackToDirectKeyDelete uses errors.Is with apiv1.NotFoundError{} instead of errors.As. - capi: document the cert-context ownership in cleanupOnePass (findCertificateInStore frees prevCert on the next call; nil means no further match) so the handle handling is clear. - tests: use the "kms" scheme in CleanupCredentials requests. EFF-232 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… keys SearchKeys aborted the entire search when a single key could not be loaded (e.g. a corrupt keyset reporting NTE_BAD_KEYSET). Because CleanupCredentials builds a Subject-Key-Identifier map up front via SearchKeys when delete-key is requested, one bad key blocked deletion of every expired certificate and its key. Make SearchKeys best-effort: it now returns every key it could load plus a *apiv1.PartialError enumerating the keys that failed, instead of failing atomically. The per-key result building is extracted into a pure buildSearchKeysResults helper over small searchableAK/searchableKey interfaces so it can be unit tested. keyNamesBySubjectKeyID keeps the map built from the keys that loaded and propagates the partial error; CleanupCredentials treats a partial error as non-fatal (keeping the map) and a non-partial error as a fall back to an empty map. Either way certificates whose key is absent from the map take the orphan/direct CNG deletion path, so expired certificates are always cleaned and the enumeration failure is surfaced rather than swallowed. The platform KMS SearchKeys wrapper forwards partial results alongside the error. Adds apiv1.KeyError and apiv1.PartialError, with tests for the new error types, the best-effort builder, keyNamesBySubjectKeyID, and degraded (partial and fatal) CleanupCredentials enumeration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address review feedback: drop the apiv1.KeyError and apiv1.PartialError types in favor of wrapping each failure with fmt.Errorf and joining them with errors.Join. No caller needs to distinguish a partial failure from a total one programmatically — they proceed with whatever keys loaded regardless — so the custom types added surface area without a use case. SearchKeys still returns the keys it could load alongside the joined error. keyNamesBySubjectKeyID builds its map from whatever was returned and joins any load failures; CleanupCredentials records that error and continues, so certificates whose key is missing from the map fall back to direct CNG deletion. The platform KMS SearchKeys wrapper forwards the partial results and the error without inspecting its type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

maraino

LGTM, but it would be nice to document why SearchKeys might fail.

Address review feedback: explain in the SearchKeys godoc and the platform KMS wrapper why the call is best-effort and what causes per-key failures (unopenable TPM, unreadable storage, or corrupt/missing key material such as a Windows PCP/CNG keyset reporting NTE_BAD_KEYSET). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ey-containers-not-deleted-for-superseded-certs

…ed-for-superseded-certs

maraino

lgtm

github-actions Bot added the needs triage label May 18, 2026

maraino reviewed May 18, 2026

View reviewed changes

darkfronza force-pushed the diego/eff-232-cng-key-containers-not-deleted-for-superseded-certs branch from 9788b30 to 131ed95 Compare June 12, 2026 21:17

hslatman reviewed Jun 18, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into diego/eff-232-cng-k…

e511f01

…ey-containers-not-deleted-for-superseded-certs # Conflicts: # kms/capi/capi.go

maraino self-requested a review June 18, 2026 18:09

maraino requested changes Jun 18, 2026

View reviewed changes

darkfronza and others added 2 commits June 19, 2026 11:57

maraino requested changes Jun 22, 2026

View reviewed changes

Comment thread kms/apiv1/options.go Outdated

Comment thread kms/tpmkms/tpmkms.go Outdated

Comment thread kms/tpmkms/tpmkms.go Outdated

Comment thread kms/platform/kms.go Outdated

maraino previously approved these changes Jun 24, 2026

View reviewed changes

Comment thread kms/platform/kms.go Outdated

darkfronza and others added 2 commits June 24, 2026 12:21

Merge remote-tracking branch 'origin/master' into diego/eff-232-cng-k…

884a66f

…ey-containers-not-deleted-for-superseded-certs

darkfronza dismissed maraino’s stale review via 884a66f June 24, 2026 15:24

Merge branch 'master' into diego/eff-232-cng-key-containers-not-delet…

dc133eb

…ed-for-superseded-certs

maraino approved these changes Jun 24, 2026

View reviewed changes

darkfronza merged commit 5939e29 into master Jun 24, 2026
10 of 12 checks passed

darkfronza deleted the diego/eff-232-cng-key-containers-not-deleted-for-superseded-certs branch June 24, 2026 18:04

Uh oh!

Conversation

darkfronza commented May 18, 2026

Uh oh!

maraino left a comment

Choose a reason for hiding this comment

Uh oh!

hslatman Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

maraino Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

darkfronza Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

maraino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maraino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maraino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants