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

acl: remove special handling of services in txn_endpoint 

Follow up to: https://github.com/hashicorp/consul/pull/10738#discussion_r680190210

Previously we were passing an Authorizer that would always allow the
operation, then later checking the authorization using vetServiceTxnOp.

On the surface this seemed strange, but I think it was actually masking
a bug as well. Over time `servicePreApply` was changed to add additional
authorization for `service.Proxy.DestinationServiceName`, but because
we were passing a nil Authorizer, that authorization was not handled on
the txn_endpoint.

`TxnServiceOp.FillAuthzContext` has some special handling in enterprise,
so we need to make sure to continue to use that from the Txn endpoint.

This commit removes the `vetServiceTxnOp` function, and passes in the
`FillAuthzContext` function so that `servicePreApply` can be used by
both the catalog and txn endpoints. This should be much less error prone
and prevent bugs like this in the future.
This commit is contained in:
Daniel Nephin 2021-08-04 18:18:51 -04:00
parent f6d5a85561
commit 0ca9e875e2
3 changed files with 4 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
agent/consul/acl.go
View File

@ -2252,23 +2252,6 @@ func vetNodeTxnOp(op *structs.TxnNodeOp, rule acl.Authorizer) error {
return nil return nil
} }
// vetServiceTxnOp applies the given ACL policy to a service transaction operation.
func vetServiceTxnOp(op *structs.TxnServiceOp, rule acl.Authorizer) error {
// Fast path if ACLs are not enabled.
if rule == nil {
return nil
}
var authzContext acl.AuthorizerContext
op.FillAuthzContext(&authzContext)
if rule.ServiceWrite(op.Service.Service, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
return nil
}
// vetCheckTxnOp applies the given ACL policy to a check transaction operation. // vetCheckTxnOp applies the given ACL policy to a check transaction operation.
func vetCheckTxnOp(op *structs.TxnCheckOp, rule acl.Authorizer) error { func vetCheckTxnOp(op *structs.TxnCheckOp, rule acl.Authorizer) error {
// Fast path if ACLs are not enabled. // Fast path if ACLs are not enabled.

6
agent/consul/catalog_endpoint.go
View File

@ -85,7 +85,7 @@ func nodePreApply(nodeName, nodeID string) error {
return nil return nil
} }
func servicePreApply(service *structs.NodeService, authz acl.Authorizer) error { func servicePreApply(service *structs.NodeService, authz acl.Authorizer, authzCtxFill func(*acl.AuthorizerContext)) error {
// Validate the service. This is in addition to the below since // 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 // the above just hasn't been moved over yet. We should move it over
// in time. // in time.
@ -110,7 +110,7 @@ func servicePreApply(service *structs.NodeService, authz acl.Authorizer) error {
} }
var authzContext acl.AuthorizerContext var authzContext acl.AuthorizerContext
service.FillAuthzContext(&authzContext) authzCtxFill(&authzContext)
// Apply the ACL policy if any. The 'consul' service is excluded // Apply the ACL policy if any. The 'consul' service is excluded
// since it is managed automatically internally (that behavior // since it is managed automatically internally (that behavior
@ -175,7 +175,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
// Handle a service registration. // Handle a service registration.
if args.Service != nil { if args.Service != nil {
if err := servicePreApply(args.Service, authz); err != nil { if err := servicePreApply(args.Service, authz, args.Service.FillAuthzContext); err != nil {
return err return err
} }
} }

13
agent/consul/txn_endpoint.go
View File

@ -81,18 +81,7 @@ func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.Tx
} }
service := &op.Service.Service service := &op.Service.Service
// acl.ManageAll is used here because the request will be authorized if err := servicePreApply(service, authorizer, op.Service.FillAuthzContext); err != nil {
// later using vetServiceTxnOp.
if err := servicePreApply(service, acl.ManageAll()); err != nil {
errors = append(errors, &structs.TxnError{
OpIndex: i,
What: err.Error(),
})
break
}
// Check that the token has permissions for the given operation.
if err := vetServiceTxnOp(op.Service, authorizer); err != nil {
errors = append(errors, &structs.TxnError{ errors = append(errors, &structs.TxnError{
OpIndex: i, OpIndex: i,
What: err.Error(), What: err.Error(),