diff --git a/consul/acl.go b/consul/acl.go index 790639e2c3..d50876c76e 100644 --- a/consul/acl.go +++ b/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) diff --git a/consul/acl_test.go b/consul/acl_test.go index 431683106a..3dc85c7cde 100644 --- a/consul/acl_test.go +++ b/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) + } } } diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 35d00dd470..1383b89165 100644 --- a/consul/catalog_endpoint.go +++ b/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) }) diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 6593fdf848..c0a329af36 100644 --- a/consul/catalog_endpoint_test.go +++ b/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) {