acl: allow service deregistration with node write permission (#5217)

With ACLs enabled if an agent is wiped and restarted without a leave
it can no longer deregister the services it had previously registered
because it no longer has the tokens the services were registered with.
To remedy that we allow service deregistration from tokens with node
write permission.

agent/consul/acl.go

@@ -1592,9 +1592,17 @@ func vetDeregisterWithACL(rule acl.Authorizer, subj *structs.DeregisterRequest,
 	// We don't apply sentinel in this path, since at this time sentinel
 	// only applies to create and update operations.
 
-	// This order must match the code in applyRegister() in fsm.go since it
+	// Allow service deregistration if the token has write permission for the node.
-	// also evaluates things in this order, and will ignore fields based on
+	// This accounts for cases where the agent no longer has a token with write permission
-	// this precedence. This lets us also ignore them from an ACL perspective.
+	// on the service to deregister it.
+	if rule.NodeWrite(subj.Node, nil) {
+		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)
@@ -1616,10 +1624,10 @@ func vetDeregisterWithACL(rule acl.Authorizer, subj *structs.DeregisterRequest,
 			}
 		}
 	} else {
-		if !rule.NodeWrite(subj.Node, nil) {
+		// Since NodeWrite is not given - otherwise the earlier check
+		// would've returned already - we can deny here.
 		return acl.ErrPermissionDenied
 	}
-	}
 
 	return nil
 }

agent/consul/acl_test.go

@@ -3268,93 +3268,242 @@ func TestACL_vetDeregisterWithACL(t *testing.T) {
 node "node" {
   policy = "write"
 }
-service "service" {
+`, acl.SyntaxLegacy, nil)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	nodePerms, err := acl.NewPolicyAuthorizer(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)
 	if err != nil {
 		t.Fatalf("err %v", err)
 	}
-	perms, err := acl.NewPolicyAuthorizer(acl.DenyAll(), []*acl.Policy{policy}, nil)
+	servicePerms, err := acl.NewPolicyAuthorizer(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.
+	for _, args := range []struct {
-	err = vetDeregisterWithACL(perms, args, nil, nil)
+		DeregisterRequest structs.DeregisterRequest
-	if !acl.IsErrPermissionDenied(err) {
+		Service           *structs.NodeService
-		t.Fatalf("bad: %v", err)
+		Check             *structs.HealthCheck
-	}
+		Perms             *acl.PolicyAuthorizer
-
+		Expected          bool
-	// Now use a permitted node name.
+		Name              string
-	args.Node = "node"
+	}{
-	if err := vetDeregisterWithACL(perms, args, nil, nil); err != nil {
+		{
-		t.Fatalf("err: %v", err)
+			DeregisterRequest: structs.DeregisterRequest{
-	}
+				Node: "nope",
-
+			},
-	// Try an unknown check.
+			Perms:    nodePerms,
-	args.CheckID = "check-id"
+			Expected: false,
-	err = vetDeregisterWithACL(perms, args, nil, nil)
+			Name:     "no right on node",
-	if err == nil || !strings.Contains(err.Error(), "Unknown check") {
+		},
-		t.Fatalf("bad: %v", err)
+		{
-	}
+			DeregisterRequest: structs.DeregisterRequest{
-
+				Node: "nope",
-	// Now pass in a check that should be blocked.
+			},
-	nc := &structs.HealthCheck{
+			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",
-		CheckID:     "check-id",
+			},
-		ServiceID:   "service-id",
+			Perms:    nodePerms,
-		ServiceName: "nope",
+			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)
 				}
-	err = vetDeregisterWithACL(perms, args, nil, nc)
 				if !acl.IsErrPermissionDenied(err) {
-		t.Fatalf("bad: %v", err)
+					t.Errorf("expected permission denied error with %+v, instead got %+v", args.DeregisterRequest, err)
 				}
-
+			} else if err != nil {
-	// Change it to an allowed service, which should go through.
+				t.Errorf("expected no error with %+v", args.DeregisterRequest)
-	nc.ServiceName = "service"
-	if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil {
-		t.Fatalf("err: %v", err)
 			}
-
+		})
-	// Switch to a node check that should be blocked.
-	args.Node = "nope"
-	nc.Node = "nope"
-	nc.ServiceID = ""
-	nc.ServiceName = ""
-	err = vetDeregisterWithACL(perms, args, nil, nc)
-	if !acl.IsErrPermissionDenied(err) {
-		t.Fatalf("bad: %v", err)
-	}
-
-	// Switch to an allowed node check, which should go through.
-	args.Node = "node"
-	nc.Node = "node"
-	if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	// Try an unknown service.
-	args.ServiceID = "service-id"
-	err = vetDeregisterWithACL(perms, args, nil, nil)
-	if err == nil || !strings.Contains(err.Error(), "Unknown service") {
-		t.Fatalf("bad: %v", err)
-	}
-
-	// Now pass in a service that should be blocked.
-	ns := &structs.NodeService{
-		ID:      "service-id",
-		Service: "nope",
-	}
-	err = vetDeregisterWithACL(perms, args, ns, nil)
-	if !acl.IsErrPermissionDenied(err) {
-		t.Fatalf("bad: %v", err)
-	}
-
-	// Change it to an allowed service, which should go through.
-	ns.Service = "service"
-	if err := vetDeregisterWithACL(perms, args, ns, nil); err != nil {
-		t.Fatalf("err: %v", err)
 	}
 }
 

agent/consul/catalog_endpoint_test.go

@@ -15,7 +15,7 @@ import (
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/consul/types"
-	"github.com/hashicorp/net-rpc-msgpackrpc"
+	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -727,7 +727,7 @@ service "service" {
 	err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister",
 		&structs.DeregisterRequest{
 			Datacenter: "dc1",
-			Node:       "node",
+			Node:       "nope",
 			ServiceID:  "nope",
 			WriteRequest: structs.WriteRequest{
 				Token: id,
@@ -738,7 +738,7 @@ service "service" {
 	err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister",
 		&structs.DeregisterRequest{
 			Datacenter: "dc1",
-			Node:       "node",
+			Node:       "nope",
 			CheckID:    "nope",
 			WriteRequest: structs.WriteRequest{
 				Token: id,