From 2cb67ecd025adcb719967fb8e5ad0032f8459c4b Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Fri, 8 May 2026 18:45:52 -0500 Subject: [PATCH 1/2] feat: add per-key key-scope option for Windows machine vs user key store Introduces a key-scope=machine|user URI parameter on tpmkms that controls whether the underlying private key lives in the local machine key store (NCRYPT_MACHINE_KEY_FLAG) or in the current user's key store. The parameter is independent of store-location, which controls cert location, so callers can request machine-store certs paired with user-owned keys (or vice versa) for security-hardening scenarios such as a service whose cert needs machine visibility but whose key should only be openable by the service identity. Architecture: - MachineKey is a per-key concern, not a per-TPM-instance one. Threaded through CreateKeyConfig, AttestKeyConfig, and CreateAKConfig, then via context into TPM.open() which sets attestConfig.MachineKey under the lock just before attest.OpenTPM. Persisted in storage so reload after restart uses the matching scope without the URI carrying it. - tpmkms.CreateKey now preserves key-scope=machine in the returned URI so downstream CreateSigner/DeleteKey calls don't lose scope. - AttestKey reads the AK's persisted scope and uses it for the attest session; the new key inherits the AK's scope. Conflicting explicit AttestKeyConfig.MachineKey is rejected with a clear error. - Defaults for back-compat: when key-scope is unset, derive from store-location (machine cert -> machine key) so existing URIs that happened to set store-location=machine continue to work. Also fixes related delete-path issues: - DeleteKey/DeleteAK now always clean up the file-store entry even if the in-TPM (NCrypt) deletion fails, preventing repeated retries from re-encountering the same failing entry. - Returns a typed *PartialDeleteError when the in-TPM deletion failed but the file-store cleanup succeeded, so callers can distinguish that case from a complete failure. Bumps github.com/smallstep/go-attestation to v0.4.9, which adds the OpenConfig.MachineKey field this change drives. Co-Authored-By: Claude Opus 4.7 (1M context) --- go.mod | 2 +- go.sum | 4 +- kms/tpmkms/tpmkms.go | 15 ++++- kms/tpmkms/uri.go | 32 ++++++++++ tpm/ak.go | 95 +++++++++++++++++++--------- tpm/context.go | 18 ++++++ tpm/errors.go | 24 +++++++ tpm/internal/key/key.go | 10 ++- tpm/internal/key/key_windows.go | 6 +- tpm/internal/key/pcp_windows.go | 21 ++++-- tpm/key.go | 109 ++++++++++++++++++++++---------- tpm/signer.go | 25 ++++---- tpm/storage/types.go | 34 +++++++--- tpm/tpm.go | 6 ++ 14 files changed, 304 insertions(+), 97 deletions(-) diff --git a/go.mod b/go.mod index 7bc45f9f..d712f006 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/peterbourgon/diskv/v3 v3.0.1 github.com/pkg/errors v0.9.1 github.com/schollz/jsonstore v1.1.0 - github.com/smallstep/go-attestation v0.4.4-0.20241119153605-2306d5b464ca + github.com/smallstep/go-attestation v0.4.9 github.com/stretchr/testify v1.11.1 go.uber.org/mock v0.6.0 golang.org/x/crypto v0.50.0 diff --git a/go.sum b/go.sum index 97fa5dd7..5724597d 100644 --- a/go.sum +++ b/go.sum @@ -758,8 +758,8 @@ github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPx github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE= github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88= github.com/sirupsen/logrus v1.7.0/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0= -github.com/smallstep/go-attestation v0.4.4-0.20241119153605-2306d5b464ca h1:VX8L0r8vybH0bPeaIxh4NQzafKQiqvlOn8pmOXbFLO4= -github.com/smallstep/go-attestation v0.4.4-0.20241119153605-2306d5b464ca/go.mod h1:vNAduivU014fubg6ewygkAvQC0IQVXqdc8vaGl/0er4= +github.com/smallstep/go-attestation v0.4.9 h1:/fVmzB8A8tk7B2PCGPlszWzyl/GDcEQbynqcg2PMaZ0= +github.com/smallstep/go-attestation v0.4.9/go.mod h1:vNAduivU014fubg6ewygkAvQC0IQVXqdc8vaGl/0er4= github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc= github.com/smartystreets/assertions v1.0.0/go.mod h1:kHHU4qYBaI3q23Pp3VPrmWhuIUrLW/7eUrw0BU5VaoM= github.com/smartystreets/go-aws-auth v0.0.0-20180515143844-0c1422d1fdb9/go.mod h1:SnhjPscd9TpLiy1LpzGSKh3bXCfxxXuqd9xmQJy3slM= diff --git a/kms/tpmkms/tpmkms.go b/kms/tpmkms/tpmkms.go index 14a1efb7..7dcf5ee0 100644 --- a/kms/tpmkms/tpmkms.go +++ b/kms/tpmkms/tpmkms.go @@ -557,12 +557,15 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons }, nil } + machineKey := properties.machineKey() + var key *tpm.Key if properties.attestBy != "" { config := tpm.AttestKeyConfig{ Algorithm: v.Type, Size: size, QualifyingData: properties.qualifyingData, + MachineKey: machineKey, } key, err = k.tpm.AttestKey(ctx, properties.attestBy, properties.name, config) if err != nil { @@ -573,8 +576,9 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons } } else { config := tpm.CreateKeyConfig{ - Algorithm: v.Type, - Size: size, + Algorithm: v.Type, + Size: size, + MachineKey: machineKey, } key, err = k.tpm.CreateKey(ctx, properties.name, config) if err != nil { @@ -598,10 +602,17 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons return nil, fmt.Errorf("failed getting signer for key: %w", err) } + // Preserve key-scope in the returned URI so subsequent CreateSigner / + // DeleteKey calls land in the right scope. The bug we hit before was + // exactly this: the returned URI had only "name=", so re-opens + // defaulted to user scope and failed to find machine-stored keys. createdKeyURI := fmt.Sprintf("tpmkms:name=%s", key.Name()) if properties.attestBy != "" { createdKeyURI = fmt.Sprintf("%s;attest-by=%s", createdKeyURI, key.AttestedBy()) } + if machineKey { + createdKeyURI = fmt.Sprintf("%s;key-scope=machine", createdKeyURI) + } return &apiv1.CreateKeyResponse{ Name: createdKeyURI, diff --git a/kms/tpmkms/uri.go b/kms/tpmkms/uri.go index 567f1e1e..1b2f4105 100644 --- a/kms/tpmkms/uri.go +++ b/kms/tpmkms/uri.go @@ -27,6 +27,28 @@ type objectProperties struct { sha1 string serial string issuer string + // keyScope, if set, is one of "machine" or "user". It controls + // whether the underlying private key lives in the local machine key + // store or in the current user's key store, and is independent of + // where the certificate is stored. When unset, [parseNameURI] derives + // it from storeLocation for backwards compatibility (machine cert + // store implies machine key scope). + keyScope string +} + +// machineKey returns true if the resolved key scope is "machine". +// When keyScope is unset on the object, the value is derived from +// storeLocation: "machine" → true, anything else → false. +func (o objectProperties) machineKey() bool { + scope := o.keyScope + if scope == "" { + if o.storeLocation == "machine" { + scope = "machine" + } else { + scope = "user" + } + } + return scope == "machine" } func parseNameURI(nameURI string) (o objectProperties, err error) { @@ -75,10 +97,20 @@ func parseNameURI(nameURI string) (o objectProperties, err error) { o.serial = u.Get("serial") o.issuer = u.Get("issuer") + // key-scope is independent of store-location: cert location and + // key ownership are orthogonal Windows concepts. See [machineKey] + // for the back-compat default when this is unset. + o.keyScope = u.Get("key-scope") + // validation if o.ak && o.attestBy != "" { return o, errors.New(`"ak" and "attest-by" are mutually exclusive`) } + switch o.keyScope { + case "", "machine", "user": + default: + return o, fmt.Errorf(`"key-scope" must be "machine" or "user", got %q`, o.keyScope) + } return } diff --git a/tpm/ak.go b/tpm/ak.go index 82903b51..af6dea3b 100644 --- a/tpm/ak.go +++ b/tpm/ak.go @@ -26,11 +26,20 @@ type AK struct { data []byte chain []*x509.Certificate createdAt time.Time + machineKey bool blobs *Blobs attestParams *attest.AttestationParameters tpm *TPM } +// CreateAKConfig is used to pass configuration when creating AKs. +type CreateAKConfig struct { + // MachineKey, when true, requests that the AK be created in the local + // machine key store rather than in the current user's key store. See + // [CreateKeyConfig.MachineKey] for details. + MachineKey bool +} + // Name returns the AK name. The name uniquely // identifies an AK if a TPM with persistent // storage is used. @@ -133,8 +142,16 @@ func (ak *AK) MarshalJSON() ([]byte, error) { // CreateAK creates and stores a new AK identified by `name`. // If no name is provided, a random 10 character name is generated. // If an AK with the same name exists, `ErrExists` is returned. +// +// To request a machine-scoped AK on Windows, use [TPM.CreateAKWithConfig]. func (t *TPM) CreateAK(ctx context.Context, name string) (ak *AK, err error) { - if err = t.open(ctx); err != nil { + return t.CreateAKWithConfig(ctx, name, CreateAKConfig{}) +} + +// CreateAKWithConfig creates and stores a new AK with the given config. +// See [TPM.CreateAK] for the simpler signature. +func (t *TPM) CreateAKWithConfig(ctx context.Context, name string, config CreateAKConfig) (ak *AK, err error) { + if err = t.open(withMachineKey(ctx, config.MachineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -167,10 +184,11 @@ func (t *TPM) CreateAK(ctx context.Context, name string) (ak *AK, err error) { } ak = &AK{ - name: name, - data: data, - createdAt: now, - tpm: t, + name: name, + data: data, + createdAt: now, + machineKey: config.MachineKey, + tpm: t, } if err := t.store.AddAK(ak.toStorage()); err != nil { @@ -259,19 +277,25 @@ func (t *TPM) ListAKs(ctx context.Context) (aks []*AK, err error) { // DeleteAK removes the AK identified by `name`. It returns `ErrNotfound` // if it doesn't exist. Keys that were attested by the AK have to be removed // before removing the AK, otherwise an error will be returned. +// +// If the in-TPM deletion fails but the file-store entry is successfully +// removed, the returned error is wrapped in a [PartialDeleteError]. See +// [TPM.DeleteKey] for the rationale. func (t *TPM) DeleteAK(ctx context.Context, name string) (err error) { - if err := t.open(ctx); err != nil { - return fmt.Errorf("failed opening TPM: %w", err) - } - defer closeTPM(ctx, t, &err) - - ak, err := t.store.GetAK(name) - if err != nil { - if errors.Is(err, storage.ErrNotFound) { + // Look up first so we know the AK's machine-key scope before opening + // attest with the right flag. + ak, getErr := t.store.GetAK(name) + if getErr != nil { + if errors.Is(getErr, storage.ErrNotFound) { return fmt.Errorf("failed getting AK %q: %w", name, ErrNotFound) } - return fmt.Errorf("failed getting AK %q: %w", name, err) + return fmt.Errorf("failed getting AK %q: %w", name, getErr) + } + + if err := t.open(withMachineKey(ctx, ak.MachineKey)); err != nil { + return fmt.Errorf("failed opening TPM: %w", err) } + defer closeTPM(ctx, t, &err) // prevent deleting the AK if the TPM (storage) contains keys that // were attested by it. While keys would still work if the AK were @@ -286,19 +310,26 @@ func (t *TPM) DeleteAK(ctx context.Context, name string) (err error) { return fmt.Errorf("failed deleting AK %q because %d key(s) exist that were attested by it", name, len(keys)) } - if err := t.attestTPM.DeleteKey(ak.Data); err != nil { // TODO: we could add a DeleteAK to go-attestation; under the hood it's loaded the same as a key though. - return fmt.Errorf("failed deleting AK %q: %w", name, err) - } + attestErr := t.attestTPM.DeleteKey(ak.Data) // TODO: we could add a DeleteAK to go-attestation; under the hood it's loaded the same as a key though. if err := t.store.DeleteAK(name); err != nil { + if attestErr != nil { + return fmt.Errorf("failed deleting AK %q from storage after TPM delete failed (%v): %w", name, attestErr, err) + } return fmt.Errorf("failed deleting AK %q from storage: %w", name, err) } if err := t.store.Persist(); err != nil { + if attestErr != nil { + return fmt.Errorf("failed persisting storage after TPM delete failed (%v): %w", attestErr, err) + } return fmt.Errorf("failed persisting storage: %w", err) } - return + if attestErr != nil { + return &PartialDeleteError{Name: name, Underlying: attestErr} + } + return nil } // AttestationParameters returns information about the AK, typically used to @@ -308,7 +339,7 @@ func (ak *AK) AttestationParameters(ctx context.Context) (params attest.Attestat return *ak.attestParams, nil } - if err = ak.tpm.open(ctx); err != nil { + if err = ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { return params, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -333,7 +364,7 @@ type EncryptedCredential attest.EncryptedCredential // generated on the same TPM as the EK. This operation is synonymous with // TPM2_ActivateCredential. func (ak *AK) ActivateCredential(ctx context.Context, in EncryptedCredential) (secret []byte, err error) { - if err := ak.tpm.open(ctx); err != nil { + if err := ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { return secret, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -359,7 +390,7 @@ func (ak *AK) Blobs(ctx context.Context) (blobs *Blobs, err error) { return ak.blobs, nil } - if err = ak.tpm.open(ctx); err != nil { + if err = ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -383,7 +414,7 @@ func (ak *AK) Blobs(ctx context.Context) (blobs *Blobs, err error) { // If the AK public key doesn't match the public key in the first certificate // in the chain (the leaf), an error is returned. func (ak *AK) SetCertificateChain(ctx context.Context, chain []*x509.Certificate) (err error) { - if err := ak.tpm.open(ctx); err != nil { + if err := ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -463,10 +494,11 @@ func (ak *AK) HasValidPermanentIdentifier(permanentIdentifier string) bool { // persisting AKs. func (ak *AK) toStorage() *storage.AK { return &storage.AK{ - Name: ak.name, - Data: ak.data, - Chain: ak.chain, - CreatedAt: ak.createdAt.UTC(), + Name: ak.name, + Data: ak.data, + Chain: ak.chain, + CreatedAt: ak.createdAt.UTC(), + MachineKey: ak.machineKey, } } @@ -474,10 +506,11 @@ func (ak *AK) toStorage() *storage.AK { // persisting AKs. func akFromStorage(sak *storage.AK, t *TPM) *AK { return &AK{ - name: sak.Name, - data: sak.Data, - chain: sak.Chain, - createdAt: sak.CreatedAt.Local(), - tpm: t, + name: sak.Name, + data: sak.Data, + chain: sak.Chain, + createdAt: sak.CreatedAt.Local(), + machineKey: sak.MachineKey, + tpm: t, } } diff --git a/tpm/context.go b/tpm/context.go index d8625921..3c1b3b44 100644 --- a/tpm/context.go +++ b/tpm/context.go @@ -37,3 +37,21 @@ func isGoTPMCall(ctx context.Context) bool { v, ok := ctx.Value(goTPMCallContextKey{}).(bool) return ok && v } + +type machineKeyContextKey struct{} + +// withMachineKey annotates ctx with a machine-key flag that the TPM open +// path consults when initializing the underlying attest.TPM. This lets +// individual key operations (CreateKey, GetKey/Public/Signer/..., +// DeleteKey) request machine-scoped vs user-scoped behaviour without +// turning MachineKey into a per-TPM-instance setting. +func withMachineKey(ctx context.Context, machineKey bool) context.Context { + return context.WithValue(ctx, machineKeyContextKey{}, machineKey) +} + +// machineKeyFromContext returns the machine-key flag annotated on ctx, or +// false if none is set. +func machineKeyFromContext(ctx context.Context) bool { + v, _ := ctx.Value(machineKeyContextKey{}).(bool) + return v +} diff --git a/tpm/errors.go b/tpm/errors.go index 08c6b66f..fbb5bac0 100644 --- a/tpm/errors.go +++ b/tpm/errors.go @@ -2,6 +2,7 @@ package tpm import ( "errors" + "fmt" "go.step.sm/crypto/tpm/storage" ) @@ -15,3 +16,26 @@ var ErrExists = errors.New("already exists") // ErrNoStorageConfigured is returned when a TPM operation is // performed that requires a storage to have been configured var ErrNoStorageConfigured = storage.ErrNoStorageConfigured + +// PartialDeleteError is returned by DeleteKey or DeleteAK when the +// in-TPM (NCrypt PCP / attest) deletion failed but the file-store +// entry was successfully removed. Callers can use [errors.As] to +// detect this case and decide whether to treat it as a transient +// failure (the named entry is gone from local bookkeeping; the +// underlying TPM key may or may not still exist). +// +// Cleaning up the file-store entry even on PCP failure prevents +// repeated retries from re-encountering the same failing entry, +// which is the failure mode that motivated this type. +type PartialDeleteError struct { + // Name is the key/AK name that was being deleted. + Name string + // Underlying is the error returned by the in-TPM deletion. + Underlying error +} + +func (e *PartialDeleteError) Error() string { + return fmt.Sprintf("file-store entry %q removed but TPM deletion failed: %v", e.Name, e.Underlying) +} + +func (e *PartialDeleteError) Unwrap() error { return e.Underlying } diff --git a/tpm/internal/key/key.go b/tpm/internal/key/key.go index cd9141d1..90569244 100644 --- a/tpm/internal/key/key.go +++ b/tpm/internal/key/key.go @@ -106,6 +106,11 @@ type CreateConfig struct { // Size is used to specify the bit size of the key or elliptic curve. For // example, '256' is used to specify curve P-256. Size int + // MachineKey, when true, requests that the key be created in the local + // machine key store rather than the current user key store. On Windows + // this causes NCRYPT_MACHINE_KEY_FLAG to be passed to the underlying + // NCrypt PCP provider. Ignored on other platforms. + MachineKey bool } func (c *CreateConfig) Validate() error { @@ -184,8 +189,9 @@ const ( ) type KeyConfig struct { - Algorithm Algorithm - Size int + Algorithm Algorithm + Size int + MachineKey bool } var ( diff --git a/tpm/internal/key/key_windows.go b/tpm/internal/key/key_windows.go index 65636a6d..13e86752 100644 --- a/tpm/internal/key/key_windows.go +++ b/tpm/internal/key/key_windows.go @@ -14,7 +14,11 @@ func create(_ io.ReadWriteCloser, keyName string, config CreateConfig) ([]byte, } defer pcp.Close() - _, pub, _, err := pcp.NewKey(keyName, &KeyConfig{Algorithm: Algorithm(config.Algorithm), Size: config.Size}) + _, pub, _, err := pcp.NewKey(keyName, &KeyConfig{ + Algorithm: Algorithm(config.Algorithm), + Size: config.Size, + MachineKey: config.MachineKey, + }) if err != nil { return nil, fmt.Errorf("pcp failed to mint application key: %w", err) } diff --git a/tpm/internal/key/pcp_windows.go b/tpm/internal/key/pcp_windows.go index 3626d9a6..af363069 100644 --- a/tpm/internal/key/pcp_windows.go +++ b/tpm/internal/key/pcp_windows.go @@ -35,6 +35,10 @@ const ( // The below is documented in this Microsoft whitepaper: // https://github.com/Microsoft/TSS.MSR/blob/master/PCPTool.v11/Using%20the%20Windows%208%20Platform%20Crypto%20Provider%20and%20Associated%20TPM%20Functionality.pdf ncryptOverwriteKeyFlag = 0x80 + // ncryptMachineKeyFlag instructs NCrypt to create or open the key in the + // local machine key store rather than the current user key store. Defined + // in ncrypt.h as NCRYPT_MACHINE_KEY_FLAG. + ncryptMachineKeyFlag uint32 = 0x00000020 // Key usage value for generic keys nCryptPropertyPCPKeyUsagePolicyGeneric = 0x3 // Key usage value for AKs. @@ -290,7 +294,7 @@ func (h *winPCP) Close() error { return closeNCryptObject(h.hProv) } -func (h *winPCP) newKey(name string, alg string, length uint32, policy uint32) (uintptr, []byte, []byte, error) { +func (h *winPCP) newKey(name string, alg string, length uint32, policy uint32, machineKey bool) (uintptr, []byte, []byte, error) { var kh uintptr utf16Name, err := windows.UTF16FromString(name) if err != nil { @@ -301,8 +305,13 @@ func (h *winPCP) newKey(name string, alg string, length uint32, policy uint32) ( return 0, nil, nil, err } + var flags uint32 + if machineKey { + flags = ncryptMachineKeyFlag + } + // Create a persistent RSA key of the specified name. - r, _, msg := nCryptCreatePersistedKey.Call(h.hProv, uintptr(unsafe.Pointer(&kh)), uintptr(unsafe.Pointer(&utf16RSA[0])), uintptr(unsafe.Pointer(&utf16Name[0])), 0, 0) + r, _, msg := nCryptCreatePersistedKey.Call(h.hProv, uintptr(unsafe.Pointer(&kh)), uintptr(unsafe.Pointer(&utf16RSA[0])), uintptr(unsafe.Pointer(&utf16Name[0])), 0, uintptr(flags)) if r != 0 { if tpmErr := maybeWinErr(r); tpmErr != nil { msg = tpmErr @@ -382,15 +391,15 @@ func (h *winPCP) newKey(name string, alg string, length uint32, policy uint32) ( // NewKey creates a persistent application key of the specified name. func (h *winPCP) NewKey(name string, config *KeyConfig) (uintptr, []byte, []byte, error) { if config.Algorithm == RSA { - return h.newKey(name, "RSA", uint32(config.Size), 0) + return h.newKey(name, "RSA", uint32(config.Size), 0, config.MachineKey) } else if config.Algorithm == ECDSA { switch config.Size { case 256: - return h.newKey(name, "ECDSA_P256", 0, 0) + return h.newKey(name, "ECDSA_P256", 0, 0, config.MachineKey) case 384: - return h.newKey(name, "ECDSA_P384", 0, 0) + return h.newKey(name, "ECDSA_P384", 0, 0, config.MachineKey) case 521: - return h.newKey(name, "ECDSA_P521", 0, 0) + return h.newKey(name, "ECDSA_P521", 0, 0, config.MachineKey) default: return 0, nil, nil, fmt.Errorf("unsupported ECDSA key size: %v", config.Size) } diff --git a/tpm/key.go b/tpm/key.go index b8c267b3..3ad7787d 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -25,6 +25,7 @@ type Key struct { attestedBy string chain []*x509.Certificate createdAt time.Time + machineKey bool blobs *Blobs tpm *TPM } @@ -69,7 +70,7 @@ func (k *Key) WasAttestedBy(ak *AK) bool { func (k *Key) Public() crypto.PublicKey { var ( err error - ctx = context.Background() + ctx = withMachineKey(context.Background(), k.machineKey) ) if err = k.tpm.open(ctx); err != nil { return nil @@ -141,6 +142,14 @@ type CreateKeyConfig struct { // Size is used to specify the bit size of the key or elliptic curve. For // example, '256' is used to specify curve P-256. Size int + // MachineKey, when true, requests that the underlying private key be + // created in the local machine key store rather than in the current + // user's key store. On Windows this causes NCRYPT_MACHINE_KEY_FLAG to + // be passed to the NCrypt PCP provider on creation, and to be applied + // every time the key is subsequently opened. The value is persisted + // alongside the key in storage so that load operations use the matching + // scope. Has no effect on non-Windows platforms. + MachineKey bool // TODO(hs): move key name to this struct? } @@ -159,6 +168,10 @@ type AttestKeyConfig struct { // When used with ACME `device-attest-01`, this contains a hash of // the key authorization. QualifyingData []byte + // MachineKey, when true, requests that the underlying private key be + // created in the local machine key store rather than in the current + // user's key store. See [CreateKeyConfig.MachineKey] for details. + MachineKey bool // TODO(hs): add akName and key name to this struct? } @@ -167,7 +180,7 @@ type AttestKeyConfig struct { // a random 10 character name is generated. If a Key with the same name exists, // `ErrExists` is returned. The Key won't be attested by an AK. func (t *TPM) CreateKey(ctx context.Context, name string, config CreateKeyConfig) (key *Key, err error) { - if err = t.open(goTPMCall(ctx)); err != nil { + if err = t.open(withMachineKey(goTPMCall(ctx), config.MachineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -186,8 +199,9 @@ func (t *TPM) CreateKey(ctx context.Context, name string, config CreateKeyConfig } createConfig := internalkey.CreateConfig{ - Algorithm: config.Algorithm, - Size: config.Size, + Algorithm: config.Algorithm, + Size: config.Size, + MachineKey: config.MachineKey, } if err := t.validate(&createConfig); err != nil { return nil, fmt.Errorf("invalid key creation parameters: %w", err) @@ -198,10 +212,11 @@ func (t *TPM) CreateKey(ctx context.Context, name string, config CreateKeyConfig } key = &Key{ - name: name, - data: data, - createdAt: now, - tpm: t, + name: name, + data: data, + createdAt: now, + machineKey: config.MachineKey, + tpm: t, } if err := t.store.AddKey(key.toStorage()); err != nil { @@ -236,7 +251,25 @@ func (w attestValidationWrapper) Validate() error { // name is generated. If a Key with the same name exists, `ErrExists` is // returned. func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestKeyConfig) (key *Key, err error) { - if err = t.open(ctx); err != nil { + // Look up the AK before opening attest so we can use its scope for the + // attest session. An attested key inherits its AK's scope: the attest + // session can only operate in one scope at a time, so the AK and the + // new key it certifies must share it. Any explicit + // [AttestKeyConfig.MachineKey] that conflicts with the AK's scope is + // rejected here rather than failing later inside attest. + ak, err := t.store.GetAK(akName) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, fmt.Errorf("failed getting AK %q: %w", akName, ErrNotFound) + } + return nil, fmt.Errorf("failed getting AK %q: %w", akName, err) + } + if config.MachineKey && !ak.MachineKey { + return nil, fmt.Errorf("cannot attest a machine-scope key with user-scope AK %q", akName) + } + machineKey := ak.MachineKey + + if err = t.open(withMachineKey(ctx, machineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -254,14 +287,6 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK return nil, fmt.Errorf("failed creating key %q: %w", name, err) } - ak, err := t.store.GetAK(akName) - if err != nil { - if errors.Is(err, storage.ErrNotFound) { - return nil, fmt.Errorf("failed getting AK %q: %w", akName, ErrNotFound) - } - return nil, fmt.Errorf("failed getting AK %q: %w", akName, err) - } - loadedAK, err := t.attestTPM.LoadAK(ak.Data) if err != nil { return nil, fmt.Errorf("failed loading AK %q: %w", akName, err) @@ -293,6 +318,7 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK data: data, attestedBy: akName, createdAt: now, + machineKey: machineKey, tpm: t, } @@ -372,33 +398,50 @@ func (t *TPM) GetKeysAttestedBy(ctx context.Context, akName string) (keys []*Key // DeleteKey removes the Key identified by `name`. It returns `ErrNotfound` // if it doesn't exist. +// +// If the in-TPM deletion fails but the file-store entry is successfully +// removed, the returned error is wrapped in a [PartialDeleteError] so +// callers can distinguish "leaked from the TPM but not from the file +// store" from a complete failure. Cleaning up the file-store entry even +// on PCP failure prevents repeated retries from re-encountering the same +// failing entry. func (t *TPM) DeleteKey(ctx context.Context, name string) (err error) { - if err := t.open(ctx); err != nil { - return fmt.Errorf("failed opening TPM: %w", err) - } - defer closeTPM(ctx, t, &err) - - key, err := t.store.GetKey(name) - if err != nil { - if errors.Is(err, storage.ErrNotFound) { + // Read storage first so we know the key's machine-key scope before + // opening attest with the right flag. Cheaper than doing it after + // open() because the store load is unconditional anyway. + key, getErr := t.store.GetKey(name) + if getErr != nil { + if errors.Is(getErr, storage.ErrNotFound) { return fmt.Errorf("failed getting key %q: %w", name, ErrNotFound) } - return fmt.Errorf("failed getting key %q: %w", name, err) + return fmt.Errorf("failed getting key %q: %w", name, getErr) } - if err := t.attestTPM.DeleteKey(key.Data); err != nil { - return fmt.Errorf("failed deleting key %q: %w", name, err) + if err := t.open(withMachineKey(ctx, key.MachineKey)); err != nil { + return fmt.Errorf("failed opening TPM: %w", err) } + defer closeTPM(ctx, t, &err) + + attestErr := t.attestTPM.DeleteKey(key.Data) if err := t.store.DeleteKey(name); err != nil { + if attestErr != nil { + return fmt.Errorf("failed deleting key %q from storage after TPM delete failed (%v): %w", name, attestErr, err) + } return fmt.Errorf("failed deleting key %q from storage: %w", name, err) } if err := t.store.Persist(); err != nil { + if attestErr != nil { + return fmt.Errorf("failed persisting storage after TPM delete failed (%v): %w", attestErr, err) + } return fmt.Errorf("failed persisting storage: %w", err) } - return + if attestErr != nil { + return &PartialDeleteError{Name: name, Underlying: attestErr} + } + return nil } // Signer returns a crypto.Signer backed by the Key. @@ -409,7 +452,7 @@ func (k *Key) Signer(ctx context.Context) (crypto.Signer, error) { // CertificationParameters returns information about the key that can be used to // verify key certification. func (k *Key) CertificationParameters(ctx context.Context) (params attest.CertificationParameters, err error) { - if err = k.tpm.open(ctx); err != nil { + if err = k.tpm.open(withMachineKey(ctx, k.machineKey)); err != nil { return params, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) @@ -435,7 +478,7 @@ func (k *Key) Blobs(ctx context.Context) (blobs *Blobs, err error) { return k.blobs, nil } - if err = k.tpm.open(ctx); err != nil { + if err = k.tpm.open(withMachineKey(ctx, k.machineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) @@ -459,7 +502,7 @@ func (k *Key) Blobs(ctx context.Context) (blobs *Blobs, err error) { // If the public key doesn't match the public key in the first certificate // in the chain (the leaf), an error is returned. func (k *Key) SetCertificateChain(ctx context.Context, chain []*x509.Certificate) (err error) { - if err = k.tpm.open(ctx); err != nil { + if err = k.tpm.open(withMachineKey(ctx, k.machineKey)); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) @@ -504,6 +547,7 @@ func (k *Key) toStorage() *storage.Key { AttestedBy: k.attestedBy, Chain: k.chain, CreatedAt: k.createdAt.UTC(), + MachineKey: k.machineKey, } } @@ -516,6 +560,7 @@ func keyFromStorage(sk *storage.Key, t *TPM) *Key { attestedBy: sk.AttestedBy, chain: sk.Chain, createdAt: sk.CreatedAt.Local(), + machineKey: sk.MachineKey, tpm: t, } } diff --git a/tpm/signer.go b/tpm/signer.go index 5b992e2a..b0fad108 100644 --- a/tpm/signer.go +++ b/tpm/signer.go @@ -27,7 +27,7 @@ func (s *signer) Public() crypto.PublicKey { // The TPM key is loaded lazily, meaning that every call to Sign() // will reload the TPM key to be used. func (s *signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) (signature []byte, err error) { - ctx := context.Background() + ctx := withMachineKey(context.Background(), s.key.machineKey) if err = s.tpm.open(ctx); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } @@ -55,19 +55,22 @@ func (s *signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) (si // GetSigner returns a crypto.Signer for a TPM Key identified by `name`. func (t *TPM) GetSigner(ctx context.Context, name string) (csigner crypto.Signer, err error) { - if err = t.open(ctx); err != nil { - return nil, fmt.Errorf("failed opening TPM: %w", err) - } - defer closeTPM(ctx, t, &err) - - key, err := t.store.GetKey(name) - if err != nil { - if errors.Is(err, storage.ErrNotFound) { + // Look up the key in the store first so we know its machine-key + // scope before opening attest. The store load is unconditional in + // open() anyway, so there's no extra round trip here. + key, getErr := t.store.GetKey(name) + if getErr != nil { + if errors.Is(getErr, storage.ErrNotFound) { return nil, fmt.Errorf("failed getting signer for key %q: %w", name, ErrNotFound) } - return nil, fmt.Errorf("failed getting signer for key %q: %w", name, err) + return nil, fmt.Errorf("failed getting signer for key %q: %w", name, getErr) } + if err = t.open(withMachineKey(ctx, key.MachineKey)); err != nil { + return nil, fmt.Errorf("failed opening TPM: %w", err) + } + defer closeTPM(ctx, t, &err) + loadedKey, err := t.attestTPM.LoadKey(key.Data) if err != nil { return nil, err @@ -85,7 +88,7 @@ func (t *TPM) GetSigner(ctx context.Context, name string) (csigner crypto.Signer csigner = &signer{ tpm: t, - key: Key{name: name, data: key.Data, attestedBy: key.AttestedBy, createdAt: key.CreatedAt, tpm: t}, + key: Key{name: name, data: key.Data, attestedBy: key.AttestedBy, createdAt: key.CreatedAt, machineKey: key.MachineKey, tpm: t}, public: loadedKey.Public(), } diff --git a/tpm/storage/types.go b/tpm/storage/types.go index b3fd5c5c..587bdef4 100644 --- a/tpm/storage/types.go +++ b/tpm/storage/types.go @@ -13,6 +13,11 @@ type AK struct { Data []byte Chain []*x509.Certificate CreatedAt time.Time + // MachineKey records whether the AK was created in the local machine + // key store (Windows: NCRYPT_MACHINE_KEY_FLAG). Persisted so that + // reload uses the matching key scope. Defaults to false for AKs + // created before this field was introduced. + MachineKey bool } // MarshalJSON marshals the AK into JSON. @@ -23,10 +28,11 @@ func (ak *AK) MarshalJSON() ([]byte, error) { } sak := serializedAK{ - Name: ak.Name, - Type: typeAK, - Data: ak.Data, - CreatedAt: ak.CreatedAt, + Name: ak.Name, + Type: typeAK, + Data: ak.Data, + CreatedAt: ak.CreatedAt, + MachineKey: ak.MachineKey, } if len(chain) > 0 { @@ -50,6 +56,7 @@ func (ak *AK) UnmarshalJSON(data []byte) error { ak.Name = sak.Name ak.Data = sak.Data ak.CreatedAt = sak.CreatedAt + ak.MachineKey = sak.MachineKey if len(sak.Chain) > 0 { chain := make([]*x509.Certificate, len(sak.Chain)) @@ -73,6 +80,11 @@ type Key struct { AttestedBy string Chain []*x509.Certificate CreatedAt time.Time + // MachineKey records whether the Key was created in the local machine + // key store (Windows: NCRYPT_MACHINE_KEY_FLAG). Persisted so that + // reload uses the matching key scope. Defaults to false for Keys + // created before this field was introduced. + MachineKey bool } // MarshalJSON marshals the Key into JSON. @@ -88,6 +100,7 @@ func (key *Key) MarshalJSON() ([]byte, error) { Data: key.Data, AttestedBy: key.AttestedBy, CreatedAt: key.CreatedAt, + MachineKey: key.MachineKey, } if len(chain) > 0 { @@ -112,6 +125,7 @@ func (key *Key) UnmarshalJSON(data []byte) error { key.Data = sk.Data key.AttestedBy = sk.AttestedBy key.CreatedAt = sk.CreatedAt + key.MachineKey = sk.MachineKey if len(sk.Chain) > 0 { chain := make([]*x509.Certificate, len(sk.Chain)) @@ -143,11 +157,12 @@ const ( // serializedAK is the struct used when marshaling // a storage AK to JSON. type serializedAK struct { - Name string `json:"name"` - Type tpmObjectType `json:"type"` - Data []byte `json:"data"` - Chain [][]byte `json:"chain"` - CreatedAt time.Time `json:"createdAt"` + Name string `json:"name"` + Type tpmObjectType `json:"type"` + Data []byte `json:"data"` + Chain [][]byte `json:"chain"` + CreatedAt time.Time `json:"createdAt"` + MachineKey bool `json:"machineKey,omitempty"` } // serializedKey is the struct used when marshaling @@ -159,6 +174,7 @@ type serializedKey struct { AttestedBy string `json:"attestedBy"` Chain [][]byte `json:"chain"` CreatedAt time.Time `json:"createdAt"` + MachineKey bool `json:"machineKey,omitempty"` } // keyForAK returns the key to use when storing an AK. diff --git a/tpm/tpm.go b/tpm/tpm.go index bd0dde01..cb2d9b53 100644 --- a/tpm/tpm.go +++ b/tpm/tpm.go @@ -190,6 +190,12 @@ func (t *TPM) open(ctx context.Context) (err error) { return fmt.Errorf("failed initializing command channel: %w", err) } + // Apply per-call machine-key scope to the attest open config. We hold + // t.lock at this point and t.attestTPM is re-created on every open, so + // mutating attestConfig here is safe and does not leak across callers. + // Always set explicitly so we never carry a previous caller's choice. + t.attestConfig.MachineKey = machineKeyFromContext(ctx) + // if a simulator was set, use it as the backing TPM device. // The simulator is currently only used for testing. if t.simulator != nil { From 7601b37ac14e50b858b069337c37014a77f4e58e Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Fri, 8 May 2026 21:59:02 -0500 Subject: [PATCH 2/2] fix(tpm): make AttestKey scope-mismatch check symmetric Previously the check only fired when the attested key was requested as machine-scope while the AK was user-scope; the reverse case (AK machine-scope, attested key requested user-scope) silently inherited the AK's scope and produced a machine-scope key, which is surprising behaviour for a caller who explicitly asked for user. Reject both mismatch directions so the inheritance rule is loud, not silent. Documented in the error message that attested keys inherit the AK's scope and that callers must set AttestKeyConfig.MachineKey to match the AK's scope explicitly. Caught by the capi-diag attest-roundtrip matrix: the ak-machine+key-user_xfail row was producing a successful machine-scope attested key instead of erroring. Co-Authored-By: Claude Opus 4.7 (1M context) --- tpm/key.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tpm/key.go b/tpm/key.go index 3ad7787d..d6646940 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -264,8 +264,15 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK } return nil, fmt.Errorf("failed getting AK %q: %w", akName, err) } - if config.MachineKey && !ak.MachineKey { - return nil, fmt.Errorf("cannot attest a machine-scope key with user-scope AK %q", akName) + // An attested key inherits its AK's scope. Reject explicit + // [AttestKeyConfig.MachineKey] mismatches in either direction so + // callers don't get a silently-promoted (or silently-demoted) key. + // To opt into "use whatever scope the AK is in," leave + // AttestKeyConfig.MachineKey at its zero value AND make sure the AK + // is in user scope; for a machine-scope AK, set MachineKey: true + // explicitly. + if config.MachineKey != ak.MachineKey { + return nil, fmt.Errorf("AttestKeyConfig.MachineKey=%v does not match AK %q scope (MachineKey=%v); attested keys inherit their AK's scope", config.MachineKey, akName, ak.MachineKey) } machineKey := ak.MachineKey