From c8eedabc7c652dfd547f1b23035cba012a80d9b1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Jul 2021 12:42:22 -0400 Subject: [PATCH] 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. --- agent/consul/acl.go | 186 ---------- agent/consul/acl_test.go | 483 ------------------------- agent/consul/catalog_endpoint.go | 182 ++++++++++ agent/consul/catalog_endpoint_test.go | 489 +++++++++++++++++++++++++- 4 files changed, 668 insertions(+), 672 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 0fa5976fd..b53d50210 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -2049,192 +2049,6 @@ func (r *ACLResolver) filterACL(token string, subj interface{}) error { 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. func vetNodeTxnOp(op *structs.TxnNodeOp, rule acl.Authorizer) error { // Fast path if ACLs are not enabled. diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index f1582c8f5..2152b099e 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "reflect" - "strings" "sync/atomic" "testing" "time" @@ -3421,488 +3420,6 @@ func TestACL_unhandledFilterType(t *testing.T) { 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) { srvid := func(name string, datacenters ...string) *structs.ACLServiceIdentity { return &structs.ACLServiceIdentity{ diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index eabe07e81..8869481a1 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -212,6 +212,124 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error 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. func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) error { 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 } +// 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 func (c *Catalog) ListDatacenters(args *structs.DatacentersRequest, reply *[]string) error { dcs, err := c.srv.router.GetDatacentersByDistance() diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index b19b90430..46be6e702 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -8,6 +8,10 @@ import ( "testing" "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/agent/structs" "github.com/hashicorp/consul/api" @@ -16,9 +20,6 @@ import ( "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "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) { @@ -3466,3 +3467,485 @@ service "gateway" { 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) + } + }) + } +}