diff --git a/kms/capi/capi.go b/kms/capi/capi.go index 661a7808..c78df46b 100644 --- a/kms/capi/capi.go +++ b/kms/capi/capi.go @@ -436,21 +436,40 @@ func (k *CAPIKMS) getCertContext(u *uriAttributes) (*windows.CertContext, error) } } case u.containerName != "": - key, err := k.GetPublicKey(&apiv1.GetPublicKeyRequest{ + // Preferred path: open the CNG key against the bound provider, + // derive its SubjectKeyID, and look the cert up by SKI. This is + // the indexed search Windows is optimized for, and it pins down + // *which* key the cert binds to even when several certs share a + // container name. + // + // Fallback path: when the key can't be opened (e.g. the bound + // provider is Microsoft Platform Crypto Provider but the cert is + // bound to a Microsoft Software KSP container, or the caller + // lacks permission on the key), enumerate the store and match + // against the cert's CERT_KEY_PROV_INFO container name. The + // property is set whenever a cert was stored with key-association + // (cryptFindCertificateKeyProvInfo) regardless of provider. + key, kpErr := k.GetPublicKey(&apiv1.GetPublicKeyRequest{ Name: uri.New(Scheme, url.Values{ ContainerNameArg: []string{u.containerName}, }).String(), }) - if err != nil { - return nil, err - } - keyID, err := x509util.GenerateSubjectKeyID(key) - if err != nil { - return nil, fmt.Errorf("error generating SubjectKeyID: %w", err) - } - if handle, err = findCertificateBySubjectKeyID(st, keyID); err != nil { - if !errors.Is(err, apiv1.NotFoundError{}) || !canLookupByIssuer { - return nil, err + if kpErr == nil { + keyID, err := x509util.GenerateSubjectKeyID(key) + if err != nil { + return nil, fmt.Errorf("error generating SubjectKeyID: %w", err) + } + if handle, err = findCertificateBySubjectKeyID(st, keyID); err != nil { + if !errors.Is(err, apiv1.NotFoundError{}) || !canLookupByIssuer { + return nil, err + } + } + } else { + handle, err = findCertificateByKeyContainerName(st, u.containerName) + if err != nil { + if !errors.Is(err, apiv1.NotFoundError{}) || !canLookupByIssuer { + return nil, err + } } } } @@ -786,6 +805,14 @@ func (k *CAPIKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ( chain := []*x509.Certificate{cert} child := cert for i := 0; i < maximumIterations; i++ { // loop a maximum number of times + // No AuthorityKeyIdentifier means there's nothing to look up: a + // self-signed leaf (own-CA, RDP self-cert, our own test fixtures) + // or a CA-issued cert from a CA that doesn't emit AKI. Returning + // the chain we have is correct; falling through would build an + // empty-key-id URI that getCertContext rejects. + if len(child.AuthorityKeyId) == 0 { + break + } authorityKeyID := hex.EncodeToString(child.AuthorityKeyId) parent, err := k.LoadCertificate(&apiv1.LoadCertificateRequest{ Name: uri.New(Scheme, url.Values{ @@ -1115,21 +1142,30 @@ func (k *CAPIKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error { prevCert = certHandle } case u.containerName != "": - key, err := k.GetPublicKey(&apiv1.GetPublicKeyRequest{ + // Mirror the same preferred/fallback split as getCertContext: + // open the CNG key for an indexed SKI lookup when possible, and + // fall back to enumerating by container-name property when the + // key can't be opened by the bound provider (Software KSP keys + // from a PCP-bound CAPIKMS, etc.). Without the fallback, the + // agent's renewal cleanup leaks one duplicate cert per cycle. + if key, kpErr := k.GetPublicKey(&apiv1.GetPublicKeyRequest{ Name: uri.New(Scheme, url.Values{ ContainerNameArg: []string{u.containerName}, }).String(), - }) - if err != nil { - return err - } - keyID, err := x509util.GenerateSubjectKeyID(key) - if err != nil { - return fmt.Errorf("error generating SubjectKeyID: %w", err) - } - certHandle, err = findCertificateBySubjectKeyID(st, keyID) - if err != nil { - return err + }); kpErr == nil { + keyID, err := x509util.GenerateSubjectKeyID(key) + if err != nil { + return fmt.Errorf("error generating SubjectKeyID: %w", err) + } + certHandle, err = findCertificateBySubjectKeyID(st, keyID) + if err != nil { + return err + } + } else { + certHandle, err = findCertificateByKeyContainerName(st, u.containerName) + if err != nil { + return err + } } if err := windows.CertDeleteCertificateFromStore(certHandle); err != nil { return fmt.Errorf("failed removing certificate: %w", err) diff --git a/kms/capi/capi_windows_test.go b/kms/capi/capi_windows_test.go new file mode 100644 index 00000000..9ef71eae --- /dev/null +++ b/kms/capi/capi_windows_test.go @@ -0,0 +1,258 @@ +//go:build windows && !nocapi + +package capi + +import ( + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "math/big" + "net/url" + "testing" + "time" + "unsafe" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" + + "go.step.sm/crypto/kms/apiv1" + "go.step.sm/crypto/kms/uri" + "go.step.sm/crypto/randutil" + "go.step.sm/crypto/x509util" +) + +// makeSoftwareKSPKeyAndCert creates a Software-KSP-backed RSA key in +// CurrentUser scope, signs a short-lived self-signed cert with it, stores +// the cert in CurrentUser\My, and binds the cert to the key via +// CryptFindCertificateKeyProvInfo. Returns the container name and the +// stored cert's SHA-1 thumbprint (hex-encoded). Registers cleanup that +// deletes both. +// +// We need a cert that's actually bound to a CNG container, not just a raw +// cert in the store — that's what every test below exercises. +func makeSoftwareKSPKeyAndCert(t *testing.T, subject string) (containerName string) { + t.Helper() + ctx := t.Context() + + suffix, err := randutil.Hex(8) + require.NoError(t, err) + containerName = "step-capi-test-" + suffix + + km, err := New(ctx, apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSKSP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = km.Close() }) + + created, err := km.CreateKey(&apiv1.CreateKeyRequest{ + Name: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSKSP}, ContainerNameArg: []string{containerName}}).String(), + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 2048, + }) + require.NoError(t, err) + + signer, err := km.CreateSigner(&created.CreateSignerRequest) + require.NoError(t, err) + + // x509.CreateCertificate doesn't auto-populate SubjectKeyId on leaf + // certs, so we have to set it explicitly. Without it the cert lacks a + // subjectKeyIdentifier extension and the SKI-indexed lookup branch + // can't find it — which is the very branch the indexedPath test is + // supposed to exercise. + ski, err := x509util.GenerateSubjectKeyID(signer.Public()) + require.NoError(t, err) + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(time.Now().UnixNano()), + Subject: pkix.Name{CommonName: subject}, + Issuer: pkix.Name{CommonName: subject}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + SubjectKeyId: ski, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, signer.Public(), signer) + require.NoError(t, err) + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + + require.NoError(t, km.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + Certificate: cert, + })) + + t.Cleanup(func() { + // Best-effort cleanup. If a test already deleted the cert, the + // second delete returns NotFound — that's fine. + _ = km.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + HashArg: []string{hexString(cert.SubjectKeyId)}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + }) + _ = km.DeleteKey(&apiv1.DeleteKeyRequest{ + Name: uri.New(Scheme, url.Values{ + ProviderNameArg: []string{ProviderMSKSP}, + ContainerNameArg: []string{containerName}, + }).String(), + }) + }) + + return containerName +} + +func hexString(b []byte) string { + const hexChars = "0123456789abcdef" + out := make([]byte, len(b)*2) + for i, v := range b { + out[i*2] = hexChars[v>>4] + out[i*2+1] = hexChars[v&0x0f] + } + return string(out) +} + +// TestCryptFindCertificateKeyContainerName verifies that the rewritten +// container-name reader actually returns the bound container, where the +// pre-fix implementation always returned "". Without this round-trip the +// fallback paths can't tell certs apart. +func TestCryptFindCertificateKeyContainerName(t *testing.T) { + container := makeSoftwareKSPKeyAndCert(t, "container-name-readback") + + // Open user\My and find the cert by SKI so we get its CertContext. + st, err := windows.CertOpenStore( + certStoreProvSystem, 0, 0, certStoreCurrentUser, + uintptr(unsafe.Pointer(wide(MyStore))), + ) + require.NoError(t, err) + defer windows.CertCloseStore(st, 0) + + got, err := findCertificateByKeyContainerName(st, container) + require.NoError(t, err) + require.NotNil(t, got) + defer windows.CertFreeCertificateContext(got) + + readback, err := cryptFindCertificateKeyContainerName(got) + require.NoError(t, err) + assert.Equal(t, container, readback) +} + +// TestLoadCertificate_containerName_fallback exercises the path that the +// agent's renewal cleanup hits: the CAPIKMS instance is bound to the +// Microsoft Platform Crypto Provider (which is how tpmkms constructs its +// windowsCertificateManager), but the cert lives in the Software KSP. The +// indexed GetPublicKey lookup against PCP fails; the fallback enumerates +// the store by CERT_KEY_PROV_INFO container name and finds the cert. +func TestLoadCertificate_containerName_fallback(t *testing.T) { + container := makeSoftwareKSPKeyAndCert(t, "fallback-load") + + // PCP-bound CAPIKMS — same shape as tpmkms's windowsCertificateManager. + pcp, err := New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSPCP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = pcp.Close() }) + + cert, err := pcp.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + ContainerNameArg: []string{container}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + }) + require.NoError(t, err) + require.NotNil(t, cert) + assert.Equal(t, "fallback-load", cert.Subject.CommonName) +} + +// TestLoadCertificate_containerName_notFound asserts the fallback path +// returns NotFoundError (not a wrapped Windows error) when nothing matches. +// The agent's cleanup wrapper specifically tests errors.Is(err, NotFound), +// so the shape matters. +func TestLoadCertificate_containerName_notFound(t *testing.T) { + suffix, err := randutil.Hex(8) + require.NoError(t, err) + missing := "step-capi-test-missing-" + suffix + + pcp, err := New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSPCP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = pcp.Close() }) + + _, err = pcp.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + ContainerNameArg: []string{missing}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + }) + assert.ErrorIs(t, err, apiv1.NotFoundError{}) +} + +// TestDeleteCertificate_containerName_fallback is the load test's twin for +// the delete path the agent's cleanup actually uses. After delete, a second +// lookup must return NotFound; otherwise duplicates keep accumulating. +func TestDeleteCertificate_containerName_fallback(t *testing.T) { + container := makeSoftwareKSPKeyAndCert(t, "fallback-delete") + + pcp, err := New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSPCP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = pcp.Close() }) + + require.NoError(t, pcp.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + ContainerNameArg: []string{container}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + })) + + _, err = pcp.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + ContainerNameArg: []string{container}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + }) + assert.ErrorIs(t, err, apiv1.NotFoundError{}) +} + +// TestLoadCertificate_containerName_indexedPath confirms the preferred +// SKI-indexed branch still works when the bound provider *can* open the +// container. The fallback must not regress the TPM-resident case. +func TestLoadCertificate_containerName_indexedPath(t *testing.T) { + container := makeSoftwareKSPKeyAndCert(t, "indexed-path") + + // Same provider that owns the key — GetPublicKey will succeed and the + // preferred SKI-indexed branch fires. + msksp, err := New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New(Scheme, url.Values{ProviderNameArg: []string{ProviderMSKSP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = msksp.Close() }) + + cert, err := msksp.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + ContainerNameArg: []string{container}, + StoreLocationArg: []string{UserStoreLocation}, + StoreNameArg: []string{MyStore}, + }).String(), + }) + require.NoError(t, err) + require.NotNil(t, cert) + assert.Equal(t, "indexed-path", cert.Subject.CommonName) +} + diff --git a/kms/capi/ncrypt_windows.go b/kms/capi/ncrypt_windows.go index 0da97c1f..5050d53c 100644 --- a/kms/capi/ncrypt_windows.go +++ b/kms/capi/ncrypt_windows.go @@ -155,6 +155,7 @@ var ( crypt32 = windows.MustLoadDLL("crypt32.dll") procCertFindCertificateInStore = crypt32.MustFindProc("CertFindCertificateInStore") + procCertEnumCertificatesInStore = crypt32.MustFindProc("CertEnumCertificatesInStore") procCryptFindCertificateKeyProvInfo = crypt32.MustFindProc("CryptFindCertificateKeyProvInfo") procCertGetCertificateContextProperty = crypt32.MustFindProc("CertGetCertificateContextProperty") procCertSetCertificateContextProperty = crypt32.MustFindProc("CertSetCertificateContextProperty") @@ -194,14 +195,21 @@ type CERT_ID_SERIAL struct { Serial CERT_ISSUER_SERIAL_NUMBER } +// CRYPT_KEY_PROV_INFO mirrors the Win32 struct of the same name. Field +// alignment matters: dwProvType/dwFlags/cProvParam are 32-bit DWORDs but +// rgProvParam is a pointer-sized field that must land on its natural +// alignment, so Go inserts the same 4-byte padding that the C compiler +// does. See: +// https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-crypt_key_prov_info type CRYPT_KEY_PROV_INFO struct { - pwszContainerName int - pwszProvName int - dwProvType int - dwFlags int - cProvParam int - rgProvParam int - dwKeySpec int + pwszContainerName *uint16 + pwszProvName *uint16 + dwProvType uint32 + dwFlags uint32 + cProvParam uint32 + _ uint32 // padding to align rgProvParam on 64-bit + rgProvParam uintptr + dwKeySpec uint32 } func errNoToStr(e uint32) string { @@ -601,41 +609,111 @@ func cryptFindCertificatePrivateKey(certContext *windows.CertContext) (uintptr, return uintptr(kh), nil } +// cryptFindCertificateKeyContainerName returns the CNG key container name +// recorded on certContext's CERT_KEY_PROV_INFO property, or the empty string +// if the property is absent. Errors reflect a Windows API failure that +// isn't simply "property not set on this cert". +// +// The container name is the same value that NCryptOpenKey accepts as its +// pwszKeyName; it's set on a cert by CryptFindCertificateKeyProvInfo or +// equivalent when a cert is bound to a CNG key. func cryptFindCertificateKeyContainerName(certContext *windows.CertContext) (string, error) { - var ( - length uint32 - provInfo *CRYPT_KEY_PROV_INFO - ) - - r1, _, err := procCertGetCertificateContextProperty.Call( + // First call with a nil buffer asks Windows for the required byte + // length of the property data. + var length uint32 + r, _, err := procCertGetCertificateContextProperty.Call( uintptr(unsafe.Pointer(certContext)), uintptr(CERT_KEY_PROV_INFO_PROP_ID), - uintptr(0), + 0, uintptr(unsafe.Pointer(&length)), ) - if !errors.Is(err, windows.Errno(0)) { - return "", fmt.Errorf("CertGetCertificateContextProperty returned %w", err) + if r == 0 { + // CRYPT_E_NOT_FOUND just means this cert has no KeyProvInfo — + // treat as "no container", not an error. + if errno, ok := err.(windows.Errno); ok && uint32(errno) == CRYPT_E_NOT_FOUND { + return "", nil + } + return "", fmt.Errorf("CertGetCertificateContextProperty (length) returned %w", err) } - if r1 == 0 { - return "", fmt.Errorf("finding key container name failed: %v", errNoToStr(uint32(r1))) + if length == 0 { + return "", nil } - r2, _, err := procCertGetCertificateContextProperty.Call( + buf := make([]byte, length) + r, _, err = procCertGetCertificateContextProperty.Call( uintptr(unsafe.Pointer(certContext)), uintptr(CERT_KEY_PROV_INFO_PROP_ID), - uintptr(0), - uintptr(unsafe.Pointer(provInfo)), + uintptr(unsafe.Pointer(&buf[0])), + uintptr(unsafe.Pointer(&length)), ) - - if !errors.Is(err, windows.Errno(0)) { + if r == 0 { return "", fmt.Errorf("CertGetCertificateContextProperty returned %w", err) } - if r2 == 0 { - return "", fmt.Errorf("finding key container name failed: %v", errNoToStr(uint32(r2))) + if uintptr(length) < unsafe.Sizeof(CRYPT_KEY_PROV_INFO{}) { + return "", fmt.Errorf("CertGetCertificateContextProperty returned %d bytes; expected at least %d", length, unsafe.Sizeof(CRYPT_KEY_PROV_INFO{})) } - return "", nil + provInfo := (*CRYPT_KEY_PROV_INFO)(unsafe.Pointer(&buf[0])) + if provInfo.pwszContainerName == nil { + return "", nil + } + return windows.UTF16PtrToString(provInfo.pwszContainerName), nil +} + +// certEnumCertificatesInStore wraps CertEnumCertificatesInStore. Pass prev=nil +// to start enumeration; on each subsequent call pass the handle returned by +// the previous one. The store frees prev internally before returning the next +// handle, so callers must not free it themselves. When enumeration is +// exhausted the function returns (nil, nil). +func certEnumCertificatesInStore(store windows.Handle, prev *windows.CertContext) (*windows.CertContext, error) { + h, _, err := procCertEnumCertificatesInStore.Call( + uintptr(store), + uintptr(unsafe.Pointer(prev)), + ) + if h == 0 { + if errno, ok := err.(windows.Errno); ok && uint32(errno) == CRYPT_E_NOT_FOUND { + return nil, nil + } + return nil, err + } + return (*windows.CertContext)(unsafe.Pointer(h)), nil +} + +// findCertificateByKeyContainerName walks store and returns the first cert +// whose CERT_KEY_PROV_INFO container name matches name. The caller owns the +// returned context and must free it with windows.CertFreeCertificateContext +// when done. Returns (nil, NotFoundError) when no match exists. +// +// Unlike findCertificateBySubjectKeyID this does not require the matching CNG +// key to be openable by the caller's provider handle — the lookup is purely +// against cert-context properties — so it works for Software-KSP-backed +// containers even when the caller's CAPIKMS is bound to a different provider +// (e.g. the Microsoft Platform Crypto Provider used by tpmkms). +func findCertificateByKeyContainerName(store windows.Handle, name string) (*windows.CertContext, error) { + var prev *windows.CertContext + for { + h, err := certEnumCertificatesInStore(store, prev) + if err != nil { + return nil, fmt.Errorf("certEnumCertificatesInStore failed: %w", err) + } + if h == nil { + return nil, apiv1.NotFoundError{Message: fmt.Sprintf("certificate with %s=%q not found", ContainerNameArg, name)} + } + + container, err := cryptFindCertificateKeyContainerName(h) + switch { + case err != nil: + // Couldn't read the property on this cert; skip it. Don't + // abort enumeration — other certs in the store may match. + prev = h + continue + case container == name: + return h, nil + default: + prev = h + } + } } func certSetCertificateContextProperty(certContext *windows.CertContext, propID uint32, pvData uintptr) error { diff --git a/kms/tpmkms/tpmkms.go b/kms/tpmkms/tpmkms.go index 77c92478..268bbeae 100644 --- a/kms/tpmkms/tpmkms.go +++ b/kms/tpmkms/tpmkms.go @@ -851,13 +851,6 @@ func (k *TPMKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([ } func (k *TPMKMS) loadCertificateChainFromWindowsCertificateStore(req *apiv1.LoadCertificateRequest) ([]*x509.Certificate, error) { - pub, err := k.GetPublicKey(&apiv1.GetPublicKeyRequest{ - Name: req.Name, - }) - if err != nil { - return nil, fmt.Errorf("failed retrieving public key: %w", err) - } - o, err := parseNameURI(req.Name) if err != nil { return nil, fmt.Errorf("failed parsing %q: %w", req.Name, err) @@ -880,22 +873,45 @@ func (k *TPMKMS) loadCertificateChainFromWindowsCertificateStore(req *apiv1.Load intermediateCAStore = o.intermediateStore } - subjectKeyID, err := generateWindowsSubjectKeyID(pub) - if err != nil { - return nil, fmt.Errorf("failed generating subject key id: %w", err) + values := url.Values{ + "store-location": []string{location}, + "store": []string{store}, + "intermediate-store-location": []string{intermediateCAStoreLocation}, + "intermediate-store": []string{intermediateCAStore}, + "issuer": []string{o.issuer}, + "friendly-name": []string{o.friendlyName}, + "description": []string{o.description}, + } + + // Preferred lookup: derive the SubjectKeyID from the TPM-resident key + // and ask CAPI for a SKI-indexed cert lookup. This is exact (pins the + // cert to *this* key) and uses Windows' indexed search. + // + // Fallback: when the key isn't in the TPM — e.g. an unprotected + // endpoint whose container lives in the Microsoft Software Key + // Storage Provider — ask CAPI to find the cert by its + // CERT_KEY_PROV_INFO container name instead. CAPI's containerName + // branch handles both providers via its own GetPublicKey-with-fallback, + // so we don't need to know the provider here. + pub, pubErr := k.GetPublicKey(&apiv1.GetPublicKeyRequest{Name: req.Name}) + switch { + case pubErr == nil: + subjectKeyID, err := generateWindowsSubjectKeyID(pub) + if err != nil { + return nil, fmt.Errorf("failed generating subject key id: %w", err) + } + values.Set("key-id", subjectKeyID) + case errors.Is(pubErr, apiv1.NotFoundError{}): + if o.name == "" { + return nil, fmt.Errorf("failed retrieving public key: %w", pubErr) + } + values.Set("key", o.name) + default: + return nil, fmt.Errorf("failed retrieving public key: %w", pubErr) } return k.windowsCertificateManager.LoadCertificateChain(&apiv1.LoadCertificateChainRequest{ - Name: uri.New("capi", url.Values{ - "key-id": []string{subjectKeyID}, - "store-location": []string{location}, - "store": []string{store}, - "intermediate-store-location": []string{intermediateCAStoreLocation}, - "intermediate-store": []string{intermediateCAStore}, - "issuer": []string{o.issuer}, - "friendly-name": []string{o.friendlyName}, - "description": []string{o.description}, - }).String(), + Name: uri.New("capi", values).String(), }) } @@ -1105,11 +1121,19 @@ func (k *TPMKMS) deleteCertificateFromWindowsCertificateStore(req *apiv1.DeleteC case o.sha1 != "": uv.Set("sha1", o.sha1) case o.name != "": + // Try the SKI-indexed lookup first (works for TPM-resident keys). + // Fall back to telling CAPI to find the cert by its + // CERT_KEY_PROV_INFO container name when the key isn't in the + // TPM — that's the unprotected/Software-KSP case. keyID, err := k.getSubjectKeyID(req.Name) - if err != nil { + switch { + case err == nil: + uv.Set("key-id", hex.EncodeToString(keyID)) + case errors.Is(err, apiv1.NotFoundError{}): + uv.Set("key", o.name) + default: return fmt.Errorf("error getting key-id: %w", err) } - uv.Set("key-id", hex.EncodeToString(keyID)) default: return errors.New(`at least one of "serial", "key-id", "sha1" or "name" is expected to be set`) } diff --git a/kms/tpmkms/tpmkms_windows_test.go b/kms/tpmkms/tpmkms_windows_test.go index 7f57ab82..b01b1ee7 100644 --- a/kms/tpmkms/tpmkms_windows_test.go +++ b/kms/tpmkms/tpmkms_windows_test.go @@ -4,8 +4,13 @@ package tpmkms import ( "context" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "math/big" "net/url" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -13,7 +18,10 @@ import ( "go.step.sm/crypto/kms/apiv1" "go.step.sm/crypto/kms/capi" "go.step.sm/crypto/kms/uri" + "go.step.sm/crypto/randutil" "go.step.sm/crypto/tpm" + "go.step.sm/crypto/tpm/storage" + "go.step.sm/crypto/x509util" ) func TestNew_windows(t *testing.T) { @@ -183,3 +191,182 @@ func TestNewWithTPM_windows(t *testing.T) { }) } } + +// newPCPCertManager constructs the same kind of CAPIKMS-bound-to-PCP cert +// manager that TPMKMS attaches as its windowsCertificateManager when +// enable-cng=true. Returning the underlying CAPIKMS lets tests reach into +// it directly to set up the Software-KSP fixtures the TPMKMS-side fallback +// is expected to find. +func newPCPCertManager(t *testing.T) apiv1.KeyManager { + t.Helper() + km, err := capi.New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New("capi", url.Values{"provider": []string{microsoftPCP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = km.Close() }) + + apiv1.Register(apiv1.CAPIKMS, func(context.Context, apiv1.Options) (apiv1.KeyManager, error) { + return km, nil + }) + return km +} + +// makeSoftwareKSPKeyAndCert mirrors the helper in capi's tests: it creates +// a Software-KSP CNG key in CurrentUser, signs a short-lived cert with it, +// stores the cert in CurrentUser\My, and binds the cert to the key. Used to +// reproduce the unprotected-endpoint configuration the agent emits today. +func makeSoftwareKSPKeyAndCert(t *testing.T, subject string) (containerName string) { + t.Helper() + + suffix, err := randutil.Hex(8) + require.NoError(t, err) + containerName = "step-tpmkms-test-" + suffix + + km, err := capi.New(t.Context(), apiv1.Options{ + Type: apiv1.CAPIKMS, + URI: uri.New("capi", url.Values{"provider": []string{capi.ProviderMSKSP}}).String(), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = km.Close() }) + + created, err := km.CreateKey(&apiv1.CreateKeyRequest{ + Name: uri.New("capi", url.Values{"provider": []string{capi.ProviderMSKSP}, "key": []string{containerName}}).String(), + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 2048, + }) + require.NoError(t, err) + + signer, err := km.CreateSigner(&created.CreateSignerRequest) + require.NoError(t, err) + + // Populate SubjectKeyId so the SKI-indexed branch in CAPI can find + // this cert; x509.CreateCertificate doesn't auto-populate it on + // leaf certs. + ski, err := x509util.GenerateSubjectKeyID(signer.Public()) + require.NoError(t, err) + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(time.Now().UnixNano()), + Subject: pkix.Name{CommonName: subject}, + Issuer: pkix.Name{CommonName: subject}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + SubjectKeyId: ski, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, signer.Public(), signer) + require.NoError(t, err) + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + + require.NoError(t, km.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: uri.New("capi", url.Values{ + "store-location": []string{"user"}, + "store": []string{"My"}, + }).String(), + Certificate: cert, + })) + + t.Cleanup(func() { + // Best-effort: tests that delete via TPMKMS will already have + // removed the cert; this cleans up the key, and removes the cert + // if the test didn't delete it itself. + _ = km.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: uri.New("capi", url.Values{ + "key": []string{containerName}, + "store-location": []string{"user"}, + "store": []string{"My"}, + }).String(), + }) + _ = km.DeleteKey(&apiv1.DeleteKeyRequest{ + Name: uri.New("capi", url.Values{ + "provider": []string{capi.ProviderMSKSP}, + "key": []string{containerName}, + }).String(), + }) + }) + + return containerName +} + +// newTPMKMSForFallback builds a TPMKMS shaped the way the agent's +// reloadsrv wires it on Windows: enable-cng=true, user/My defaults, a +// real TPM with an empty per-test storage directory (so GetPublicKey on +// a Software-KSP container resolves to tpm.ErrNotFound and drives the +// fallback path — without storage the call returns "no storage +// configured", which does NOT satisfy errors.Is(err, NotFoundError{}) +// and therefore wouldn't reach the fallback in production), and a +// PCP-bound CAPIKMS as the windowsCertificateManager. +func newTPMKMSForFallback(t *testing.T) *TPMKMS { + t.Helper() + tp, err := tpm.New(tpm.WithStore(storage.NewDirstore(t.TempDir()))) + require.NoError(t, err) + + cm := newPCPCertManager(t) + return &TPMKMS{ + tpm: tp, + opts: &options{ + windowsCNG: true, + windowsCertificateStore: "My", + windowsCertificateStoreLocation: "user", + windowsIntermediateStore: defaultIntermediateStore, + windowsIntermediateStoreLocation: defaultIntermediateStoreLocation, + }, + windowsCertificateManager: cm.(capiCertificateManager), + } +} + +// TestLoadCertificateChain_softwareKSP_fallback exercises the scenario from +// the agent's renewal cleanup: a TPMKMS configured with enable-cng=true is +// asked to load a cert by name, but the name refers to a Software-KSP +// container instead of a TPM key. Pre-fix, TPMKMS.GetPublicKey returned +// NotFoundError and the cert was never found. With the fallback, TPMKMS +// hands the container name to CAPI, which enumerates by KeyProvInfo. +func TestLoadCertificateChain_softwareKSP_fallback(t *testing.T) { + k := newTPMKMSForFallback(t) + + container := makeSoftwareKSPKeyAndCert(t, "tpmkms-fallback-load") + + chain, err := k.LoadCertificateChain(&apiv1.LoadCertificateChainRequest{ + Name: uri.New(Scheme, url.Values{ + "name": []string{container}, + "store-location": []string{"user"}, + "store": []string{"My"}, + }).String(), + }) + require.NoError(t, err) + require.NotEmpty(t, chain) + assert.Equal(t, "tpmkms-fallback-load", chain[0].Subject.CommonName) +} + +// TestDeleteCertificate_softwareKSP_fallback is the delete-by-name version +// of the load test. This is the call the agent's storeCertificateChain +// wrapper makes in its deferred cleanup; without this path working, +// renewals leak one duplicate per cycle on unprotected endpoints. +func TestDeleteCertificate_softwareKSP_fallback(t *testing.T) { + k := newTPMKMSForFallback(t) + + container := makeSoftwareKSPKeyAndCert(t, "tpmkms-fallback-delete") + + require.NoError(t, k.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: uri.New(Scheme, url.Values{ + "name": []string{container}, + "store-location": []string{"user"}, + "store": []string{"My"}, + }).String(), + })) + + // Second load must miss — proves the fallback delete actually + // removed the cert from the store and we don't return the same one + // repeatedly. + _, err := k.LoadCertificateChain(&apiv1.LoadCertificateChainRequest{ + Name: uri.New(Scheme, url.Values{ + "name": []string{container}, + "store-location": []string{"user"}, + "store": []string{"My"}, + }).String(), + }) + assert.ErrorIs(t, err, apiv1.NotFoundError{}) +}