Adds complete ACL coverage for /v1/catalog/register.

consul/acl.go

@@ -544,3 +544,103 @@ func (s *Server) filterACL(token string, subj interface{}) error {
 
 	return nil
 }
+
+// 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.
+//
+// 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(acl acl.ACL, subj *structs.RegisterRequest, ns *structs.NodeServices) error {
+	// Fast path if ACLs are not enabled.
+	if acl == nil {
+		return nil
+	}
+
+	// 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 && !acl.NodeWrite(subj.Node) {
+		return permissionDeniedErr
+	}
+
+	// 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 !acl.ServiceWrite(subj.Service.Service) {
+			return permissionDeniedErr
+		}
+
+		if ns != nil {
+			other, ok := ns.Services[subj.Service.ID]
+			if ok && !acl.ServiceWrite(other.Service) {
+				return permissionDeniedErr
+			}
+		}
+	}
+
+	// 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 !acl.NodeWrite(subj.Node) {
+				return permissionDeniedErr
+			}
+			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)
+		} else {
+			other, ok := ns.Services[check.ServiceID]
+			if !ok {
+				return fmt.Errorf("Unknown service '%s' for check '%s'",
+					check.ServiceID, check.CheckID)
+			}
+			if !acl.ServiceWrite(other.Service) {
+				return permissionDeniedErr
+			}
+		}
+	}
+
+	return nil
+}

consul/acl_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/consul/acl"
@@ -12,6 +13,15 @@ import (
 	"github.com/hashicorp/consul/testutil"
 )
 
+var testACLPolicy = `
+key "" {
+	policy = "deny"
+}
+key "foo/" {
+	policy = "write"
+}
+`
+
 func TestACL_Disabled(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
@@ -1106,11 +1116,228 @@ func TestACL_unhandledFilterType(t *testing.T) {
 	srv.filterACL(token, &structs.HealthCheck{})
 }
 
-var testACLPolicy = `
-key "" {
-	policy = "deny"
+func TestACL_vetRegisterWithACL(t *testing.T) {
+	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.Parse(`
+node "node" {
+  policy = "write"
 }
-key "foo/" {
-	policy = "write"
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err := acl.New(acl.DenyAll(), policy)
+	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 err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		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 err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		t.Fatalf("bad: %v", err)
+	}
+
+	// Chain on a basic service policy.
+	policy, err = acl.Parse(`
+service "service" {
+  policy = "write"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err = acl.New(perms, policy)
+	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 err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		t.Fatalf("bad: %v", err)
+	}
+
+	// Chain on a policy that allows them to write to the other service.
+	policy, err = acl.Parse(`
+service "other" {
+  policy = "write"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err = acl.New(perms, policy)
+	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.Parse(`
+service "other" {
+  policy = "deny"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err = acl.New(perms, policy)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// This should get rejected.
+	err = vetRegisterWithACL(perms, args, ns)
+	if err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		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.Parse(`
+node "node" {
+  policy = "deny"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err = acl.New(perms, policy)
+	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 err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		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 err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		t.Fatalf("bad: %v", err)
+	}
 }
-`

consul/catalog_endpoint.go

@@ -21,7 +21,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
 	}
 	defer metrics.MeasureSince([]string{"consul", "catalog", "register"}, time.Now())
 
-	// Verify the args
+	// Verify the args.
 	if args.Node == "" || args.Address == "" {
 		return fmt.Errorf("Must provide node and address")
 	}
@@ -32,30 +32,31 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
 		return err
 	}
 
+	// Handle a service registration.
 	if args.Service != nil {
 		// If no service id, but service name, use default
 		if args.Service.ID == "" && args.Service.Service != "" {
 			args.Service.ID = args.Service.Service
 		}
 
-		// Verify ServiceName provided if ID
+		// Verify ServiceName provided if ID.
 		if args.Service.ID != "" && args.Service.Service == "" {
 			return fmt.Errorf("Must provide service name with ID")
 		}
 
 		// Apply the ACL policy if any. The 'consul' service is excluded
 		// since it is managed automatically internally (that behavior
-		// is going away after version 0.8).
-		if c.srv.config.ACLEnforceVersion8 ||
-			(args.Service.Service != ConsulServiceName) {
+		// is going away after version 0.8). We check this same policy
+		// later if version 0.8 is enabled, so we can eventually just
+		// delete this and do all the ACL checks down there.
+		if args.Service.Service != ConsulServiceName {
 			if acl != nil && !acl.ServiceWrite(args.Service.Service) {
-				c.srv.logger.Printf("[WARN] consul.catalog: Register of service '%s' on '%s' denied due to ACLs",
-					args.Service.Service, args.Node)
 				return permissionDeniedErr
 			}
 		}
 	}
 
+	// Move the old format single check into the slice, and fixup IDs.
 	if args.Check != nil {
 		args.Checks = append(args.Checks, args.Check)
 		args.Check = nil
@@ -69,6 +70,18 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
 		}
 	}
 
+	// Check the complete register request against the given ACL policy.
+	if c.srv.config.ACLEnforceVersion8 {
+		state := c.srv.fsm.State()
+		_, ns, err := state.NodeServices(args.Node)
+		if err != nil {
+			return fmt.Errorf("Node lookup failed: %v", err)
+		}
+		if err := vetRegisterWithACL(acl, args, ns); err != nil {
+			return err
+		}
+	}
+
 	_, err = c.srv.raftApply(structs.RegisterRequestType, args)
 	if err != nil {
 		c.srv.logger.Printf("[ERR] consul.catalog: Register failed: %v", err)

consul/catalog_endpoint_test.go

@@ -30,6 +30,9 @@ func TestCatalogRegister(t *testing.T) {
 			Tags:    []string{"master"},
 			Port:    8000,
 		},
+		Check: &structs.HealthCheck{
+			ServiceID: "db",
+		},
 	}
 	var out struct{}
 
@@ -46,7 +49,7 @@ func TestCatalogRegister(t *testing.T) {
 	})
 }
 
-func TestCatalogRegister_ACLDeny_Service(t *testing.T) {
+func TestCatalogRegister_ACLDeny(t *testing.T) {
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
 		c.ACLDatacenter = "dc1"
 		c.ACLMasterToken = "root"
@@ -115,6 +118,28 @@ func TestCatalogRegister_ACLDeny_Service(t *testing.T) {
 	if err == nil || !strings.Contains(err.Error(), permissionDenied) {
 		t.Fatalf("err: %v", err)
 	}
+
+	// Register a db service using the root token.
+	argR.Service.Service = "db"
+	argR.Service.ID = "my-id"
+	argR.Token = "root"
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Prove that we are properly looking up the node services and passing
+	// that to the ACL helper. We can vet the helper independently in its
+	// own unit test after this. This is trying to register over the db
+	// service we created above, which is a check that depends on looking
+	// at the existing registration data with that service ID.
+	argR.Service.Service = "foo"
+	argR.Service.ID = "my-id"
+	argR.Token = id
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR)
+	if err == nil || !strings.Contains(err.Error(), permissionDenied) {
+		t.Fatalf("err: %v", err)
+	}
 }
 
 func TestCatalogRegister_ForwardLeader(t *testing.T) {

consul/state/state_store.go

@@ -438,6 +438,12 @@ func (s *StateStore) ensureRegistrationTxn(tx *memdb.Txn, idx uint64, watches *D
 		}
 	}
 
+	// TODO (slackpad) In Consul 0.8 ban checks that don't have the same
+	// node as the top-level registration. This is just weird to be able to
+	// update unrelated nodes' checks from in here. In 0.7.2 we banned this
+	// up in the ACL check since that's guarded behind an opt-in flag until
+	// Consul 0.8.
+
 	// Add the checks, if any.
 	if req.Check != nil {
 		if err := s.ensureCheckTxn(tx, idx, watches, req.Check); err != nil {

consul/structs/structs.go

@@ -183,6 +183,25 @@ func (r *RegisterRequest) RequestDatacenter() string {
 	return r.Datacenter
 }
 
+// ChangesNode returns true if the given register request changes the given
+// node, which can be nil. This only looks for changes to the node record itself,
+// not any of the health checks.
+func (r *RegisterRequest) ChangesNode(node *Node) bool {
+	// This means it's creating the node.
+	if node == nil {
+		return true
+	}
+
+	// Check if any of the node-level fields are being changed.
+	if r.Node != node.Node ||
+		r.Address != node.Address ||
+		!reflect.DeepEqual(r.TaggedAddresses, node.TaggedAddresses) {
+		return true
+	}
+
+	return false
+}
+
 // DeregisterRequest is used for the Catalog.Deregister endpoint
 // to deregister a node as providing a service. If no service is
 // provided the entire node is deregistered.

consul/structs/structs_test.go

@@ -105,6 +105,44 @@ func TestStructs_ACL_IsSame(t *testing.T) {
 	check(func() { other.Rules = "" }, func() { other.Rules = "service \"\" { policy = \"read\" }" })
 }
 
+func TestStructs_RegisterRequest_ChangesNode(t *testing.T) {
+	req := &RegisterRequest{
+		Node:            "test",
+		Address:         "127.0.0.1",
+		TaggedAddresses: make(map[string]string),
+	}
+
+	node := &Node{
+		Node:            "test",
+		Address:         "127.0.0.1",
+		TaggedAddresses: make(map[string]string),
+	}
+
+	check := func(twiddle, restore func()) {
+		if req.ChangesNode(node) {
+			t.Fatalf("should not change")
+		}
+
+		twiddle()
+		if !req.ChangesNode(node) {
+			t.Fatalf("should change")
+		}
+
+		restore()
+		if req.ChangesNode(node) {
+			t.Fatalf("should not change")
+		}
+	}
+
+	check(func() { req.Node = "nope" }, func() { req.Node = "test" })
+	check(func() { req.Address = "127.0.0.2" }, func() { req.Address = "127.0.0.1" })
+	check(func() { req.TaggedAddresses["wan"] = "nope" }, func() { delete(req.TaggedAddresses, "wan") })
+
+	if !req.ChangesNode(nil) {
+		t.Fatalf("should change")
+	}
+}
+
 // testServiceNode gives a fully filled out ServiceNode instance.
 func testServiceNode() *ServiceNode {
 	return &ServiceNode{