Cleanup keys with same SPIRE key id (#5058)

Signed-off-by: Matteo Kamm <[email protected]>

Author: InverseIntegral

Diff for: pkg/server/plugin/keymanager/hashicorpvault/hashicorp_vault.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
+	"github.com/andres-erbsen/clock"
 	"github.com/gofrs/uuid/v5"
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/hcl"
@@ -18,6 +19,7 @@ import (
 	"google.golang.org/grpc/status"
 	"os"
 	"sync"
+	"time"
 )
 
 const (
@@ -41,8 +43,10 @@ type keyEntry struct {
 }
 
 type pluginHooks struct {
+	clk clock.Clock
 	// Used for testing only.
-	lookupEnv func(string) (string, bool)
+	lookupEnv            func(string) (string, bool)
+	scheduleDeleteSignal chan error
 }
 
 // Config provides configuration context for the plugin.
@@ -134,6 +138,9 @@ type Plugin struct {
 	cc         *ClientConfig
 	vc         *Client
 
+	scheduleDelete chan string
+	cancelTasks    context.CancelFunc
+
 	hooks pluginHooks
 }
 
@@ -148,7 +155,9 @@ func newPlugin() *Plugin {
 		entries: make(map[string]keyEntry),
 		hooks: pluginHooks{
 			lookupEnv: os.LookupEnv,
+			clk:       clock.New(),
 		},
+		scheduleDelete: make(chan string, 120),
 	}
 }
 
@@ -214,6 +223,15 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest)
 
 	p.setCache(keyEntries)
 
+	// Cancel previous tasks in case of re-configure.
+	if p.cancelTasks != nil {
+		p.cancelTasks()
+	}
+
+	// start tasks
+	ctx, p.cancelTasks = context.WithCancel(context.Background())
+	go p.scheduleDeleteTask(ctx)
+
 	return &configv1.ConfigureResponse{}, nil
 }
 
@@ -301,6 +319,61 @@ func (p *Plugin) getEnvOrDefault(envKey, fallback string) string {
 	return fallback
 }
 
+// scheduleDeleteTask is a long-running task that deletes keys that are stale
+func (p *Plugin) scheduleDeleteTask(ctx context.Context) {
+	backoffMin := 1 * time.Second
+	backoffMax := 60 * time.Second
+	backoff := backoffMin
+
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case keyID := <-p.scheduleDelete:
+			log := p.logger.With("key_id", keyID)
+
+			if p.vc == nil {
+				err := p.genVaultClient()
+				if err != nil {
+					log.Error("Failed to generate vault client", "reason", err)
+					p.notifyDelete(err)
+					// TODO: Should we re-enqueue here?
+				}
+			}
+
+			err := p.vc.DeleteKey(ctx, keyID)
+
+			if err == nil {
+				log.Debug("Key deleted")
+				backoff = backoffMin
+				p.notifyDelete(nil)
+				continue
+			}
+
+			// For any other error, log it and re-enqueue the key for deletion as it might be a recoverable error
+			log.Error("It was not possible to schedule key for deletion. Trying to re-enqueue it for deletion", "reason", err)
+
+			select {
+			case p.scheduleDelete <- keyID:
+				log.Debug("Key re-enqueued for deletion")
+			default:
+				log.Error("Failed to re-enqueue key for deletion")
+			}
+
+			p.notifyDelete(nil)
+			backoff = min(backoff*2, backoffMax)
+			p.hooks.clk.Sleep(backoff)
+		}
+	}
+}
+
+// Used for testing only
+func (p *Plugin) notifyDelete(err error) {
+	if p.hooks.scheduleDeleteSignal != nil {
+		p.hooks.scheduleDeleteSignal <- err
+	}
+}
+
 func (p *Plugin) GenerateKey(ctx context.Context, req *keymanagerv1.GenerateKeyRequest) (*keymanagerv1.GenerateKeyResponse, error) {
 	if req.KeyId == "" {
 		return nil, status.Error(codes.InvalidArgument, "key id is required")
@@ -318,6 +391,15 @@ func (p *Plugin) GenerateKey(ctx context.Context, req *keymanagerv1.GenerateKeyR
 		return nil, err
 	}
 
+	if keyEntry, ok := p.getKeyEntry(spireKeyID); ok {
+		select {
+		case p.scheduleDelete <- keyEntry.KeyName:
+			p.logger.Debug("Key enqueued for deletion", "key_name", keyEntry.KeyName)
+		default:
+			p.logger.Error("Failed to enqueue key for deletion", "key_name", keyEntry.KeyName)
+		}
+	}
+
 	p.setKeyEntry(spireKeyID, *newKeyEntry)
 
 	return &keymanagerv1.GenerateKeyResponse{

Diff for: pkg/server/plugin/keymanager/hashicorpvault/hashicorp_vault_test.go

@@ -472,6 +472,22 @@ func TestPluginGenerateKey(t *testing.T) {
 			expectCode:      codes.Internal,
 			expectMsgPrefix: "keymanager(hashicorp_vault): unable to decode PEM key",
 		},
+		{
+			name:       "Generate key with existing SPIRE key id",
+			id:         "x509-CA-A",
+			keyType:    keymanager.ECP256,
+			config:     successfulConfig,
+			authMethod: TOKEN,
+			fakeServer: func() *FakeVaultServerConfig {
+				fakeServer := setupSuccessFakeVaultServer("test-transit")
+				fakeServer.LookupSelfResponse = []byte(testLookupSelfResponse)
+				fakeServer.CertAuthResponse = []byte{}
+				fakeServer.AppRoleAuthResponse = []byte{}
+				fakeServer.GetKeysResponse = []byte(testGetKeysResponseOneKey)
+
+				return fakeServer
+			},
+		},
 	} {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
@@ -484,10 +500,12 @@ func TestPluginGenerateKey(t *testing.T) {
 			defer s.Close()
 
 			p := New()
+
 			options := []plugintest.Option{
 				plugintest.CaptureConfigureError(&err),
 				plugintest.CoreConfig(catalog.CoreConfig{TrustDomain: spiffeid.RequireTrustDomainFromString("example.org")}),
 			}
+
 			if tt.config != nil {
 				tt.config.KeyIdentifierFile = createKeyIdentifierFile(t)
 				tt.config.VaultAddr = fmt.Sprintf("https://%s", addr)
@@ -747,13 +765,19 @@ func setupSuccessFakeVaultServer(transitEnginePath string) *FakeVaultServerConfi
 	fakeVaultServer.K8sAuthReqEndpoint = "/v1/auth/test-k8s-auth/login"
 	fakeVaultServer.K8sAuthResponse = []byte(testK8sAuthResponse)
 
+	fakeVaultServer.LookupSelfResponseCode = 200
 	fakeVaultServer.LookupSelfResponse = []byte(testLookupSelfResponse)
 	fakeVaultServer.LookupSelfReqEndpoint = "GET /v1/auth/token/lookup-self"
-	fakeVaultServer.LookupSelfResponseCode = 200
 
 	fakeVaultServer.CreateKeyResponseCode = 200
 	fakeVaultServer.CreateKeyReqEndpoint = fmt.Sprintf("PUT /v1/%s/keys/{id}", transitEnginePath)
 
+	fakeVaultServer.DeleteKeyResponseCode = 204
+	fakeVaultServer.DeleteKeyReqEndpoint = fmt.Sprintf("DELETE /v1/%s/keys/{id}", transitEnginePath)
+
+	fakeVaultServer.UpdateKeyConfigurationResponseCode = 204
+	fakeVaultServer.UpdateKeyConfigurationReqEndpoint = fmt.Sprintf("PUT /v1/%s/keys/{id}/config", transitEnginePath)
+
 	fakeVaultServer.GetKeyResponseCode = 200
 	fakeVaultServer.GetKeyReqEndpoint = fmt.Sprintf("GET /v1/%s/keys/{id}", transitEnginePath)
 	fakeVaultServer.GetKeyResponse = []byte(testGetKeyResponseP256)

Diff for: pkg/server/plugin/keymanager/hashicorpvault/vault_client.go

@@ -389,7 +389,28 @@ func (c *Client) CreateKey(ctx context.Context, keyName string, keyType TransitK
 	return nil
 }
 
-// SignData signs the data using the transit engine key with the provided spire key id.
+// DeleteKey deletes a key in the specified transit secret engine
+// See: https://developer.hashicorp.com/vault/api-docs/secret/transit#update-key-configuration and https://developer.hashicorp.com/vault/api-docs/secret/transit#delete-key
+func (c *Client) DeleteKey(ctx context.Context, keyName string) error {
+	arguments := map[string]interface{}{
+		"deletion_allowed": "true",
+	}
+
+	// First, we need to enable deletion of the key. This is disabled by default.
+	_, err := c.vaultClient.Logical().WriteWithContext(ctx, fmt.Sprintf("/%s/keys/%s/config", c.clientParams.TransitEnginePath, keyName), arguments)
+	if err != nil {
+		return status.Errorf(codes.Internal, "failed to enable deletion of transit engine key: %v", err)
+	}
+
+	_, err = c.vaultClient.Logical().DeleteWithContext(ctx, fmt.Sprintf("/%s/keys/%s", c.clientParams.TransitEnginePath, keyName))
+	if err != nil {
+		return status.Errorf(codes.Internal, "failed to delete transit engine key: %v", err)
+	}
+
+	return nil
+}
+
+// SignData signs the data using the transit engine key with the key name.
 // See: https://developer.hashicorp.com/vault/api-docs/secret/transit#sign-data
 func (c *Client) SignData(ctx context.Context, keyName string, data []byte, hashAlgo TransitHashAlgorithm, signatureAlgo TransitSignatureAlgorithm) ([]byte, error) {
 	encodedData := base64.StdEncoding.EncodeToString(data)