[v12] fix: avoid inadvertent deletion of active HSM keys

lib/auth/auth.go

@@ -4823,7 +4823,7 @@ func (a *Server) deleteUnusedKeys(ctx context.Context) error {
 		return trace.Wrap(err)
 	}
 
-	var usedKeys [][]byte
+	var activeKeys [][]byte
 	for _, caType := range types.CertAuthTypes {
 		caID := types.CertAuthID{Type: caType, DomainName: clusterName.GetClusterName()}
 		ca, err := a.Services.GetCertAuthority(ctx, caID, true)
@@ -4832,17 +4832,23 @@ func (a *Server) deleteUnusedKeys(ctx context.Context) error {
 		}
 		for _, keySet := range []types.CAKeySet{ca.GetActiveKeys(), ca.GetAdditionalTrustedKeys()} {
 			for _, sshKeyPair := range keySet.SSH {
-				usedKeys = append(usedKeys, sshKeyPair.PrivateKey)
+				activeKeys = append(activeKeys, sshKeyPair.PrivateKey)
 			}
 			for _, tlsKeyPair := range keySet.TLS {
-				usedKeys = append(usedKeys, tlsKeyPair.Key)
+				activeKeys = append(activeKeys, tlsKeyPair.Key)
 			}
 			for _, jwtKeyPair := range keySet.JWT {
-				usedKeys = append(usedKeys, jwtKeyPair.PrivateKey)
+				activeKeys = append(activeKeys, jwtKeyPair.PrivateKey)
 			}
 		}
 	}
-	return trace.Wrap(a.keyStore.DeleteUnusedKeys(ctx, usedKeys))
+	if err := a.keyStore.DeleteUnusedKeys(ctx, activeKeys); err != nil {
+		// Key deletion is best-effort, log a warning if it fails and carry on.
+		// We don't want to prevent a CA rotation, which may be necessary in
+		// some cases where this would fail.
+		log.WithError(err).Warning("Failed attempt to delete unused HSM keys")
+	}
+	return nil
 }
 
 // GetLicense return the license used the start the teleport enterprise auth server

lib/auth/keystore/gcp_kms.go

@@ -230,16 +230,20 @@ func (g *gcpKMSKeyStore) canSignWithKey(ctx context.Context, raw []byte, keyType
 
 // DeleteUnusedKeys deletes all keys from KMS if they are:
 // 1. Labeled by this server (matching HostUUID) when they were created
-// 2. Not included in the argument usedKeys
-func (g *gcpKMSKeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte) error {
-	usedKmsKeyVersions := make(map[string]struct{})
-	for _, usedKey := range usedKeys {
-		keyID, err := parseGCPKMSKeyID(usedKey)
+// 2. Not included in the argument activeKeys
+func (g *gcpKMSKeyStore) DeleteUnusedKeys(ctx context.Context, activeKeys [][]byte) error {
+	// Make a map of currently active key versions, this is used for lookups to
+	// check which keys in KMS are unused, and holds a count of how many times
+	// they are found in KMS. If any active keys are not found in KMS, we are in
+	// a bad state, so key deletion will be aborted.
+	activeKmsKeyVersions := make(map[string]int)
+	for _, activeKey := range activeKeys {
+		keyID, err := parseGCPKMSKeyID(activeKey)
 		if err != nil {
 			// could be a different type of key
 			continue
 		}
-		usedKmsKeyVersions[keyID.keyVersionName] = struct{}{}
+		activeKmsKeyVersions[keyID.keyVersionName] = 0
 	}
 
 	var unusedKeyIDs []gcpKMSKeyID
@@ -252,7 +256,9 @@ func (g *gcpKMSKeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte
 	key, err := iter.Next()
 	for err == nil {
 		keyVersionName := key.Name + keyVersionSuffix
-		if _, used := usedKmsKeyVersions[keyVersionName]; !used {
+		if _, active := activeKmsKeyVersions[keyVersionName]; active {
+			activeKmsKeyVersions[keyVersionName]++
+		} else {
 			unusedKeyIDs = append(unusedKeyIDs, gcpKMSKeyID{
 				keyVersionName: keyVersionName,
 			})
@@ -263,6 +269,16 @@ func (g *gcpKMSKeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte
 		return trace.Wrap(err)
 	}
 
+	for keyVersion, found := range activeKmsKeyVersions {
+		if found == 0 {
+			// Failed to find a currently active key owned by this host.
+			// The cluster is in a bad state, refuse to delete any keys.
+			return trace.NotFound(
+				"cannot find currently active CA key in %q GCP KMS, aborting attempt to delete unused keys",
+				keyVersion)
+		}
+	}
+
 	for _, unusedKey := range unusedKeyIDs {
 		g.log.WithFields(logrus.Fields{"key_version": unusedKey.keyVersionName}).Info("deleting unused GCP KMS key created by this server")
 		err := g.deleteKey(ctx, unusedKey.marshal())

lib/auth/keystore/keystore_test.go

@@ -163,10 +163,11 @@ func TestKeyStore(t *testing.T) {
 
 	yubiSlotNumber := 0
 	backends := []struct {
-		desc       string
-		config     Config
-		isSoftware bool
-		shouldSkip func() bool
+		desc        string
+		config      Config
+		isSoftware  bool
+		shouldSkip  func() bool
+		fakeKeyHack func([]byte) []byte
 	}{
 		{
 			desc: "software",
@@ -233,6 +234,15 @@ func TestKeyStore(t *testing.T) {
 			shouldSkip: func() bool {
 				return false
 			},
+			fakeKeyHack: func(key []byte) []byte {
+				// GCP KMS keys are never really deleted, their state is just
+				// set to destroyed, so this hack modifies a key to make it
+				// unrecognizable
+				kmsKey, err := parseGCPKMSKeyID(key)
+				require.NoError(t, err)
+				kmsKey.keyVersionName += "fake"
+				return kmsKey.marshal()
+			},
 		},
 	}
 
@@ -420,6 +430,17 @@ func TestKeyStore(t *testing.T) {
 				require.Error(t, err)
 			}
 
+			// Make sure key deletion is aborted when one of the active keys
+			// cannot be found.
+			// Use rawKeys[1] as a fake active key, it was just deleted in the
+			// previous step.
+			fakeActiveKey := rawKeys[1]
+			if tc.fakeKeyHack != nil {
+				fakeActiveKey = tc.fakeKeyHack(fakeActiveKey)
+			}
+			err = keyStore.DeleteUnusedKeys(ctx, [][]byte{fakeActiveKey})
+			require.True(t, trace.IsNotFound(err), "expected NotFound error, got %v", err)
+
 			// delete the final key so we don't leak it
 			err = keyStore.deleteKey(ctx, rawKeys[0])
 			require.NoError(t, err)

lib/auth/keystore/manager.go

@@ -57,8 +57,8 @@ func WithDigestAlgorithm(alg crypto.Hash) RSAKeyOption {
 type backend interface {
 	// DeleteUnusedKeys deletes all keys from the KeyStore if they are:
 	// 1. Labeled by this KeyStore when they were created
-	// 2. Not included in the argument usedKeys
-	DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte) error
+	// 2. Not included in the argument activeKeys
+	DeleteUnusedKeys(ctx context.Context, activeKeys [][]byte) error
 
 	// generateRSA creates a new RSA private key and returns its identifier and
 	// a crypto.Signer. The returned identifier can be passed to getSigner

lib/auth/keystore/pkcs11.go

@@ -212,37 +212,55 @@ func (p *pkcs11KeyStore) deleteKey(_ context.Context, rawKey []byte) error {
 }
 
 // DeleteUnusedKeys deletes all keys from the KeyStore if they are:
-// 1. Labeled by this KeyStore when they were created
-// 2. Not included in the argument usedKeys
-func (p *pkcs11KeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte) error {
+// 1. Labeled with the local HostUUID when they were created
+// 2. Not included in the argument activeKeys
+// This is meant to delete unused keys after they have been rotated out by a CA
+// rotation.
+func (p *pkcs11KeyStore) DeleteUnusedKeys(ctx context.Context, activeKeys [][]byte) error {
 	p.log.Debug("Deleting unused keys from HSM")
-	var usedPublicKeys []*rsa.PublicKey
-	for _, usedKey := range usedKeys {
-		if keyType(usedKey) != types.PrivateKeyType_PKCS11 {
+
+	// It's necessary to fetch all PublicKeys for the known activeKeys in order to
+	// compare with the signers returned by FindKeyPairs below. We have no way
+	// to find the CKA_ID of an unused key if it is not known.
+	var activePublicKeys []*rsa.PublicKey
+	for _, activeKey := range activeKeys {
+		if keyType(activeKey) != types.PrivateKeyType_PKCS11 {
 			continue
 		}
-		signer, err := p.getSigner(ctx, usedKey)
-		if trace.IsNotFound(err) {
-			// key is for different host, or truly not found in HSM. Either
-			// way, it won't be deleted below.
+		keyID, err := parseKeyID(activeKey)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		if keyID.HostID != p.hostUUID {
+			// This key was labeled with a foreign host UUID, it is likely not
+			// present on the attached HSM and definitely will not be returned
+			// by FindKeyPairs below which queries by host UUID.
 			continue
 		}
+		signer, err := p.getSigner(ctx, activeKey)
+		if trace.IsNotFound(err) {
+			// Failed to find a currently active key owned by this host.
+			// The cluster is in a bad state, refuse to delete any keys.
+			return trace.NotFound(
+				"cannot find currently active CA key %q in HSM, aborting attempt to delete unused keys",
+				keyID.KeyID)
+		}
 		if err != nil {
 			return trace.Wrap(err)
 		}
 		rsaPublicKey, ok := signer.Public().(*rsa.PublicKey)
 		if !ok {
 			return trace.BadParameter("unknown public key type: %T", signer.Public())
 		}
-		usedPublicKeys = append(usedPublicKeys, rsaPublicKey)
+		activePublicKeys = append(activePublicKeys, rsaPublicKey)
 	}
-	keyIsUsed := func(signer crypto.Signer) bool {
+	keyIsActive := func(signer crypto.Signer) bool {
 		rsaPublicKey, ok := signer.Public().(*rsa.PublicKey)
 		if !ok {
 			// unknown key type... we don't know what this is, so don't delete it
 			return true
 		}
-		for _, k := range usedPublicKeys {
+		for _, k := range activePublicKeys {
 			if rsaPublicKey.Equal(k) {
 				return true
 			}
@@ -254,13 +272,14 @@ func (p *pkcs11KeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte
 		return trace.Wrap(err)
 	}
 	for _, signer := range signers {
-		if keyIsUsed(signer) {
+		if keyIsActive(signer) {
 			continue
 		}
 		if err := signer.Delete(); err != nil {
-			// Key deletion is best-effort, log a warning on errors. Errors have
-			// been observed when FindKeyPairs returns duplicate keys.
-			p.log.Warnf("failed deleting unused key from HSM: %v", err)
+			// Key deletion is best-effort, log a warning on errors, and
+			// continue trying to delete other keys. Errors have been observed
+			// when FindKeyPairs returns duplicate keys.
+			p.log.Warnf("Failed deleting unused key from HSM: %v", err)
 		}
 	}
 	return nil

lib/auth/keystore/software.go

@@ -84,8 +84,8 @@ func (s *softwareKeyStore) deleteKey(_ context.Context, _ []byte) error {
 
 // DeleteUnusedKeys deletes all keys from the KeyStore if they are:
 // 1. Labeled by this KeyStore when they were created
-// 2. Not included in the argument usedKeys
+// 2. Not included in the argument activeKeys
 // This is a no-op for rawKeyStore.
-func (s *softwareKeyStore) DeleteUnusedKeys(ctx context.Context, usedKeys [][]byte) error {
+func (s *softwareKeyStore) DeleteUnusedKeys(ctx context.Context, activeKeys [][]byte) error {
 	return nil
 }