diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 0fa5976fd0..99711cb27a 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -2048,231 +2048,3 @@ 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. - if rule == nil { - return nil - } - - var authzContext acl.AuthorizerContext - op.FillAuthzContext(&authzContext) - - if rule.NodeWrite(op.Node.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - - return nil -} - -// vetCheckTxnOp applies the given ACL policy to a check transaction operation. -func vetCheckTxnOp(op *structs.TxnCheckOp, rule acl.Authorizer) error { - // Fast path if ACLs are not enabled. - if rule == nil { - return nil - } - - var authzContext acl.AuthorizerContext - op.FillAuthzContext(&authzContext) - - if op.Check.ServiceID == "" { - // Node-level check. - if rule.NodeWrite(op.Check.Node, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - } else { - // Service-level check. - if rule.ServiceWrite(op.Check.ServiceName, &authzContext) != acl.Allow { - return acl.ErrPermissionDenied - } - } - - return nil -} diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index f1582c8f51..2152b099e7 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 eabe07e811..26eb394175 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( + authz 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 && authz.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 authz.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 authz.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 authz.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 authz.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( + authz 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 authz.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 authz.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 authz.ServiceWrite(nc.ServiceName, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + } else { + if authz.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 b19b904308..46be6e702a 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) + } + }) + } +} diff --git a/agent/consul/txn_endpoint.go b/agent/consul/txn_endpoint.go index 2f0081ee59..8c8ab41400 100644 --- a/agent/consul/txn_endpoint.go +++ b/agent/consul/txn_endpoint.go @@ -108,6 +108,36 @@ func (t *Txn) preCheck(authorizer acl.Authorizer, ops structs.TxnOps) structs.Tx return errors } +// vetNodeTxnOp applies the given ACL policy to a node transaction operation. +func vetNodeTxnOp(op *structs.TxnNodeOp, authz acl.Authorizer) error { + var authzContext acl.AuthorizerContext + op.FillAuthzContext(&authzContext) + + if authz.NodeWrite(op.Node.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + return nil +} + +// vetCheckTxnOp applies the given ACL policy to a check transaction operation. +func vetCheckTxnOp(op *structs.TxnCheckOp, authz acl.Authorizer) error { + var authzContext acl.AuthorizerContext + op.FillAuthzContext(&authzContext) + + if op.Check.ServiceID == "" { + // Node-level check. + if authz.NodeWrite(op.Check.Node, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + } else { + // Service-level check. + if authz.ServiceWrite(op.Check.ServiceName, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + } + return nil +} + // Apply is used to apply multiple operations in a single, atomic transaction. func (t *Txn) Apply(args *structs.TxnRequest, reply *structs.TxnResponse) error { if done, err := t.srv.ForwardRPC("Txn.Apply", args, reply); done {