From 7a21a591b8f20576b91a2c1a3f327f7a48bd1524 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 12 Dec 2016 16:28:52 -0800 Subject: [PATCH] Adds full ACL coverage for /v1/health endpoints. --- consul/acl.go | 4 +- consul/acl_test.go | 232 ++++++++++++++++++++++++++------- consul/health_endpoint_test.go | 24 ++++ 3 files changed, 211 insertions(+), 49 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index ba51e2a8f..790639e2c 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -350,7 +350,7 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) { hc := *checks for i := 0; i < len(hc); i++ { check := hc[i] - if f.allowService(check.ServiceName) { + if f.allowNode(check.Node) && f.allowService(check.ServiceName) { continue } f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", check.CheckID) @@ -403,7 +403,7 @@ func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) { csn := *nodes for i := 0; i < len(csn); i++ { node := csn[i] - if f.allowService(node.Service.Service) { + if f.allowNode(node.Node.Node) && f.allowService(node.Service.Service) { continue } f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node.Node.Node) diff --git a/consul/acl_test.go b/consul/acl_test.go index 9a6834c9d..431683106 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -808,27 +808,93 @@ func TestACL_MultiDC_Found(t *testing.T) { } func TestACL_filterHealthChecks(t *testing.T) { - // Create some health checks - hc := structs.HealthChecks{ - &structs.HealthCheck{ - Node: "node1", - CheckID: "check1", - ServiceName: "foo", - }, + // Create some health checks. + fill := func() structs.HealthChecks { + return structs.HealthChecks{ + &structs.HealthCheck{ + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, + } } - // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil, false) - filt.filterHealthChecks(&hc) - if len(hc) != 1 { - t.Fatalf("bad: %#v", hc) + // Try permissive filtering. + { + hc := fill() + 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, false) - filt.filterHealthChecks(&hc) - if len(hc) != 0 { - t.Fatalf("bad: %#v", hc) + // Try restrictive filtering. + { + hc := fill() + filt := newAclFilter(acl.DenyAll(), nil, false) + filt.filterHealthChecks(&hc) + if len(hc) != 0 { + t.Fatalf("bad: %#v", hc) + } + } + + // 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. + { + hc := fill() + filt := newAclFilter(perms, nil, false) + filt.filterHealthChecks(&hc) + if len(hc) != 1 { + t.Fatalf("bad: %#v", hc) + } + } + + // But with version 8 the node will block it. + { + hc := fill() + filt := newAclFilter(perms, nil, true) + filt.filterHealthChecks(&hc) + if len(hc) != 0 { + t.Fatalf("bad: %#v", hc) + } + } + + // 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. + { + hc := fill() + filt := newAclFilter(perms, nil, true) + filt.filterHealthChecks(&hc) + if len(hc) != 1 { + t.Fatalf("bad: %#v", hc) + } } } @@ -899,7 +965,7 @@ service "foo" { t.Fatalf("err: %v", err) } - /// This will work because version 8 ACLs aren't being enforced. + // This will work because version 8 ACLs aren't being enforced. { nodes := fill() filt := newAclFilter(perms, nil, false) @@ -974,41 +1040,113 @@ func TestACL_filterNodeServices(t *testing.T) { } func TestACL_filterCheckServiceNodes(t *testing.T) { - // Create some nodes - nodes := structs.CheckServiceNodes{ - structs.CheckServiceNode{ - Node: &structs.Node{ - Node: "node1", - }, - Service: &structs.NodeService{ - ID: "foo", - Service: "foo", - }, - Checks: structs.HealthChecks{ - &structs.HealthCheck{ - Node: "node1", - CheckID: "check1", - ServiceName: "foo", + // Create some nodes. + fill := func() structs.CheckServiceNodes { + return structs.CheckServiceNodes{ + structs.CheckServiceNode{ + Node: &structs.Node{ + Node: "node1", + }, + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Checks: structs.HealthChecks{ + &structs.HealthCheck{ + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, }, }, - }, + } } - // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil, false) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) - } - if len(nodes[0].Checks) != 1 { - t.Fatalf("bad: %#v", nodes[0].Checks) + // Try permissive filtering. + { + nodes := fill() + filt := newAclFilter(acl.AllowAll(), nil, false) + filt.filterCheckServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } + if len(nodes[0].Checks) != 1 { + t.Fatalf("bad: %#v", nodes[0].Checks) + } } - // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil, false) - filt.filterCheckServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) + // Try restrictive filtering. + { + nodes := fill() + filt := newAclFilter(acl.DenyAll(), nil, false) + filt.filterCheckServiceNodes(&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.filterCheckServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } + if len(nodes[0].Checks) != 1 { + t.Fatalf("bad: %#v", nodes[0].Checks) + } + } + + // But with version 8 the node will block it. + { + nodes := fill() + filt := newAclFilter(perms, nil, true) + filt.filterCheckServiceNodes(&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.filterCheckServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } + if len(nodes[0].Checks) != 1 { + t.Fatalf("bad: %#v", nodes[0].Checks) + } } } diff --git a/consul/health_endpoint_test.go b/consul/health_endpoint_test.go index 23a063de3..b5db5b9f2 100644 --- a/consul/health_endpoint_test.go +++ b/consul/health_endpoint_test.go @@ -507,6 +507,12 @@ func TestHealth_NodeChecks_FilterACL(t *testing.T) { if !found { t.Fatalf("bad: %#v", reply.HealthChecks) } + + // 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 TestHealth_ServiceChecks_FilterACL(t *testing.T) { @@ -543,6 +549,12 @@ func TestHealth_ServiceChecks_FilterACL(t *testing.T) { if len(reply.HealthChecks) != 0 { t.Fatalf("bad: %#v", reply.HealthChecks) } + + // 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 TestHealth_ServiceNodes_FilterACL(t *testing.T) { @@ -572,6 +584,12 @@ func TestHealth_ServiceNodes_FilterACL(t *testing.T) { if len(reply.Nodes) != 0 { t.Fatalf("bad: %#v", reply.Nodes) } + + // 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 TestHealth_ChecksInState_FilterACL(t *testing.T) { @@ -602,4 +620,10 @@ func TestHealth_ChecksInState_FilterACL(t *testing.T) { if !found { t.Fatalf("missing service 'foo': %#v", reply.HealthChecks) } + + // 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). }