Follow-ups to the certificate searcher: bounds-checked property reads, store handle leaks

[darkfronza]

Stacked on #1089 — review that one first; this PR's diff is against it, not against master.

Six follow-ups from review of #1089. They either change behavior that PR deliberately scoped out, or touch pre-existing code, so they are kept separate.

The container name reader no longer follows a raw pointer

cryptFindCertificateKeyContainerName dereferenced the pwszContainerName pointer CryptoAPI wrote into the property buffer, walking UTF-16 until a NUL with no relation to the size the API reported, and pinned the buffer with runtime.KeepAlive.

The KeepAlive was a no-op. Go's GC retains an object when any pointer into it is live, and both info and info.pwszContainerName are interior pointers into buf. Noscan means the collector doesn't scan inside buf for outgoing pointers; it says nothing about incoming references. The comment justifying the KeepAlive asserted the opposite, which is the kind of reasoning that gets copied to somewhere it matters.

The hazard that was real is the unbounded read. The string is now decoded out of buf, so every read is bounds-checked against the reported size and a short or malformed property returns an error instead of reading past the buffer. The new utf16StringInBuffer helper is reusable for pwszProvName.

Certificates with unreadable key metadata are reported, not hidden

SearchCertificates dropped a certificate whenever its CERT_KEY_PROV_INFO couldn't be read. A certificate with no private-key association already reads back as ("", nil) via CRYPT_E_NOT_FOUND, so the only ones reaching that path had an association that exists but is corrupt — exactly the population this API was added to find.

apiv1.SearchCertificateResult gains an Err field carrying the read failure. That also lets a caller tell "no key association at all" apart from "association present but unreadable". A certificate whose DER can't be parsed is still skipped; there's no certificate to report.

A test that actually pins the fix

The capi test in #1089 stores with skip-find-certificate-key=true and asserts the container name comes back empty — it passes just as happily against the reader #1089 replaced, which returned "" unconditionally. The only assertion of a real container name needs both a TPM and Windows.

The new capi test pins the name on any Windows box. StoreCertificate's key= branch records the association via setCertificateKeyProvInfo without checking the container exists, so no key, no TPM and no administrative rights are needed — and it sets up precisely the state this feature exists to detect: a certificate naming a key container that isn't there.

Certificate store handles are closed

Six functions opened a store with the same twenty lines of location switch, CertOpenStore and error wrap; five never closed the handle. SearchCertificates was the exception, on the grounds that a search invites frequent calls — but FindCertificatesByIssuer and CleanupCredentials are called just as often by the same sweeps, and every LoadCertificate, CreateSigner and GetPublicKey leaked one through getCertContext.

Extracted openCertStore and closed the handle at all six. Closing with no flags only drops a reference — a certificate context holds its own — so getCertContext can close with a defer while still handing a live context back. CERT_CLOSE_STORE_FORCE_FLAG would not be safe there and is deliberately not used.

Consolidating also fixed a latent GC hazard all six copies shared: the store name was converted to uintptr inside the call, leaving no Go pointer to it, so it could be reclaimed while CertOpenStore ran. The compiler's special case for that pattern covers only conversions written inside a syscall.Syscall call, which this isn't. It's now held in a variable and kept alive across the call — the genuine version of the pattern the first commit removes, and the two comments explain the difference.

StoreCertificate opens its store before building the certificate context, preserving the early store-location validation so an unusable location still fails before key association, which can prompt for a smart card.

Smaller fixes

• Five bare err.(windows.Errno) assertions replaced with an isNotFound helper built on errors.As, which keeps working when the error arrives wrapped.
• Removed the certHandle == nil branch in SearchCertificates. x/sys sets err whenever CertEnumCertificatesInStore returns a nil context (errnoErr maps a zero errno to EINVAL, not nil), so it was unreachable, and its presence implied the two conditions were independent.
• platform.KMS.SearchCertificates rejects a nil request instead of panicking in clone(req). capi and tpmkms keep their own checks — both are exported backends used directly too. platform.CleanupCredentials has the same gap, left alone here.

Verification

CI here is ubuntu-only, so none of the windows-tagged code runs there. Stating exactly what was run locally:

• GOOS=windows GOARCH=amd64 go build ./kms/... — clean, and clean at each of the six commits individually
• GOOS=windows GOARCH=amd64 go vet ./kms/capi/... ./kms/platform/... ./kms/tpmkms/... — the single finding (ncrypt_windows.go possible misuse of unsafe.Pointer in findCertificateInStore) is byte-identical to the base branch; only its line number moved. Nothing new around the uintptr arithmetic in utf16StringInBuffer.
• go test ./kms/... on the host; windows-tagged tests compile under GOOS=windows
• utf16StringInBuffer was validated against a synthetic blob laid out the way CryptoAPI lays one out: correct container and provider names, out-of-buffer pointer rejected (an underflowing offset wraps to a huge uintptr, so one check covers both directions), unterminated string rejected
• golangci-lint could not be run locally: the shared smallstep config requires a newer version than 2.4.0, and once patched around that the linter panics type-checking a dependency needing go1.26 while built with go1.25. It fails identically on the base branch, so it's a local toolchain gap rather than something this branch introduces — CI covers it.

Still outstanding, not addressed here: the KeyContainerName field is a CAPI/CNG concept sitting in cross-platform apiv1, and unlike SearchKeys it returns a raw container name rather than a KMS URI the platform wrapper can translate. Worth settling while the interface is still Experimental.

🤖 Generated with Claude Code