feat: add per-key key-scope option for Windows machine vs user key store

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.52.0

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=

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)
+}

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(),
@@ -563,6 +576,7 @@ func (k *TPMKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons
 			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 +587,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 +613,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,

kms/tpmkms/uri.go

@@ -27,6 +27,21 @@ 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
+}
+
+// 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) isMachineKey() bool {
+	return o.keyScope == "machine" ||
+		(o.keyScope == "" && o.storeLocation == "machine")
 }
 
 func parseNameURI(nameURI string) (o objectProperties, err error) {
@@ -75,10 +90,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
 	}

tpm/ak.go

@@ -1,18 +1,18 @@
 package tpm
 
 import (
 	"context"
 	"crypto"
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"time"
 
 	"github.com/smallstep/go-attestation/attest"
 	x509ext "github.com/smallstep/go-attestation/x509"
 
 	"go.step.sm/crypto/tpm/storage"
 )
@@ -26,11 +26,20 @@
 	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 @@
 // 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 @@
 	}
 
 	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,30 @@
 // 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) {
+	// 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) {
 			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 +315,26 @@
 		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 +344,7 @@
 		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 +369,7 @@
 // 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 +395,7 @@
 		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 +419,7 @@
 // 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,21 +499,23 @@
 // 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,
 	}
 }
 
 // akFromStorage recreates an AK from the struct used for
 // 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,
 	}
 }

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
+}