From 0ee86935f0c2786afd25add5ff7f7ee079de7c44 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 17 Nov 2020 17:10:21 -0500 Subject: [PATCH 1/2] Remove two unused delegate methods --- agent/acl.go | 10 +++------- agent/acl_test.go | 8 +++++--- agent/agent.go | 13 ++++++++++--- agent/consul/acl.go | 12 ++++-------- agent/consul/acl_client.go | 8 -------- 5 files changed, 22 insertions(+), 29 deletions(-) diff --git a/agent/acl.go b/agent/acl.go index d679d633a6..c9b42ba837 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -3,9 +3,10 @@ package agent import ( "fmt" + "github.com/hashicorp/serf/serf" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/serf/serf" ) // resolveToken is the primary interface used by ACL-checkers in the agent @@ -36,16 +37,11 @@ func (a *Agent) resolveTokenAndDefaultMeta(id string, entMeta *structs.Enterpris return a.delegate.ResolveTokenAndDefaultMeta(id, entMeta, authzContext) } -// resolveIdentityFromToken is used to resolve an ACLToken's secretID to a structs.ACLIdentity -func (a *Agent) resolveIdentityFromToken(secretID string) (structs.ACLIdentity, error) { - return a.delegate.ResolveTokenToIdentity(secretID) -} - // 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 (a *Agent) aclAccessorID(secretID string) string { - ident, err := a.resolveIdentityFromToken(secretID) + ident, err := a.delegate.ResolveTokenToIdentity(secretID) if acl.IsErrNotFound(err) { return "" } diff --git a/agent/acl_test.go b/agent/acl_test.go index 79ade86b20..e5d4df594f 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -7,6 +7,9 @@ import ( "time" "github.com/armon/go-metrics" + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/serf/serf" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/consul" @@ -15,8 +18,6 @@ import ( "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/types" - "github.com/hashicorp/go-hclog" - "github.com/hashicorp/serf/serf" "github.com/stretchr/testify/require" ) @@ -519,6 +520,7 @@ func TestACL_filterChecks(t *testing.T) { require.False(t, ok) } +// TODO: remove? func TestACL_ResolveIdentity(t *testing.T) { t.Parallel() a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, catalogIdent) @@ -526,7 +528,7 @@ func TestACL_ResolveIdentity(t *testing.T) { // 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.resolveIdentityFromToken(nodeROSecret) + ident, err := a.delegate.ResolveTokenToIdentity(nodeROSecret) require.NoError(t, err) require.NotNil(t, ident) diff --git a/agent/agent.go b/agent/agent.go index 07f21a2306..59154949f4 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -129,9 +129,16 @@ type delegate interface { LocalMember() serf.Member JoinLAN(addrs []string) (n int, err error) RemoveFailedNode(node string, prune bool) error - ResolveToken(secretID string) (acl.Authorizer, error) - ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error) - ResolveTokenAndDefaultMeta(secretID string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, 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 namespace from the token. + ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) + RPC(method string, args interface{}, reply interface{}) error UseLegacyACLs() bool SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 1ec7bc4193..fcef154c4d 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -8,12 +8,13 @@ import ( "github.com/armon/go-metrics" "github.com/armon/go-metrics/prometheus" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/logging" "github.com/hashicorp/go-hclog" "golang.org/x/sync/singleflight" "golang.org/x/time/rate" + + "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/logging" ) var ACLCounters = []prometheus.CounterDefinition{ @@ -1039,11 +1040,6 @@ func (r *ACLResolver) collectRolesForIdentity(identity structs.ACLIdentity, role return roles, nil } -func (r *ACLResolver) resolveTokenToPolicies(token string) (structs.ACLPolicies, error) { - _, policies, err := r.resolveTokenToIdentityAndPolicies(token) - return policies, err -} - func (r *ACLResolver) resolveTokenToIdentityAndPolicies(token string) (structs.ACLIdentity, structs.ACLPolicies, error) { var lastErr error var lastIdentity structs.ACLIdentity diff --git a/agent/consul/acl_client.go b/agent/consul/acl_client.go index 1b9c6a46d6..a1bb58adfa 100644 --- a/agent/consul/acl_client.go +++ b/agent/consul/acl_client.go @@ -86,10 +86,6 @@ func (c *Client) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error return false, nil, nil } -func (c *Client) ResolveToken(token string) (acl.Authorizer, error) { - return c.acls.ResolveToken(token) -} - func (c *Client) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) { // not using ResolveTokenToIdentityAndAuthorizer because in this case we don't // need to resolve the roles, policies and namespace but just want the identity @@ -97,10 +93,6 @@ func (c *Client) ResolveTokenToIdentity(token string) (structs.ACLIdentity, erro return c.acls.ResolveTokenToIdentity(token) } -func (c *Client) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) { - return c.acls.ResolveTokenToIdentityAndAuthorizer(token) -} - func (c *Client) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) { identity, authz, err := c.acls.ResolveTokenToIdentityAndAuthorizer(token) if err != nil { From 3885835e8c57eb9f3d86efdb8c78d7531f13ff28 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 17 Nov 2020 18:15:07 -0500 Subject: [PATCH 2/2] acl: remove a test-only method --- agent/consul/acl.go | 7 +------ agent/consul/acl_test.go | 43 +++++++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index fcef154c4d..acd1fa787d 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1187,11 +1187,6 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs return identity, acl.NewChainedAuthorizer(chain), nil } -func (r *ACLResolver) ResolveToken(token string) (acl.Authorizer, error) { - _, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) - return authz, err -} - func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) { if !r.ACLsEnabled() { return nil, nil @@ -1975,7 +1970,7 @@ func (r *ACLResolver) filterACLWithAuthorizer(authorizer acl.Authorizer, subj in // rules configured for the provided token. func (r *ACLResolver) filterACL(token string, subj interface{}) error { // Get the ACL from the token - authorizer, err := r.ResolveToken(token) + _, authorizer, err := r.ResolveTokenToIdentityAndAuthorizer(token) if err != nil { return err } diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 2544ad79af..68d18e451a 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -9,14 +9,15 @@ import ( "testing" "time" + "github.com/mitchellh/copystructure" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/mitchellh/copystructure" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) var testACLPolicy = ` @@ -61,10 +62,23 @@ func verifyAuthorizerChain(t *testing.T, expected acl.Authorizer, actual acl.Aut } func resolveTokenAsync(r *ACLResolver, token string, ch chan *asyncResolutionResult) { - authz, err := r.ResolveToken(token) + _, authz, err := r.ResolveTokenToIdentityAndAuthorizer(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) + require.NoError(t, err) + return authz +} + func testIdentityForToken(token string) (bool, structs.ACLIdentity, error) { switch token { case "missing-policy": @@ -1739,57 +1753,50 @@ func testACLResolver_variousTokens(t *testing.T, delegate *ACLResolverTestDelega }) runTwiceAndReset("Missing Policy", func(t *testing.T) { - authz, err := r.ResolveToken("missing-policy") - require.NoError(t, err) + authz := resolveToken(t, r, "missing-policy") require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.ACLRead(nil)) require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil)) }) runTwiceAndReset("Missing Role", func(t *testing.T) { - authz, err := r.ResolveToken("missing-role") - require.NoError(t, err) + authz := resolveToken(t, r, "missing-role") require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.ACLRead(nil)) require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil)) }) runTwiceAndReset("Missing Policy on Role", func(t *testing.T) { - authz, err := r.ResolveToken("missing-policy-on-role") - require.NoError(t, err) + authz := resolveToken(t, r, "missing-policy-on-role") require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.ACLRead(nil)) require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil)) }) runTwiceAndReset("Normal with Policy", func(t *testing.T) { - authz, err := r.ResolveToken("found") + authz := resolveToken(t, r, "found") require.NotNil(t, authz) - require.NoError(t, err) require.Equal(t, acl.Deny, authz.ACLRead(nil)) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) }) runTwiceAndReset("Normal with Role", func(t *testing.T) { - authz, err := r.ResolveToken("found-role") + authz := resolveToken(t, r, "found-role") require.NotNil(t, authz) - require.NoError(t, err) require.Equal(t, acl.Deny, authz.ACLRead(nil)) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) }) runTwiceAndReset("Normal with Policy and Role", func(t *testing.T) { - authz, err := r.ResolveToken("found-policy-and-role") + authz := resolveToken(t, r, "found-policy-and-role") require.NotNil(t, authz) - require.NoError(t, err) require.Equal(t, acl.Deny, authz.ACLRead(nil)) require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) require.Equal(t, acl.Allow, authz.ServiceRead("bar", nil)) }) runTwiceAndReset("Role With Node Identity", func(t *testing.T) { - authz, err := r.ResolveToken("found-role-node-identity") - require.NoError(t, err) + authz := resolveToken(t, r, "found-role-node-identity") require.NotNil(t, authz) require.Equal(t, acl.Allow, authz.NodeWrite("test-node", nil)) require.Equal(t, acl.Deny, authz.NodeWrite("test-node-dc2", nil))