diff --git a/command/agentproxyshared/cache/static_secret_cache_updater.go b/command/agentproxyshared/cache/static_secret_cache_updater.go index 0af90f923e..9d316deee8 100644 --- a/command/agentproxyshared/cache/static_secret_cache_updater.go +++ b/command/agentproxyshared/cache/static_secret_cache_updater.go @@ -4,6 +4,7 @@ package cache import ( + "bufio" "bytes" "context" "encoding/json" @@ -12,7 +13,6 @@ import ( "io" "net/http" "net/url" - "strconv" "strings" "sync/atomic" "time" @@ -218,18 +218,6 @@ func (updater *StaticSecretCacheUpdater) streamStaticSecretEvents(ctx context.Co } } - // For all other operations, we *only* care about the latest version. - // However, if we know the current version, we should update that too - currentVersion := 0 - currentVersionString, ok := metadata["current_version"].(string) - if ok { - versionInt, err := strconv.Atoi(currentVersionString) - if err != nil { - return fmt.Errorf("unexpected event format when decoding 'current_version' element, message: %s\nerror: %w", string(message), err) - } - currentVersion = versionInt - } - // Note: For delete/destroy events, we continue through to updating the secret itself, too. // This means that if the latest version of the secret gets deleted, then the cache keeps // knowledge of which the latest version is. @@ -237,7 +225,7 @@ func (updater *StaticSecretCacheUpdater) streamStaticSecretEvents(ctx context.Co // to update the secret will 404. This is consistent with other behaviour. For Proxy, this means // the secret may be evicted. That's okay. - err = updater.updateStaticSecret(ctx, path, currentVersion) + err = updater.updateStaticSecret(ctx, path) if err != nil { // While we are kind of 'missing' an event this way, re-calling this function will // result in the secret remaining up to date. @@ -363,7 +351,7 @@ func (updater *StaticSecretCacheUpdater) preEventStreamUpdate(ctx context.Contex if index.Type != cacheboltdb.StaticSecretType { continue } - err = updater.updateStaticSecret(ctx, index.RequestPath, 0) + err = updater.updateStaticSecret(ctx, index.RequestPath) if err != nil { errs = multierror.Append(errs, err) } @@ -411,9 +399,9 @@ func (updater *StaticSecretCacheUpdater) handleDeleteDestroyVersions(path string } // updateStaticSecret checks for updates for a static secret on the path given, -// and updates the cache if appropriate. If currentVersion is not 0, we will also update -// will also update the version at index.Versions[currentVersion] with the same data. -func (updater *StaticSecretCacheUpdater) updateStaticSecret(ctx context.Context, path string, currentVersion int) error { +// and updates the cache if appropriate. For KVv2 secrets, we will also update +// the version at index.Versions[currentVersion] with the same data. +func (updater *StaticSecretCacheUpdater) updateStaticSecret(ctx context.Context, path string) error { // We clone the client, as we won't be using the same token. client, err := updater.client.Clone() if err != nil { @@ -514,19 +502,37 @@ func (updater *StaticSecretCacheUpdater) updateStaticSecret(ctx context.Context, return err } - // Set the index's Response index.Response = respBytes.Bytes() index.LastRenewed = time.Now().UTC() - if currentVersion != 0 { - // It should always be non-nil, but avoid a panic just in case. - if index.Versions == nil { - index.Versions = map[int][]byte{} - } - index.Versions[currentVersion] = index.Response + + // For KVv2 secrets, let's also update index.Versions[version_of_secret] + // with the response we received from the current version. + // Instead of relying on current_version in the event, we should + // check the message we received, since it's possible the secret + // got updated between receipt of the event and when we received + // the request for the secret. + // First, re-read secret into response so that we can parse it again: + reader := bufio.NewReader(bytes.NewReader(index.Response)) + resp, err := http.ReadResponse(reader, nil) + if err != nil { + // This shouldn't happen, but log just in case it does. There's + // no real negative consequences of the following function though. + updater.logger.Warn("failed to deserialize response", "error", err) } + secret, err := api.ParseSecret(resp.Body) + if err != nil { + // This shouldn't happen, but log just in case it does. There's + // no real negative consequences of the following function though. + updater.logger.Warn("failed to serialize response", "error", err) + } + + // In case of failures or KVv1 secrets, this function will simply fail silently, + // which is fine (and expected) since this could be arbitrary JSON. + updater.leaseCache.addToVersionListForCurrentVersionKVv2Secret(index, secret) + // Lastly, store the secret - updater.logger.Debug("storing response into the cache due to update", "path", path, "currentVersion", currentVersion) + updater.logger.Debug("storing response into the cache due to update", "path", path) err = updater.leaseCache.db.Set(index) if err != nil { return err diff --git a/command/agentproxyshared/cache/static_secret_cache_updater_test.go b/command/agentproxyshared/cache/static_secret_cache_updater_test.go index 69154975ec..81fc76508e 100644 --- a/command/agentproxyshared/cache/static_secret_cache_updater_test.go +++ b/command/agentproxyshared/cache/static_secret_cache_updater_test.go @@ -65,9 +65,7 @@ func testNewStaticSecretCacheUpdater(t *testing.T, client *api.Client) *StaticSe Logger: logging.NewVaultLogger(hclog.Trace).Named("cache.updater"), TokenSink: tokenSink, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) return updater } @@ -80,9 +78,7 @@ func TestNewStaticSecretCacheUpdater(t *testing.T) { config := api.DefaultConfig() logger := logging.NewVaultLogger(hclog.Trace).Named("cache.updater") client, err := api.NewClient(config) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) tokenSink := newMockSink(t) // Expect an error if any of the arguments are nil: @@ -129,9 +125,7 @@ func TestNewStaticSecretCacheUpdater(t *testing.T) { Logger: logging.NewVaultLogger(hclog.Trace).Named("cache.updater"), TokenSink: tokenSink, }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) require.NotNil(t, updater) } @@ -194,9 +188,7 @@ func TestOpenWebSocketConnection_BadPolicyToken(t *testing.T) { select { case <-ctx.Done(): case err := <-errCh: - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } }() @@ -245,9 +237,7 @@ func TestOpenWebSocketConnection_AutoAuthSelfHeal(t *testing.T) { select { case <-ctx.Done(): case err := <-errCh: - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } }() @@ -305,9 +295,7 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) { updater := testNewStaticSecretCacheUpdater(t, client) conn, err := updater.openWebSocketConnection(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) require.NotNil(t, conn) t.Cleanup(func() { @@ -321,24 +309,17 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) { } // Put a secret, which should trigger an event err = client.KVv1("secret").Put(context.Background(), "foo", makeData(100)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for i := 0; i < 5; i++ { // Do a fresh PUT just to refresh the secret and send a new message err = client.KVv1("secret").Put(context.Background(), "foo", makeData(i)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // This method blocks until it gets a secret, so this test // will only pass if we're receiving events correctly. - _, message, err := conn.Read(context.Background()) - if err != nil { - t.Fatal(err) - } - t.Log(string(message)) + _, _, err = conn.Read(context.Background()) + require.NoError(t, err) } } @@ -364,9 +345,7 @@ func TestOpenWebSocketConnectionReceivesEventsKVV1(t *testing.T) { updater := testNewStaticSecretCacheUpdater(t, client) conn, err := updater.openWebSocketConnection(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) require.NotNil(t, conn) t.Cleanup(func() { @@ -376,9 +355,7 @@ func TestOpenWebSocketConnectionReceivesEventsKVV1(t *testing.T) { err = client.Sys().Mount("secret-v1", &api.MountInput{ Type: "kv", }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) makeData := func(i int) map[string]interface{} { return map[string]interface{}{ @@ -387,23 +364,17 @@ func TestOpenWebSocketConnectionReceivesEventsKVV1(t *testing.T) { } // Put a secret, which should trigger an event err = client.KVv1("secret-v1").Put(context.Background(), "foo", makeData(100)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for i := 0; i < 5; i++ { // Do a fresh PUT just to refresh the secret and send a new message err = client.KVv1("secret-v1").Put(context.Background(), "foo", makeData(i)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // This method blocks until it gets a secret, so this test // will only pass if we're receiving events correctly. _, _, err := conn.Read(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } } @@ -429,9 +400,7 @@ func TestOpenWebSocketConnectionReceivesEventsKVV2(t *testing.T) { updater := testNewStaticSecretCacheUpdater(t, client) conn, err := updater.openWebSocketConnection(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) require.NotNil(t, conn) t.Cleanup(func() { @@ -447,29 +416,21 @@ func TestOpenWebSocketConnectionReceivesEventsKVV2(t *testing.T) { err = client.Sys().Mount("secret-v2", &api.MountInput{ Type: "kv-v2", }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Put a secret, which should trigger an event _, err = client.KVv2("secret-v2").Put(context.Background(), "foo", makeData(100)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for i := 0; i < 5; i++ { // Do a fresh PUT just to refresh the secret and send a new message _, err = client.KVv2("secret-v2").Put(context.Background(), "foo", makeData(i)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // This method blocks until it gets a secret, so this test // will only pass if we're receiving events correctly. _, _, err := conn.Read(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } } @@ -489,24 +450,18 @@ func TestOpenWebSocketConnectionTestServer(t *testing.T) { keys, rootToken := vault.TestCoreInit(t, core) for _, key := range keys { _, err := core.Unseal(key) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } config := api.DefaultConfig() config.Address = addr client, err := api.NewClient(config) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) client.SetToken(rootToken) updater := testNewStaticSecretCacheUpdater(t, client) conn, err := updater.openWebSocketConnection(context.Background()) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) require.NotNil(t, conn) } @@ -636,7 +591,7 @@ func TestUpdateStaticSecret(t *testing.T) { require.NoError(t, err) // attempt the update - err = updater.updateStaticSecret(context.Background(), path, 0) + err = updater.updateStaticSecret(context.Background(), path) require.NoError(t, err) newIndex, err := leaseCache.db.Get(cachememdb.IndexNameID, indexId) @@ -649,6 +604,72 @@ func TestUpdateStaticSecret(t *testing.T) { require.Len(t, newIndex.Versions, 0) } +// TestUpdateStaticSecret_KVv2 tests that updateStaticSecret works as expected, reaching out +// to Vault to get an updated secret when called. It should also update the corresponding +// version of that secret in the cache index's Versions field. +func TestUpdateStaticSecret_KVv2(t *testing.T) { + t.Parallel() + // We need a valid cluster for the connection to succeed. + cluster := vault.NewTestCluster(t, &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "kv": kv.VersionedKVFactory, + }, + }, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + client := cluster.Cores[0].Client + + updater := testNewStaticSecretCacheUpdater(t, client) + leaseCache := updater.leaseCache + + path := "secret-v2/data/foo" + indexId := hashStaticSecretIndex(path) + initialTime := time.Now().UTC() + // pre-populate the leaseCache with a secret to update + index := &cachememdb.Index{ + Namespace: "root/", + RequestPath: path, + LastRenewed: initialTime, + ID: indexId, + Versions: map[int][]byte{}, + // Valid token provided, so update should work. + Tokens: map[string]struct{}{client.Token(): {}}, + Response: []byte{}, + } + err := leaseCache.db.Set(index) + require.NoError(t, err) + + secretData := map[string]interface{}{ + "foo": "bar", + } + + err = client.Sys().Mount("secret-v2", &api.MountInput{ + Type: "kv-v2", + }) + require.NoError(t, err) + + // create the secret in Vault + _, err = client.KVv2("secret-v2").Put(context.Background(), "foo", secretData) + require.NoError(t, err) + + // attempt the update + err = updater.updateStaticSecret(context.Background(), path) + require.NoError(t, err) + + newIndex, err := leaseCache.db.Get(cachememdb.IndexNameID, indexId) + require.NoError(t, err) + require.NotNil(t, newIndex) + require.Truef(t, initialTime.Before(newIndex.LastRenewed), "last updated time not updated on index") + require.NotEqual(t, []byte{}, newIndex.Response) + require.Equal(t, index.RequestPath, newIndex.RequestPath) + require.Equal(t, index.Tokens, newIndex.Tokens) + + // It should have also updated version 1 with the same version. + require.Len(t, newIndex.Versions, 1) + require.NotNil(t, newIndex.Versions[1]) + require.Equal(t, newIndex.Versions[1], newIndex.Response) +} + // TestUpdateStaticSecret_EvictsIfInvalidTokens tests that updateStaticSecret will // evict secrets from the cache if no valid tokens are left. func TestUpdateStaticSecret_EvictsIfInvalidTokens(t *testing.T) { @@ -676,9 +697,7 @@ func TestUpdateStaticSecret_EvictsIfInvalidTokens(t *testing.T) { Tokens: map[string]struct{}{"invalid token": {}}, } err := leaseCache.db.Set(index) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) secretData := map[string]interface{}{ "foo": "bar", @@ -686,15 +705,11 @@ func TestUpdateStaticSecret_EvictsIfInvalidTokens(t *testing.T) { // create the secret in Vault. n.b. the test cluster has already mounted the KVv1 backend at "secret" err = client.KVv1("secret").Put(context.Background(), "foo", secretData) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // attempt the update - err = updater.updateStaticSecret(context.Background(), path, 0) - if err != nil { - t.Fatal(err) - } + err = updater.updateStaticSecret(context.Background(), path) + require.NoError(t, err) newIndex, err := leaseCache.db.Get(cachememdb.IndexNameID, indexId) require.Equal(t, cachememdb.ErrCacheItemNotFound, err) @@ -715,13 +730,8 @@ func TestUpdateStaticSecret_HandlesNonCachedPaths(t *testing.T) { path := "secret/foo" - // Attempt the update for with currentVersion 0 - err := updater.updateStaticSecret(context.Background(), path, 0) - require.NoError(t, err) - require.Nil(t, err) - - // Attempt a higher currentVersion just to be sure - err = updater.updateStaticSecret(context.Background(), path, 100) + // Attempt the update + err := updater.updateStaticSecret(context.Background(), path) require.NoError(t, err) require.Nil(t, err) } @@ -821,9 +831,7 @@ func TestPreEventStreamUpdateErrorUpdating(t *testing.T) { Type: cacheboltdb.StaticSecretType, } err := leaseCache.db.Set(index) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) secretData := map[string]interface{}{ "foo": "bar", @@ -832,15 +840,11 @@ func TestPreEventStreamUpdateErrorUpdating(t *testing.T) { err = client.Sys().Mount("secret-v2", &api.MountInput{ Type: "kv-v2", }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Put a secret (with different values to what's currently in the cache) _, err = client.KVv2("secret-v2").Put(context.Background(), "foo", secretData) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // Seal Vault, so that the update will fail cluster.EnsureCoresSealed(t)