Update secret version hashing algorithm

CHANGELOG.md

@@ -9,6 +9,10 @@ CHANGES:
     [`tokenRequests`](https://github.com/kubernetes-sigs/secrets-store-csi-driver/tree/main/charts/secrets-store-csi-driver#configuration)
     option from the _driver_ helm chart via the flag `--set tokenRequests[0].audience="vault"`. See
     [CSI TokenRequests documentation](https://kubernetes-csi.github.io/docs/token-requests.html) for further details.
+* Vault CSI Provider now creates a Kubernetes secret with an HMAC key to produce consistent hashes for secret versions. [[GH-198](https://github.com/hashicorp/vault-csi-provider/pull/198)]
+  * Requires RBAC permissions to create secrets, and read the same specific secret back. Versions are not generated otherwise and a warning
+    is logged on each mount that fails to generate a version.
+  * Supports creating the secret with custom name via `-hmac-secret-name`
 
 IMPROVEMENTS:
 

Makefile

@@ -78,6 +78,7 @@ e2e-setup:
 		--set linux.image.pullPolicy="IfNotPresent" \
 		--set syncSecret.enabled=true \
 		--set tokenRequests[0].audience="vault"
+	kubectl apply --namespace=csi -f test/bats/configs/vault/hmac-secret-role.yaml
 	helm install vault-bootstrap test/bats/configs/vault \
 		--namespace=csi
 	helm install vault vault \

go.mod

@@ -11,6 +11,7 @@ require (
 	k8s.io/api v0.25.4
 	k8s.io/apimachinery v0.25.4
 	k8s.io/client-go v0.25.4
+	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed
 	sigs.k8s.io/secrets-store-csi-driver v1.2.4
 )
 
@@ -84,7 +85,6 @@ require (
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	k8s.io/klog/v2 v2.70.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect
-	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
 	sigs.k8s.io/yaml v1.3.0 // indirect

internal/config/config.go

@@ -33,6 +33,8 @@ type FlagsConfig struct {
 	Version    bool
 	HealthAddr string
 
+	HMACSecretName string
+
 	VaultAddr      string
 	VaultMount     string
 	VaultNamespace string

internal/hmac/hmac.go

@@ -0,0 +1,96 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package hmac
+
+import (
+	"context"
+	"crypto/rand"
+	"errors"
+	"fmt"
+
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+)
+
+const (
+	hmacKeyName   = "key"
+	hmacKeyLength = 32
+)
+
+var errDeleteSecret = errors.New("delete the kubernetes secret to trigger an automatic regeneration")
+
+func NewHMACGenerator(client kubernetes.Interface, secretSpec *corev1.Secret) *HMACGenerator {
+	return &HMACGenerator{
+		client:     client,
+		secretSpec: secretSpec,
+	}
+}
+
+type HMACGenerator struct {
+	client     kubernetes.Interface
+	secretSpec *corev1.Secret
+}
+
+// GetOrCreateHMACKey will try to read an HMAC key from a Kubernetes secret and
+// race with other pods to create it if not found. The HMAC key is persisted to
+// a Kubernetes secret to ensure all pods are deterministically producing the
+// same version hashes when given the same inputs.
+func (g *HMACGenerator) GetOrCreateHMACKey(ctx context.Context) ([]byte, error) {
+	// Fast path - most of the time the secret will already be created.
+	secret, err := g.client.CoreV1().Secrets(g.secretSpec.Namespace).Get(ctx, g.secretSpec.Name, metav1.GetOptions{})
+	if err == nil {
+		return hmacKeyFromSecret(secret)
+	}
+	if !apierrors.IsNotFound(err) {
+		return nil, err
+	}
+
+	// Secret not found. We'll join the race to create it.
+	hmacKeyCandidate := make([]byte, hmacKeyLength)
+	_, err = rand.Read(hmacKeyCandidate)
+	if err != nil {
+		return nil, err
+	}
+
+	// Make a copy of the secretSpec to avoid a data race.
+	secretSpec := *g.secretSpec
+	secretSpec.Data = map[string][]byte{
+		hmacKeyName: hmacKeyCandidate,
+	}
+
+	var persistedHMACSecret *corev1.Secret
+
+	// Try to create first
+	persistedHMACSecret, err = g.client.CoreV1().Secrets(secretSpec.Namespace).Create(ctx, &secretSpec, metav1.CreateOptions{})
+	switch {
+	case err == nil:
+		// We created the secret, nothing to handle.
+	case apierrors.IsAlreadyExists(err):
+		// We lost the race to create the secret. Read the existing secret instead.
+		persistedHMACSecret, err = g.client.CoreV1().Secrets(secretSpec.Namespace).Get(ctx, secretSpec.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+	default:
+		// Unexpected error case.
+		return nil, err
+	}
+
+	return hmacKeyFromSecret(persistedHMACSecret)
+}
+
+func hmacKeyFromSecret(secret *corev1.Secret) ([]byte, error) {
+	hmacKey, ok := secret.Data[hmacKeyName]
+	if !ok {
+		return nil, fmt.Errorf("expected secret %q to have a key %q; %w", secret.Name, hmacKeyName, errDeleteSecret)
+	}
+
+	if len(hmacKey) == 0 {
+		return nil, fmt.Errorf("expected secret %q to have a non-zero HMAC key; %w", secret.Name, errDeleteSecret)
+	}
+
+	return hmacKey, nil
+}

internal/hmac/hmac_test.go

@@ -0,0 +1,110 @@
+package hmac
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
+)
+
+const (
+	secretName      = "test-secret"
+	secretNamespace = "test-namespace"
+)
+
+var secretSpec = &corev1.Secret{
+	ObjectMeta: metav1.ObjectMeta{
+		Name:      secretName,
+		Namespace: secretNamespace,
+	},
+	Data: map[string][]byte{
+		hmacKeyName: []byte(strings.Repeat("a", 32)),
+	},
+}
+
+func setup(t *testing.T) (*HMACGenerator, *fake.Clientset) {
+	client := fake.NewSimpleClientset()
+	return NewHMACGenerator(client, secretSpec), client
+}
+
+func TestGenerateSecretIfNoneExists(t *testing.T) {
+	gen, client := setup(t)
+
+	// Add counter functions.
+	createCount := countAPICalls(client, "create", "secrets")
+	getCount := countAPICalls(client, "get", "secrets")
+
+	// Get an HMAC key, which should create the k8s secret.
+	key, err := gen.GetOrCreateHMACKey(context.Background())
+	require.NoError(t, err)
+	assert.Len(t, key, hmacKeyLength)
+	assert.Equal(t, 1, *createCount)
+	assert.Equal(t, 1, *getCount)
+	assert.NotEqual(t, string(secretSpec.Data[hmacKeyName]), string(key))
+	assert.NotEmpty(t, string(key))
+}
+
+func TestReadSecretIfAlreadyExists(t *testing.T) {
+	gen, client := setup(t)
+
+	ctx := context.Background()
+	_, err := client.CoreV1().Secrets(secretNamespace).Create(ctx, secretSpec, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	// Add counter functions.
+	createCount := countAPICalls(client, "create", "secrets")
+	getCount := countAPICalls(client, "get", "secrets")
+
+	// Get an HMAC key, which should read the existing k8s secret.
+	key, err := gen.GetOrCreateHMACKey(ctx)
+	require.NoError(t, err)
+	assert.Len(t, key, hmacKeyLength)
+	assert.Equal(t, 0, *createCount)
+	assert.Equal(t, 1, *getCount)
+	assert.Equal(t, string(secretSpec.Data[hmacKeyName]), string(key))
+}
+
+func TestGracefullyHandlesLosingTheRace(t *testing.T) {
+	gen, client := setup(t)
+
+	ctx := context.Background()
+
+	client.PrependReactor("create", "secrets", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+		// Intercept the create call and create the secret just before.
+		err = client.Tracker().Create(schema.GroupVersionResource{
+			Group:    "",
+			Version:  "v1",
+			Resource: "secrets",
+		}, secretSpec, secretNamespace)
+		require.NoError(t, err)
+		return false, nil, nil
+	})
+	createCount := countAPICalls(client, "create", "secrets")
+	getCount := countAPICalls(client, "get", "secrets")
+
+	// Get an HMAC key, which should initially find no secret, and then lose the race for creating it.
+	key, err := gen.GetOrCreateHMACKey(ctx)
+	require.NoError(t, err)
+	assert.Len(t, key, hmacKeyLength)
+	assert.Equal(t, 1, *createCount)
+	assert.Equal(t, 2, *getCount)
+	assert.Equal(t, string(secretSpec.Data[hmacKeyName]), string(key))
+}
+
+// Counts the number of times an API is called.
+func countAPICalls(client *fake.Clientset, verb string, resource string) *int {
+	i := 0
+	client.PrependReactor(verb, resource, func(_ k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+		i++
+		return false, nil, nil
+	})
+	return &i
+}

internal/provider/provider.go

@@ -5,6 +5,7 @@ package provider
 
 import (
 	"context"
+	"crypto/hmac"
 	"crypto/sha256"
 	"encoding/base64"
 	"encoding/hex"
@@ -18,6 +19,7 @@ import (
 	"github.com/hashicorp/go-hclog"
 	vaultclient "github.com/hashicorp/vault-csi-provider/internal/client"
 	"github.com/hashicorp/vault-csi-provider/internal/config"
+	hmacgen "github.com/hashicorp/vault-csi-provider/internal/hmac"
 	"github.com/hashicorp/vault/api"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,14 +34,16 @@ type provider struct {
 	cache  map[cacheKey]*api.Secret
 
 	// Allows mocking Kubernetes API for tests.
-	k8sClient kubernetes.Interface
+	k8sClient     kubernetes.Interface
+	hmacGenerator *hmacgen.HMACGenerator
 }
 
-func NewProvider(logger hclog.Logger, k8sClient kubernetes.Interface) *provider {
+func NewProvider(logger hclog.Logger, k8sClient kubernetes.Interface, hmacGenerator *hmacgen.HMACGenerator) *provider {
 	p := &provider{
-		logger:    logger,
-		cache:     make(map[cacheKey]*api.Secret),
-		k8sClient: k8sClient,
+		logger:        logger,
+		cache:         make(map[cacheKey]*api.Secret),
+		k8sClient:     k8sClient,
+		hmacGenerator: hmacGenerator,
 	}
 
 	return p
@@ -234,7 +238,7 @@ func (p *provider) getSecret(ctx context.Context, client *api.Client, secretConf
 		}
 
 		for _, w := range secret.Warnings {
-			p.logger.Warn("warning in response from Vault API", "warning", w)
+			p.logger.Warn("Warning in response from Vault API", "warning", w)
 		}
 
 		p.cache[key] = secret
@@ -267,6 +271,10 @@ func (p *provider) getSecret(ctx context.Context, client *api.Client, secretConf
 
 // MountSecretsStoreObjectContent mounts content of the vault object to target path
 func (p *provider) HandleMountRequest(ctx context.Context, cfg config.Config, flagsConfig config.FlagsConfig) (*pb.MountResponse, error) {
+	hmacKey, err := p.hmacGenerator.GetOrCreateHMACKey(ctx)
+	if err != nil {
+		p.logger.Warn("Error generating HMAC key. Mounted secrets will not be assigned a version", "error", err)
+	}
 	client, err := vaultclient.New(cfg.Parameters, flagsConfig)
 	if err != nil {
 		return nil, err
@@ -291,7 +299,7 @@ func (p *provider) HandleMountRequest(ctx context.Context, cfg config.Config, fl
 			return nil, err
 		}
 
-		version, err := generateObjectVersion(secret, content)
+		version, err := generateObjectVersion(secret, hmacKey, content)
 		if err != nil {
 			return nil, fmt.Errorf("failed to generate version for object name %q: %w", secret.ObjectName, err)
 		}
@@ -311,14 +319,30 @@ func (p *provider) HandleMountRequest(ctx context.Context, cfg config.Config, fl
 	}, nil
 }
 
-func generateObjectVersion(secret config.Secret, content []byte) (*pb.ObjectVersion, error) {
-	hash := sha256.New()
+func generateObjectVersion(secret config.Secret, hmacKey []byte, content []byte) (*pb.ObjectVersion, error) {
+	// If something went wrong with generating the HMAC key, we log the error and
+	// treat generating the version as best-effort instead, as delivering the secret
+	// is generally more critical to workloads than assigning a version for it.
+	if hmacKey == nil {
+		return &pb.ObjectVersion{
+			Id:      secret.ObjectName,
+			Version: "",
+		}, nil
+	}
+
 	// We include the secret config in the hash input to avoid leaking information
 	// about different secrets that could have the same content.
-	_, err := hash.Write([]byte(fmt.Sprintf("%v:%s", secret, content)))
+	hash := hmac.New(sha256.New, hmacKey)
+	cfg, err := json.Marshal(secret)
 	if err != nil {
 		return nil, err
 	}
+	if _, err := hash.Write(cfg); err != nil {
+		return nil, err
+	}
+	if _, err := hash.Write(content); err != nil {
+		return nil, err
+	}
 
 	return &pb.ObjectVersion{
 		Id:      secret.ObjectName,