Adds complete ACL coverage for /v1/catalog/service/<service>.

consul/acl.go

@@ -328,8 +328,16 @@ func newAclFilter(acl acl.ACL, logger *log.Logger, enforceVersion8 bool) *aclFil
 	}
 }
 
-// filterService is used to determine if a service is accessible for an ACL.
+// allowNode is used to determine if a node is accessible for an ACL.
-func (f *aclFilter) filterService(service string) bool {
+func (f *aclFilter) allowNode(node string) bool {
+	if !f.enforceVersion8 {
+		return true
+	}
+	return f.acl.NodeRead(node)
+}
+
+// allowService is used to determine if a service is accessible for an ACL.
+func (f *aclFilter) allowService(service string) bool {
 	if service == "" || service == ConsulServiceID {
 		return true
 	}
@@ -342,7 +350,7 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) {
 	hc := *checks
 	for i := 0; i < len(hc); i++ {
 		check := hc[i]
-		if f.filterService(check.ServiceName) {
+		if f.allowService(check.ServiceName) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", check.CheckID)
@@ -355,7 +363,7 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) {
 // filterServices is used to filter a set of services based on ACLs.
 func (f *aclFilter) filterServices(services structs.Services) {
 	for svc, _ := range services {
-		if f.filterService(svc) {
+		if f.allowService(svc) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc)
@@ -369,7 +377,7 @@ func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) {
 	sn := *nodes
 	for i := 0; i < len(sn); i++ {
 		node := sn[i]
-		if f.filterService(node.ServiceName) {
+		if f.allowNode(node.Node) && f.allowService(node.ServiceName) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node.Node)
@@ -382,7 +390,7 @@ 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 {
-		if f.filterService(svc) {
+		if f.allowService(svc) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc)
@@ -395,7 +403,7 @@ func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) {
 	csn := *nodes
 	for i := 0; i < len(csn); i++ {
 		node := csn[i]
-		if f.filterService(node.Service.Service) {
+		if f.allowService(node.Service.Service) {
 			continue
 		}
 		f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node.Node.Node)
@@ -415,7 +423,7 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) {
 		// Filter services
 		for i := 0; i < len(info.Services); i++ {
 			svc := info.Services[i].Service
-			if f.filterService(svc) {
+			if f.allowService(svc) {
 				continue
 			}
 			f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc)
@@ -426,7 +434,7 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) {
 		// Filter checks
 		for i := 0; i < len(info.Checks); i++ {
 			chk := info.Checks[i]
-			if f.filterService(chk.ServiceName) {
+			if f.allowService(chk.ServiceName) {
 				continue
 			}
 			f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", chk.CheckID)

consul/acl_test.go

@@ -855,26 +855,92 @@ func TestACL_filterServices(t *testing.T) {
 }
 
 func TestACL_filterServiceNodes(t *testing.T) {
-	// Create some service nodes
+	// Create some service nodes.
-	nodes := structs.ServiceNodes{
+	fill := func() structs.ServiceNodes {
-		&structs.ServiceNode{
+		return structs.ServiceNodes{
-			Node:        "node1",
+			&structs.ServiceNode{
-			ServiceName: "foo",
+				Node:        "node1",
-		},
+				ServiceName: "foo",
+			},
+		}
 	}
 
-	// Try permissive filtering
+	// Try permissive filtering.
-	filt := newAclFilter(acl.AllowAll(), nil, false)
+	{
-	filt.filterServiceNodes(&nodes)
+		nodes := fill()
-	if len(nodes) != 1 {
+		filt := newAclFilter(acl.AllowAll(), nil, false)
-		t.Fatalf("bad: %#v", nodes)
+		filt.filterServiceNodes(&nodes)
+		if len(nodes) != 1 {
+			t.Fatalf("bad: %#v", nodes)
+		}
 	}
 
-	// Try restrictive filtering
+	// Try restrictive filtering.
-	filt = newAclFilter(acl.DenyAll(), nil, false)
+	{
-	filt.filterServiceNodes(&nodes)
+		nodes := fill()
-	if len(nodes) != 0 {
+		filt := newAclFilter(acl.DenyAll(), nil, false)
-		t.Fatalf("bad: %#v", nodes)
+		filt.filterServiceNodes(&nodes)
+		if len(nodes) != 0 {
+			t.Fatalf("bad: %#v", nodes)
+		}
+	}
+
+	// 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.
+	{
+		nodes := fill()
+		filt := newAclFilter(perms, nil, false)
+		filt.filterServiceNodes(&nodes)
+		if len(nodes) != 1 {
+			t.Fatalf("bad: %#v", nodes)
+		}
+	}
+
+	// But with version 8 the node will block it.
+	{
+		nodes := fill()
+		filt := newAclFilter(perms, nil, true)
+		filt.filterServiceNodes(&nodes)
+		if len(nodes) != 0 {
+			t.Fatalf("bad: %#v", nodes)
+		}
+	}
+
+	// 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.
+	{
+		nodes := fill()
+		filt := newAclFilter(perms, nil, true)
+		filt.filterServiceNodes(&nodes)
+		if len(nodes) != 1 {
+			t.Fatalf("bad: %#v", nodes)
+		}
 	}
 }
 

consul/catalog_endpoint_test.go

@@ -1369,6 +1369,7 @@ func testACLFilterServer(t *testing.T) (dir, token string, srv *Server, codec rp
 		c.ACLDatacenter = "dc1"
 		c.ACLMasterToken = "root"
 		c.ACLDefaultPolicy = "deny"
+		c.ACLEnforceVersion8 = false
 	})
 
 	codec = rpcClient(t, srv)
@@ -1499,6 +1500,12 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) {
 			t.Fatalf("bad: %#v", reply.ServiceNodes)
 		}
 	}
+
+	// We've already proven that we call the ACL filtering function so we
+	// test node filtering down in acl.go for node cases. This also proves
+	// that we respect the version 8 ACL flag, since the test server sets
+	// that to false (the regression value of *not* changing this is better
+	// for now until we change the sense of the version 8 ACL flag).
 }
 
 func TestCatalog_NodeServices_ACLDeny(t *testing.T) {