From 87c44a3b8d3cae1ed2b5ecb5f382168a8f927fd5 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Fri, 25 Oct 2019 13:05:43 -0400 Subject: [PATCH] =?UTF-8?q?Ensure=20that=20cache=20entries=20for=20tokens?= =?UTF-8?q?=20are=20prefixed=20=E2=80=9Ctoken-secret=E2=80=A6=20(#6688)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be necessary once we store other types of identities in here. --- agent/consul/acl.go | 22 ++++++++++++++-------- agent/consul/acl_endpoint.go | 6 +++--- agent/consul/acl_endpoint_legacy.go | 2 +- agent/consul/acl_test.go | 24 ++++++++++++------------ agent/consul/acl_token_exp.go | 2 +- 5 files changed, 31 insertions(+), 25 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 6d1715c59..a9b52981c 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -89,6 +89,10 @@ func IsACLRemoteError(err error) bool { return ok } +func tokenSecretCacheID(token string) string { + return "token-secret:" + token +} + type ACLResolverDelegate interface { ACLsEnabled() bool ACLDatacenter(legacy bool) string @@ -343,6 +347,8 @@ func (r *ACLResolver) resolveTokenLegacy(token string) (acl.Authorizer, error) { } func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *structs.IdentityCacheEntry) (structs.ACLIdentity, error) { + cacheID := tokenSecretCacheID(token) + req := structs.ACLTokenGetRequest{ Datacenter: r.delegate.ACLDatacenter(false), TokenID: token, @@ -357,17 +363,17 @@ func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *struc err := r.delegate.RPC("ACL.TokenRead", &req, &resp) if err == nil { if resp.Token == nil { - r.cache.PutIdentity(token, nil) + r.cache.PutIdentity(cacheID, nil) return nil, acl.ErrNotFound } else { - r.cache.PutIdentity(token, resp.Token) + r.cache.PutIdentity(cacheID, resp.Token) return resp.Token, nil } } if acl.IsErrNotFound(err) { // Make sure to remove from the cache if it was deleted - r.cache.PutIdentity(token, nil) + r.cache.PutIdentity(cacheID, nil) return nil, acl.ErrNotFound } @@ -375,11 +381,11 @@ func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *struc // some other RPC error if cached != nil && (r.config.ACLDownPolicy == "extend-cache" || r.config.ACLDownPolicy == "async-cache") { // extend the cache - r.cache.PutIdentity(token, cached.Identity) + r.cache.PutIdentity(cacheID, cached.Identity) return cached.Identity, nil } - r.cache.PutIdentity(token, nil) + r.cache.PutIdentity(cacheID, nil) return nil, err } @@ -390,7 +396,7 @@ func (r *ACLResolver) resolveIdentityFromToken(token string) (structs.ACLIdentit } // Check the cache before making any RPC requests - cacheEntry := r.cache.GetIdentity(token) + cacheEntry := r.cache.GetIdentity(tokenSecretCacheID(token)) if cacheEntry != nil && cacheEntry.Age() <= r.config.ACLTokenTTL { metrics.IncrCounter([]string{"acl", "token", "cache_hit"}, 1) return cacheEntry.Identity, nil @@ -540,7 +546,7 @@ func (r *ACLResolver) maybeHandleIdentityErrorDuringFetch(identity structs.ACLId if acl.IsErrNotFound(err) { // make sure to indicate that this identity is no longer valid within // the cache - r.cache.PutIdentity(identity.SecretToken(), nil) + r.cache.PutIdentity(tokenSecretCacheID(identity.SecretToken()), nil) // Do not touch the cache. Getting a top level ACL not found error // only indicates that the secret token used in the request @@ -551,7 +557,7 @@ func (r *ACLResolver) maybeHandleIdentityErrorDuringFetch(identity structs.ACLId if acl.IsErrPermissionDenied(err) { // invalidate our ID cache so that identity resolution will take place // again in the future - r.cache.RemoveIdentity(identity.SecretToken()) + r.cache.RemoveIdentity(tokenSecretCacheID(identity.SecretToken())) // Do not remove from the cache for permission denied // what this does indicate is that our view of the token is out of date diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 7ac827e47..62fa6aafb 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -634,7 +634,7 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. } // Purge the identity from the cache to prevent using the previous definition of the identity - a.srv.acls.cache.RemoveIdentity(token.SecretID) + a.srv.acls.cache.RemoveIdentity(tokenSecretCacheID(token.SecretID)) if respErr, ok := resp.(error); ok { return respErr @@ -776,7 +776,7 @@ func (a *ACL) TokenDelete(args *structs.ACLTokenDeleteRequest, reply *string) er } // Purge the identity from the cache to prevent using the previous definition of the identity - a.srv.acls.cache.RemoveIdentity(token.SecretID) + a.srv.acls.cache.RemoveIdentity(tokenSecretCacheID(token.SecretID)) if respErr, ok := resp.(error); ok { return respErr @@ -2249,7 +2249,7 @@ func (a *ACL) Logout(args *structs.ACLLogoutRequest, reply *bool) error { // Purge the identity from the cache to prevent using the previous definition of the identity if token != nil { - a.srv.acls.cache.RemoveIdentity(token.SecretID) + a.srv.acls.cache.RemoveIdentity(tokenSecretCacheID(token.SecretID)) } if respErr, ok := resp.(error); ok { diff --git a/agent/consul/acl_endpoint_legacy.go b/agent/consul/acl_endpoint_legacy.go index e5bad0966..9c8b58d0f 100644 --- a/agent/consul/acl_endpoint_legacy.go +++ b/agent/consul/acl_endpoint_legacy.go @@ -186,7 +186,7 @@ func (a *ACL) Apply(args *structs.ACLRequest, reply *string) error { // Clear the cache if applicable if args.ACL.ID != "" { - a.srv.acls.cache.RemoveIdentity(args.ACL.ID) + a.srv.acls.cache.RemoveIdentity(tokenSecretCacheID(args.ACL.ID)) } return nil diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index cd9ff4f3b..94efed9f2 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -693,10 +693,10 @@ func TestACLResolver_ResolveRootACL(t *testing.T) { func TestACLResolver_DownPolicy(t *testing.T) { t.Parallel() - requireIdentityCached := func(t *testing.T, r *ACLResolver, token string, present bool, msg string) { + requireIdentityCached := func(t *testing.T, r *ACLResolver, id string, present bool, msg string) { t.Helper() - cacheVal := r.cache.GetIdentity(token) + cacheVal := r.cache.GetIdentity(id) require.NotNil(t, cacheVal) if present { require.NotNil(t, cacheVal.Identity, msg) @@ -738,7 +738,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.NotNil(t, authz) require.Equal(t, authz, acl.DenyAll()) - requireIdentityCached(t, r, "foo", false, "not present") + requireIdentityCached(t, r, tokenSecretCacheID("foo"), false, "not present") }) t.Run("Allow", func(t *testing.T) { @@ -763,7 +763,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.NotNil(t, authz) require.Equal(t, authz, acl.AllowAll()) - requireIdentityCached(t, r, "foo", false, "not present") + requireIdentityCached(t, r, tokenSecretCacheID("foo"), false, "not present") }) t.Run("Expired-Policy", func(t *testing.T) { @@ -857,7 +857,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - requireIdentityCached(t, r, "found", true, "cached") + requireIdentityCached(t, r, tokenSecretCacheID("found"), true, "cached") authz2, err := r.ResolveToken("found") require.NoError(t, err) @@ -888,7 +888,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - requireIdentityCached(t, r, "found-role", true, "still cached") + requireIdentityCached(t, r, tokenSecretCacheID("found-role"), true, "still cached") authz2, err := r.ResolveToken("found-role") require.NoError(t, err) @@ -1154,7 +1154,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - requireIdentityCached(t, r, "found", true, "cached") + requireIdentityCached(t, r, tokenSecretCacheID("found"), true, "cached") // The identity should have been cached so this should still be valid authz2, err := r.ResolveToken("found") @@ -1171,7 +1171,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { assert.Nil(t, authz3) }) - requireIdentityCached(t, r, "found", false, "no longer cached") + requireIdentityCached(t, r, tokenSecretCacheID("found"), false, "no longer cached") }) t.Run("PolicyResolve-TokenNotFound", func(t *testing.T) { @@ -1225,7 +1225,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) // Verify that the caches are setup properly. - requireIdentityCached(t, r, secretID, true, "cached") + requireIdentityCached(t, r, tokenSecretCacheID(secretID), true, "cached") requirePolicyCached(t, r, "node-wr", true, "cached") // from "found" token requirePolicyCached(t, r, "dc2-key-wr", true, "cached") // from "found" token @@ -1236,7 +1236,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { _, err = r.ResolveToken(secretID) require.True(t, acl.IsErrNotFound(err)) - requireIdentityCached(t, r, secretID, false, "identity not found cached") + requireIdentityCached(t, r, tokenSecretCacheID(secretID), false, "identity not found cached") requirePolicyCached(t, r, "node-wr", true, "still cached") require.Nil(t, r.cache.GetPolicy("dc2-key-wr"), "not stored at all") }) @@ -1287,7 +1287,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) // Verify that the caches are setup properly. - requireIdentityCached(t, r, secretID, true, "cached") + requireIdentityCached(t, r, tokenSecretCacheID(secretID), true, "cached") requirePolicyCached(t, r, "node-wr", true, "cached") // from "found" token requirePolicyCached(t, r, "dc2-key-wr", true, "cached") // from "found" token @@ -1298,7 +1298,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { _, err = r.ResolveToken(secretID) require.True(t, acl.IsErrPermissionDenied(err)) - require.Nil(t, r.cache.GetIdentity(secretID), "identity not stored at all") + require.Nil(t, r.cache.GetIdentity(tokenSecretCacheID(secretID)), "identity not stored at all") requirePolicyCached(t, r, "node-wr", true, "still cached") require.Nil(t, r.cache.GetPolicy("dc2-key-wr"), "not stored at all") }) diff --git a/agent/consul/acl_token_exp.go b/agent/consul/acl_token_exp.go index 38ac9513f..70eb9899c 100644 --- a/agent/consul/acl_token_exp.go +++ b/agent/consul/acl_token_exp.go @@ -106,7 +106,7 @@ func (s *Server) reapExpiredACLTokens(local, global bool) (int, error) { // Purge the identities from the cache for _, secretID := range secretIDs { - s.acls.cache.RemoveIdentity(secretID) + s.acls.cache.RemoveIdentity(tokenSecretCacheID(secretID)) } if respErr, ok := resp.(error); ok {