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

Add source of authority annotations to the PermissionDeniedError output. (#12567) 

This extends the acl.AllowAuthorizer with source of authority information.

The next step is to unify the AllowAuthorizer and ACLResolveResult structures; that will be done in a separate PR.

Part of #12481

Signed-off-by: Mark Anderson <manderson@hashicorp.com>
This commit is contained in:
Mark Anderson 2022-03-18 10:32:25 -07:00 committed by GitHub
parent 6363cb16c3
commit 2b367626f0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 93 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/12567.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:feature
acl: Add token information to PermissionDeniedErrors
```

1
acl/authorizer.go
View File

@ -171,6 +171,7 @@ type Authorizer interface {
// is moved into acl.
type AllowAuthorizer struct {
Authorizer
AccessorID string
}
// ACLReadAllowed checks for permission to list all the ACLs

19
acl/errors.go
View File

@ -117,12 +117,23 @@ func PermissionDenied(msg string, args ...interface{}) PermissionDeniedError {
}
// TODO Extract information from Authorizer
func PermissionDeniedByACL(_ Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) PermissionDeniedError {
func PermissionDeniedByACL(authz Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) PermissionDeniedError {
desc := NewResourceDescriptor(resourceID, context)
return PermissionDeniedError{Accessor: "", Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
return PermissionDeniedError{Accessor: extractAccessorID(authz), Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
}
func PermissionDeniedByACLUnnamed(_ Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel) PermissionDeniedError {
func PermissionDeniedByACLUnnamed(authz Authorizer, context *AuthorizerContext, resource Resource, accessLevel AccessLevel) PermissionDeniedError {
desc := NewResourceDescriptor("", context)
return PermissionDeniedError{Accessor: "", Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
return PermissionDeniedError{Accessor: extractAccessorID(authz), Resource: resource, AccessLevel: accessLevel, ResourceID: desc}
}
func extractAccessorID(authz interface{}) string {
var accessor string
switch v := authz.(type) {
case AllowAuthorizer:
accessor = v.AccessorID
case string:
accessor = v
}
return accessor
}

8
acl/testing.go
View File

@ -6,7 +6,7 @@ import (
"testing"
)
func RequirePermissionDeniedError(t testing.TB, err error, _ Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
func RequirePermissionDeniedError(t testing.TB, err error, authz Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
t.Helper()
if err == nil {
t.Fatal("An error is expected but got nil.")
@ -20,11 +20,11 @@ func RequirePermissionDeniedError(t testing.TB, err error, _ Authorizer, _ *Auth
}
}
func RequirePermissionDeniedMessage(t testing.TB, msg string, auth Authorizer, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
func RequirePermissionDeniedMessage(t testing.TB, msg string, authz interface{}, _ *AuthorizerContext, resource Resource, accessLevel AccessLevel, resourceID string) {
require.NotEmpty(t, msg, "expected non-empty error message")
var resourceIDFound string
if auth == nil {
if authz == nil {
expr := "^Permission denied" + `: provided accessor lacks permission '(\S*):(\S*)' on (.*)\s*$`
re, _ := regexp.Compile(expr)
matched := re.FindStringSubmatch(msg)
@ -37,7 +37,7 @@ func RequirePermissionDeniedMessage(t testing.TB, msg string, auth Authorizer, _
re, _ := regexp.Compile(expr)
matched := re.FindStringSubmatch(msg)
require.Equal(t, auth, matched[1], "auth")
require.Equal(t, extractAccessorID(authz), matched[1], "auth")
require.Equal(t, string(resource), matched[2], "resource")
require.Equal(t, accessLevel.String(), matched[3], "access level")
resourceIDFound = matched[4]

4
agent/consul/acl.go
View File

@ -1134,6 +1134,10 @@ func (a ACLResolveResult) Identity() structs.ACLIdentity {
return a.ACLIdentity
}
func (a ACLResolveResult) ToAllowAuthorizer() acl.AllowAuthorizer {
return acl.AllowAuthorizer{Authorizer: a, AccessorID: a.AccessorID()}
}
func (r *ACLResolver) ACLsEnabled() bool {
// Whether we desire ACLs to be enabled according to configuration
if !r.config.ACLsEnabled {

2
agent/consul/acl_endpoint.go
View File

@ -276,7 +276,7 @@ func (a *ACL) TokenRead(args *structs.ACLTokenGetRequest, reply *structs.ACLToke
return err
}
var authz acl.Authorizer
var authz ACLResolveResult
if args.TokenIDType == structs.ACLTokenAccessor {
var err error

6
agent/consul/catalog_endpoint.go
View File

@ -159,7 +159,7 @@ func nodePreApply(nodeName, nodeID string) error {
return nil
}
func servicePreApply(service *structs.NodeService, authz acl.Authorizer, authzCtxFill func(*acl.AuthorizerContext)) error {
func servicePreApply(service *structs.NodeService, authz ACLResolveResult, authzCtxFill func(*acl.AuthorizerContext)) error {
// Validate the service. This is in addition to the below since
// the above just hasn't been moved over yet. We should move it over
// in time.
@ -229,7 +229,7 @@ func checkPreApply(check *structs.HealthCheck) {
// worst let a service update revert a recent node update, so it doesn't open up
// too much abuse).
func vetRegisterWithACL(
authz acl.Authorizer,
authz ACLResolveResult,
subj *structs.RegisterRequest,
ns *structs.NodeServices,
) error {
@ -395,7 +395,7 @@ func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) e
// endpoint. The NodeService for the referenced service must be supplied, and can
// be nil; similar for the HealthCheck for the referenced health check.
func vetDeregisterWithACL(
authz acl.Authorizer,
authz ACLResolveResult,
subj *structs.DeregisterRequest,
ns *structs.NodeService,
nc *structs.HealthCheck,

69
agent/consul/catalog_endpoint_test.go
View File

@ -262,12 +262,16 @@ node "foo" {
}
}
func createToken(t *testing.T, cc rpc.ClientCodec, policyRules string) string {
func createTokenFull(t *testing.T, cc rpc.ClientCodec, policyRules string) *structs.ACLToken {
t.Helper()
return createTokenWithPolicyName(t, cc, "the-policy", policyRules, "root")
return createTokenWithPolicyNameFull(t, cc, "the-policy", policyRules, "root")
}
func createTokenWithPolicyName(t *testing.T, cc rpc.ClientCodec, policyName string, policyRules string, token string) string {
func createToken(t *testing.T, cc rpc.ClientCodec, policyRules string) string {
return createTokenFull(t, cc, policyRules).SecretID
}
func createTokenWithPolicyNameFull(t *testing.T, cc rpc.ClientCodec, policyName string, policyRules string, token string) *structs.ACLToken {
t.Helper()
reqPolicy := structs.ACLPolicySetRequest{
@ -292,9 +296,16 @@ func createTokenWithPolicyName(t *testing.T, cc rpc.ClientCodec, policyName stri
},
WriteRequest: structs.WriteRequest{Token: token},
}
err = msgpackrpc.CallWithCodec(cc, "ACL.TokenSet", &reqToken, &structs.ACLToken{})
resp := &structs.ACLToken{}
err = msgpackrpc.CallWithCodec(cc, "ACL.TokenSet", &reqToken, &resp)
require.NoError(t, err)
return secretId
return resp
}
func createTokenWithPolicyName(t *testing.T, cc rpc.ClientCodec, policyName string, policyRules string, token string) string {
return createTokenWithPolicyNameFull(t, cc, policyName, policyRules, token).SecretID
}
func TestCatalog_Register_ForwardLeader(t *testing.T) {
@ -3449,10 +3460,11 @@ func TestVetRegisterWithACL(t *testing.T) {
}
// With an "allow all" authorizer the update should be allowed.
require.NoError(t, vetRegisterWithACL(acl.ManageAll(), args, nil))
require.NoError(t, vetRegisterWithACL(ACLResolveResult{Authorizer: acl.ManageAll()}, args, nil))
})
var perms acl.Authorizer = acl.DenyAll()
var resolvedPerms ACLResolveResult
args := &structs.RegisterRequest{
Node: "nope",
@ -3464,9 +3476,10 @@ func TestVetRegisterWithACL(t *testing.T) {
node "node" {
policy = "write"
} `)
resolvedPerms = ACLResolveResult{Authorizer: perms}
// With that policy, the update should now be blocked for node reasons.
err := vetRegisterWithACL(perms, args, nil)
err := vetRegisterWithACL(resolvedPerms, args, nil)
require.True(t, acl.IsErrPermissionDenied(err))
// Now use a permitted node name.
@ -3474,7 +3487,7 @@ func TestVetRegisterWithACL(t *testing.T) {
Node: "node",
Address: "127.0.0.1",
}
require.NoError(t, vetRegisterWithACL(perms, args, nil))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, nil))
// Build some node info that matches what we have now.
ns := &structs.NodeServices{
@ -3494,7 +3507,7 @@ func TestVetRegisterWithACL(t *testing.T) {
ID: "my-id",
},
}
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(ACLResolveResult{Authorizer: perms}, args, ns)
require.True(t, acl.IsErrPermissionDenied(err))
// Chain on a basic service policy.
@ -3502,9 +3515,10 @@ func TestVetRegisterWithACL(t *testing.T) {
service "service" {
policy = "write"
} `)
resolvedPerms = ACLResolveResult{Authorizer: perms}
// With the service ACL, the update should go through.
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Add an existing service that they are clobbering and aren't allowed
// to write to.
@ -3520,7 +3534,7 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
require.True(t, acl.IsErrPermissionDenied(err))
// Chain on a policy that allows them to write to the other service.
@ -3528,14 +3542,15 @@ func TestVetRegisterWithACL(t *testing.T) {
service "other" {
policy = "write"
} `)
resolvedPerms = ACLResolveResult{Authorizer: perms}
// Now it should go through.
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Try creating the node and the service at once by having no existing
// node record. This should be ok since we have node and service
// permissions.
require.NoError(t, vetRegisterWithACL(perms, args, nil))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, nil))
// Add a node-level check to the member, which should be rejected.
args = &structs.RegisterRequest{
@ -3549,7 +3564,7 @@ func TestVetRegisterWithACL(t *testing.T) {
Node: "node",
},
}
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
testutil.RequireErrorContains(t, err, "check member must be nil")
// Move the check into the slice, but give a bad node name.
@ -3566,7 +3581,7 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
testutil.RequireErrorContains(t, err, "doesn't match register request node")
// Fix the node name, which should now go through.
@ -3583,7 +3598,7 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Add a service-level check.
args = &structs.RegisterRequest{
@ -3603,12 +3618,12 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Try creating everything at once. This should be ok since we have all
// the permissions we need. It also makes sure that we can register a
// new node, service, and associated checks.
require.NoError(t, vetRegisterWithACL(perms, args, nil))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, nil))
// Nil out the service registration, which'll skip the special case
// and force us to look at the ns data (it will look like we are
@ -3626,16 +3641,17 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Chain on a policy that forbids them to write to the other service.
perms = appendAuthz(t, perms, `
service "other" {
policy = "deny"
} `)
resolvedPerms = ACLResolveResult{Authorizer: perms}
// This should get rejected.
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
require.True(t, acl.IsErrPermissionDenied(err))
// Change the existing service data to point to a service name they
@ -3652,16 +3668,17 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Chain on a policy that forbids them to write to the node.
perms = appendAuthz(t, perms, `
node "node" {
policy = "deny"
} `)
resolvedPerms = ACLResolveResult{Authorizer: perms}
// This should get rejected because there's a node-level check in here.
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
require.True(t, acl.IsErrPermissionDenied(err))
// Change the node-level check into a service check, and then it should
@ -3680,7 +3697,7 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
require.NoError(t, vetRegisterWithACL(perms, args, ns))
require.NoError(t, vetRegisterWithACL(resolvedPerms, args, ns))
// Finally, attempt to update the node part of the data and make sure
// that gets rejected since they no longer have permissions.
@ -3698,7 +3715,7 @@ func TestVetRegisterWithACL(t *testing.T) {
},
},
}
err = vetRegisterWithACL(perms, args, ns)
err = vetRegisterWithACL(resolvedPerms, args, ns)
require.True(t, acl.IsErrPermissionDenied(err))
}
@ -3709,7 +3726,7 @@ func TestVetDeregisterWithACL(t *testing.T) {
}
// With an "allow all" authorizer the update should be allowed.
if err := vetDeregisterWithACL(acl.ManageAll(), args, nil, nil); err != nil {
if err := vetDeregisterWithACL(ACLResolveResult{Authorizer: acl.ManageAll()}, args, nil, nil); err != nil {
t.Fatalf("err: %v", err)
}
@ -3942,7 +3959,7 @@ node "node" {
},
} {
t.Run(args.Name, func(t *testing.T) {
err = vetDeregisterWithACL(args.Perms, &args.DeregisterRequest, args.Service, args.Check)
err = vetDeregisterWithACL(ACLResolveResult{Authorizer: args.Perms}, &args.DeregisterRequest, args.Service, args.Check)
if !args.Expected {
if err == nil {
t.Errorf("expected error with %+v", args.DeregisterRequest)

2
agent/consul/kvs_endpoint.go
View File

@ -32,7 +32,7 @@ type KVS struct {
// preApply does all the verification of a KVS update that is performed BEFORE
// we submit as a Raft log entry. This includes enforcing the lock delay which
// must only be done on the leader.
func kvsPreApply(logger hclog.Logger, srv *Server, authz acl.Authorizer, op api.KVOp, dirEnt *structs.DirEntry) (bool, error) {
func kvsPreApply(logger hclog.Logger, srv *Server, authz ACLResolveResult, op api.KVOp, dirEnt *structs.DirEntry) (bool, error) {
// Verify the entry.
if dirEnt.Key == "" && op != api.KVDeleteTree {
return false, fmt.Errorf("Must provide key")

6
agent/consul/txn_endpoint.go
View File

@ -32,7 +32,7 @@ type Txn struct {
// preCheck is used to verify the incoming operations before any further
// processing takes place. This checks things like ACLs.
func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.TxnErrors {
func (t *Txn) preCheck(authorizer ACLResolveResult, ops structs.TxnOps) structs.TxnErrors {
var errors structs.TxnErrors
// Perform the pre-apply checks for any KV operations.
@ -109,7 +109,7 @@ func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.Tx
}
// vetNodeTxnOp applies the given ACL policy to a node transaction operation.
func vetNodeTxnOp(op *structs.TxnNodeOp, authz acl.Authorizer) error {
func vetNodeTxnOp(op *structs.TxnNodeOp, authz ACLResolveResult) error {
var authzContext acl.AuthorizerContext
op.FillAuthzContext(&authzContext)
@ -120,7 +120,7 @@ func vetNodeTxnOp(op *structs.TxnNodeOp, authz acl.Authorizer) error {
}
// vetCheckTxnOp applies the given ACL policy to a check transaction operation.
func vetCheckTxnOp(op *structs.TxnCheckOp, authz acl.Authorizer) error {
func vetCheckTxnOp(op *structs.TxnCheckOp, authz ACLResolveResult) error {
var authzContext acl.AuthorizerContext
op.FillAuthzContext(&authzContext)

26
agent/consul/txn_endpoint_test.go
View File

@ -345,7 +345,8 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
check := structs.HealthCheck{Node: "nope", CheckID: types.CheckID("nope")}
state.EnsureCheck(4, &check)
id := createToken(t, rpcClient(t, s1), testTxnRules)
token := createTokenFull(t, rpcClient(t, s1), testTxnRules)
id := token.SecretID
// Set up a transaction where every operation should get blocked due to
// ACLs.
@ -564,11 +565,11 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
// These get filtered but won't result in an error.
case api.KVSet, api.KVDelete, api.KVDeleteCAS, api.KVDeleteTree, api.KVCAS, api.KVLock, api.KVUnlock, api.KVCheckNotExists:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceKey, acl.AccessWrite, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceKey, acl.AccessWrite, "nope")
outPos++
default:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceKey, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceKey, acl.AccessRead, "nope")
outPos++
}
case op.Node != nil:
@ -577,11 +578,11 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
// These get filtered but won't result in an error.
case api.NodeSet, api.NodeDelete, api.NodeDeleteCAS, api.NodeCAS:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceNode, acl.AccessWrite, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceNode, acl.AccessWrite, "nope")
outPos++
default:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceNode, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceNode, acl.AccessRead, "nope")
outPos++
}
case op.Service != nil:
@ -590,11 +591,11 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
// These get filtered but won't result in an error.
case api.ServiceSet, api.ServiceCAS, api.ServiceDelete, api.ServiceDeleteCAS:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceService, acl.AccessWrite, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceService, acl.AccessWrite, "nope")
outPos++
default:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceService, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceService, acl.AccessRead, "nope")
outPos++
}
case op.Check != nil:
@ -603,11 +604,11 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
// These get filtered but won't result in an error.
case api.CheckSet, api.CheckCAS, api.CheckDelete, api.CheckDeleteCAS:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceNode, acl.AccessWrite, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceNode, acl.AccessWrite, "nope")
outPos++
default:
require.Equal(t, err.OpIndex, i)
acl.RequirePermissionDeniedMessage(t, err.What, nil, nil, acl.ResourceNode, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, err.What, token.AccessorID, nil, acl.ResourceNode, acl.AccessRead, "nope")
outPos++
}
}
@ -873,7 +874,8 @@ func TestTxn_Read_ACLDeny(t *testing.T) {
check := structs.HealthCheck{Node: "nope", CheckID: types.CheckID("nope")}
state.EnsureCheck(4, &check)
id := createToken(t, codec, testTxnRules)
token := createTokenFull(t, codec, testTxnRules)
id := token.AccessorID
t.Run("simple read operations (results get filtered out)", func(t *testing.T) {
arg := structs.TxnReadRequest{
@ -934,8 +936,8 @@ func TestTxn_Read_ACLDeny(t *testing.T) {
var out structs.TxnReadResponse
err := msgpackrpc.CallWithCodec(codec, "Txn.Read", &arg, &out)
require.NoError(t, err)
acl.RequirePermissionDeniedMessage(t, out.Errors[0].What, nil, nil, acl.ResourceKey, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, out.Errors[1].What, nil, nil, acl.ResourceKey, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, out.Errors[0].What, token.AccessorID, nil, acl.ResourceKey, acl.AccessRead, "nope")
acl.RequirePermissionDeniedMessage(t, out.Errors[1].What, token.AccessorID, nil, acl.ResourceKey, acl.AccessRead, "nope")
require.Empty(t, out.Results)
})

1
agent/structs/config_entry.go
View File

@ -60,6 +60,7 @@ type ConfigEntry interface {
// CanRead and CanWrite return whether or not the given Authorizer
// has permission to read or write to the config entry, respectively.
// TODO(acl-error-enhancements) This should be ACLResolveResult or similar but we have to wait until we move things to the acl package
CanRead(acl.Authorizer) error
CanWrite(acl.Authorizer) error