From 8038f2168479328c6293614115bd7b762337f1aa Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sat, 10 Dec 2016 16:00:11 -0800 Subject: [PATCH] Adds complete ACL coverage for /v1/catalog/nodes. --- consul/acl.go | 60 +++++++++++---- consul/acl_test.go | 67 ++++++++++++----- consul/catalog_endpoint.go | 3 + consul/catalog_endpoint_test.go | 127 ++++++++++++++++++++++++++------ 4 files changed, 202 insertions(+), 55 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index d622e5ef2a..1ab8f435b7 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -311,16 +311,21 @@ func (c *aclCache) useACLPolicy(id, authDC string, cached *aclCacheEntry, p *str // aclFilter is used to filter results from our state store based on ACL rules // configured for the provided token. type aclFilter struct { - acl acl.ACL - logger *log.Logger + acl acl.ACL + logger *log.Logger + enforceVersion8 bool } // newAclFilter constructs a new aclFilter. -func newAclFilter(acl acl.ACL, logger *log.Logger) *aclFilter { +func newAclFilter(acl acl.ACL, logger *log.Logger, enforceVersion8 bool) *aclFilter { if logger == nil { logger = log.New(os.Stdout, "", log.LstdFlags) } - return &aclFilter{acl, logger} + return &aclFilter{ + acl: acl, + logger: logger, + enforceVersion8: enforceVersion8, + } } // filterService is used to determine if a service is accessible for an ACL. @@ -432,6 +437,26 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { *dump = nd } +// filterNodes is used to filter through all parts of a node list and remove +// elements the provided ACL token cannot access. +func (f *aclFilter) filterNodes(nodes *structs.Nodes) { + if !f.enforceVersion8 { + return + } + + n := *nodes + for i := 0; i < len(n); i++ { + node := n[i].Node + if f.acl.NodeRead(node) { + continue + } + f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node) + n = append(n[:i], n[i+1:]...) + i-- + } + *nodes = n +} + // redactPreparedQueryTokens will redact any tokens unless the client has a // management token. This eases the transition to delegated authority over // prepared queries, since it was easy to capture management tokens in Consul @@ -506,31 +531,34 @@ func (s *Server) filterACL(token string, subj interface{}) error { } // Create the filter - filt := newAclFilter(acl, s.logger) + filt := newAclFilter(acl, s.logger, s.config.ACLEnforceVersion8) switch v := subj.(type) { + case *structs.CheckServiceNodes: + filt.filterCheckServiceNodes(v) + + case *structs.IndexedCheckServiceNodes: + filt.filterCheckServiceNodes(&v.Nodes) + case *structs.IndexedHealthChecks: filt.filterHealthChecks(&v.HealthChecks) - case *structs.IndexedServices: - filt.filterServices(v.Services) + case *structs.IndexedNodeDump: + filt.filterNodeDump(&v.Dump) - case *structs.IndexedServiceNodes: - filt.filterServiceNodes(&v.ServiceNodes) + case *structs.IndexedNodes: + filt.filterNodes(&v.Nodes) case *structs.IndexedNodeServices: if v.NodeServices != nil { filt.filterNodeServices(v.NodeServices) } - case *structs.IndexedCheckServiceNodes: - filt.filterCheckServiceNodes(&v.Nodes) + case *structs.IndexedServiceNodes: + filt.filterServiceNodes(&v.ServiceNodes) - case *structs.CheckServiceNodes: - filt.filterCheckServiceNodes(v) - - case *structs.IndexedNodeDump: - filt.filterNodeDump(&v.Dump) + case *structs.IndexedServices: + filt.filterServices(v.Services) case *structs.IndexedPreparedQueries: filt.filterPreparedQueries(&v.Queries) diff --git a/consul/acl_test.go b/consul/acl_test.go index d1baa1a8d4..c9232b39e3 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -818,14 +818,14 @@ func TestACL_filterHealthChecks(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + 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) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterHealthChecks(&hc) if len(hc) != 0 { t.Fatalf("bad: %#v", hc) @@ -840,14 +840,14 @@ func TestACL_filterServices(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterServices(services) if len(services) != 2 { t.Fatalf("bad: %#v", services) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterServices(services) if len(services) != 0 { t.Fatalf("bad: %#v", services) @@ -864,14 +864,14 @@ func TestACL_filterServiceNodes(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + 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) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterServiceNodes(&nodes) if len(nodes) != 0 { t.Fatalf("bad: %#v", nodes) @@ -893,14 +893,14 @@ func TestACL_filterNodeServices(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterNodeServices(&services) if len(services.Services) != 1 { t.Fatalf("bad: %#v", services.Services) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterNodeServices(&services) if len(services.Services) != 0 { t.Fatalf("bad: %#v", services.Services) @@ -929,7 +929,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterCheckServiceNodes(&nodes) if len(nodes) != 1 { t.Fatalf("bad: %#v", nodes) @@ -939,7 +939,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) { } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterCheckServiceNodes(&nodes) if len(nodes) != 0 { t.Fatalf("bad: %#v", nodes) @@ -968,7 +968,7 @@ func TestACL_filterNodeDump(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterNodeDump(&dump) if len(dump) != 1 { t.Fatalf("bad: %#v", dump) @@ -981,7 +981,7 @@ func TestACL_filterNodeDump(t *testing.T) { } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterNodeDump(&dump) if len(dump) != 1 { t.Fatalf("bad: %#v", dump) @@ -994,6 +994,39 @@ func TestACL_filterNodeDump(t *testing.T) { } } +func TestACL_filterNodes(t *testing.T) { + // Create a nodes list. + nodes := structs.Nodes{ + &structs.Node{ + Node: "foo", + }, + &structs.Node{ + Node: "bar", + }, + } + + // Try permissive filtering. + filt := newAclFilter(acl.AllowAll(), nil, true) + filt.filterNodes(&nodes) + if len(nodes) != 2 { + t.Fatalf("bad: %#v", nodes) + } + + // Try restrictive filtering but with version 8 enforcement turned off. + filt = newAclFilter(acl.DenyAll(), nil, false) + filt.filterNodes(&nodes) + if len(nodes) != 2 { + t.Fatalf("bad: %#v", nodes) + } + + // Try restrictive filtering with version 8 enforcement turned on. + filt = newAclFilter(acl.DenyAll(), nil, true) + filt.filterNodes(&nodes) + if len(nodes) != 0 { + t.Fatalf("bad: %#v", nodes) + } +} + func TestACL_redactPreparedQueryTokens(t *testing.T) { query := &structs.PreparedQuery{ ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", @@ -1007,7 +1040,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) { // Try permissive filtering with a management token. This will allow the // embedded token to be seen. - filt := newAclFilter(acl.ManageAll(), nil) + filt := newAclFilter(acl.ManageAll(), nil, false) filt.redactPreparedQueryTokens(&query) if !reflect.DeepEqual(query, expected) { t.Fatalf("bad: %#v", &query) @@ -1019,7 +1052,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) { // Now try permissive filtering with a client token, which should cause // the embedded token to get redacted. - filt = newAclFilter(acl.AllowAll(), nil) + filt = newAclFilter(acl.AllowAll(), nil, false) filt.redactPreparedQueryTokens(&query) expected.Token = redactedToken if !reflect.DeepEqual(query, expected) { @@ -1065,7 +1098,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { // Try permissive filtering with a management token. This will allow the // embedded token to be seen. - filt := newAclFilter(acl.ManageAll(), nil) + filt := newAclFilter(acl.ManageAll(), nil, false) filt.filterPreparedQueries(&queries) if !reflect.DeepEqual(queries, expected) { t.Fatalf("bad: %#v", queries) @@ -1078,7 +1111,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { // Now try permissive filtering with a client token, which should cause // the embedded token to get redacted, and the query with no name to get // filtered out. - filt = newAclFilter(acl.AllowAll(), nil) + filt = newAclFilter(acl.AllowAll(), nil, false) filt.filterPreparedQueries(&queries) expected[2].Token = redactedToken expected = append(structs.PreparedQueries{}, expected[1], expected[2]) @@ -1092,7 +1125,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { } // Now try restrictive filtering. - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterPreparedQueries(&queries) if len(queries) != 0 { t.Fatalf("bad: %#v", queries) diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index a943aa91cd..35d00dd470 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -169,6 +169,9 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde } reply.Index, reply.Nodes = index, nodes + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } return c.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) }) } diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index afb41a34b5..76a3e2703c 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -14,7 +14,7 @@ import ( "github.com/hashicorp/net-rpc-msgpackrpc" ) -func TestCatalogRegister(t *testing.T) { +func TestCatalog_Register(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -49,7 +49,7 @@ func TestCatalogRegister(t *testing.T) { }) } -func TestCatalogRegister_ACLDeny(t *testing.T) { +func TestCatalog_Register_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -150,7 +150,7 @@ service "foo" { } } -func TestCatalogRegister_ForwardLeader(t *testing.T) { +func TestCatalog_Register_ForwardLeader(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -197,7 +197,7 @@ func TestCatalogRegister_ForwardLeader(t *testing.T) { } } -func TestCatalogRegister_ForwardDC(t *testing.T) { +func TestCatalog_Register_ForwardDC(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -233,7 +233,7 @@ func TestCatalogRegister_ForwardDC(t *testing.T) { } } -func TestCatalogDeregister(t *testing.T) { +func TestCatalog_Deregister(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -258,7 +258,7 @@ func TestCatalogDeregister(t *testing.T) { } } -func TestCatalogDeregister_ACLDeny(t *testing.T) { +func TestCatalog_Deregister_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -469,7 +469,7 @@ service "service" { } } -func TestCatalogListDatacenters(t *testing.T) { +func TestCatalog_ListDatacenters(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -506,7 +506,7 @@ func TestCatalogListDatacenters(t *testing.T) { } } -func TestCatalogListDatacenters_DistanceSort(t *testing.T) { +func TestCatalog_ListDatacenters_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -550,7 +550,7 @@ func TestCatalogListDatacenters_DistanceSort(t *testing.T) { } } -func TestCatalogListNodes(t *testing.T) { +func TestCatalog_ListNodes(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -592,7 +592,7 @@ func TestCatalogListNodes(t *testing.T) { } } -func TestCatalogListNodes_StaleRaad(t *testing.T) { +func TestCatalog_ListNodes_StaleRaad(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -660,7 +660,7 @@ func TestCatalogListNodes_StaleRaad(t *testing.T) { } } -func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) { +func TestCatalog_ListNodes_ConsistentRead_Fail(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -710,7 +710,7 @@ func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) { } } -func TestCatalogListNodes_ConsistentRead(t *testing.T) { +func TestCatalog_ListNodes_ConsistentRead(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -758,7 +758,7 @@ func TestCatalogListNodes_ConsistentRead(t *testing.T) { } } -func TestCatalogListNodes_DistanceSort(t *testing.T) { +func TestCatalog_ListNodes_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -846,7 +846,84 @@ func TestCatalogListNodes_DistanceSort(t *testing.T) { } } -func BenchmarkCatalogListNodes(t *testing.B) { +func TestCatalog_ListNodes_ACLFilter(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testutil.WaitForLeader(t, s1.RPC, "dc1") + + // We scope the reply in each of these since msgpack won't clear out an + // existing slice if the incoming one is nil, so it's best to start + // clean each time. + + // Prior to version 8, the node policy should be ignored. + args := structs.DCSpecificRequest{ + Datacenter: "dc1", + } + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 1 { + t.Fatalf("bad: %v", reply.Nodes) + } + } + + // Now turn on version 8 enforcement and try again. + s1.config.ACLEnforceVersion8 = true + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 0 { + t.Fatalf("bad: %v", reply.Nodes) + } + } + + // Create an ACL that can read the node. + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: fmt.Sprintf(` +node "%s" { + policy = "read" +} +`, s1.config.NodeName), + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var id string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil { + t.Fatalf("err: %v", err) + } + + // Now try with the token and it will go through. + args.Token = id + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 1 { + t.Fatalf("bad: %v", reply.Nodes) + } + } +} + +func Benchmark_Catalog_ListNodes(t *testing.B) { dir1, s1 := testServer(nil) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -869,7 +946,7 @@ func BenchmarkCatalogListNodes(t *testing.B) { } } -func TestCatalogListServices(t *testing.T) { +func TestCatalog_ListServices(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -919,7 +996,7 @@ func TestCatalogListServices(t *testing.T) { } } -func TestCatalogListServices_Blocking(t *testing.T) { +func TestCatalog_ListServices_Blocking(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -977,7 +1054,7 @@ func TestCatalogListServices_Blocking(t *testing.T) { } } -func TestCatalogListServices_Timeout(t *testing.T) { +func TestCatalog_ListServices_Timeout(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1018,7 +1095,7 @@ func TestCatalogListServices_Timeout(t *testing.T) { } } -func TestCatalogListServices_Stale(t *testing.T) { +func TestCatalog_ListServices_Stale(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1055,7 +1132,7 @@ func TestCatalogListServices_Stale(t *testing.T) { } } -func TestCatalogListServiceNodes(t *testing.T) { +func TestCatalog_ListServiceNodes(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1104,7 +1181,7 @@ func TestCatalogListServiceNodes(t *testing.T) { } } -func TestCatalogListServiceNodes_DistanceSort(t *testing.T) { +func TestCatalog_ListServiceNodes_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1241,7 +1318,7 @@ func TestCatalog_NodeServices(t *testing.T) { } // Used to check for a regression against a known bug -func TestCatalogRegister_FailedCase1(t *testing.T) { +func TestCatalog_Register_FailedCase1(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1447,6 +1524,9 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) { if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { t.Fatalf("err: %v", err) } + if reply.NodeServices == nil { + t.Fatalf("should not be nil") + } // Now turn on version 8 enforcement and try again. s1.config.ACLEnforceVersion8 = true @@ -1464,7 +1544,7 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) { Type: structs.ACLTypeClient, Rules: fmt.Sprintf(` node "%s" { - policy = "write" + policy = "read" } `, s1.config.NodeName), }, @@ -1480,6 +1560,9 @@ node "%s" { if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { t.Fatalf("err: %v", err) } + if reply.NodeServices == nil { + t.Fatalf("should not be nil") + } } func TestCatalog_NodeServices_FilterACL(t *testing.T) {