luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

Fix identity resolution on clients and in secondary dcs (#7862) 

Previously this happened to be using the method on the Server/Client that was meant to allow the ACLResolver to locally resolve tokens. On Servers that had tokens (primary or secondary dc + token replication) this function would lookup the token from raft and return the ACLIdentity. On clients this was always a noop. We inadvertently used this function instead of creating a new one when we added logging accessor ids for permission denied RPC requests. 

With this commit, a new method is used for resolving the identity properly via the ACLResolver which may still resolve locally in the case of being on a server with tokens but also supports remote token resolution.
This commit is contained in:
Matt Keeler 2020-05-13 13:00:08 -04:00 committed by GitHub
parent 38ba851993
commit 849eedd142
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 198 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
agent/acl.go
View File

@ -42,25 +42,15 @@ func (a *Agent) resolveTokenAndDefaultMeta(id string, entMeta *structs.Enterpris
}
// resolveIdentityFromToken is used to resolve an ACLToken's secretID to a structs.ACLIdentity
func (a *Agent) resolveIdentityFromToken(secretID string) (bool, structs.ACLIdentity, error) {
// ACLs are disabled
if !a.delegate.ACLsEnabled() {
return false, nil, nil
}
// Disable ACLs if version 8 enforcement isn't enabled.
if !a.config.ACLEnforceVersion8 {
return false, nil, nil
}
return a.delegate.ResolveIdentityFromToken(secretID)
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.resolveIdentityFromToken(secretID)
if acl.IsErrNotFound(err) {
return ""
}

123
agent/acl_test.go
View File

@ -21,6 +21,9 @@ import (
"github.com/stretchr/testify/require"
)
type authzResolver func(string) (structs.ACLIdentity, acl.Authorizer, error)
type identResolver func(string) (structs.ACLIdentity, error)
type TestACLAgent struct {
// Name is an optional name of the agent.
Name string
@ -43,16 +46,17 @@ type TestACLAgent struct {
// Shutdown() is called.
DataDir string
resolveTokenFn func(string) (structs.ACLIdentity, acl.Authorizer, error)
resolveAuthzFn authzResolver
resolveIdentFn identResolver
*Agent
}
// NewTestACLAGent does just enough so that all the code within agent/acl.go can work
// NewTestACLAgent does just enough so that all the code within agent/acl.go can work
// Basically it needs a local state for some of the vet* functions, a logger and a delegate.
// The key is that we are the delegate so we can control the ResolveToken responses
func NewTestACLAgent(t *testing.T, name string, hcl string, resolveFn func(string) (structs.ACLIdentity, acl.Authorizer, error)) *TestACLAgent {
a := &TestACLAgent{Name: name, HCL: hcl, resolveTokenFn: resolveFn}
func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzResolver, resolveIdent identResolver) *TestACLAgent {
a := &TestACLAgent{Name: name, HCL: hcl, resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent}
hclDataDir := `data_dir = "acl-agent"`
logOutput := testutil.TestWriter(t)
@ -68,9 +72,7 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveFn func(strin
)
agent, err := New(a.Config, logger)
if err != nil {
panic(fmt.Sprintf("Error creating agent: %v", err))
}
require.NoError(t, err)
a.Agent = agent
agent.LogOutput = logOutput
@ -93,20 +95,28 @@ func (a *TestACLAgent) UseLegacyACLs() bool {
}
func (a *TestACLAgent) ResolveToken(secretID string) (acl.Authorizer, error) {
if a.resolveTokenFn == nil {
panic("This agent is useless without providing a token resolution function")
if a.resolveAuthzFn == nil {
return nil, fmt.Errorf("ResolveToken call is unexpected - no authz resolver callback set")
}
_, authz, err := a.resolveTokenFn(secretID)
_, authz, err := a.resolveAuthzFn(secretID)
return authz, err
}
func (a *TestACLAgent) ResolveTokenToIdentityAndAuthorizer(secretID string) (structs.ACLIdentity, acl.Authorizer, error) {
if a.resolveTokenFn == nil {
panic("This agent is useless without providing a token resolution function")
if a.resolveAuthzFn == nil {
return nil, nil, fmt.Errorf("ResolveTokenToIdentityAndAuthorizer call is unexpected - no authz resolver callback set")
}
return a.resolveTokenFn(secretID)
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) {
@ -129,19 +139,6 @@ func (a *TestACLAgent) ResolveTokenAndDefaultMeta(secretID string, entMeta *stru
return authz, err
}
func (a *TestACLAgent) ResolveIdentityFromToken(secretID string) (bool, structs.ACLIdentity, error) {
if a.resolveTokenFn == nil {
panic("This agent is useless without providing a token resolution function")
}
identity, _, err := a.resolveTokenFn(secretID)
if err != nil {
return true, nil, err
}
return true, identity, nil
}
// All of these are stubs to satisfy the interface
func (a *TestACLAgent) GetLANCoordinate() (lib.CoordinateSet, error) {
return nil, fmt.Errorf("Unimplemented")
@ -188,14 +185,9 @@ func TestACL_Version8(t *testing.T) {
t.Parallel()
t.Run("version 8 disabled", func(t *testing.T) {
resolveFn := func(string) (structs.ACLIdentity, acl.Authorizer, error) {
require.Fail(t, "should not have called delegate.ResolveToken")
return nil, nil, fmt.Errorf("should not have called delegate.ResolveToken")
}
a := NewTestACLAgent(t, t.Name(), TestACLConfig()+`
acl_enforce_version_8 = false
`, resolveFn)
`, nil, nil)
token, err := a.resolveToken("nope")
require.Nil(t, token)
@ -210,7 +202,7 @@ func TestACL_Version8(t *testing.T) {
}
a := NewTestACLAgent(t, t.Name(), TestACLConfig()+`
acl_enforce_version_8 = true
`, resolveFn)
`, resolveFn, nil)
_, err := a.resolveToken("nope")
require.Error(t, err)
@ -221,12 +213,7 @@ func TestACL_Version8(t *testing.T) {
func TestACL_AgentMasterToken(t *testing.T) {
t.Parallel()
resolveFn := func(string) (structs.ACLIdentity, acl.Authorizer, error) {
require.Fail(t, "should not have called delegate.ResolveToken")
return nil, nil, fmt.Errorf("should not have called delegate.ResolveToken")
}
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), resolveFn)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, nil)
a.loadTokens(a.config)
authz, err := a.resolveToken("towel")
require.NotNil(t, authz)
@ -241,12 +228,7 @@ func TestACL_AgentMasterToken(t *testing.T) {
func TestACL_RootAuthorizersDenied(t *testing.T) {
t.Parallel()
resolveFn := func(string) (structs.ACLIdentity, acl.Authorizer, error) {
require.Fail(t, "should not have called delegate.ResolveToken")
return nil, nil, fmt.Errorf("should not have called delegate.ResolveToken")
}
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), resolveFn)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), nil, nil)
authz, err := a.resolveToken("deny")
require.Nil(t, authz)
require.Error(t, err)
@ -280,35 +262,35 @@ var (
otherRWSecret = "a38e8016-91b6-4876-b3e7-a307abbb2002"
testTokens = map[string]testToken{
nodeROSecret: testToken{
nodeROSecret: {
token: structs.ACLToken{
AccessorID: "9df2d1a4-2d07-414e-8ead-6053f56ed2eb",
SecretID: nodeROSecret,
},
rules: `node_prefix "Node" { policy = "read" }`,
},
nodeRWSecret: testToken{
nodeRWSecret: {
token: structs.ACLToken{
AccessorID: "efb6b7d5-d343-47c1-b4cb-aa6b94d2f490",
SecretID: nodeROSecret,
},
rules: `node_prefix "Node" { policy = "write" }`,
},
serviceROSecret: testToken{
serviceROSecret: {
token: structs.ACLToken{
AccessorID: "0da53edb-36e5-4603-9c31-79965bad45f5",
SecretID: serviceROSecret,
},
rules: `service_prefix "service" { policy = "read" }`,
},
serviceRWSecret: testToken{
serviceRWSecret: {
token: structs.ACLToken{
AccessorID: "52504258-137a-41e6-9326-01f40e80872e",
SecretID: serviceRWSecret,
},
rules: `service_prefix "service" { policy = "write" }`,
},
otherRWSecret: testToken{
otherRWSecret: {
token: structs.ACLToken{
AccessorID: "5e032c5b-c39e-4552-b5ad-8a9365b099c4",
SecretID: otherRWSecret,
@ -333,9 +315,18 @@ func catalogPolicy(token string) (structs.ACLIdentity, acl.Authorizer, error) {
return &tok.token, authz, err
}
func catalogIdent(token string) (structs.ACLIdentity, error) {
tok, ok := testTokens[token]
if !ok {
return nil, acl.ErrNotFound
}
return &tok.token, nil
}
func TestACL_vetServiceRegister(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
// Register a new service, with permission.
err := a.vetServiceRegister(serviceRWSecret, &structs.NodeService{
@ -366,7 +357,7 @@ func TestACL_vetServiceRegister(t *testing.T) {
func TestACL_vetServiceUpdate(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
// Update a service that doesn't exist.
err := a.vetServiceUpdate(serviceRWSecret, structs.NewServiceID("my-service", nil))
@ -389,7 +380,7 @@ func TestACL_vetServiceUpdate(t *testing.T) {
func TestACL_vetCheckRegister(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
// Register a new service check with write privs.
err := a.vetCheckRegister(serviceRWSecret, &structs.HealthCheck{
@ -455,7 +446,7 @@ func TestACL_vetCheckRegister(t *testing.T) {
func TestACL_vetCheckUpdate(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
// Update a check that doesn't exist.
err := a.vetCheckUpdate(nodeRWSecret, structs.NewCheckID("my-check", nil))
@ -495,7 +486,7 @@ func TestACL_vetCheckUpdate(t *testing.T) {
func TestACL_filterMembers(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
var members []serf.Member
require.NoError(t, a.filterMembers(nodeROSecret, &members))
@ -514,7 +505,7 @@ func TestACL_filterMembers(t *testing.T) {
func TestACL_filterServices(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
services := make(map[structs.ServiceID]*structs.NodeService)
require.NoError(t, a.filterServices(nodeROSecret, &services))
@ -528,7 +519,7 @@ func TestACL_filterServices(t *testing.T) {
func TestACL_filterChecks(t *testing.T) {
t.Parallel()
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy)
a := NewTestACLAgent(t, t.Name(), TestACLConfig(), catalogPolicy, catalogIdent)
checks := make(map[structs.CheckID]*structs.HealthCheck)
require.NoError(t, a.filterChecks(nodeROSecret, &checks))
@ -555,3 +546,21 @@ func TestACL_filterChecks(t *testing.T) {
_, ok = checks[structs.NewCheckID("my-other", nil)]
require.False(t, ok)
}
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.resolveIdentityFromToken(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.resolveToken(nodeROSecret)
require.Error(t, err)
}

2
agent/agent.go
View File

@ -136,8 +136,8 @@ type delegate interface {
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)
ResolveIdentityFromToken(secretID string) (bool, structs.ACLIdentity, error)
RPC(method string, args interface{}, reply interface{}) error
ACLsEnabled() bool
UseLegacyACLs() bool

24
agent/consul/acl.go
View File

@ -1114,6 +1114,30 @@ func (r *ACLResolver) ResolveToken(token string) (acl.Authorizer, error) {
return authz, err
}
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 r.delegate.UseLegacyACLs() {
identity, _, err := r.resolveTokenLegacy(token)
return identity, r.disableACLsWhenUpstreamDisabled(err)
}
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.delegate.ACLsEnabled() {

7
agent/consul/acl_client.go
View File

@ -94,6 +94,13 @@ 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
// information such as accessor id.
return c.acls.ResolveTokenToIdentity(token)
}
func (c *Client) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) {
return c.acls.ResolveTokenToIdentityAndAuthorizer(token)
}

7
agent/consul/acl_server.go
View File

@ -222,6 +222,13 @@ func (s *Server) ResolveToken(token string) (acl.Authorizer, error) {
return authz, err
}
func (s *Server) 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
// information such as accessor id.
return s.acls.ResolveTokenToIdentity(token)
}
func (s *Server) ResolveTokenToIdentityAndAuthorizer(token string) (structs.ACLIdentity, acl.Authorizer, error) {
if id, authz := s.ResolveEntTokenToIdentityAndAuthorizer(token); id != nil && authz != nil {
return id, authz, nil

88
agent/consul/acl_test.go
View File

@ -462,6 +462,14 @@ type ACLResolverTestDelegate struct {
policyResolveFn func(*structs.ACLPolicyBatchGetRequest, *structs.ACLPolicyBatchResponse) error
roleResolveFn func(*structs.ACLRoleBatchGetRequest, *structs.ACLRoleBatchResponse) error
localTokenResolutions int32
remoteTokenResolutions int32
localPolicyResolutions int32
remotePolicyResolutions int32
localRoleResolutions int32
remoteRoleResolutions int32
remoteLegacyResolutions int32
// state for the optional default resolver function defaultTokenReadFn
tokenCached bool
// state for the optional default resolver function defaultPolicyResolveFn
@ -570,6 +578,7 @@ func (d *ACLResolverTestDelegate) ResolveIdentityFromToken(token string) (bool,
return false, nil, nil
}
atomic.AddInt32(&d.localTokenResolutions, 1)
return testIdentityForToken(token)
}
@ -578,6 +587,7 @@ func (d *ACLResolverTestDelegate) ResolvePolicyFromID(policyID string) (bool, *s
return false, nil, nil
}
atomic.AddInt32(&d.localPolicyResolutions, 1)
return testPolicyForID(policyID)
}
@ -586,27 +596,32 @@ func (d *ACLResolverTestDelegate) ResolveRoleFromID(roleID string) (bool, *struc
return false, nil, nil
}
atomic.AddInt32(&d.localRoleResolutions, 1)
return testRoleForID(roleID)
}
func (d *ACLResolverTestDelegate) RPC(method string, args interface{}, reply interface{}) error {
switch method {
case "ACL.GetPolicy":
atomic.AddInt32(&d.remoteLegacyResolutions, 1)
if d.getPolicyFn != nil {
return d.getPolicyFn(args.(*structs.ACLPolicyResolveLegacyRequest), reply.(*structs.ACLPolicyResolveLegacyResponse))
}
panic("Bad Test Implementation: should provide a getPolicyFn to the ACLResolverTestDelegate")
case "ACL.TokenRead":
atomic.AddInt32(&d.remoteTokenResolutions, 1)
if d.tokenReadFn != nil {
return d.tokenReadFn(args.(*structs.ACLTokenGetRequest), reply.(*structs.ACLTokenResponse))
}
panic("Bad Test Implementation: should provide a tokenReadFn to the ACLResolverTestDelegate")
case "ACL.PolicyResolve":
atomic.AddInt32(&d.remotePolicyResolutions, 1)
if d.policyResolveFn != nil {
return d.policyResolveFn(args.(*structs.ACLPolicyBatchGetRequest), reply.(*structs.ACLPolicyBatchResponse))
}
panic("Bad Test Implementation: should provide a policyResolveFn to the ACLResolverTestDelegate")
case "ACL.RoleResolve":
atomic.AddInt32(&d.remoteRoleResolutions, 1)
if d.roleResolveFn != nil {
return d.roleResolveFn(args.(*structs.ACLRoleBatchGetRequest), reply.(*structs.ACLRoleBatchResponse))
}
@ -1446,6 +1461,79 @@ 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("Resolve-Identity-Legacy", func(t *testing.T) {
t.Parallel()
delegate := &ACLResolverTestDelegate{
enabled: true,
datacenter: "dc1",
legacy: true,
localTokens: false,
localPolicies: false,
getPolicyFn: func(args *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error {
reply.Parent = "deny"
reply.TTL = 30
reply.ETag = "nothing"
reply.Policy = &acl.Policy{
ID: "not-needed",
PolicyRules: acl.PolicyRules{
Nodes: []*acl.NodeRule{
&acl.NodeRule{
Name: "foo",
Policy: acl.PolicyWrite,
},
},
},
}
return nil
},
}
r := newTestACLResolver(t, delegate, nil)
ident, err := r.ResolveTokenToIdentity("found-policy-and-role")
require.NoError(t, err)
require.NotNil(t, ident)
require.Equal(t, "legacy-token", ident.ID())
require.EqualValues(t, 0, delegate.localTokenResolutions)
require.EqualValues(t, 0, 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, 1, delegate.remoteLegacyResolutions)
})
t.Run("Concurrent-Token-Resolve", func(t *testing.T) {
t.Parallel()

4
agent/local/state.go
View File

@ -121,7 +121,7 @@ func (c *CheckState) CriticalFor() time.Duration {
type rpc interface {
RPC(method string, args interface{}, reply interface{}) error
ResolveIdentityFromToken(secretID string) (bool, structs.ACLIdentity, error)
ResolveTokenToIdentity(secretID string) (structs.ACLIdentity, error)
}
// State is used to represent the node's services,
@ -1364,7 +1364,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.ResolveIdentityFromToken(secretID)
ident, err := l.Delegate.ResolveTokenToIdentity(secretID)
if acl.IsErrNotFound(err) {
return ""
}