Skip to content

Commit

Permalink
Update secret version hashing algorithm (#198)
Browse files Browse the repository at this point in the history
Updates the version generation algorithm to include an HMAC key. On each mount request, the provider will try to read the HMAC key from a Kubernetes secret and race to create it if not found. This ensures each provider produces consistent versions, and also makes recovering from unexpected errors easy (an admin just deletes the secret) without introducing the complexity and overhead of leader elections.

Also makes generating versions best-effort. If we can't use an HMAC key, we log a warning and revert to our pre-1.2.0 behaviour of not reporting a version at all, as consistency and reliability seem much more important than accurate version reporting. If versions thrash about unnecessarily it will cause lots of thrashing for any systems that observe the version.
  • Loading branch information
tomhjp authored Mar 30, 2023
1 parent ae45bb9 commit b0ab1fe
Show file tree
Hide file tree
Showing 15 changed files with 424 additions and 47 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ type FlagsConfig struct {
Version bool
HealthAddr string

HMACSecretName string

VaultAddr string
VaultMount string
VaultNamespace string
Expand Down
96 changes: 96 additions & 0 deletions internal/hmac/hmac.go
Original file line number Diff line number Diff line change
@@ -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
}
110 changes: 110 additions & 0 deletions internal/hmac/hmac_test.go
Original file line number Diff line number Diff line change
@@ -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
}
44 changes: 34 additions & 10 deletions internal/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package provider

import (
"context"
"crypto/hmac"
"crypto/sha256"
"encoding/base64"
"encoding/hex"
Expand All @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
Expand All @@ -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,
Expand Down
Loading

0 comments on commit b0ab1fe

Please sign in to comment.