From 2cb67ecd025adcb719967fb8e5ad0032f8459c4b Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Fri, 8 May 2026 18:45:52 -0500 Subject: [PATCH 01/11] 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 02/11] 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 From 5f2cf523eb0197acf9b79741b3d18d1aa6298d7e Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Tue, 12 May 2026 20:58:08 -0500 Subject: [PATCH 03/11] refactor(tpmkms): rename machineKey() to isMachineKey() and simplify Inlines the back-compat resolution into a single boolean expression so the rule reads top-to-bottom: explicit key-scope=machine wins; absent that, fall back to store-location=machine. The previous if/else chain made the back-compat branch look more important than it is. Co-Authored-By: Claude Opus 4.7 (1M context) --- kms/tpmkms/tpmkms.go | 2 +- kms/tpmkms/uri.go | 15 ++++----------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/kms/tpmkms/tpmkms.go b/kms/tpmkms/tpmkms.go index 7dcf5ee0..5a520748 100644 --- a/kms/tpmkms/tpmkms.go +++ b/kms/tpmkms/tpmkms.go @@ -557,7 +557,7 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons }, nil } - machineKey := properties.machineKey() + machineKey := properties.isMachineKey() var key *tpm.Key if properties.attestBy != "" { diff --git a/kms/tpmkms/uri.go b/kms/tpmkms/uri.go index 1b2f4105..26d191a6 100644 --- a/kms/tpmkms/uri.go +++ b/kms/tpmkms/uri.go @@ -36,19 +36,12 @@ type objectProperties struct { keyScope string } -// machineKey returns true if the resolved key scope is "machine". +// isMachineKey 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 (o objectProperties) isMachineKey() bool { + return o.keyScope == "machine" || + (o.keyScope == "" && o.storeLocation == "machine") } func parseNameURI(nameURI string) (o objectProperties, err error) { From c3de912e714a494b98078e44ce7cd982e68b7e7a Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Tue, 12 May 2026 21:07:59 -0500 Subject: [PATCH 04/11] fix(tpm): keep store reads after t.store.Load in scope-discovery paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Herman's review feedback on PR #1021: the original change moved t.store.GetAK / t.store.GetKey ahead of t.open so the call could discover the persisted MachineKey scope before opening attest. That inverted the invariant that every t.store read happens after a t.store.Load (which today only runs inside t.open). For the in-tree Dirstore this didn't matter, but other Storage implementations may return stale or empty data if read before Load. AttestKey: open with the caller's requested scope first, then look up the AK and validate. A scope mismatch produces the same error as before but after a wasted attest open in the error path — strictly correct. DeleteKey, DeleteAK, GetSigner: keep the read-before-open shape (they need the persisted scope to set MachineKey on context, and the caller doesn't supply it), but call t.store.Load explicitly first so the invariant holds. t.open also Loads under the lock; both Loads are idempotent for the storage implementations in this package. Co-Authored-By: Claude Opus 4.7 (1M context) --- tpm/ak.go | 9 ++++++-- tpm/key.go | 61 +++++++++++++++++++++++++++------------------------ tpm/signer.go | 10 ++++++--- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/tpm/ak.go b/tpm/ak.go index af6dea3b..39eb8128 100644 --- a/tpm/ak.go +++ b/tpm/ak.go @@ -282,8 +282,13 @@ func (t *TPM) ListAKs(ctx context.Context) (aks []*AK, err error) { // 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) { - // Look up first so we know the AK's machine-key scope before opening - // attest with the right flag. + // We need the AK's persisted machine-key scope before t.open so we + // can pass it through context. t.open also calls t.store.Load + // internally; calling it here explicitly preserves the invariant that + // every t.store read happens after a t.store.Load. + if err := t.store.Load(); err != nil { + return fmt.Errorf("failed loading from TPM storage: %w", err) + } ak, getErr := t.store.GetAK(name) if getErr != nil { if errors.Is(getErr, storage.ErrNotFound) { diff --git a/tpm/key.go b/tpm/key.go index d6646940..bc15f24a 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -251,32 +251,13 @@ 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) { - // 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) - } - // 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 - - if err = t.open(withMachineKey(ctx, machineKey)); err != nil { + // Open with the caller's requested scope. The AK's persisted scope is + // validated below — if it doesn't match, we return an error and the + // deferred close runs. Doing it this way keeps every t.store read + // after the t.store.Load that t.open performs, which matters for + // storage implementations whose in-memory state doesn't reflect + // on-disk state until Load is called. + if err = t.open(withMachineKey(ctx, config.MachineKey)); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -294,6 +275,23 @@ 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) + } + // 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. Reject explicit [AttestKeyConfig.MachineKey] + // mismatches in either direction so callers don't get a + // silently-promoted (or silently-demoted) key. + 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 + loadedAK, err := t.attestTPM.LoadAK(ak.Data) if err != nil { return nil, fmt.Errorf("failed loading AK %q: %w", akName, err) @@ -413,9 +411,14 @@ func (t *TPM) GetKeysAttestedBy(ctx context.Context, akName string) (keys []*Key // on PCP failure prevents repeated retries from re-encountering the same // failing entry. func (t *TPM) DeleteKey(ctx context.Context, name string) (err error) { - // 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. + // We need the key's persisted machine-key scope before t.open so we + // can pass it through context. t.open also calls t.store.Load + // internally; calling it here explicitly preserves the invariant that + // every t.store read happens after a t.store.Load. Both Loads are + // idempotent for the storage implementations in this package. + if err := t.store.Load(); err != nil { + return fmt.Errorf("failed loading from TPM storage: %w", err) + } key, getErr := t.store.GetKey(name) if getErr != nil { if errors.Is(getErr, storage.ErrNotFound) { diff --git a/tpm/signer.go b/tpm/signer.go index b0fad108..b5709e80 100644 --- a/tpm/signer.go +++ b/tpm/signer.go @@ -55,9 +55,13 @@ 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) { - // 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. + // We need the key's persisted machine-key scope before t.open so we + // can pass it through context. t.open also calls t.store.Load + // internally; calling it here explicitly preserves the invariant + // that every t.store read happens after a t.store.Load. + if err := t.store.Load(); err != nil { + return nil, fmt.Errorf("failed loading from TPM storage: %w", err) + } key, getErr := t.store.GetKey(name) if getErr != nil { if errors.Is(getErr, storage.ErrNotFound) { From 6f6a52e757ae5f19b7393feb386b8f7ce6b04b66 Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Tue, 2 Jun 2026 16:50:37 -0500 Subject: [PATCH 05/11] fix(tpmkms): honor key-scope when creating an AK CreateKey's ak=true branch called tpm.CreateAK, which uses the user-default scope, and ignored the resolved machineKey (computed only after the branch returned). So an AK requested with key-scope=machine was silently created in user scope (MachineKey=false). The attested-key path *does* pass MachineKey, so AttestKey's symmetric scope check then rejected every machine-scoped attested key by that AK: AttestKeyConfig.MachineKey=true does not match AK "step-agent-ak" scope (MachineKey=false); attested keys inherit their AK's scope This broke machine-scoped device attestation enrollment outright (seen in the smallstep agent on Windows running as LocalSystem). Create the AK with CreateAKWithConfig{MachineKey: machineKey} (machineKey is now computed before the branch), and preserve key-scope=machine in the returned AK URI so re-opens use the matching scope (mirroring the non-AK path). CreateAK is exactly CreateAKWithConfig with an empty config, so the user-scope path is unchanged. Adds a tpmsimulator regression test reproducing the agent's enrollment path (machine AK -> machine attested key); it fails without this fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- kms/tpmkms/keyscope_repro_test.go | 47 +++++++++++++++++++++++++++++++ kms/tpmkms/tpmkms.go | 17 +++++++++-- 2 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 kms/tpmkms/keyscope_repro_test.go diff --git a/kms/tpmkms/keyscope_repro_test.go b/kms/tpmkms/keyscope_repro_test.go new file mode 100644 index 00000000..1b44b910 --- /dev/null +++ b/kms/tpmkms/keyscope_repro_test.go @@ -0,0 +1,47 @@ +//go:build tpmsimulator + +package tpmkms + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.step.sm/crypto/kms/apiv1" +) + +// TestTPMKMS_CreateKey_machineScopedAKAttestation reproduces the agent's +// enrollment path: create a machine-scoped AK via tpmkms (ak=true; +// key-scope=machine), then attest a machine-scoped device key by it. Before +// the fix, the AK was created with the user-default scope (CreateAK ignored +// key-scope), so AttestKey's symmetric check rejected the machine-scoped +// attested key with "MachineKey=true does not match AK ... (MachineKey=false)". +func TestTPMKMS_CreateKey_machineScopedAKAttestation(t *testing.T) { + k := &TPMKMS{tpm: newSimulatedTPM(t)} + + akResp, err := k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=ak1;ak=true;key-scope=machine", + }) + require.NoError(t, err) + // The returned AK URI must preserve the machine scope for re-opens. + assert.Equal(t, "tpmkms:name=ak1;ak=true;key-scope=machine", akResp.Name) + + // This is the exact URI the agent builds in attester.Attest; before the + // fix it failed with a scope mismatch. + _, err = k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=key1;attest-by=ak1;key-scope=machine;store-location=machine", + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 1024, + }) + require.NoError(t, err) + + // A user-scoped attested key by the machine AK must still be rejected + // (the symmetric check works in both directions). + _, err = k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=key2;attest-by=ak1", + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 1024, + }) + require.Error(t, err) +} diff --git a/kms/tpmkms/tpmkms.go b/kms/tpmkms/tpmkms.go index b202961f..77c92478 100644 --- a/kms/tpmkms/tpmkms.go +++ b/kms/tpmkms/tpmkms.go @@ -531,9 +531,17 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons size = v.Curve } + machineKey := properties.isMachineKey() + var privateKey any if properties.ak { - ak, err := k.tpm.CreateAK(ctx, properties.name) // NOTE: size is never passed for AKs; it's hardcoded to 2048 in lower levels. + // NOTE: size is never passed for AKs; it's hardcoded to 2048 in lower + // levels. The AK must honor the requested key-scope: an attested key + // inherits its AK's scope (AttestKey enforces this symmetrically), so + // a machine-scoped attested key requires a machine-scoped AK. Creating + // the AK with the plain (user-default) CreateAK here would make every + // machine-scoped attestation fail with a scope mismatch. + ak, err := k.tpm.CreateAKWithConfig(ctx, properties.name, tpm.CreateAKConfig{MachineKey: machineKey}) if err != nil { if errors.Is(err, tpm.ErrExists) { return nil, apiv1.AlreadyExistsError{Message: err.Error()} @@ -549,7 +557,12 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons privateKey = tpmKey } + // Preserve key-scope in the returned URI so re-opens use the matching + // scope (mirrors the non-AK path below). createdAKURI := fmt.Sprintf("tpmkms:name=%s;ak=true", ak.Name()) + if machineKey { + createdAKURI = fmt.Sprintf("%s;key-scope=machine", createdAKURI) + } return &apiv1.CreateKeyResponse{ Name: createdAKURI, PublicKey: ak.Public(), @@ -557,8 +570,6 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons }, nil } - machineKey := properties.isMachineKey() - var key *tpm.Key if properties.attestBy != "" { config := tpm.AttestKeyConfig{ From 681cc4fde0255f867b672299c6de593444a2478b Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Wed, 3 Jun 2026 16:35:35 -0500 Subject: [PATCH 06/11] mod tidy --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 8574cb7c..64bfc01e 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.9 + github.com/smallstep/go-attestation v0.4.4-0.20260603212853-e1a87a0b07d9 github.com/stretchr/testify v1.11.1 go.uber.org/mock v0.6.0 golang.org/x/crypto v0.52.0 diff --git a/go.sum b/go.sum index 8f8b85eb..5c0683f6 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.9 h1:/fVmzB8A8tk7B2PCGPlszWzyl/GDcEQbynqcg2PMaZ0= -github.com/smallstep/go-attestation v0.4.9/go.mod h1:vNAduivU014fubg6ewygkAvQC0IQVXqdc8vaGl/0er4= +github.com/smallstep/go-attestation v0.4.4-0.20260603212853-e1a87a0b07d9 h1:n+X1wnMKJMcCRd98YKAo/56tMRSPUg+qjAvNNS1EZeM= +github.com/smallstep/go-attestation v0.4.4-0.20260603212853-e1a87a0b07d9/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= From 4e76861165bc970908bef7f3b775dfa805808f6e Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Wed, 3 Jun 2026 17:14:51 -0500 Subject: [PATCH 07/11] Drop key-scope context for 'openOptions' --- tpm/ak.go | 12 ++++++------ tpm/context.go | 18 ------------------ tpm/key.go | 16 ++++++++-------- tpm/signer.go | 6 +++--- tpm/tpm.go | 8 ++++++-- 5 files changed, 23 insertions(+), 37 deletions(-) diff --git a/tpm/ak.go b/tpm/ak.go index 39eb8128..794f9d02 100644 --- a/tpm/ak.go +++ b/tpm/ak.go @@ -151,7 +151,7 @@ func (t *TPM) CreateAK(ctx context.Context, name string) (ak *AK, err error) { // 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 { + if err = t.open(ctx, openOptions{machineKey: config.MachineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -297,7 +297,7 @@ func (t *TPM) DeleteAK(ctx context.Context, name string) (err error) { return fmt.Errorf("failed getting AK %q: %w", name, getErr) } - if err := t.open(withMachineKey(ctx, ak.MachineKey)); err != nil { + if err := t.open(ctx, openOptions{machineKey: ak.MachineKey}); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -344,7 +344,7 @@ func (ak *AK) AttestationParameters(ctx context.Context) (params attest.Attestat return *ak.attestParams, nil } - if err = ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { + if err = ak.tpm.open(ctx, openOptions{machineKey: ak.machineKey}); err != nil { return params, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -369,7 +369,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(withMachineKey(ctx, ak.machineKey)); err != nil { + if err = ak.tpm.open(ctx, openOptions{machineKey: ak.machineKey}); err != nil { return secret, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -395,7 +395,7 @@ func (ak *AK) Blobs(ctx context.Context) (blobs *Blobs, err error) { return ak.blobs, nil } - if err = ak.tpm.open(withMachineKey(ctx, ak.machineKey)); err != nil { + if err = ak.tpm.open(ctx, openOptions{machineKey: ak.machineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) @@ -419,7 +419,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(withMachineKey(ctx, ak.machineKey)); err != nil { + if err = ak.tpm.open(ctx, openOptions{machineKey: ak.machineKey}); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, ak.tpm, &err) diff --git a/tpm/context.go b/tpm/context.go index 3c1b3b44..d8625921 100644 --- a/tpm/context.go +++ b/tpm/context.go @@ -37,21 +37,3 @@ 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/key.go b/tpm/key.go index bc15f24a..b0b0096e 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -70,9 +70,9 @@ func (k *Key) WasAttestedBy(ak *AK) bool { func (k *Key) Public() crypto.PublicKey { var ( err error - ctx = withMachineKey(context.Background(), k.machineKey) + ctx = context.Background() ) - if err = k.tpm.open(ctx); err != nil { + if err = k.tpm.open(ctx, openOptions{k.machineKey}); err != nil { return nil } defer closeTPM(context.Background(), k.tpm, &err) @@ -180,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(withMachineKey(goTPMCall(ctx), config.MachineKey)); err != nil { + if err = t.open(goTPMCall(ctx), openOptions{machineKey: config.MachineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -257,7 +257,7 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK // after the t.store.Load that t.open performs, which matters for // storage implementations whose in-memory state doesn't reflect // on-disk state until Load is called. - if err = t.open(withMachineKey(ctx, config.MachineKey)); err != nil { + if err = t.open(openOptions{machineKey: config.MachineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -427,7 +427,7 @@ func (t *TPM) DeleteKey(ctx context.Context, name string) (err error) { return fmt.Errorf("failed getting key %q: %w", name, getErr) } - if err := t.open(withMachineKey(ctx, key.MachineKey)); err != nil { + if err := t.open(ctx, openOptions{machineKey: key.MachineKey}); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -462,7 +462,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(withMachineKey(ctx, k.machineKey)); err != nil { + if err = k.tpm.open(ctx, openOptions{machineKey: k.machineKey}); err != nil { return params, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) @@ -488,7 +488,7 @@ func (k *Key) Blobs(ctx context.Context) (blobs *Blobs, err error) { return k.blobs, nil } - if err = k.tpm.open(withMachineKey(ctx, k.machineKey)); err != nil { + if err = k.tpm.open(ctx, openOptions{machineKey: k.machineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) @@ -512,7 +512,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(withMachineKey(ctx, k.machineKey)); err != nil { + if err = k.tpm.open(ctx, openOptions{machineKey: k.machineKey}); err != nil { return fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, k.tpm, &err) diff --git a/tpm/signer.go b/tpm/signer.go index b5709e80..22f98347 100644 --- a/tpm/signer.go +++ b/tpm/signer.go @@ -27,8 +27,8 @@ 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 := withMachineKey(context.Background(), s.key.machineKey) - if err = s.tpm.open(ctx); err != nil { + ctx := context.Background() + if err = s.tpm.open(ctx, openOptions{machineKey: s.key.machineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, s.tpm, &err) @@ -70,7 +70,7 @@ func (t *TPM) GetSigner(ctx context.Context, name string) (csigner crypto.Signer return nil, fmt.Errorf("failed getting signer for key %q: %w", name, getErr) } - if err = t.open(withMachineKey(ctx, key.MachineKey)); err != nil { + if err = t.open(ctx, openOptions{machineKey: key.MachineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/tpm.go b/tpm/tpm.go index cb2d9b53..b7ea1005 100644 --- a/tpm/tpm.go +++ b/tpm/tpm.go @@ -160,10 +160,14 @@ func New(opts ...NewTPMOption) (*TPM, error) { }, nil } +type openOptions struct { + machineKey bool +} + // Open readies the TPM for usage and marks it as being // in use. This makes using the instance safe for // concurrent use. -func (t *TPM) open(ctx context.Context) (err error) { +func (t *TPM) open(ctx context.Context, opts openOptions) (err error) { // prevent opening the TPM multiple times if Open is called // within the package multiple times. if isInternalCall(ctx) { @@ -194,7 +198,7 @@ func (t *TPM) open(ctx context.Context) (err error) { // 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) + t.attestConfig.MachineKey = opts.machineKey // if a simulator was set, use it as the backing TPM device. // The simulator is currently only used for testing. From 84f5a31d5e97f2702d733acfbc32d52ab4bf64e2 Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Wed, 3 Jun 2026 17:19:37 -0500 Subject: [PATCH 08/11] Consolidate test files --- kms/tpmkms/keyscope_repro_test.go | 47 ----------------------------- kms/tpmkms/tpmkms_simulator_test.go | 35 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 47 deletions(-) delete mode 100644 kms/tpmkms/keyscope_repro_test.go diff --git a/kms/tpmkms/keyscope_repro_test.go b/kms/tpmkms/keyscope_repro_test.go deleted file mode 100644 index 1b44b910..00000000 --- a/kms/tpmkms/keyscope_repro_test.go +++ /dev/null @@ -1,47 +0,0 @@ -//go:build tpmsimulator - -package tpmkms - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "go.step.sm/crypto/kms/apiv1" -) - -// TestTPMKMS_CreateKey_machineScopedAKAttestation reproduces the agent's -// enrollment path: create a machine-scoped AK via tpmkms (ak=true; -// key-scope=machine), then attest a machine-scoped device key by it. Before -// the fix, the AK was created with the user-default scope (CreateAK ignored -// key-scope), so AttestKey's symmetric check rejected the machine-scoped -// attested key with "MachineKey=true does not match AK ... (MachineKey=false)". -func TestTPMKMS_CreateKey_machineScopedAKAttestation(t *testing.T) { - k := &TPMKMS{tpm: newSimulatedTPM(t)} - - akResp, err := k.CreateKey(&apiv1.CreateKeyRequest{ - Name: "tpmkms:name=ak1;ak=true;key-scope=machine", - }) - require.NoError(t, err) - // The returned AK URI must preserve the machine scope for re-opens. - assert.Equal(t, "tpmkms:name=ak1;ak=true;key-scope=machine", akResp.Name) - - // This is the exact URI the agent builds in attester.Attest; before the - // fix it failed with a scope mismatch. - _, err = k.CreateKey(&apiv1.CreateKeyRequest{ - Name: "tpmkms:name=key1;attest-by=ak1;key-scope=machine;store-location=machine", - SignatureAlgorithm: apiv1.SHA256WithRSA, - Bits: 1024, - }) - require.NoError(t, err) - - // A user-scoped attested key by the machine AK must still be rejected - // (the symmetric check works in both directions). - _, err = k.CreateKey(&apiv1.CreateKeyRequest{ - Name: "tpmkms:name=key2;attest-by=ak1", - SignatureAlgorithm: apiv1.SHA256WithRSA, - Bits: 1024, - }) - require.Error(t, err) -} diff --git a/kms/tpmkms/tpmkms_simulator_test.go b/kms/tpmkms/tpmkms_simulator_test.go index 0a06f395..b4526afd 100644 --- a/kms/tpmkms/tpmkms_simulator_test.go +++ b/kms/tpmkms/tpmkms_simulator_test.go @@ -2467,3 +2467,38 @@ func TestTPMKMS_SearchKeys(t *testing.T) { }) } } + +// TestTPMKMS_CreateKey_machineScopedAKAttestation reproduces the agent's +// enrollment path: create a machine-scoped AK via tpmkms (ak=true; +// key-scope=machine), then attest a machine-scoped device key by it. Before +// the fix, the AK was created with the user-default scope (CreateAK ignored +// key-scope), so AttestKey's symmetric check rejected the machine-scoped +// attested key with "MachineKey=true does not match AK ... (MachineKey=false)". +func TestTPMKMS_CreateKey_machineScopedAKAttestation(t *testing.T) { + k := &TPMKMS{tpm: newSimulatedTPM(t)} + + akResp, err := k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=ak1;ak=true;key-scope=machine", + }) + require.NoError(t, err) + // The returned AK URI must preserve the machine scope for re-opens. + assert.Equal(t, "tpmkms:name=ak1;ak=true;key-scope=machine", akResp.Name) + + // This is the exact URI the agent builds in attester.Attest; before the + // fix it failed with a scope mismatch. + _, err = k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=key1;attest-by=ak1;key-scope=machine;store-location=machine", + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 1024, + }) + require.NoError(t, err) + + // A user-scoped attested key by the machine AK must still be rejected + // (the symmetric check works in both directions). + _, err = k.CreateKey(&apiv1.CreateKeyRequest{ + Name: "tpmkms:name=key2;attest-by=ak1", + SignatureAlgorithm: apiv1.SHA256WithRSA, + Bits: 1024, + }) + require.Error(t, err) +} From 93f45f91d687264af614da1e2f62c85759a5080e Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Wed, 3 Jun 2026 17:26:38 -0500 Subject: [PATCH 09/11] Update all 'tpm.open' call sites --- tpm/ak.go | 6 +++--- tpm/caps.go | 2 +- tpm/ek.go | 2 +- tpm/info.go | 2 +- tpm/key.go | 8 ++++---- tpm/random.go | 4 ++-- tpm/signer.go | 4 ++-- tpm/tpm_test.go | 4 ++-- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tpm/ak.go b/tpm/ak.go index 794f9d02..022660c5 100644 --- a/tpm/ak.go +++ b/tpm/ak.go @@ -205,7 +205,7 @@ func (t *TPM) CreateAKWithConfig(ctx context.Context, name string, config Create // GetAK returns the AK identified by `name`. It returns `ErrNotfound` // if it doesn't exist. func (t *TPM) GetAK(ctx context.Context, name string) (ak *AK, err error) { - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -229,7 +229,7 @@ var ( // exists with `permanentIdentifier` as one of the Subject Alternative // Names. It returns `ErrNotFound` if it doesn't exist. func (t *TPM) GetAKByPermanentIdentifier(ctx context.Context, permanentIdentifier string) (ak *AK, err error) { - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -254,7 +254,7 @@ func (t *TPM) GetAKByPermanentIdentifier(ctx context.Context, permanentIdentifie // ListAKs returns a slice of AKs. The result is (currently) // not ordered. func (t *TPM) ListAKs(ctx context.Context) (aks []*AK, err error) { - if err := t.open(ctx); err != nil { + if err := t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/caps.go b/tpm/caps.go index 539a9dc8..88d3be8c 100644 --- a/tpm/caps.go +++ b/tpm/caps.go @@ -42,7 +42,7 @@ func (t *TPM) GetCapabilities(ctx context.Context) (caps *Capabilities, err erro return t.caps, nil } - if err = t.open(goTPMCall(ctx)); err != nil { + if err = t.open(goTPMCall(ctx), openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/ek.go b/tpm/ek.go index fce75a44..f153d925 100644 --- a/tpm/ek.go +++ b/tpm/ek.go @@ -153,7 +153,7 @@ func (t *TPM) GetEKs(ctx context.Context) (eks []*EK, err error) { return t.eks, nil } - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/info.go b/tpm/info.go index ae9495b7..45af5292 100644 --- a/tpm/info.go +++ b/tpm/info.go @@ -130,7 +130,7 @@ func (t *TPM) Info(ctx context.Context) (info *Info, err error) { return t.info, nil } - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/key.go b/tpm/key.go index b0b0096e..4502d596 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -257,7 +257,7 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK // after the t.store.Load that t.open performs, which matters for // storage implementations whose in-memory state doesn't reflect // on-disk state until Load is called. - if err = t.open(openOptions{machineKey: config.MachineKey}); err != nil { + if err = t.open(ctx, openOptions{machineKey: config.MachineKey}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -341,7 +341,7 @@ func (t *TPM) AttestKey(ctx context.Context, akName, name string, config AttestK // GetKey returns the Key identified by `name`. It returns `ErrNotfound` // if it doesn't exist. func (t *TPM) GetKey(ctx context.Context, name string) (key *Key, err error) { - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -360,7 +360,7 @@ func (t *TPM) GetKey(ctx context.Context, name string) (key *Key, err error) { // ListKeys returns a slice of Keys. The result is (currently) // not ordered. func (t *TPM) ListKeys(ctx context.Context) (keys []*Key, err error) { - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -381,7 +381,7 @@ func (t *TPM) ListKeys(ctx context.Context) (keys []*Key, err error) { // GetKeysAttestedBy returns a slice of Keys attested by the AK // identified by `akName`. The result is (currently) not ordered. func (t *TPM) GetKeysAttestedBy(ctx context.Context, akName string) (keys []*Key, err error) { - if err = t.open(ctx); err != nil { + if err = t.open(ctx, openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/random.go b/tpm/random.go index 384d2ebf..82f2cec3 100644 --- a/tpm/random.go +++ b/tpm/random.go @@ -23,7 +23,7 @@ func (s ShortRandomReadError) Error() string { // GenerateRandom returns `size` number of random bytes generated by the TPM. func (t *TPM) GenerateRandom(ctx context.Context, size uint16) (random []byte, err error) { - if err = t.open(goTPMCall(ctx)); err != nil { + if err = t.open(goTPMCall(ctx), openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -65,7 +65,7 @@ func (g *generator) Read(p []byte) (n int, err error) { } ctx := context.Background() - if err = g.t.open(goTPMCall(ctx)); err != nil { + if err = g.t.open(goTPMCall(ctx), openOptions{}); err != nil { return 0, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, g.t, &err) diff --git a/tpm/signer.go b/tpm/signer.go index 22f98347..4a4c957b 100644 --- a/tpm/signer.go +++ b/tpm/signer.go @@ -108,7 +108,7 @@ type tss2Signer struct { func (s *tss2Signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) (signature []byte, err error) { ctx := context.Background() - if err = s.tpm.open(goTPMCall(ctx)); err != nil { + if err = s.tpm.open(goTPMCall(ctx), openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, s.tpm, &err) @@ -119,7 +119,7 @@ func (s *tss2Signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) // CreateTSS2Signer returns a crypto.Signer using the given [TPM] and [tss2.TPMKey]. func CreateTSS2Signer(ctx context.Context, t *TPM, key *tss2.TPMKey) (csigner crypto.Signer, err error) { - if err := t.open(goTPMCall(ctx)); err != nil { + if err := t.open(goTPMCall(ctx), openOptions{}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/tpm_test.go b/tpm/tpm_test.go index fc14ccbe..c14de34f 100644 --- a/tpm/tpm_test.go +++ b/tpm/tpm_test.go @@ -41,7 +41,7 @@ func newOpenedTPM(t *testing.T) *TPM { t.Helper() tpm, err := New(WithSimulator(&closeSimulator{})) require.NoError(t, err) - err = tpm.open(context.Background()) + err = tpm.open(context.Background(), openOptions{}) require.NoError(t, err) return tpm } @@ -52,7 +52,7 @@ func newCloseErrorTPM(t *testing.T) *TPM { closeErr: errors.New("closeErr"), })) require.NoError(t, err) - err = tpm.open(context.Background()) + err = tpm.open(context.Background(), openOptions{}) require.NoError(t, err) tpm.simulator = nil // required to skip returning when similator is configured return tpm From 2f934c5f1dfb2ca851281ca7f3166a54366d2371 Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Wed, 3 Jun 2026 17:34:38 -0500 Subject: [PATCH 10/11] lint --- tpm/ak.go | 4 ++-- tpm/key.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tpm/ak.go b/tpm/ak.go index 022660c5..7a82d003 100644 --- a/tpm/ak.go +++ b/tpm/ak.go @@ -319,14 +319,14 @@ func (t *TPM) DeleteAK(ctx context.Context, name string) (err error) { 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 after TPM delete failed (%w): %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 after TPM delete failed (%w): %w", attestErr, err) } return fmt.Errorf("failed persisting storage: %w", err) } diff --git a/tpm/key.go b/tpm/key.go index 4502d596..c7383242 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -436,14 +436,14 @@ func (t *TPM) DeleteKey(ctx context.Context, name string) (err error) { 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 after TPM delete failed (%w): %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 after TPM delete failed (%w): %w", attestErr, err) } return fmt.Errorf("failed persisting storage: %w", err) } From fef78a254eb332b9552da3225264cbac6810f812 Mon Sep 17 00:00:00 2001 From: Josh Drake Date: Thu, 4 Jun 2026 09:38:02 -0500 Subject: [PATCH 11/11] Add 'direct' to openOptions for using underlying TPM channel --- tpm/caps.go | 2 +- tpm/context.go | 11 ----------- tpm/key.go | 4 ++-- tpm/random.go | 4 ++-- tpm/signer.go | 4 ++-- tpm/tpm.go | 6 +++++- 6 files changed, 12 insertions(+), 19 deletions(-) diff --git a/tpm/caps.go b/tpm/caps.go index 88d3be8c..0a81f6c9 100644 --- a/tpm/caps.go +++ b/tpm/caps.go @@ -42,7 +42,7 @@ func (t *TPM) GetCapabilities(ctx context.Context) (caps *Capabilities, err erro return t.caps, nil } - if err = t.open(goTPMCall(ctx), openOptions{}); err != nil { + if err = t.open(ctx, openOptions{direct: true}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/context.go b/tpm/context.go index d8625921..c422b58f 100644 --- a/tpm/context.go +++ b/tpm/context.go @@ -26,14 +26,3 @@ func isInternalCall(ctx context.Context) bool { v, ok := ctx.Value(internalCallContextKey{}).(bool) return ok && v } - -type goTPMCallContextKey struct{} - -func goTPMCall(ctx context.Context) context.Context { - return context.WithValue(ctx, goTPMCallContextKey{}, true) -} - -func isGoTPMCall(ctx context.Context) bool { - v, ok := ctx.Value(goTPMCallContextKey{}).(bool) - return ok && v -} diff --git a/tpm/key.go b/tpm/key.go index c7383242..a0c262b0 100644 --- a/tpm/key.go +++ b/tpm/key.go @@ -72,7 +72,7 @@ func (k *Key) Public() crypto.PublicKey { err error ctx = context.Background() ) - if err = k.tpm.open(ctx, openOptions{k.machineKey}); err != nil { + if err = k.tpm.open(ctx, openOptions{machineKey: k.machineKey}); err != nil { return nil } defer closeTPM(context.Background(), k.tpm, &err) @@ -180,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), openOptions{machineKey: config.MachineKey}); err != nil { + if err = t.open(ctx, openOptions{machineKey: config.MachineKey, direct: true}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/random.go b/tpm/random.go index 82f2cec3..d18ab9c6 100644 --- a/tpm/random.go +++ b/tpm/random.go @@ -23,7 +23,7 @@ func (s ShortRandomReadError) Error() string { // GenerateRandom returns `size` number of random bytes generated by the TPM. func (t *TPM) GenerateRandom(ctx context.Context, size uint16) (random []byte, err error) { - if err = t.open(goTPMCall(ctx), openOptions{}); err != nil { + if err = t.open(ctx, openOptions{direct: true}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) @@ -65,7 +65,7 @@ func (g *generator) Read(p []byte) (n int, err error) { } ctx := context.Background() - if err = g.t.open(goTPMCall(ctx), openOptions{}); err != nil { + if err = g.t.open(ctx, openOptions{direct: true}); err != nil { return 0, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, g.t, &err) diff --git a/tpm/signer.go b/tpm/signer.go index 4a4c957b..149c3cb2 100644 --- a/tpm/signer.go +++ b/tpm/signer.go @@ -108,7 +108,7 @@ type tss2Signer struct { func (s *tss2Signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) (signature []byte, err error) { ctx := context.Background() - if err = s.tpm.open(goTPMCall(ctx), openOptions{}); err != nil { + if err = s.tpm.open(ctx, openOptions{direct: true}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, s.tpm, &err) @@ -119,7 +119,7 @@ func (s *tss2Signer) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) // CreateTSS2Signer returns a crypto.Signer using the given [TPM] and [tss2.TPMKey]. func CreateTSS2Signer(ctx context.Context, t *TPM, key *tss2.TPMKey) (csigner crypto.Signer, err error) { - if err := t.open(goTPMCall(ctx), openOptions{}); err != nil { + if err := t.open(ctx, openOptions{direct: true}); err != nil { return nil, fmt.Errorf("failed opening TPM: %w", err) } defer closeTPM(ctx, t, &err) diff --git a/tpm/tpm.go b/tpm/tpm.go index b7ea1005..bbfe0031 100644 --- a/tpm/tpm.go +++ b/tpm/tpm.go @@ -162,6 +162,10 @@ func New(opts ...NewTPMOption) (*TPM, error) { type openOptions struct { machineKey bool + // direct opens a raw go-tpm command channel (t.rwc) instead of the + // go-attestation backend, for low-level operations such as generating + // random bytes, signing, and reading capabilities. + direct bool } // Open readies the TPM for usage and marks it as being @@ -217,7 +221,7 @@ func (t *TPM) open(ctx context.Context, opts openOptions) (err error) { // there's a possibility of a nil pointer exception. At the moment, // the only "go-tpm" call is for GetRandom(), but this could change // in the future. - if isGoTPMCall(ctx) { + if opts.direct { rwc, err := open.TPM(t.deviceName) if err != nil { return fmt.Errorf("failed opening TPM: %w", err)