From 99e810f9c77b8c9e5254a1b75b5efc0c2b8193b3 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 12 Dec 2016 18:21:00 -0800 Subject: [PATCH] Adds complete ACL coverage for /v1/internal/ui/node endpoints. --- consul/acl.go | 24 +++-- consul/acl_test.go | 146 ++++++++++++++++++++++++------- consul/internal_endpoint_test.go | 12 +++ 3 files changed, 140 insertions(+), 42 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index d50876c76e..7def53c3ad 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -445,26 +445,34 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { for i := 0; i < len(nd); i++ { info := nd[i] + // Filter nodes + if node := info.Node; !f.allowNode(node) { + f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node) + nd = append(nd[:i], nd[i+1:]...) + i-- + continue + } + // Filter services - for i := 0; i < len(info.Services); i++ { - svc := info.Services[i].Service + for j := 0; j < len(info.Services); j++ { + svc := info.Services[j].Service if f.allowService(svc) { continue } f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc) - info.Services = append(info.Services[:i], info.Services[i+1:]...) - i-- + info.Services = append(info.Services[:j], info.Services[j+1:]...) + j-- } // Filter checks - for i := 0; i < len(info.Checks); i++ { - chk := info.Checks[i] + for j := 0; j < len(info.Checks); j++ { + chk := info.Checks[j] if f.allowService(chk.ServiceName) { continue } f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", chk.CheckID) - info.Checks = append(info.Checks[:i], info.Checks[i+1:]...) - i-- + info.Checks = append(info.Checks[:j], info.Checks[j+1:]...) + j-- } } *dump = nd diff --git a/consul/acl_test.go b/consul/acl_test.go index 3dc85c7cde..52a458195a 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -1107,7 +1107,7 @@ node "node1" { // Now it should go through. { services := fill() - filt := newAclFilter(perms, nil, false) + filt := newAclFilter(perms, nil, true) filt.filterNodeServices(&services) if len((*services).Services) != 1 { t.Fatalf("bad: %#v", (*services).Services) @@ -1262,50 +1262,128 @@ func TestACL_filterCoordinates(t *testing.T) { } func TestACL_filterNodeDump(t *testing.T) { - // Create a node dump - dump := structs.NodeDump{ - &structs.NodeInfo{ - Node: "node1", - Services: []*structs.NodeService{ - &structs.NodeService{ - ID: "foo", - Service: "foo", + // Create a node dump. + fill := func() structs.NodeDump { + return structs.NodeDump{ + &structs.NodeInfo{ + Node: "node1", + Services: []*structs.NodeService{ + &structs.NodeService{ + ID: "foo", + Service: "foo", + }, }, - }, - Checks: []*structs.HealthCheck{ - &structs.HealthCheck{ - Node: "node1", - CheckID: "check1", - ServiceName: "foo", + Checks: []*structs.HealthCheck{ + &structs.HealthCheck{ + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, }, }, - }, + } } - // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil, false) - filt.filterNodeDump(&dump) - if len(dump) != 1 { - t.Fatalf("bad: %#v", dump) + // Try permissive filtering. + { + dump := fill() + filt := newAclFilter(acl.AllowAll(), nil, false) + filt.filterNodeDump(&dump) + if len(dump) != 1 { + t.Fatalf("bad: %#v", dump) + } + if len(dump[0].Services) != 1 { + t.Fatalf("bad: %#v", dump[0].Services) + } + if len(dump[0].Checks) != 1 { + t.Fatalf("bad: %#v", dump[0].Checks) + } + } + + // Try restrictive filtering. + { + dump := fill() + filt := newAclFilter(acl.DenyAll(), nil, false) + filt.filterNodeDump(&dump) + if len(dump) != 1 { + t.Fatalf("bad: %#v", dump) + } + if len(dump[0].Services) != 0 { + t.Fatalf("bad: %#v", dump[0].Services) + } + if len(dump[0].Checks) != 0 { + t.Fatalf("bad: %#v", dump[0].Checks) + } + } + + // 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) } - if len(dump[0].Services) != 1 { - t.Fatalf("bad: %#v", dump[0].Services) + perms, err := acl.New(acl.DenyAll(), policy) + if err != nil { + t.Fatalf("err: %v", err) } - if len(dump[0].Checks) != 1 { - t.Fatalf("bad: %#v", dump[0].Checks) + + // This will work because version 8 ACLs aren't being enforced. + { + dump := fill() + filt := newAclFilter(perms, nil, false) + filt.filterNodeDump(&dump) + if len(dump) != 1 { + t.Fatalf("bad: %#v", dump) + } + if len(dump[0].Services) != 1 { + t.Fatalf("bad: %#v", dump[0].Services) + } + if len(dump[0].Checks) != 1 { + t.Fatalf("bad: %#v", dump[0].Checks) + } } - // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil, false) - filt.filterNodeDump(&dump) - if len(dump) != 1 { - t.Fatalf("bad: %#v", dump) + // But with version 8 the node will block it. + { + dump := fill() + filt := newAclFilter(perms, nil, true) + filt.filterNodeDump(&dump) + if len(dump) != 0 { + t.Fatalf("bad: %#v", dump) + } + } + + // Chain on access to the node. + policy, err = acl.Parse(` +node "node1" { + policy = "read" +} +`) + if err != nil { + t.Fatalf("err %v", err) } - if len(dump[0].Services) != 0 { - t.Fatalf("bad: %#v", dump[0].Services) + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) } - if len(dump[0].Checks) != 0 { - t.Fatalf("bad: %#v", dump[0].Checks) + + // Now it should go through. + { + dump := fill() + filt := newAclFilter(perms, nil, true) + filt.filterNodeDump(&dump) + if len(dump) != 1 { + t.Fatalf("bad: %#v", dump) + } + if len(dump[0].Services) != 1 { + t.Fatalf("bad: %#v", dump[0].Services) + } + if len(dump[0].Checks) != 1 { + t.Fatalf("bad: %#v", dump[0].Checks) + } } } diff --git a/consul/internal_endpoint_test.go b/consul/internal_endpoint_test.go index 94a59cafbd..042ddd8e37 100644 --- a/consul/internal_endpoint_test.go +++ b/consul/internal_endpoint_test.go @@ -284,6 +284,12 @@ func TestInternal_NodeInfo_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", info.Services) } } + + // 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 TestInternal_NodeDump_FilterACL(t *testing.T) { @@ -327,6 +333,12 @@ func TestInternal_NodeDump_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", info.Services) } } + + // 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 TestInternal_EventFire_Token(t *testing.T) {