From ef3021f1a4ea9f32a596f6608987ddb82e62439b Mon Sep 17 00:00:00 2001 From: Violet Hynes Date: Wed, 29 Nov 2023 09:35:59 -0500 Subject: [PATCH] Fix bug in static secret caching where no token is present in a request to Proxy (#24287) --- command/agentproxyshared/cache/lease_cache.go | 6 +- command/proxy_test.go | 197 ++++++++++++++++++ 2 files changed, 200 insertions(+), 3 deletions(-) diff --git a/command/agentproxyshared/cache/lease_cache.go b/command/agentproxyshared/cache/lease_cache.go index 1b6dcc1e11..6e8b564a62 100644 --- a/command/agentproxyshared/cache/lease_cache.go +++ b/command/agentproxyshared/cache/lease_cache.go @@ -400,18 +400,18 @@ func (c *LeaseCache) Send(ctx context.Context, req *SendRequest) (*SendResponse, return nil, err } if cachedResp != nil { - c.logger.Debug("returning cached response", "path", req.Request.URL.Path) + c.logger.Debug("returning cached dynamic secret response", "path", req.Request.URL.Path) return cachedResp, nil } // Check if the response for this request is already in the static secret cache - if staticSecretCacheId != "" && req.Request.Method == http.MethodGet { + if staticSecretCacheId != "" && req.Request.Method == http.MethodGet && req.Token != "" { cachedResp, err = c.checkCacheForStaticSecretRequest(staticSecretCacheId, req) if err != nil { return nil, err } if cachedResp != nil { - c.logger.Debug("returning cached response", "id", staticSecretCacheId, "path", req.Request.URL.Path) + c.logger.Debug("returning cached static secret response", "id", staticSecretCacheId, "path", req.Request.URL.Path) return cachedResp, nil } } diff --git a/command/proxy_test.go b/command/proxy_test.go index ef856e9127..c8feb3e53e 100644 --- a/command/proxy_test.go +++ b/command/proxy_test.go @@ -1278,6 +1278,203 @@ log_level = "trace" // We expect that the cached value got updated by the event system. require.Equal(t, secretData2, data2) + // Lastly, ensure that a client without a token fails to access the secret. + proxyClient.SetToken("") + req = proxyClient.NewRequest(http.MethodGet, "/v1/secret-v2/data/my-secret") + _, err = proxyClient.RawRequest(req) + require.NotNil(t, err) + + _, err = proxyClient.KVv2("secret-v2").Get(context.Background(), "my-secret") + require.NotNil(t, err) + + close(cmd.ShutdownCh) + wg.Wait() +} + +// TestProxy_Cache_EventSystemUpdatesCacheUseAutoAuthToken Tests that the cache successfully caches a static secret +// going through the Proxy for a KVV2 secret, and that the cache works as expected with the +// use_auto_auth_token=force option. +func TestProxy_Cache_EventSystemUpdatesCacheUseAutoAuthToken(t *testing.T) { + logger := logging.NewVaultLogger(hclog.Trace) + cluster := vault.NewTestCluster(t, &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "kv": logicalKv.VersionedKVFactory, + }, + }, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + + serverClient := cluster.Cores[0].Client + + // Unset the environment variable so that proxy picks up the right test + // cluster address + defer os.Setenv(api.EnvVaultAddress, os.Getenv(api.EnvVaultAddress)) + os.Unsetenv(api.EnvVaultAddress) + + tokenFileName := makeTempFile(t, "token-file", serverClient.Token()) + defer os.Remove(tokenFileName) + // We need auto-auth so that the event system can run. + // For ease, we use the token file path with the root token. + autoAuthConfig := fmt.Sprintf(` +auto_auth { + method { + type = "token_file" + config = { + token_file_path = "%s" + } + } +}`, tokenFileName) + + cacheConfig := ` +cache { + cache_static_secrets = true +} +` + listenAddr := generateListenerAddress(t) + listenConfig := fmt.Sprintf(` +listener "tcp" { + address = "%s" + tls_disable = true +} +`, listenAddr) + + config := fmt.Sprintf(` +vault { + address = "%s" + tls_skip_verify = true +} +%s +%s +%s + +api_proxy { + use_auto_auth_token = "force" +} + +log_level = "trace" +`, serverClient.Address(), cacheConfig, listenConfig, autoAuthConfig) + configPath := makeTempFile(t, "config.hcl", config) + defer os.Remove(configPath) + + // Start proxy + ui, cmd := testProxyCommand(t, logger) + cmd.startedCh = make(chan struct{}) + + wg := &sync.WaitGroup{} + wg.Add(1) + go func() { + cmd.Run([]string{"-config", configPath}) + wg.Done() + }() + + select { + case <-cmd.startedCh: + case <-time.After(5 * time.Second): + t.Errorf("timeout") + t.Errorf("stdout: %s", ui.OutputWriter.String()) + t.Errorf("stderr: %s", ui.ErrorWriter.String()) + } + + proxyClient, err := api.NewClient(api.DefaultConfig()) + if err != nil { + t.Fatal(err) + } + proxyClient.SetToken(serverClient.Token()) + proxyClient.SetMaxRetries(0) + err = proxyClient.SetAddress("http://" + listenAddr) + if err != nil { + t.Fatal(err) + } + + secretData := map[string]interface{}{ + "foo": "bar", + } + + secretData2 := map[string]interface{}{ + "bar": "baz", + } + + // Wait for the event system to successfully connect. + // This is longer than it needs to be to account for unnatural slowness/avoiding + // flakiness. + time.Sleep(5 * time.Second) + + // Mount the KVV2 engine + err = serverClient.Sys().Mount("secret-v2", &api.MountInput{ + Type: "kv-v2", + }) + if err != nil { + t.Fatal(err) + } + + // Create kvv2 secret + _, err = serverClient.KVv2("secret-v2").Put(context.Background(), "my-secret", secretData) + if err != nil { + t.Fatal(err) + } + + // We use raw requests so we can check the headers for cache hit/miss. + req := proxyClient.NewRequest(http.MethodGet, "/v1/secret-v2/data/my-secret") + resp1, err := proxyClient.RawRequest(req) + if err != nil { + t.Fatal(err) + } + + cacheValue := resp1.Header.Get("X-Cache") + require.Equal(t, "MISS", cacheValue) + + // Update the secret using the proxy client + _, err = proxyClient.KVv2("secret-v2").Put(context.Background(), "my-secret", secretData2) + if err != nil { + t.Fatal(err) + } + + // Give some time for the event to actually get sent and the cache to be updated. + // This is longer than it needs to be to account for unnatural slowness/avoiding + // flakiness. + time.Sleep(5 * time.Second) + + // We expect this to be a cache hit, with the new value + resp2, err := proxyClient.RawRequest(req) + if err != nil { + t.Fatal(err) + } + + cacheValue = resp2.Header.Get("X-Cache") + require.Equal(t, "HIT", cacheValue) + + // Lastly, we check to make sure the actual data we received is + // as we expect. We must use ParseSecret due to the raw requests. + secret1, err := api.ParseSecret(resp1.Body) + require.Nil(t, err) + data, ok := secret1.Data["data"] + require.True(t, ok) + require.Equal(t, secretData, data) + + secret2, err := api.ParseSecret(resp2.Body) + require.Nil(t, err) + data2, ok := secret2.Data["data"] + require.True(t, ok) + // We expect that the cached value got updated by the event system. + require.Equal(t, secretData2, data2) + + // Lastly, ensure that a client without a token succeeds + // at accessing the secret, due to the use_auto_auth_token = "force" + // option. + proxyClient.SetToken("") + req = proxyClient.NewRequest(http.MethodGet, "/v1/secret-v2/data/my-secret") + resp3, err := proxyClient.RawRequest(req) + require.Nil(t, err) + cacheValue = resp3.Header.Get("X-Cache") + require.Equal(t, "HIT", cacheValue) + + secret3, err := api.ParseSecret(resp3.Body) + require.Nil(t, err) + data3, ok := secret3.Data["data"] + require.True(t, ok) + // We expect that the cached value got updated by the event system. + require.Equal(t, secretData2, data3) + close(cmd.ShutdownCh) wg.Wait() }