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

consul/acl.go

@@ -313,14 +313,19 @@ func (c *aclCache) useACLPolicy(id, authDC string, cached *aclCacheEntry, p *str
 type aclFilter struct {
 	acl             acl.ACL
 	logger          *log.Logger
+	enforceVersion8 bool
 }
 
 // newAclFilter constructs a new aclFilter.
-func newAclFilter(acl acl.ACL, logger *log.Logger) *aclFilter {
+func newAclFilter(acl acl.ACL, logger *log.Logger, enforceVersion8 bool) *aclFilter {
 	if logger == nil {
 		logger = log.New(os.Stdout, "", log.LstdFlags)
 	}
-	return &aclFilter{acl, logger}
+	return &aclFilter{
+		acl:             acl,
+		logger:          logger,
+		enforceVersion8: enforceVersion8,
+	}
 }
 
 // filterService is used to determine if a service is accessible for an ACL.
@@ -432,6 +437,26 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) {
 	*dump = nd
 }
 
+// filterNodes is used to filter through all parts of a node list and remove
+// elements the provided ACL token cannot access.
+func (f *aclFilter) filterNodes(nodes *structs.Nodes) {
+	if !f.enforceVersion8 {
+		return
+	}
+
+	n := *nodes
+	for i := 0; i < len(n); i++ {
+		node := n[i].Node
+		if f.acl.NodeRead(node) {
+			continue
+		}
+		f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node)
+		n = append(n[:i], n[i+1:]...)
+		i--
+	}
+	*nodes = n
+}
+
 // redactPreparedQueryTokens will redact any tokens unless the client has a
 // management token. This eases the transition to delegated authority over
 // prepared queries, since it was easy to capture management tokens in Consul
@@ -506,31 +531,34 @@ func (s *Server) filterACL(token string, subj interface{}) error {
 	}
 
 	// Create the filter
-	filt := newAclFilter(acl, s.logger)
+	filt := newAclFilter(acl, s.logger, s.config.ACLEnforceVersion8)
 
 	switch v := subj.(type) {
+	case *structs.CheckServiceNodes:
+		filt.filterCheckServiceNodes(v)
+
+	case *structs.IndexedCheckServiceNodes:
+		filt.filterCheckServiceNodes(&v.Nodes)
+
 	case *structs.IndexedHealthChecks:
 		filt.filterHealthChecks(&v.HealthChecks)
 
-	case *structs.IndexedServices:
+	case *structs.IndexedNodeDump:
-		filt.filterServices(v.Services)
+		filt.filterNodeDump(&v.Dump)
 
-	case *structs.IndexedServiceNodes:
+	case *structs.IndexedNodes:
-		filt.filterServiceNodes(&v.ServiceNodes)
+		filt.filterNodes(&v.Nodes)
 
 	case *structs.IndexedNodeServices:
 		if v.NodeServices != nil {
 			filt.filterNodeServices(v.NodeServices)
 		}
 
-	case *structs.IndexedCheckServiceNodes:
+	case *structs.IndexedServiceNodes:
-		filt.filterCheckServiceNodes(&v.Nodes)
+		filt.filterServiceNodes(&v.ServiceNodes)
 
-	case *structs.CheckServiceNodes:
+	case *structs.IndexedServices:
-		filt.filterCheckServiceNodes(v)
+		filt.filterServices(v.Services)
-
-	case *structs.IndexedNodeDump:
-		filt.filterNodeDump(&v.Dump)
 
 	case *structs.IndexedPreparedQueries:
 		filt.filterPreparedQueries(&v.Queries)

consul/acl_test.go

@@ -818,14 +818,14 @@ func TestACL_filterHealthChecks(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterHealthChecks(&hc)
 	if len(hc) != 1 {
 		t.Fatalf("bad: %#v", hc)
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterHealthChecks(&hc)
 	if len(hc) != 0 {
 		t.Fatalf("bad: %#v", hc)
@@ -840,14 +840,14 @@ func TestACL_filterServices(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterServices(services)
 	if len(services) != 2 {
 		t.Fatalf("bad: %#v", services)
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterServices(services)
 	if len(services) != 0 {
 		t.Fatalf("bad: %#v", services)
@@ -864,14 +864,14 @@ func TestACL_filterServiceNodes(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterServiceNodes(&nodes)
 	if len(nodes) != 1 {
 		t.Fatalf("bad: %#v", nodes)
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterServiceNodes(&nodes)
 	if len(nodes) != 0 {
 		t.Fatalf("bad: %#v", nodes)
@@ -893,14 +893,14 @@ func TestACL_filterNodeServices(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterNodeServices(&services)
 	if len(services.Services) != 1 {
 		t.Fatalf("bad: %#v", services.Services)
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterNodeServices(&services)
 	if len(services.Services) != 0 {
 		t.Fatalf("bad: %#v", services.Services)
@@ -929,7 +929,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterCheckServiceNodes(&nodes)
 	if len(nodes) != 1 {
 		t.Fatalf("bad: %#v", nodes)
@@ -939,7 +939,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) {
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterCheckServiceNodes(&nodes)
 	if len(nodes) != 0 {
 		t.Fatalf("bad: %#v", nodes)
@@ -968,7 +968,7 @@ func TestACL_filterNodeDump(t *testing.T) {
 	}
 
 	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil)
+	filt := newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterNodeDump(&dump)
 	if len(dump) != 1 {
 		t.Fatalf("bad: %#v", dump)
@@ -981,7 +981,7 @@ func TestACL_filterNodeDump(t *testing.T) {
 	}
 
 	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterNodeDump(&dump)
 	if len(dump) != 1 {
 		t.Fatalf("bad: %#v", dump)
@@ -994,6 +994,39 @@ func TestACL_filterNodeDump(t *testing.T) {
 	}
 }
 
+func TestACL_filterNodes(t *testing.T) {
+	// Create a nodes list.
+	nodes := structs.Nodes{
+		&structs.Node{
+			Node: "foo",
+		},
+		&structs.Node{
+			Node: "bar",
+		},
+	}
+
+	// Try permissive filtering.
+	filt := newAclFilter(acl.AllowAll(), nil, true)
+	filt.filterNodes(&nodes)
+	if len(nodes) != 2 {
+		t.Fatalf("bad: %#v", nodes)
+	}
+
+	// Try restrictive filtering but with version 8 enforcement turned off.
+	filt = newAclFilter(acl.DenyAll(), nil, false)
+	filt.filterNodes(&nodes)
+	if len(nodes) != 2 {
+		t.Fatalf("bad: %#v", nodes)
+	}
+
+	// Try restrictive filtering with version 8 enforcement turned on.
+	filt = newAclFilter(acl.DenyAll(), nil, true)
+	filt.filterNodes(&nodes)
+	if len(nodes) != 0 {
+		t.Fatalf("bad: %#v", nodes)
+	}
+}
+
 func TestACL_redactPreparedQueryTokens(t *testing.T) {
 	query := &structs.PreparedQuery{
 		ID:    "f004177f-2c28-83b7-4229-eacc25fe55d1",
@@ -1007,7 +1040,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) {
 
 	// Try permissive filtering with a management token. This will allow the
 	// embedded token to be seen.
-	filt := newAclFilter(acl.ManageAll(), nil)
+	filt := newAclFilter(acl.ManageAll(), nil, false)
 	filt.redactPreparedQueryTokens(&query)
 	if !reflect.DeepEqual(query, expected) {
 		t.Fatalf("bad: %#v", &query)
@@ -1019,7 +1052,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) {
 
 	// Now try permissive filtering with a client token, which should cause
 	// the embedded token to get redacted.
-	filt = newAclFilter(acl.AllowAll(), nil)
+	filt = newAclFilter(acl.AllowAll(), nil, false)
 	filt.redactPreparedQueryTokens(&query)
 	expected.Token = redactedToken
 	if !reflect.DeepEqual(query, expected) {
@@ -1065,7 +1098,7 @@ func TestACL_filterPreparedQueries(t *testing.T) {
 
 	// Try permissive filtering with a management token. This will allow the
 	// embedded token to be seen.
-	filt := newAclFilter(acl.ManageAll(), nil)
+	filt := newAclFilter(acl.ManageAll(), nil, false)
 	filt.filterPreparedQueries(&queries)
 	if !reflect.DeepEqual(queries, expected) {
 		t.Fatalf("bad: %#v", queries)
@@ -1078,7 +1111,7 @@ func TestACL_filterPreparedQueries(t *testing.T) {
 	// Now try permissive filtering with a client token, which should cause
 	// the embedded token to get redacted, and the query with no name to get
 	// filtered out.
-	filt = newAclFilter(acl.AllowAll(), nil)
+	filt = newAclFilter(acl.AllowAll(), nil, false)
 	filt.filterPreparedQueries(&queries)
 	expected[2].Token = redactedToken
 	expected = append(structs.PreparedQueries{}, expected[1], expected[2])
@@ -1092,7 +1125,7 @@ func TestACL_filterPreparedQueries(t *testing.T) {
 	}
 
 	// Now try restrictive filtering.
-	filt = newAclFilter(acl.DenyAll(), nil)
+	filt = newAclFilter(acl.DenyAll(), nil, false)
 	filt.filterPreparedQueries(&queries)
 	if len(queries) != 0 {
 		t.Fatalf("bad: %#v", queries)

consul/catalog_endpoint.go

@@ -169,6 +169,9 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde
 			}
 
 			reply.Index, reply.Nodes = index, nodes
+			if err := c.srv.filterACL(args.Token, reply); err != nil {
+				return err
+			}
 			return c.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes)
 		})
 }

consul/catalog_endpoint_test.go

@@ -14,7 +14,7 @@ import (
 	"github.com/hashicorp/net-rpc-msgpackrpc"
 )
 
-func TestCatalogRegister(t *testing.T) {
+func TestCatalog_Register(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -49,7 +49,7 @@ func TestCatalogRegister(t *testing.T) {
 	})
 }
 
-func TestCatalogRegister_ACLDeny(t *testing.T) {
+func TestCatalog_Register_ACLDeny(t *testing.T) {
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
 		c.ACLDatacenter = "dc1"
 		c.ACLMasterToken = "root"
@@ -150,7 +150,7 @@ service "foo" {
 	}
 }
 
-func TestCatalogRegister_ForwardLeader(t *testing.T) {
+func TestCatalog_Register_ForwardLeader(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -197,7 +197,7 @@ func TestCatalogRegister_ForwardLeader(t *testing.T) {
 	}
 }
 
-func TestCatalogRegister_ForwardDC(t *testing.T) {
+func TestCatalog_Register_ForwardDC(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -233,7 +233,7 @@ func TestCatalogRegister_ForwardDC(t *testing.T) {
 	}
 }
 
-func TestCatalogDeregister(t *testing.T) {
+func TestCatalog_Deregister(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -258,7 +258,7 @@ func TestCatalogDeregister(t *testing.T) {
 	}
 }
 
-func TestCatalogDeregister_ACLDeny(t *testing.T) {
+func TestCatalog_Deregister_ACLDeny(t *testing.T) {
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
 		c.ACLDatacenter = "dc1"
 		c.ACLMasterToken = "root"
@@ -469,7 +469,7 @@ service "service" {
 	}
 }
 
-func TestCatalogListDatacenters(t *testing.T) {
+func TestCatalog_ListDatacenters(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -506,7 +506,7 @@ func TestCatalogListDatacenters(t *testing.T) {
 	}
 }
 
-func TestCatalogListDatacenters_DistanceSort(t *testing.T) {
+func TestCatalog_ListDatacenters_DistanceSort(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -550,7 +550,7 @@ func TestCatalogListDatacenters_DistanceSort(t *testing.T) {
 	}
 }
 
-func TestCatalogListNodes(t *testing.T) {
+func TestCatalog_ListNodes(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -592,7 +592,7 @@ func TestCatalogListNodes(t *testing.T) {
 	}
 }
 
-func TestCatalogListNodes_StaleRaad(t *testing.T) {
+func TestCatalog_ListNodes_StaleRaad(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -660,7 +660,7 @@ func TestCatalogListNodes_StaleRaad(t *testing.T) {
 	}
 }
 
-func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) {
+func TestCatalog_ListNodes_ConsistentRead_Fail(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -710,7 +710,7 @@ func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) {
 	}
 }
 
-func TestCatalogListNodes_ConsistentRead(t *testing.T) {
+func TestCatalog_ListNodes_ConsistentRead(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -758,7 +758,7 @@ func TestCatalogListNodes_ConsistentRead(t *testing.T) {
 	}
 }
 
-func TestCatalogListNodes_DistanceSort(t *testing.T) {
+func TestCatalog_ListNodes_DistanceSort(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -846,7 +846,84 @@ func TestCatalogListNodes_DistanceSort(t *testing.T) {
 	}
 }
 
-func BenchmarkCatalogListNodes(t *testing.B) {
+func TestCatalog_ListNodes_ACLFilter(t *testing.T) {
+	dir1, s1 := testServerWithConfig(t, func(c *Config) {
+		c.ACLDatacenter = "dc1"
+		c.ACLMasterToken = "root"
+		c.ACLDefaultPolicy = "deny"
+		c.ACLEnforceVersion8 = false
+	})
+	defer os.RemoveAll(dir1)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	defer codec.Close()
+
+	testutil.WaitForLeader(t, s1.RPC, "dc1")
+
+	// We scope the reply in each of these since msgpack won't clear out an
+	// existing slice if the incoming one is nil, so it's best to start
+	// clean each time.
+
+	// Prior to version 8, the node policy should be ignored.
+	args := structs.DCSpecificRequest{
+		Datacenter: "dc1",
+	}
+	{
+		reply := structs.IndexedNodes{}
+		if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		if len(reply.Nodes) != 1 {
+			t.Fatalf("bad: %v", reply.Nodes)
+		}
+	}
+
+	// Now turn on version 8 enforcement and try again.
+	s1.config.ACLEnforceVersion8 = true
+	{
+		reply := structs.IndexedNodes{}
+		if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		if len(reply.Nodes) != 0 {
+			t.Fatalf("bad: %v", reply.Nodes)
+		}
+	}
+
+	// Create an ACL that can read the node.
+	arg := structs.ACLRequest{
+		Datacenter: "dc1",
+		Op:         structs.ACLSet,
+		ACL: structs.ACL{
+			Name: "User token",
+			Type: structs.ACLTypeClient,
+			Rules: fmt.Sprintf(`
+node "%s" {
+	policy = "read"
+}
+`, s1.config.NodeName),
+		},
+		WriteRequest: structs.WriteRequest{Token: "root"},
+	}
+	var id string
+	if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Now try with the token and it will go through.
+	args.Token = id
+	{
+		reply := structs.IndexedNodes{}
+		if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		if len(reply.Nodes) != 1 {
+			t.Fatalf("bad: %v", reply.Nodes)
+		}
+	}
+}
+
+func Benchmark_Catalog_ListNodes(t *testing.B) {
 	dir1, s1 := testServer(nil)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -869,7 +946,7 @@ func BenchmarkCatalogListNodes(t *testing.B) {
 	}
 }
 
-func TestCatalogListServices(t *testing.T) {
+func TestCatalog_ListServices(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -919,7 +996,7 @@ func TestCatalogListServices(t *testing.T) {
 	}
 }
 
-func TestCatalogListServices_Blocking(t *testing.T) {
+func TestCatalog_ListServices_Blocking(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -977,7 +1054,7 @@ func TestCatalogListServices_Blocking(t *testing.T) {
 	}
 }
 
-func TestCatalogListServices_Timeout(t *testing.T) {
+func TestCatalog_ListServices_Timeout(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -1018,7 +1095,7 @@ func TestCatalogListServices_Timeout(t *testing.T) {
 	}
 }
 
-func TestCatalogListServices_Stale(t *testing.T) {
+func TestCatalog_ListServices_Stale(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -1055,7 +1132,7 @@ func TestCatalogListServices_Stale(t *testing.T) {
 	}
 }
 
-func TestCatalogListServiceNodes(t *testing.T) {
+func TestCatalog_ListServiceNodes(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -1104,7 +1181,7 @@ func TestCatalogListServiceNodes(t *testing.T) {
 	}
 }
 
-func TestCatalogListServiceNodes_DistanceSort(t *testing.T) {
+func TestCatalog_ListServiceNodes_DistanceSort(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -1241,7 +1318,7 @@ func TestCatalog_NodeServices(t *testing.T) {
 }
 
 // Used to check for a regression against a known bug
-func TestCatalogRegister_FailedCase1(t *testing.T) {
+func TestCatalog_Register_FailedCase1(t *testing.T) {
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -1447,6 +1524,9 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) {
 	if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil {
 		t.Fatalf("err: %v", err)
 	}
+	if reply.NodeServices == nil {
+		t.Fatalf("should not be nil")
+	}
 
 	// Now turn on version 8 enforcement and try again.
 	s1.config.ACLEnforceVersion8 = true
@@ -1464,7 +1544,7 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) {
 			Type: structs.ACLTypeClient,
 			Rules: fmt.Sprintf(`
 node "%s" {
-	policy = "write"
+	policy = "read"
 }
 `, s1.config.NodeName),
 		},
@@ -1480,6 +1560,9 @@ node "%s" {
 	if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil {
 		t.Fatalf("err: %v", err)
 	}
+	if reply.NodeServices == nil {
+		t.Fatalf("should not be nil")
+	}
 }
 
 func TestCatalog_NodeServices_FilterACL(t *testing.T) {