From 2ace618bf9cf4ec2869857d7e8dfd0246ae166bb Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sun, 11 Dec 2016 13:22:14 -0800 Subject: [PATCH] Adds complete ACL coverage for /v1/catalog/service/. --- consul/acl.go | 26 ++++++--- consul/acl_test.go | 98 +++++++++++++++++++++++++++------ consul/catalog_endpoint_test.go | 7 +++ 3 files changed, 106 insertions(+), 25 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index 1ab8f435b7..cfbf744f77 100644 --- a/consul/acl.go +++ b/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. -func (f *aclFilter) filterService(service string) bool { +// allowNode is used to determine if a node is accessible for an ACL. +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) diff --git a/consul/acl_test.go b/consul/acl_test.go index c9232b39e3..221e09fe97 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -855,26 +855,92 @@ func TestACL_filterServices(t *testing.T) { } func TestACL_filterServiceNodes(t *testing.T) { - // Create some service nodes - nodes := structs.ServiceNodes{ - &structs.ServiceNode{ - Node: "node1", - ServiceName: "foo", - }, + // Create some service nodes. + fill := func() structs.ServiceNodes { + return structs.ServiceNodes{ + &structs.ServiceNode{ + Node: "node1", + ServiceName: "foo", + }, + } } - // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil, false) - filt.filterServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) + // Try permissive filtering. + { + nodes := fill() + 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, false) - filt.filterServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) + // Try restrictive filtering. + { + nodes := fill() + filt := newAclFilter(acl.DenyAll(), nil, false) + 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) + } } } diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 76a3e2703c..6593fdf848 100644 --- a/consul/catalog_endpoint_test.go +++ b/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) {