From edca8d61a365c5c45daaba7bc3f20075e80745d6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sat, 22 Jan 2022 14:47:59 -0500 Subject: [PATCH] acl: remove ResolveTokenToIdentity By exposing the AccessorID from the primary ResolveToken method we can remove this duplication. --- agent/acl.go | 7 ++----- agent/acl_test.go | 19 ------------------- agent/agent.go | 3 --- agent/consul/acl.go | 27 --------------------------- agent/consul/acl_test.go | 30 ------------------------------ agent/local/state.go | 10 ++++------ agent/local/state_test.go | 6 ++++-- 7 files changed, 10 insertions(+), 92 deletions(-) diff --git a/agent/acl.go b/agent/acl.go index fa2259cfd3..7b92210ed8 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 diff --git a/agent/acl_test.go b/agent/acl_test.go index 7d8039511a..b205220f67 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -523,22 +523,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 ba587d3434..7e2a7f110a 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -167,9 +167,6 @@ 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 diff --git a/agent/consul/acl.go b/agent/consul/acl.go index d033db1b97..da89e2dd3d 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1128,33 +1128,6 @@ func (a ACLResolveResult) AccessorID() string { return a.ACLIdentity.ID() } -// 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 - } - - 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) -} - func (r *ACLResolver) ACLsEnabled() bool { // Whether we desire ACLs to be enabled according to configuration if !r.config.ACLsEnabled { diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 2d4f7dc5ec..0e663120a9 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/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 }