acl: move vetRegisterWithACL and vetDeregisterWithACL

These functions are used in only one place. Move the functions next to their one caller
to improve code locality.

This change is being made in preparation for moving the ACLResolver into an
acl package. The moved functions were previously in the same file as the ACLResolver.
By moving them out of that file we may be able to move the entire file
with fewer modifications.
This commit is contained in:
Daniel Nephin 2021-07-30 12:42:22 -04:00
parent b223c2bc25
commit c8eedabc7c
4 changed files with 668 additions and 672 deletions

View File

@ -2049,192 +2049,6 @@ func (r *ACLResolver) filterACL(token string, subj interface{}) error {
return r.filterACLWithAuthorizer(authorizer, subj) return r.filterACLWithAuthorizer(authorizer, subj)
} }
// vetRegisterWithACL applies the given ACL's policy to the catalog update and
// determines if it is allowed. Since the catalog register request is so
// dynamic, this is a pretty complex algorithm and was worth breaking out of the
// endpoint. The NodeServices record for the node must be supplied, and can be
// nil.
//
// This is a bit racy because we have to check the state store outside of a
// transaction. It's the best we can do because we don't want to flow ACL
// checking down there. The node information doesn't change in practice, so this
// will be fine. If we expose ways to change node addresses in a later version,
// then we should split the catalog API at the node and service level so we can
// address this race better (even then it would be super rare, and would at
// worst let a service update revert a recent node update, so it doesn't open up
// too much abuse).
func vetRegisterWithACL(rule acl.Authorizer, subj *structs.RegisterRequest,
ns *structs.NodeServices) error {
// Fast path if ACLs are not enabled.
if rule == nil {
return nil
}
var authzContext acl.AuthorizerContext
subj.FillAuthzContext(&authzContext)
// Vet the node info. This allows service updates to re-post the required
// node info for each request without having to have node "write"
// privileges.
needsNode := ns == nil || subj.ChangesNode(ns.Node)
if needsNode && rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
// Vet the service change. This includes making sure they can register
// the given service, and that we can write to any existing service that
// is being modified by id (if any).
if subj.Service != nil {
if rule.ServiceWrite(subj.Service.Service, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
if ns != nil {
other, ok := ns.Services[subj.Service.ID]
if ok {
// This is effectively a delete, so we DO NOT apply the
// sentinel scope to the service we are overwriting, just
// the regular ACL policy.
var secondaryCtx acl.AuthorizerContext
other.FillAuthzContext(&secondaryCtx)
if rule.ServiceWrite(other.Service, &secondaryCtx) != acl.Allow {
return acl.ErrPermissionDenied
}
}
}
}
// Make sure that the member was flattened before we got there. This
// keeps us from having to verify this check as well.
if subj.Check != nil {
return fmt.Errorf("check member must be nil")
}
// Vet the checks. Node-level checks require node write, and
// service-level checks require service write.
for _, check := range subj.Checks {
// Make sure that the node matches - we don't allow you to mix
// checks from other nodes because we'd have to pull a bunch
// more state store data to check this. If ACLs are enabled then
// we simply require them to match in a given request. There's a
// note in state_store.go to ban this down there in Consul 0.8,
// but it's good to leave this here because it's required for
// correctness wrt. ACLs.
if check.Node != subj.Node {
return fmt.Errorf("Node '%s' for check '%s' doesn't match register request node '%s'",
check.Node, check.CheckID, subj.Node)
}
// Node-level check.
if check.ServiceID == "" {
if rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
continue
}
// Service-level check, check the common case where it
// matches the service part of this request, which has
// already been vetted above, and might be being registered
// along with its checks.
if subj.Service != nil && subj.Service.ID == check.ServiceID {
continue
}
// Service-level check for some other service. Make sure they've
// got write permissions for that service.
if ns == nil {
return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
}
other, ok := ns.Services[check.ServiceID]
if !ok {
return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
}
// We are only adding a check here, so we don't add the scope,
// since the sentinel policy doesn't apply to adding checks at
// this time.
var secondaryCtx acl.AuthorizerContext
other.FillAuthzContext(&secondaryCtx)
if rule.ServiceWrite(other.Service, &secondaryCtx) != acl.Allow {
return acl.ErrPermissionDenied
}
}
return nil
}
// vetDeregisterWithACL applies the given ACL's policy to the catalog update and
// determines if it is allowed. Since the catalog deregister request is so
// dynamic, this is a pretty complex algorithm and was worth breaking out of the
// 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(rule acl.Authorizer, subj *structs.DeregisterRequest,
ns *structs.NodeService, nc *structs.HealthCheck) error {
// Fast path if ACLs are not enabled.
if rule == nil {
return nil
}
// We don't apply sentinel in this path, since at this time sentinel
// only applies to create and update operations.
var authzContext acl.AuthorizerContext
// fill with the defaults for use with the NodeWrite check
subj.FillAuthzContext(&authzContext)
// Allow service deregistration if the token has write permission for the node.
// This accounts for cases where the agent no longer has a token with write permission
// on the service to deregister it.
if rule.NodeWrite(subj.Node, &authzContext) == acl.Allow {
return nil
}
// This order must match the code in applyDeregister() in
// fsm/commands_oss.go since it also evaluates things in this order,
// and will ignore fields based on this precedence. This lets us also
// ignore them from an ACL perspective.
if subj.ServiceID != "" {
if ns == nil {
return fmt.Errorf("Unknown service '%s'", subj.ServiceID)
}
ns.FillAuthzContext(&authzContext)
if rule.ServiceWrite(ns.Service, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
} else if subj.CheckID != "" {
if nc == nil {
return fmt.Errorf("Unknown check '%s'", subj.CheckID)
}
nc.FillAuthzContext(&authzContext)
if nc.ServiceID != "" {
if rule.ServiceWrite(nc.ServiceName, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
} else {
if rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
}
} else {
// Since NodeWrite is not given - otherwise the earlier check
// would've returned already - we can deny here.
return acl.ErrPermissionDenied
}
return nil
}
// vetNodeTxnOp applies the given ACL policy to a node transaction operation. // vetNodeTxnOp applies the given ACL policy to a node transaction operation.
func vetNodeTxnOp(op *structs.TxnNodeOp, rule acl.Authorizer) error { func vetNodeTxnOp(op *structs.TxnNodeOp, rule acl.Authorizer) error {
// Fast path if ACLs are not enabled. // Fast path if ACLs are not enabled.

View File

@ -4,7 +4,6 @@ import (
"fmt" "fmt"
"os" "os"
"reflect" "reflect"
"strings"
"sync/atomic" "sync/atomic"
"testing" "testing"
"time" "time"
@ -3421,488 +3420,6 @@ func TestACL_unhandledFilterType(t *testing.T) {
srv.filterACL(token, &structs.HealthCheck{}) srv.filterACL(token, &structs.HealthCheck{})
} }
func TestACL_vetRegisterWithACL(t *testing.T) {
t.Parallel()
args := &structs.RegisterRequest{
Node: "nope",
Address: "127.0.0.1",
}
// With a nil ACL, the update should be allowed.
if err := vetRegisterWithACL(nil, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Create a basic node policy.
policy, err := acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// With that policy, the update should now be blocked for node reasons.
err = vetRegisterWithACL(perms, args, nil)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Now use a permitted node name.
args.Node = "node"
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Build some node info that matches what we have now.
ns := &structs.NodeServices{
Node: &structs.Node{
Node: "node",
Address: "127.0.0.1",
},
Services: make(map[string]*structs.NodeService),
}
// Try to register a service, which should be blocked.
args.Service = &structs.NodeService{
Service: "service",
ID: "my-id",
}
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Chain on a basic service policy.
policy, err = acl.NewPolicyFromSource("", 0, `
service "service" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// With the service ACL, the update should go through.
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Add an existing service that they are clobbering and aren't allowed
// to write to.
ns.Services["my-id"] = &structs.NodeService{
Service: "other",
ID: "my-id",
}
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Chain on a policy that allows them to write to the other service.
policy, err = acl.NewPolicyFromSource("", 0, `
service "other" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// Now it should go through.
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// 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.
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Add a node-level check to the member, which should be rejected.
args.Check = &structs.HealthCheck{
Node: "node",
}
err = vetRegisterWithACL(perms, args, ns)
if err == nil || !strings.Contains(err.Error(), "check member must be nil") {
t.Fatalf("bad: %v", err)
}
// Move the check into the slice, but give a bad node name.
args.Check.Node = "nope"
args.Checks = append(args.Checks, args.Check)
args.Check = nil
err = vetRegisterWithACL(perms, args, ns)
if err == nil || !strings.Contains(err.Error(), "doesn't match register request node") {
t.Fatalf("bad: %v", err)
}
// Fix the node name, which should now go through.
args.Checks[0].Node = "node"
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Add a service-level check.
args.Checks = append(args.Checks, &structs.HealthCheck{
Node: "node",
ServiceID: "my-id",
})
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// 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.
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// 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
// writing to the "other" service which also has "my-id").
args.Service = nil
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Chain on a policy that forbids them to write to the other service.
policy, err = acl.NewPolicyFromSource("", 0, `
service "other" {
policy = "deny"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// This should get rejected.
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Change the existing service data to point to a service name they
// car write to. This should go through.
ns.Services["my-id"] = &structs.NodeService{
Service: "service",
ID: "my-id",
}
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Chain on a policy that forbids them to write to the node.
policy, err = acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "deny"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// This should get rejected because there's a node-level check in here.
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Change the node-level check into a service check, and then it should
// go through.
args.Checks[0].ServiceID = "my-id"
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Finally, attempt to update the node part of the data and make sure
// that gets rejected since they no longer have permissions.
args.Address = "127.0.0.2"
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
}
func TestACL_vetDeregisterWithACL(t *testing.T) {
t.Parallel()
args := &structs.DeregisterRequest{
Node: "nope",
}
// With a nil ACL, the update should be allowed.
if err := vetDeregisterWithACL(nil, args, nil, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Create a basic node policy.
policy, err := acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
nodePerms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
policy, err = acl.NewPolicyFromSource("", 0, `
service "my-service" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
servicePerms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
for _, args := range []struct {
DeregisterRequest structs.DeregisterRequest
Service *structs.NodeService
Check *structs.HealthCheck
Perms acl.Authorizer
Expected bool
Name string
}{
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: servicePerms,
Expected: false,
Name: "right on service but node dergister request",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: false,
Name: "no rights on node nor service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "no rights on node but rights on service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node nor service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
Name: "no rights on node but rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but not on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "rights on service for service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
Name: "right on node for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
Name: "rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service for node check",
},
} {
t.Run(args.Name, func(t *testing.T) {
err = vetDeregisterWithACL(args.Perms, &args.DeregisterRequest, args.Service, args.Check)
if !args.Expected {
if err == nil {
t.Errorf("expected error with %+v", args.DeregisterRequest)
}
if !acl.IsErrPermissionDenied(err) {
t.Errorf("expected permission denied error with %+v, instead got %+v", args.DeregisterRequest, err)
}
} else if err != nil {
t.Errorf("expected no error with %+v", args.DeregisterRequest)
}
})
}
}
func TestDedupeServiceIdentities(t *testing.T) { func TestDedupeServiceIdentities(t *testing.T) {
srvid := func(name string, datacenters ...string) *structs.ACLServiceIdentity { srvid := func(name string, datacenters ...string) *structs.ACLServiceIdentity {
return &structs.ACLServiceIdentity{ return &structs.ACLServiceIdentity{

View File

@ -212,6 +212,124 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
return err return err
} }
// vetRegisterWithACL applies the given ACL's policy to the catalog update and
// determines if it is allowed. Since the catalog register request is so
// dynamic, this is a pretty complex algorithm and was worth breaking out of the
// endpoint. The NodeServices record for the node must be supplied, and can be
// nil.
//
// This is a bit racy because we have to check the state store outside of a
// transaction. It's the best we can do because we don't want to flow ACL
// checking down there. The node information doesn't change in practice, so this
// will be fine. If we expose ways to change node addresses in a later version,
// then we should split the catalog API at the node and service level so we can
// address this race better (even then it would be super rare, and would at
// worst let a service update revert a recent node update, so it doesn't open up
// too much abuse).
func vetRegisterWithACL(
rule acl.Authorizer,
subj *structs.RegisterRequest,
ns *structs.NodeServices,
) error {
var authzContext acl.AuthorizerContext
subj.FillAuthzContext(&authzContext)
// Vet the node info. This allows service updates to re-post the required
// node info for each request without having to have node "write"
// privileges.
needsNode := ns == nil || subj.ChangesNode(ns.Node)
if needsNode && rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
// Vet the service change. This includes making sure they can register
// the given service, and that we can write to any existing service that
// is being modified by id (if any).
if subj.Service != nil {
if rule.ServiceWrite(subj.Service.Service, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
if ns != nil {
other, ok := ns.Services[subj.Service.ID]
if ok {
// This is effectively a delete, so we DO NOT apply the
// sentinel scope to the service we are overwriting, just
// the regular ACL policy.
var secondaryCtx acl.AuthorizerContext
other.FillAuthzContext(&secondaryCtx)
if rule.ServiceWrite(other.Service, &secondaryCtx) != acl.Allow {
return acl.ErrPermissionDenied
}
}
}
}
// Make sure that the member was flattened before we got there. This
// keeps us from having to verify this check as well.
if subj.Check != nil {
return fmt.Errorf("check member must be nil")
}
// Vet the checks. Node-level checks require node write, and
// service-level checks require service write.
for _, check := range subj.Checks {
// Make sure that the node matches - we don't allow you to mix
// checks from other nodes because we'd have to pull a bunch
// more state store data to check this. If ACLs are enabled then
// we simply require them to match in a given request. There's a
// note in state_store.go to ban this down there in Consul 0.8,
// but it's good to leave this here because it's required for
// correctness wrt. ACLs.
if check.Node != subj.Node {
return fmt.Errorf("Node '%s' for check '%s' doesn't match register request node '%s'",
check.Node, check.CheckID, subj.Node)
}
// Node-level check.
if check.ServiceID == "" {
if rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
continue
}
// Service-level check, check the common case where it
// matches the service part of this request, which has
// already been vetted above, and might be being registered
// along with its checks.
if subj.Service != nil && subj.Service.ID == check.ServiceID {
continue
}
// Service-level check for some other service. Make sure they've
// got write permissions for that service.
if ns == nil {
return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
}
other, ok := ns.Services[check.ServiceID]
if !ok {
return fmt.Errorf("Unknown service '%s' for check '%s'", check.ServiceID, check.CheckID)
}
// We are only adding a check here, so we don't add the scope,
// since the sentinel policy doesn't apply to adding checks at
// this time.
var secondaryCtx acl.AuthorizerContext
other.FillAuthzContext(&secondaryCtx)
if rule.ServiceWrite(other.Service, &secondaryCtx) != acl.Allow {
return acl.ErrPermissionDenied
}
}
return nil
}
// Deregister is used to remove a service registration for a given node. // Deregister is used to remove a service registration for a given node.
func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) error { func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) error {
if done, err := c.srv.ForwardRPC("Catalog.Deregister", args, reply); done { if done, err := c.srv.ForwardRPC("Catalog.Deregister", args, reply); done {
@ -261,6 +379,70 @@ func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) e
return err return err
} }
// vetDeregisterWithACL applies the given ACL's policy to the catalog update and
// determines if it is allowed. Since the catalog deregister request is so
// dynamic, this is a pretty complex algorithm and was worth breaking out of the
// 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(
rule acl.Authorizer,
subj *structs.DeregisterRequest,
ns *structs.NodeService,
nc *structs.HealthCheck,
) error {
// We don't apply sentinel in this path, since at this time sentinel
// only applies to create and update operations.
var authzContext acl.AuthorizerContext
// fill with the defaults for use with the NodeWrite check
subj.FillAuthzContext(&authzContext)
// Allow service deregistration if the token has write permission for the node.
// This accounts for cases where the agent no longer has a token with write permission
// on the service to deregister it.
if rule.NodeWrite(subj.Node, &authzContext) == acl.Allow {
return nil
}
// This order must match the code in applyDeregister() in
// fsm/commands_oss.go since it also evaluates things in this order,
// and will ignore fields based on this precedence. This lets us also
// ignore them from an ACL perspective.
if subj.ServiceID != "" {
if ns == nil {
return fmt.Errorf("Unknown service '%s'", subj.ServiceID)
}
ns.FillAuthzContext(&authzContext)
if rule.ServiceWrite(ns.Service, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
} else if subj.CheckID != "" {
if nc == nil {
return fmt.Errorf("Unknown check '%s'", subj.CheckID)
}
nc.FillAuthzContext(&authzContext)
if nc.ServiceID != "" {
if rule.ServiceWrite(nc.ServiceName, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
} else {
if rule.NodeWrite(subj.Node, &authzContext) != acl.Allow {
return acl.ErrPermissionDenied
}
}
} else {
// Since NodeWrite is not given - otherwise the earlier check
// would've returned already - we can deny here.
return acl.ErrPermissionDenied
}
return nil
}
// ListDatacenters is used to query for the list of known datacenters // ListDatacenters is used to query for the list of known datacenters
func (c *Catalog) ListDatacenters(args *structs.DatacentersRequest, reply *[]string) error { func (c *Catalog) ListDatacenters(args *structs.DatacentersRequest, reply *[]string) error {
dcs, err := c.srv.router.GetDatacentersByDistance() dcs, err := c.srv.router.GetDatacentersByDistance()

View File

@ -8,6 +8,10 @@ import (
"testing" "testing"
"time" "time"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/api" "github.com/hashicorp/consul/api"
@ -16,9 +20,6 @@ import (
"github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/testrpc"
"github.com/hashicorp/consul/types" "github.com/hashicorp/consul/types"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
) )
func TestCatalog_Register(t *testing.T) { func TestCatalog_Register(t *testing.T) {
@ -3466,3 +3467,485 @@ service "gateway" {
assert.Equal(r, expect, resp.Services) assert.Equal(r, expect, resp.Services)
}) })
} }
func TestVetRegisterWithACL(t *testing.T) {
t.Parallel()
args := &structs.RegisterRequest{
Node: "nope",
Address: "127.0.0.1",
}
// With an "allow all" authorizer the update should be allowed.
if err := vetRegisterWithACL(acl.ManageAll(), args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Create a basic node policy.
policy, err := acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// With that policy, the update should now be blocked for node reasons.
err = vetRegisterWithACL(perms, args, nil)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Now use a permitted node name.
args.Node = "node"
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Build some node info that matches what we have now.
ns := &structs.NodeServices{
Node: &structs.Node{
Node: "node",
Address: "127.0.0.1",
},
Services: make(map[string]*structs.NodeService),
}
// Try to register a service, which should be blocked.
args.Service = &structs.NodeService{
Service: "service",
ID: "my-id",
}
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Chain on a basic service policy.
policy, err = acl.NewPolicyFromSource("", 0, `
service "service" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// With the service ACL, the update should go through.
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Add an existing service that they are clobbering and aren't allowed
// to write to.
ns.Services["my-id"] = &structs.NodeService{
Service: "other",
ID: "my-id",
}
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Chain on a policy that allows them to write to the other service.
policy, err = acl.NewPolicyFromSource("", 0, `
service "other" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// Now it should go through.
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// 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.
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Add a node-level check to the member, which should be rejected.
args.Check = &structs.HealthCheck{
Node: "node",
}
err = vetRegisterWithACL(perms, args, ns)
if err == nil || !strings.Contains(err.Error(), "check member must be nil") {
t.Fatalf("bad: %v", err)
}
// Move the check into the slice, but give a bad node name.
args.Check.Node = "nope"
args.Checks = append(args.Checks, args.Check)
args.Check = nil
err = vetRegisterWithACL(perms, args, ns)
if err == nil || !strings.Contains(err.Error(), "doesn't match register request node") {
t.Fatalf("bad: %v", err)
}
// Fix the node name, which should now go through.
args.Checks[0].Node = "node"
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Add a service-level check.
args.Checks = append(args.Checks, &structs.HealthCheck{
Node: "node",
ServiceID: "my-id",
})
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// 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.
if err := vetRegisterWithACL(perms, args, nil); err != nil {
t.Fatalf("err: %v", err)
}
// 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
// writing to the "other" service which also has "my-id").
args.Service = nil
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Chain on a policy that forbids them to write to the other service.
policy, err = acl.NewPolicyFromSource("", 0, `
service "other" {
policy = "deny"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// This should get rejected.
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Change the existing service data to point to a service name they
// car write to. This should go through.
ns.Services["my-id"] = &structs.NodeService{
Service: "service",
ID: "my-id",
}
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Chain on a policy that forbids them to write to the node.
policy, err = acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "deny"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
// This should get rejected because there's a node-level check in here.
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
// Change the node-level check into a service check, and then it should
// go through.
args.Checks[0].ServiceID = "my-id"
if err := vetRegisterWithACL(perms, args, ns); err != nil {
t.Fatalf("err: %v", err)
}
// Finally, attempt to update the node part of the data and make sure
// that gets rejected since they no longer have permissions.
args.Address = "127.0.0.2"
err = vetRegisterWithACL(perms, args, ns)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}
}
func TestVetDeregisterWithACL(t *testing.T) {
t.Parallel()
args := &structs.DeregisterRequest{
Node: "nope",
}
// With an "allow all" authorizer the update should be allowed.
if err := vetDeregisterWithACL(acl.ManageAll(), args, nil, nil); err != nil {
t.Fatalf("err: %v", err)
}
// Create a basic node policy.
policy, err := acl.NewPolicyFromSource("", 0, `
node "node" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
nodePerms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
policy, err = acl.NewPolicyFromSource("", 0, `
service "my-service" {
policy = "write"
}
`, acl.SyntaxLegacy, nil, nil)
if err != nil {
t.Fatalf("err %v", err)
}
servicePerms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}
for _, args := range []struct {
DeregisterRequest structs.DeregisterRequest
Service *structs.NodeService
Check *structs.HealthCheck
Perms acl.Authorizer
Expected bool
Name string
}{
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: servicePerms,
Expected: false,
Name: "right on service but node dergister request",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: false,
Name: "no rights on node nor service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "no rights on node but rights on service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node nor service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
Name: "no rights on node but rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but not on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "rights on service for service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
Name: "right on node for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
Name: "rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service for node check",
},
} {
t.Run(args.Name, func(t *testing.T) {
err = vetDeregisterWithACL(args.Perms, &args.DeregisterRequest, args.Service, args.Check)
if !args.Expected {
if err == nil {
t.Errorf("expected error with %+v", args.DeregisterRequest)
}
if !acl.IsErrPermissionDenied(err) {
t.Errorf("expected permission denied error with %+v, instead got %+v", args.DeregisterRequest, err)
}
} else if err != nil {
t.Errorf("expected no error with %+v", args.DeregisterRequest)
}
})
}
}