Fixes implementation of node ACLs for /v1/catalog/node/<node>.

This would return a "permission denied" error, but this changes it to
return the same response as a node that doesn't exist (as was originally
intended and written in the code comments).

consul/acl.go

@@ -388,13 +388,22 @@ func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) {
 }
 
 // filterNodeServices is used to filter services on a given node base on ACLs.
-func (f *aclFilter) filterNodeServices(services *structs.NodeServices) {
-	for svc, _ := range services.Services {
+func (f *aclFilter) filterNodeServices(services **structs.NodeServices) {
+	if *services == nil {
+		return
+	}
+
+	if !f.allowNode((*services).Node.Node) {
+		*services = nil
+		return
+	}
+
+	for svc, _ := range (*services).Services {
 		if f.allowService(svc) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc)
-		delete(services.Services, svc)
+		delete((*services).Services, svc)
 	}
 }
 
@@ -573,9 +582,7 @@ func (s *Server) filterACL(token string, subj interface{}) error {
 		filt.filterNodes(&v.Nodes)
 
 	case *structs.IndexedNodeServices:
-		if v.NodeServices != nil {
-			filt.filterNodeServices(v.NodeServices)
-		}
+		filt.filterNodeServices(&v.NodeServices)
 
 	case *structs.IndexedServiceNodes:
 		filt.filterServiceNodes(&v.ServiceNodes)

consul/acl_test.go

@@ -1011,31 +1011,107 @@ node "node1" {
 }
 
 func TestACL_filterNodeServices(t *testing.T) {
-	// Create some node services
-	services := structs.NodeServices{
-		Node: &structs.Node{
-			Node: "node1",
-		},
-		Services: map[string]*structs.NodeService{
-			"foo": &structs.NodeService{
-				ID:      "foo",
-				Service: "foo",
+	// Create some node services.
+	fill := func() *structs.NodeServices {
+		return &structs.NodeServices{
+			Node: &structs.Node{
+				Node: "node1",
 			},
-		},
+			Services: map[string]*structs.NodeService{
+				"foo": &structs.NodeService{
+					ID:      "foo",
+					Service: "foo",
+				},
+			},
+		}
 	}
 
-	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil, false)
-	filt.filterNodeServices(&services)
-	if len(services.Services) != 1 {
-		t.Fatalf("bad: %#v", services.Services)
+	// Try nil, which is a possible input.
+	{
+		var services *structs.NodeServices
+		filt := newAclFilter(acl.AllowAll(), nil, false)
+		filt.filterNodeServices(&services)
+		if services != nil {
+			t.Fatalf("bad: %#v", services)
+		}
 	}
 
-	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil, false)
-	filt.filterNodeServices(&services)
-	if len(services.Services) != 0 {
-		t.Fatalf("bad: %#v", services.Services)
+	// Try permissive filtering.
+	{
+		services := fill()
+		filt := newAclFilter(acl.AllowAll(), nil, false)
+		filt.filterNodeServices(&services)
+		if len(services.Services) != 1 {
+			t.Fatalf("bad: %#v", services.Services)
+		}
+	}
+
+	// Try restrictive filtering.
+	{
+		services := fill()
+		filt := newAclFilter(acl.DenyAll(), nil, false)
+		filt.filterNodeServices(&services)
+		if len((*services).Services) != 0 {
+			t.Fatalf("bad: %#v", (*services).Services)
+		}
+	}
+
+	// Allowed to see the service but not the node.
+	policy, err := acl.Parse(`
+service "foo" {
+  policy = "read"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
+	}
+	perms, err := acl.New(acl.DenyAll(), policy)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// This will work because version 8 ACLs aren't being enforced.
+	{
+		services := fill()
+		filt := newAclFilter(perms, nil, false)
+		filt.filterNodeServices(&services)
+		if len((*services).Services) != 1 {
+			t.Fatalf("bad: %#v", (*services).Services)
+		}
+	}
+
+	// But with version 8 the node will block it.
+	{
+		services := fill()
+		filt := newAclFilter(perms, nil, true)
+		filt.filterNodeServices(&services)
+		if services != nil {
+			t.Fatalf("bad: %#v", services)
+		}
+	}
+
+	// Chain on access to the node.
+	policy, err = acl.Parse(`
+node "node1" {
+  policy = "read"
+}
+`)
+	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.
+	{
+		services := fill()
+		filt := newAclFilter(perms, nil, false)
+		filt.filterNodeServices(&services)
+		if len((*services).Services) != 1 {
+			t.Fatalf("bad: %#v", (*services).Services)
+		}
 	}
 }
 

consul/catalog_endpoint.go

@@ -271,20 +271,6 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs
 				return err
 			}
 
-			// Node read access is required with version 8 ACLs. We
-			// just return the same response as if the node doesn't
-			// exist, which is consistent with how the rest of the
-			// catalog filtering works and doesn't disclose the node.
-			acl, err := c.srv.resolveToken(args.Token)
-			if err != nil {
-				return err
-			}
-			if acl != nil && c.srv.config.ACLEnforceVersion8 {
-				if !acl.NodeRead(args.Node) {
-					return permissionDeniedErr
-				}
-			}
-
 			reply.Index, reply.NodeServices = index, services
 			return c.srv.filterACL(args.Token, reply)
 		})

consul/catalog_endpoint_test.go

@@ -1537,10 +1537,12 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) {
 
 	// Now turn on version 8 enforcement and try again.
 	s1.config.ACLEnforceVersion8 = true
-	err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply)
-	if err == nil || !strings.Contains(err.Error(), permissionDenied) {
+	if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil {
 		t.Fatalf("err: %v", err)
 	}
+	if reply.NodeServices != nil {
+		t.Fatalf("should not nil")
+	}
 
 	// Create an ACL that can read the node.
 	arg := structs.ACLRequest{
@@ -1570,6 +1572,15 @@ node "%s" {
 	if reply.NodeServices == nil {
 		t.Fatalf("should not be nil")
 	}
+
+	// Make sure an unknown node doesn't cause trouble.
+	args.Node = "nope"
+	if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if reply.NodeServices != nil {
+		t.Fatalf("should not nil")
+	}
 }
 
 func TestCatalog_NodeServices_FilterACL(t *testing.T) {