From 343b6deb7970a07011ed37469cd8fec49540c569 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 23 Jan 2022 12:31:48 -0500 Subject: [PATCH] acl: rename ResolveTokenToIdentityAndAuthorizer to ResolveToken This change allows us to remove one of the last remaining duplicate resolve token methods (Server.ResolveToken). With this change we are down to only 2, where the second one also handles setting the default EnterpriseMeta from the token. --- agent/acl_test.go | 13 ++-- agent/consul/acl.go | 34 ++++++----- agent/consul/acl_server.go | 6 -- agent/consul/acl_test.go | 67 ++++++++++----------- agent/consul/intention_endpoint.go | 14 +---- agent/consul/internal_endpoint.go | 4 +- agent/consul/operator_autopilot_endpoint.go | 16 ++--- agent/consul/operator_raft_endpoint.go | 8 +-- 8 files changed, 73 insertions(+), 89 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index a26798bba8..5ad4880ed0 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -86,16 +86,13 @@ func (a *TestACLAgent) ResolveToken(secretID string) (acl.Authorizer, error) { return authz, err } -func (a *TestACLAgent) ResolveTokenToIdentityAndAuthorizer(secretID string) (structs.ACLIdentity, acl.Authorizer, error) { - if a.resolveAuthzFn == nil { - return nil, nil, fmt.Errorf("ResolveTokenToIdentityAndAuthorizer call is unexpected - no authz resolver callback set") +func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) { + authz, err := a.ResolveToken(secretID) + if err != nil { + return consul.ACLResolveResult{}, err } - return a.resolveAuthzFn(secretID) -} - -func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (consul.ACLResolveResult, error) { - identity, authz, err := a.ResolveTokenToIdentityAndAuthorizer(secretID) + identity, err := a.resolveIdentFn(secretID) if err != nil { return consul.ACLResolveResult{}, err } diff --git a/agent/consul/acl.go b/agent/consul/acl.go index ab637b7209..4dcd5e623f 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1051,13 +1051,16 @@ func (r *ACLResolver) resolveLocallyManagedToken(token string) (structs.ACLIdent return r.resolveLocallyManagedEnterpriseToken(token) } -func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) { +// ResolveToken to an acl.Authorizer and structs.ACLIdentity. The acl.Authorizer +// can be used to check permissions granted to the token, and the ACLIdentity +// describes the token and any defaults applied to it. +func (r *ACLResolver) ResolveToken(token string) (ACLResolveResult, error) { if !r.ACLsEnabled() { - return nil, acl.ManageAll(), nil + return ACLResolveResult{Authorizer: acl.ManageAll()}, nil } if acl.RootAuthorizer(token) != nil { - return nil, nil, acl.ErrRootDenied + return ACLResolveResult{}, acl.ErrRootDenied } // handle the anonymous token @@ -1066,7 +1069,7 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs } if ident, authz, ok := r.resolveLocallyManagedToken(token); ok { - return ident, authz, nil + return ACLResolveResult{Authorizer: authz, ACLIdentity: ident}, nil } defer metrics.MeasureSince([]string{"acl", "ResolveToken"}, time.Now()) @@ -1076,10 +1079,11 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs r.handleACLDisabledError(err) if IsACLRemoteError(err) { r.logger.Error("Error resolving token", "error", err) - return &missingIdentity{reason: "primary-dc-down", token: token}, r.down, nil + ident := &missingIdentity{reason: "primary-dc-down", token: token} + return ACLResolveResult{Authorizer: r.down, ACLIdentity: ident}, nil } - return nil, nil, err + return ACLResolveResult{}, err } // Build the Authorizer @@ -1092,7 +1096,7 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs authz, err := policies.Compile(r.cache, &conf) if err != nil { - return nil, nil, err + return ACLResolveResult{}, err } chain = append(chain, authz) @@ -1100,15 +1104,15 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs if err != nil { if IsACLRemoteError(err) { r.logger.Error("Error resolving identity defaults", "error", err) - return identity, r.down, nil + return ACLResolveResult{Authorizer: r.down, ACLIdentity: identity}, nil } - return nil, nil, err + return ACLResolveResult{}, err } else if authz != nil { chain = append(chain, authz) } chain = append(chain, acl.RootAuthorizer(r.config.ACLDefaultPolicy)) - return identity, acl.NewChainedAuthorizer(chain), nil + return ACLResolveResult{Authorizer: acl.NewChainedAuthorizer(chain), ACLIdentity: identity}, nil } type ACLResolveResult struct { @@ -1141,7 +1145,7 @@ func (r *ACLResolver) ACLsEnabled() bool { } func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (ACLResolveResult, error) { - identity, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) + result, err := r.ResolveToken(token) if err != nil { return ACLResolveResult{}, err } @@ -1152,8 +1156,8 @@ func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs. // Default the EnterpriseMeta based on the Tokens meta or actual defaults // in the case of unknown identity - if identity != nil { - entMeta.Merge(identity.EnterpriseMetadata()) + if result.ACLIdentity != nil { + entMeta.Merge(result.ACLIdentity.EnterpriseMetadata()) } else { entMeta.Merge(structs.DefaultEnterpriseMetaInDefaultPartition()) } @@ -1161,7 +1165,7 @@ func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs. // Use the meta to fill in the ACL authorization context entMeta.FillAuthzContext(authzContext) - return ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err + return result, err } // aclFilter is used to filter results from our state store based on ACL rules @@ -1971,7 +1975,7 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub // not authorized for read access will be removed from subj. func filterACL(r *ACLResolver, token string, subj interface{}) error { // Get the ACL from the token - _, authorizer, err := r.ResolveTokenToIdentityAndAuthorizer(token) + authorizer, err := r.ResolveToken(token) if err != nil { return err } diff --git a/agent/consul/acl_server.go b/agent/consul/acl_server.go index 21b4d95032..e21c066b41 100644 --- a/agent/consul/acl_server.go +++ b/agent/consul/acl_server.go @@ -159,12 +159,6 @@ func (s *Server) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error 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 -} - 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 0e663120a9..b88f57d883 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -46,10 +46,11 @@ type asyncResolutionResult struct { err error } -func verifyAuthorizerChain(t *testing.T, expected acl.Authorizer, actual acl.Authorizer) { - expectedChainAuthz, ok := expected.(*acl.ChainedAuthorizer) +func verifyAuthorizerChain(t *testing.T, expected ACLResolveResult, actual ACLResolveResult) { + t.Helper() + expectedChainAuthz, ok := expected.Authorizer.(*acl.ChainedAuthorizer) require.True(t, ok, "expected Authorizer is not a ChainedAuthorizer") - actualChainAuthz, ok := actual.(*acl.ChainedAuthorizer) + actualChainAuthz, ok := actual.Authorizer.(*acl.ChainedAuthorizer) require.True(t, ok, "actual Authorizer is not a ChainedAuthorizer") expectedChain := expectedChainAuthz.AuthorizerChain() @@ -65,19 +66,13 @@ func verifyAuthorizerChain(t *testing.T, expected acl.Authorizer, actual acl.Aut } func resolveTokenAsync(r *ACLResolver, token string, ch chan *asyncResolutionResult) { - _, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) + authz, err := r.ResolveToken(token) ch <- &asyncResolutionResult{authz: authz, err: err} } -// Deprecated: use resolveToken or ACLResolver.ResolveTokenToIdentityAndAuthorizer instead -func (r *ACLResolver) ResolveToken(token string) (acl.Authorizer, error) { - _, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) - return authz, err -} - func resolveToken(t *testing.T, r *ACLResolver, token string) acl.Authorizer { t.Helper() - _, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) + authz, err := r.ResolveToken(token) require.NoError(t, err) return authz } @@ -739,7 +734,7 @@ func TestACLResolver_Disabled(t *testing.T) { r := newTestACLResolver(t, delegate, nil) authz, err := r.ResolveToken("does not exist") - require.Equal(t, acl.ManageAll(), authz) + require.Equal(t, ACLResolveResult{Authorizer: acl.ManageAll()}, authz) require.Nil(t, err) } @@ -753,22 +748,19 @@ func TestACLResolver_ResolveRootACL(t *testing.T) { r := newTestACLResolver(t, delegate, nil) t.Run("Allow", func(t *testing.T) { - authz, err := r.ResolveToken("allow") - require.Nil(t, authz) + _, err := r.ResolveToken("allow") require.Error(t, err) require.True(t, acl.IsErrRootDenied(err)) }) t.Run("Deny", func(t *testing.T) { - authz, err := r.ResolveToken("deny") - require.Nil(t, authz) + _, err := r.ResolveToken("deny") require.Error(t, err) require.True(t, acl.IsErrRootDenied(err)) }) t.Run("Manage", func(t *testing.T) { - authz, err := r.ResolveToken("manage") - require.Nil(t, authz) + _, err := r.ResolveToken("manage") require.Error(t, err) require.True(t, acl.IsErrRootDenied(err)) }) @@ -817,7 +809,11 @@ func TestACLResolver_DownPolicy(t *testing.T) { authz, err := r.ResolveToken("foo") require.NoError(t, err) require.NotNil(t, authz) - require.Equal(t, authz, acl.DenyAll()) + expected := ACLResolveResult{ + Authorizer: acl.DenyAll(), + ACLIdentity: &missingIdentity{reason: "primary-dc-down", token: "foo"}, + } + require.Equal(t, expected, authz) requireIdentityCached(t, r, tokenSecretCacheID("foo"), false, "not present") }) @@ -841,7 +837,11 @@ func TestACLResolver_DownPolicy(t *testing.T) { authz, err := r.ResolveToken("foo") require.NoError(t, err) require.NotNil(t, authz) - require.Equal(t, authz, acl.AllowAll()) + expected := ACLResolveResult{ + Authorizer: acl.AllowAll(), + ACLIdentity: &missingIdentity{reason: "primary-dc-down", token: "foo"}, + } + require.Equal(t, expected, authz) requireIdentityCached(t, r, tokenSecretCacheID("foo"), false, "not present") }) @@ -958,7 +958,7 @@ func TestACLResolver_DownPolicy(t *testing.T) { config.Config.ACLDownPolicy = "extend-cache" }) - _, authz, err := r.ResolveTokenToIdentityAndAuthorizer("not-found") + authz, err := r.ResolveToken("not-found") require.NoError(t, err) require.NotNil(t, authz) require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil)) @@ -1255,10 +1255,9 @@ func TestACLResolver_DownPolicy(t *testing.T) { // the go routine spawned will eventually return and this will be a not found error retry.Run(t, func(t *retry.R) { - authz3, err := r.ResolveToken("found") + _, err := r.ResolveToken("found") assert.Error(t, err) assert.True(t, acl.IsErrNotFound(err)) - assert.Nil(t, authz3) }) requireIdentityCached(t, r, tokenSecretCacheID("found"), false, "no longer cached") @@ -1526,7 +1525,6 @@ func TestACLResolver_Client(t *testing.T) { // policies within the cache) authz, err = r.ResolveToken("a1a54629-5050-4d17-8a4e-560d2423f835") require.EqualError(t, err, acl.ErrNotFound.Error()) - require.Nil(t, authz) require.True(t, modified) require.True(t, deleted) @@ -1675,8 +1673,7 @@ func testACLResolver_variousTokens(t *testing.T, delegate *ACLResolverTestDelega runTwiceAndReset("Missing Identity", func(t *testing.T) { delegate.UseTestLocalData(nil) - authz, err := r.ResolveToken("doesn't exist") - require.Nil(t, authz) + _, err := r.ResolveToken("doesn't exist") require.Error(t, err) require.True(t, acl.IsErrNotFound(err)) }) @@ -3929,12 +3926,12 @@ func TestACLResolver_AgentRecovery(t *testing.T) { tokens.UpdateAgentRecoveryToken("9a184a11-5599-459e-b71a-550e5f9a5a23", token.TokenSourceConfig) - ident, authz, err := r.ResolveTokenToIdentityAndAuthorizer("9a184a11-5599-459e-b71a-550e5f9a5a23") + authz, err := r.ResolveToken("9a184a11-5599-459e-b71a-550e5f9a5a23") require.NoError(t, err) - require.NotNil(t, ident) - require.Equal(t, "agent-recovery:foo", ident.ID()) - require.NotNil(t, authz) - require.Equal(t, r.agentRecoveryAuthz, authz) + require.NotNil(t, authz.ACLIdentity) + require.Equal(t, "agent-recovery:foo", authz.ACLIdentity.ID()) + require.NotNil(t, authz.Authorizer) + require.Equal(t, r.agentRecoveryAuthz, authz.Authorizer) require.Equal(t, acl.Allow, authz.AgentWrite("foo", nil)) require.Equal(t, acl.Allow, authz.NodeRead("bar", nil)) require.Equal(t, acl.Deny, authz.NodeWrite("bar", nil)) @@ -3998,7 +3995,7 @@ func TestACLResolver_ACLsEnabled(t *testing.T) { } -func TestACLResolver_ResolveTokenToIdentityAndAuthorizer_UpdatesPurgeTheCache(t *testing.T) { +func TestACLResolver_ResolveToken_UpdatesPurgeTheCache(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -4035,7 +4032,7 @@ func TestACLResolver_ResolveTokenToIdentityAndAuthorizer_UpdatesPurgeTheCache(t require.NoError(t, err) runStep(t, "first resolve", func(t *testing.T) { - _, authz, err := srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token) + authz, err := srv.ACLResolver.ResolveToken(token) require.NoError(t, err) require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.KeyRead("foo", nil)) @@ -4054,7 +4051,7 @@ func TestACLResolver_ResolveTokenToIdentityAndAuthorizer_UpdatesPurgeTheCache(t err := msgpackrpc.CallWithCodec(codec, "ACL.PolicySet", &reqPolicy, &structs.ACLPolicy{}) require.NoError(t, err) - _, authz, err := srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token) + authz, err := srv.ACLResolver.ResolveToken(token) require.NoError(t, err) require.NotNil(t, authz) require.Equal(t, acl.Deny, authz.KeyRead("foo", nil)) @@ -4070,7 +4067,7 @@ func TestACLResolver_ResolveTokenToIdentityAndAuthorizer_UpdatesPurgeTheCache(t err := msgpackrpc.CallWithCodec(codec, "ACL.TokenDelete", &req, &resp) require.NoError(t, err) - _, _, err = srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token) + _, err = srv.ACLResolver.ResolveToken(token) require.True(t, acl.IsErrNotFound(err), "Error %v is not acl.ErrNotFound", err) }) } diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index 2031641caa..1fb8946623 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -100,21 +100,13 @@ 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) + var entMeta structs.EnterpriseMeta + authz, err := s.srv.ACLResolver.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil) if err != nil { return err } - var accessorID string - var entMeta structs.EnterpriseMeta - if identity != nil { - entMeta.Merge(identity.EnterpriseMetadata()) - accessorID = identity.ID() - } else { - entMeta.Merge(structs.DefaultEnterpriseMetaInDefaultPartition()) - } - + accessorID := authz.AccessorID() var ( mut *structs.IntentionMutation legacyWrite bool diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index 5213349e79..44b6af5aaf 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -433,11 +433,11 @@ func (m *Internal) KeyringOperation( } // Check ACLs - identity, authz, err := m.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := m.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := m.srv.validateEnterpriseToken(identity); err != nil { + if err := m.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } switch args.Operation { diff --git a/agent/consul/operator_autopilot_endpoint.go b/agent/consul/operator_autopilot_endpoint.go index d600df83cb..f4a3db65e6 100644 --- a/agent/consul/operator_autopilot_endpoint.go +++ b/agent/consul/operator_autopilot_endpoint.go @@ -17,11 +17,11 @@ func (op *Operator) AutopilotGetConfiguration(args *structs.DCSpecificRequest, r } // This action requires operator read access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorRead(nil) != acl.Allow { @@ -49,11 +49,11 @@ func (op *Operator) AutopilotSetConfiguration(args *structs.AutopilotSetConfigRe } // This action requires operator write access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorWrite(nil) != acl.Allow { @@ -84,11 +84,11 @@ func (op *Operator) ServerHealth(args *structs.DCSpecificRequest, reply *structs } // This action requires operator read access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorRead(nil) != acl.Allow { @@ -151,11 +151,11 @@ func (op *Operator) AutopilotState(args *structs.DCSpecificRequest, reply *autop } // This action requires operator read access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorRead(nil) != acl.Allow { diff --git a/agent/consul/operator_raft_endpoint.go b/agent/consul/operator_raft_endpoint.go index 72ae5b195b..431b455c08 100644 --- a/agent/consul/operator_raft_endpoint.go +++ b/agent/consul/operator_raft_endpoint.go @@ -81,11 +81,11 @@ func (op *Operator) RaftRemovePeerByAddress(args *structs.RaftRemovePeerRequest, // This is a super dangerous operation that requires operator write // access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorWrite(nil) != acl.Allow { @@ -134,11 +134,11 @@ func (op *Operator) RaftRemovePeerByID(args *structs.RaftRemovePeerRequest, repl // This is a super dangerous operation that requires operator write // access. - identity, authz, err := op.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) + authz, err := op.srv.ACLResolver.ResolveToken(args.Token) if err != nil { return err } - if err := op.srv.validateEnterpriseToken(identity); err != nil { + if err := op.srv.validateEnterpriseToken(authz.ACLIdentity); err != nil { return err } if authz.OperatorWrite(nil) != acl.Allow {