diff --git a/.changelog/12166.txt b/.changelog/12166.txt new file mode 100644 index 0000000000..06b4340003 --- /dev/null +++ b/.changelog/12166.txt @@ -0,0 +1,3 @@ +```release-note:deprecation +acl: The `consul.acl.ResolveTokenToIdentity` metric is no longer reported. The values that were previous reported as part of this metric will now be part of the `consul.acl.ResolveToken` metric. +``` diff --git a/agent/acl.go b/agent/acl.go index fa2259cfd3..0c0334a2f0 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -15,7 +15,7 @@ import ( // critical purposes, such as logging. Therefore we interpret all errors as empty-string // so we can safely log it without handling non-critical errors at the usage site. func (a *Agent) aclAccessorID(secretID string) string { - ident, err := a.delegate.ResolveTokenToIdentity(secretID) + ident, err := a.delegate.ResolveTokenAndDefaultMeta(secretID, nil, nil) if acl.IsErrNotFound(err) { return "" } @@ -23,10 +23,7 @@ func (a *Agent) aclAccessorID(secretID string) string { a.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err) return "" } - if ident == nil { - return "" - } - return ident.ID() + return ident.AccessorID() } // vetServiceRegister makes sure the service registration action is allowed by @@ -174,7 +171,7 @@ func (a *Agent) filterMembers(token string, members *[]serf.Member) error { if authz.NodeRead(node, &authzContext) == acl.Allow { continue } - accessorID := a.aclAccessorID(token) + accessorID := authz.AccessorID() a.logger.Debug("dropping node from result due to ACLs", "node", node, "accessorID", accessorID) m = append(m[:i], m[i+1:]...) i-- diff --git a/agent/acl_test.go b/agent/acl_test.go index c085e49a39..a26798bba8 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -94,18 +94,10 @@ func (a *TestACLAgent) ResolveTokenToIdentityAndAuthorizer(secretID string) (str return a.resolveAuthzFn(secretID) } -func (a *TestACLAgent) ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error) { - if a.resolveIdentFn == nil { - return nil, fmt.Errorf("ResolveTokenToIdentity call is unexpected - no ident resolver callback set") - } - - return a.resolveIdentFn(secretID) -} - -func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) { +func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) { identity, authz, err := a.ResolveTokenToIdentityAndAuthorizer(secretID) if err != nil { - return nil, err + return consul.ACLResolveResult{}, err } // Default the EnterpriseMeta based on the Tokens meta or actual defaults @@ -119,7 +111,7 @@ func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *stru // Use the meta to fill in the ACL authorization context entMeta.FillAuthzContext(authzContext) - return authz, err + return consul.ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err } // All of these are stubs to satisfy the interface @@ -523,22 +515,3 @@ func TestACL_filterChecksWithAuthorizer(t *testing.T) { _, ok = checks["my-other"] require.False(t, ok) } - -// TODO: remove? -func TestACL_ResolveIdentity(t *testing.T) { - t.Parallel() - a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, catalogIdent) - - // this test is meant to ensure we are calling the correct function - // which is ResolveTokenToIdentity on the Agent delegate. Our - // nil authz resolver will cause it to emit an error if used - ident, err := a.delegate.ResolveTokenToIdentity(nodeROSecret) - require.NoError(t, err) - require.NotNil(t, ident) - - // just double checkingto ensure if we had used the wrong function - // that an error would be produced - _, err = a.delegate.ResolveTokenAndDefaultMeta(nodeROSecret, nil, nil) - require.Error(t, err) - -} diff --git a/agent/agent.go b/agent/agent.go index 41bab89e7f..7e2a7f110a 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -167,14 +167,11 @@ type delegate interface { // RemoveFailedNode is used to remove a failed node from the cluster. RemoveFailedNode(node string, prune bool, entMeta *structs.EnterpriseMeta) error - // TODO: replace this method with consul.ACLResolver - ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) - // ResolveTokenAndDefaultMeta returns an acl.Authorizer which authorizes // actions based on the permissions granted to the token. // If either entMeta or authzContext are non-nil they will be populated with the // default partition and namespace from the token. - ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) + ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) RPC(method string, args interface{}, reply interface{}) error SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 8e965db38e..70840f9502 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -1640,8 +1640,8 @@ type fakeResolveTokenDelegate struct { authorizer acl.Authorizer } -func (f fakeResolveTokenDelegate) ResolveTokenAndDefaultMeta(_ string, _ *structs.EnterpriseMeta, _ *acl.AuthorizerContext) (acl.Authorizer, error) { - return f.authorizer, nil +func (f fakeResolveTokenDelegate) ResolveTokenAndDefaultMeta(_ string, _ *structs.EnterpriseMeta, _ *acl.AuthorizerContext) (consul.ACLResolveResult, error) { + return consul.ACLResolveResult{Authorizer: f.authorizer}, nil } func TestAgent_Reload(t *testing.T) { diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 9f12bf9baf..846e02d26c 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -34,10 +34,6 @@ var ACLSummaries = []prometheus.SummaryDefinition{ Name: []string{"acl", "ResolveToken"}, Help: "This measures the time it takes to resolve an ACL token.", }, - { - Name: []string{"acl", "ResolveTokenToIdentity"}, - Help: "This measures the time it takes to resolve an ACL token to an Identity.", - }, } // These must be kept in sync with the constants in command/agent/acl.go. @@ -1117,31 +1113,17 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs return identity, acl.NewChainedAuthorizer(chain), nil } -// TODO: rename to AccessorIDFromToken. This method is only used to retrieve the -// ACLIdentity.ID, so we don't need to return a full ACLIdentity. We could -// return a much smaller type (instad of just a string) to allow for changes -// in the future. -func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) { - if !r.ACLsEnabled() { - return nil, nil +type ACLResolveResult struct { + acl.Authorizer + // TODO: likely we can reduce this interface + structs.ACLIdentity +} + +func (a ACLResolveResult) AccessorID() string { + if a.ACLIdentity == nil { + return "" } - - if acl.RootAuthorizer(token) != nil { - return nil, acl.ErrRootDenied - } - - // handle the anonymous token - if token == "" { - token = anonymousToken - } - - if ident, _, ok := r.resolveLocallyManagedToken(token); ok { - return ident, nil - } - - defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now()) - - return r.resolveIdentityFromToken(token) + return a.ACLIdentity.ID() } func (r *ACLResolver) ACLsEnabled() bool { @@ -1160,10 +1142,10 @@ func (r *ACLResolver) ACLsEnabled() bool { return true } -func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) { +func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (ACLResolveResult, error) { identity, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) if err != nil { - return nil, err + return ACLResolveResult{}, err } if entMeta == nil { @@ -1181,7 +1163,7 @@ func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs. // Use the meta to fill in the ACL authorization context entMeta.FillAuthzContext(authzContext) - return authz, err + return ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err } // aclFilter is used to filter results from our state store based on ACL rules diff --git a/agent/consul/acl_server.go b/agent/consul/acl_server.go index f1567b496f..30187f9f12 100644 --- a/agent/consul/acl_server.go +++ b/agent/consul/acl_server.go @@ -165,13 +165,12 @@ func (s *serverACLResolverBackend) ResolveRoleFromID(roleID string) (bool, *stru return s.InPrimaryDatacenter() || index > 0, role, acl.ErrNotFound } +// TODO: remove func (s *Server) ResolveToken(token string) (acl.Authorizer, error) { _, authz, err := s.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token) return authz, err } -// TODO: Client has an identical implementation, remove duplication - func (s *Server) filterACL(token string, subj interface{}) error { return filterACL(s.ACLResolver, token, subj) } diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 729ed869e6..3219cebb4f 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -1534,36 +1534,6 @@ func TestACLResolver_Client(t *testing.T) { require.Equal(t, policyResolves, int32(3)) }) - t.Run("Resolve-Identity", func(t *testing.T) { - t.Parallel() - - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: false, - localTokens: false, - localPolicies: false, - } - - delegate.tokenReadFn = delegate.plainTokenReadFn - delegate.policyResolveFn = delegate.plainPolicyResolveFn - delegate.roleResolveFn = delegate.plainRoleResolveFn - - r := newTestACLResolver(t, delegate, nil) - - ident, err := r.ResolveTokenToIdentity("found-policy-and-role") - require.NoError(t, err) - require.NotNil(t, ident) - require.Equal(t, "5f57c1f6-6a89-4186-9445-531b316e01df", ident.ID()) - require.EqualValues(t, 0, delegate.localTokenResolutions) - require.EqualValues(t, 1, delegate.remoteTokenResolutions) - require.EqualValues(t, 0, delegate.localPolicyResolutions) - require.EqualValues(t, 0, delegate.remotePolicyResolutions) - require.EqualValues(t, 0, delegate.localRoleResolutions) - require.EqualValues(t, 0, delegate.remoteRoleResolutions) - require.EqualValues(t, 0, delegate.remoteLegacyResolutions) - }) - t.Run("Concurrent-Token-Resolve", func(t *testing.T) { t.Parallel() diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index 814d1590a6..2031641caa 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -100,6 +100,7 @@ func (s *Intention) Apply(args *structs.IntentionRequest, reply *string) error { } // Get the ACL token for the request for the checks below. + // TODO: use ResolveTokenAndDefaultMeta identity, authz, err := s.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) if err != nil { return err @@ -432,7 +433,8 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde // Get the ACL token for the request for the checks below. var entMeta structs.EnterpriseMeta - if _, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil); err != nil { + authz, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil) + if err != nil { return err } @@ -479,13 +481,11 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde reply.Intentions = structs.Intentions{ixn} // Filter - if err := s.srv.filterACL(args.Token, reply); err != nil { - return err - } + s.srv.filterACLWithAuthorizer(authz, reply) // If ACLs prevented any responses, error if len(reply.Intentions) == 0 { - accessorID := s.aclAccessorID(args.Token) + accessorID := authz.AccessorID() // todo(kit) Migrate intention access denial logging over to audit logging when we implement it s.logger.Warn("Request to get intention denied due to ACLs", "intention", args.IntentionID, "accessorID", accessorID) return acl.ErrPermissionDenied @@ -618,7 +618,7 @@ func (s *Intention) Match(args *structs.IntentionQueryRequest, reply *structs.In for _, entry := range args.Match.Entries { entry.FillAuthzContext(&authzContext) if prefix := entry.Name; prefix != "" && authz.IntentionRead(prefix, &authzContext) != acl.Allow { - accessorID := s.aclAccessorID(args.Token) + accessorID := authz.AccessorID() // todo(kit) Migrate intention access denial logging over to audit logging when we implement it s.logger.Warn("Operation on intention prefix denied due to ACLs", "prefix", prefix, "accessorID", accessorID) return acl.ErrPermissionDenied @@ -708,7 +708,7 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In var authzContext acl.AuthorizerContext query.FillAuthzContext(&authzContext) if authz.ServiceRead(prefix, &authzContext) != acl.Allow { - accessorID := s.aclAccessorID(args.Token) + accessorID := authz.AccessorID() // todo(kit) Migrate intention access denial logging over to audit logging when we implement it s.logger.Warn("test on intention denied due to ACLs", "prefix", prefix, "accessorID", accessorID) return acl.ErrPermissionDenied @@ -760,24 +760,6 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In return nil } -// aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non- -// critical purposes, such as logging. Therefore we interpret all errors as empty-string -// so we can safely log it without handling non-critical errors at the usage site. -func (s *Intention) aclAccessorID(secretID string) string { - _, ident, err := s.srv.ResolveIdentityFromToken(secretID) - if acl.IsErrNotFound(err) { - return "" - } - if err != nil { - s.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err) - return "" - } - if ident == nil { - return "" - } - return ident.ID() -} - func (s *Intention) validateEnterpriseIntention(ixn *structs.Intention) error { if err := s.srv.validateEnterpriseIntentionPartition(ixn.SourcePartition); err != nil { return fmt.Errorf("Invalid source partition %q: %v", ixn.SourcePartition, err) diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index dea21cfc6f..5213349e79 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -401,13 +401,13 @@ func (m *Internal) EventFire(args *structs.EventFireRequest, } // Check ACLs - authz, err := m.srv.ResolveToken(args.Token) + authz, err := m.srv.ResolveTokenAndDefaultMeta(args.Token, nil, nil) if err != nil { return err } if authz.EventWrite(args.Name, nil) != acl.Allow { - accessorID := m.aclAccessorID(args.Token) + accessorID := authz.AccessorID() m.logger.Warn("user event blocked by ACLs", "event", args.Name, "accessorID", accessorID) return acl.ErrPermissionDenied } @@ -545,21 +545,3 @@ func (m *Internal) executeKeyringOpMgr( return serfResp, err } - -// aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non- -// critical purposes, such as logging. Therefore we interpret all errors as empty-string -// so we can safely log it without handling non-critical errors at the usage site. -func (m *Internal) aclAccessorID(secretID string) string { - _, ident, err := m.srv.ResolveIdentityFromToken(secretID) - if acl.IsErrNotFound(err) { - return "" - } - if err != nil { - m.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err) - return "" - } - if ident == nil { - return "" - } - return ident.ID() -} diff --git a/agent/delegate_mock_test.go b/agent/delegate_mock_test.go index 678b0b87bc..d2c6e267ce 100644 --- a/agent/delegate_mock_test.go +++ b/agent/delegate_mock_test.go @@ -47,11 +47,6 @@ func (m *delegateMock) RemoveFailedNode(node string, prune bool, entMeta *struct return m.Called(node, prune, entMeta).Error(0) } -func (m *delegateMock) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) { - ret := m.Called(token) - return ret.Get(0).(structs.ACLIdentity), ret.Error(1) -} - func (m *delegateMock) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) { ret := m.Called(token, entMeta, authzContext) return ret.Get(0).(acl.Authorizer), ret.Error(1) diff --git a/agent/local/state.go b/agent/local/state.go index 5c70b0c8d0..1eb5733bbe 100644 --- a/agent/local/state.go +++ b/agent/local/state.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/go-hclog" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/api" @@ -150,7 +151,7 @@ func (c *CheckState) CriticalFor() time.Duration { type rpc interface { RPC(method string, args interface{}, reply interface{}) error - ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error) + ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) } // State is used to represent the node's services, @@ -1538,7 +1539,7 @@ func (l *State) notifyIfAliased(serviceID structs.ServiceID) { // critical purposes, such as logging. Therefore we interpret all errors as empty-string // so we can safely log it without handling non-critical errors at the usage site. func (l *State) aclAccessorID(secretID string) string { - ident, err := l.Delegate.ResolveTokenToIdentity(secretID) + ident, err := l.Delegate.ResolveTokenAndDefaultMeta(secretID, nil, nil) if acl.IsErrNotFound(err) { return "" } @@ -1546,8 +1547,5 @@ func (l *State) aclAccessorID(secretID string) string { l.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err) return "" } - if ident == nil { - return "" - } - return ident.ID() + return ident.AccessorID() } diff --git a/agent/local/state_test.go b/agent/local/state_test.go index 036e156fc1..1be9274e1a 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -12,8 +12,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent/config" + "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/local" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/token" @@ -2372,6 +2374,6 @@ func (f *fakeRPC) RPC(method string, args interface{}, reply interface{}) error return nil } -func (f *fakeRPC) ResolveTokenToIdentity(_ string) (structs.ACLIdentity, error) { - return nil, nil +func (f *fakeRPC) ResolveTokenAndDefaultMeta(string, *structs.EnterpriseMeta, *acl.AuthorizerContext) (consul.ACLResolveResult, error) { + return consul.ACLResolveResult{}, nil } diff --git a/agent/structs/acl.go b/agent/structs/acl.go index 756cadbb1b..42fa558215 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -89,8 +89,8 @@ var ACLBootstrapNotAllowedErr = errors.New("ACL bootstrap no longer allowed") var ACLBootstrapInvalidResetIndexErr = errors.New("Invalid ACL bootstrap reset index") type ACLIdentity interface { - // ID returns a string that can be used for logging and telemetry. This should not - // contain any secret data used for authentication + // ID returns the accessor ID, a string that can be used for logging and + // telemetry. It is not the secret ID used for authentication. ID() string SecretToken() string PolicyIDs() []string diff --git a/website/content/docs/agent/telemetry.mdx b/website/content/docs/agent/telemetry.mdx index 9bb2ef5399..136c634d1f 100644 --- a/website/content/docs/agent/telemetry.mdx +++ b/website/content/docs/agent/telemetry.mdx @@ -388,7 +388,7 @@ These metrics are used to monitor the health of the Consul servers. | Metric | Description | Unit | Type | | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | ------- | | `consul.acl.ResolveToken` | Measures the time it takes to resolve an ACL token. | ms | timer | -| `consul.acl.ResolveTokenToIdentity` | Measures the time it takes to resolve an ACL token to an Identity. | ms | timer | +| `consul.acl.ResolveTokenToIdentity` | Measures the time it takes to resolve an ACL token to an Identity. This metric was removed in Consul 1.12. The time will now be reflected in `consul.acl.ResolveToken`. | ms | timer | | `consul.acl.token.cache_hit` | Increments if Consul is able to resolve a token's identity, or a legacy token, from the cache. | cache read op | counter | | `consul.acl.token.cache_miss` | Increments if Consul cannot resolve a token's identity, or a legacy token, from the cache. | cache read op | counter | | `consul.cache.bypass` | Counts how many times a request bypassed the cache because no cache-key was provided. | counter | counter |