From 8d2ed1999d41fbfd846178a8a3994ce725c0f525 Mon Sep 17 00:00:00 2001 From: malizz Date: Wed, 9 Nov 2022 13:02:58 -0800 Subject: [PATCH] update ACLs for cluster peering (#15317) * update ACLs for cluster peering * add changelog * Update .changelog/15317.txt Co-authored-by: Eric Haberkorn Co-authored-by: Eric Haberkorn --- .changelog/15317.txt | 3 + acl/acl.go | 4 + agent/consul/prepared_query_endpoint_test.go | 2 +- agent/structs/aclfilter/filter.go | 7 +- agent/structs/aclfilter/filter_test.go | 245 ++++++++++++++----- agent/structs/structs.go | 9 + agent/structs/structs_test.go | 47 +++- 7 files changed, 244 insertions(+), 73 deletions(-) create mode 100644 .changelog/15317.txt diff --git a/.changelog/15317.txt b/.changelog/15317.txt new file mode 100644 index 000000000..8425dda8b --- /dev/null +++ b/.changelog/15317.txt @@ -0,0 +1,3 @@ +```release-note:improvements +acl: Allow reading imported services and nodes from cluster peers with read all permissions +``` \ No newline at end of file diff --git a/acl/acl.go b/acl/acl.go index d56383d9f..5c0ee9e89 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -20,6 +20,10 @@ type ExportFetcher interface { } type ExportedServices struct { + // Data is a map of [namespace] -> [service] -> [list of partitions the service is exported to] + // This includes both the names of typical service instances and their corresponding sidecar proxy + // instance names. Meaning that if "web" is exported, "web-sidecar-proxy" instances will also be + // shown as exported. Data map[string]map[string][]string } diff --git a/agent/consul/prepared_query_endpoint_test.go b/agent/consul/prepared_query_endpoint_test.go index dea017083..418710b8b 100644 --- a/agent/consul/prepared_query_endpoint_test.go +++ b/agent/consul/prepared_query_endpoint_test.go @@ -1578,7 +1578,7 @@ func TestPreparedQuery_Execute(t *testing.T) { execNoNodesToken := createTokenWithPolicyName(t, codec1, "no-nodes", `service_prefix "foo" { policy = "read" }`, "root") rules := ` - service_prefix "foo" { policy = "read" } + service_prefix "" { policy = "read" } node_prefix "" { policy = "read" } ` execToken := createTokenWithPolicyName(t, codec1, "with-read", rules, "root") diff --git a/agent/structs/aclfilter/filter.go b/agent/structs/aclfilter/filter.go index c93e9e97f..b976627dc 100644 --- a/agent/structs/aclfilter/filter.go +++ b/agent/structs/aclfilter/filter.go @@ -142,6 +142,9 @@ func (f *Filter) Filter(subject any) { if f.filterGatewayServices(&v.Gateways) { v.QueryMeta.ResultsFilteredByACLs = true } + if f.filterCheckServiceNodes(&v.ImportedNodes) { + v.QueryMeta.ResultsFilteredByACLs = true + } default: panic(fmt.Errorf("Unhandled type passed to ACL filter: %T %#v", subject, subject)) @@ -319,13 +322,11 @@ func (f *Filter) filterNodeServiceList(services *structs.NodeServiceList) bool { // true if any elements were removed. func (f *Filter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) bool { csn := *nodes - var authzContext acl.AuthorizerContext var removed bool for i := 0; i < len(csn); i++ { node := csn[i] - node.Service.FillAuthzContext(&authzContext) - if f.allowNode(node.Node.Node, &authzContext) && f.allowService(node.Service.Service, &authzContext) { + if node.CanRead(f.authorizer) == acl.Allow { continue } f.logger.Debug("dropping node from result due to ACLs", "node", structs.NodeNameString(node.Node.Node, node.Node.GetEnterpriseMeta())) diff --git a/agent/structs/aclfilter/filter_test.go b/agent/structs/aclfilter/filter_test.go index b9539ac77..1093f436b 100644 --- a/agent/structs/aclfilter/filter_test.go +++ b/agent/structs/aclfilter/filter_test.go @@ -1107,12 +1107,51 @@ func TestACL_filterIndexedNodesWithGateways(t *testing.T) { {Service: structs.ServiceNameFromString("foo")}, {Service: structs.ServiceNameFromString("bar")}, }, + ImportedNodes: structs.CheckServiceNodes{ + { + Node: &structs.Node{ + Node: "imported-node", + PeerName: "cluster-02", + }, + Service: &structs.NodeService{ + ID: "zip", + Service: "zip", + PeerName: "cluster-02", + }, + Checks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "zip", + PeerName: "cluster-02", + }, + }, + }, + }, } } - t.Run("allowed", func(t *testing.T) { + type testCase struct { + authzFn func() acl.Authorizer + expect *structs.IndexedNodesWithGateways + } - policy, err := acl.NewPolicyFromSource(` + run := func(t *testing.T, tc testCase) { + authz := tc.authzFn() + + list := makeList() + New(authz, logger).Filter(list) + + require.Equal(t, tc.expect, list) + } + + tt := map[string]testCase{ + "not filtered": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` + service "baz" { + policy = "write" + } service "foo" { policy = "read" } @@ -1123,22 +1162,63 @@ func TestACL_filterIndexedNodesWithGateways(t *testing.T) { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) - - list := makeList() - New(authz, logger).Filter(list) - - require.Len(t, list.Nodes, 1) - require.Len(t, list.Gateways, 2) - require.False(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") - }) - - t.Run("not allowed to read the node", func(t *testing.T) { - - policy, err := acl.NewPolicyFromSource(` + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + return authz + }, + expect: &structs.IndexedNodesWithGateways{ + Nodes: structs.CheckServiceNodes{ + { + Node: &structs.Node{ + Node: "node1", + }, + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Checks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, + }, + }, + }, + Gateways: structs.GatewayServices{ + {Service: structs.ServiceNameFromString("foo")}, + {Service: structs.ServiceNameFromString("bar")}, + }, + // Service write to "bar" allows reading all imported services + ImportedNodes: structs.CheckServiceNodes{ + { + Node: &structs.Node{ + Node: "imported-node", + PeerName: "cluster-02", + }, + Service: &structs.NodeService{ + ID: "zip", + Service: "zip", + PeerName: "cluster-02", + }, + Checks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "zip", + PeerName: "cluster-02", + }, + }, + }, + }, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: false}, + }, + }, + "not allowed to read the node": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -1146,22 +1226,25 @@ func TestACL_filterIndexedNodesWithGateways(t *testing.T) { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) - - list := makeList() - New(authz, logger).Filter(list) - - require.Empty(t, list.Nodes) - require.Len(t, list.Gateways, 2) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) - - t.Run("allowed to read the node, but not the service", func(t *testing.T) { - - policy, err := acl.NewPolicyFromSource(` + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + return authz + }, + expect: &structs.IndexedNodesWithGateways{ + Nodes: structs.CheckServiceNodes{}, + Gateways: structs.GatewayServices{ + {Service: structs.ServiceNameFromString("foo")}, + {Service: structs.ServiceNameFromString("bar")}, + }, + ImportedNodes: structs.CheckServiceNodes{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "not allowed to read the service": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -1169,22 +1252,24 @@ func TestACL_filterIndexedNodesWithGateways(t *testing.T) { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) - - list := makeList() - New(authz, logger).Filter(list) - - require.Empty(t, list.Nodes) - require.Len(t, list.Gateways, 1) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) - - t.Run("not allowed to read the other gatway service", func(t *testing.T) { - - policy, err := acl.NewPolicyFromSource(` + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + return authz + }, + expect: &structs.IndexedNodesWithGateways{ + Nodes: structs.CheckServiceNodes{}, + Gateways: structs.GatewayServices{ + {Service: structs.ServiceNameFromString("bar")}, + }, + ImportedNodes: structs.CheckServiceNodes{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "not allowed to read the other gateway service": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -1192,28 +1277,54 @@ func TestACL_filterIndexedNodesWithGateways(t *testing.T) { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + return authz + }, + expect: &structs.IndexedNodesWithGateways{ + Nodes: structs.CheckServiceNodes{ + { + Node: &structs.Node{ + Node: "node1", + }, + Service: &structs.NodeService{ + ID: "foo", + Service: "foo", + }, + Checks: structs.HealthChecks{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, + }, + }, + }, + Gateways: structs.GatewayServices{ + {Service: structs.ServiceNameFromString("foo")}, + }, + ImportedNodes: structs.CheckServiceNodes{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "denied": { + authzFn: acl.DenyAll, + expect: &structs.IndexedNodesWithGateways{ + Nodes: structs.CheckServiceNodes{}, + Gateways: structs.GatewayServices{}, + ImportedNodes: structs.CheckServiceNodes{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + } - list := makeList() - New(authz, logger).Filter(list) - - require.Len(t, list.Nodes, 1) - require.Len(t, list.Gateways, 1) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) - - t.Run("denied", func(t *testing.T) { - - list := makeList() - New(acl.DenyAll(), logger).Filter(list) - - require.Empty(t, list.Nodes) - require.Empty(t, list.Gateways) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + run(t, tc) + }) + } } func TestACL_filterIndexedServiceDump(t *testing.T) { diff --git a/agent/structs/structs.go b/agent/structs/structs.go index f8bd52366..6235acb7c 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1998,6 +1998,15 @@ func (csn *CheckServiceNode) CanRead(authz acl.Authorizer) acl.EnforcementDecisi authzContext := new(acl.AuthorizerContext) csn.Service.EnterpriseMeta.FillAuthzContext(authzContext) + if csn.Node.PeerName != "" || csn.Service.PeerName != "" { + if authz.ServiceReadAll(authzContext) == acl.Allow || + authz.ServiceWriteAny(authzContext) == acl.Allow { + + return acl.Allow + } + return acl.Deny + } + if authz.NodeRead(csn.Node.Node, authzContext) != acl.Allow { return acl.Deny } diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index 87402a2d4..bb5b2a7b4 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -1759,6 +1759,33 @@ func TestCheckServiceNode_CanRead(t *testing.T) { authz: acl.AllowAll(), expected: acl.Allow, }, + { + name: "can read imported csn if can read all", + csn: CheckServiceNode{ + Node: &Node{Node: "name", PeerName: "cluster-2"}, + Service: &NodeService{Service: "service-name", PeerName: "cluster-2"}, + }, + authz: aclAuthorizerCheckServiceNode{allowReadAllServices: true}, + expected: acl.Allow, + }, + { + name: "can read imported csn if can write any", + csn: CheckServiceNode{ + Node: &Node{Node: "name", PeerName: "cluster-2"}, + Service: &NodeService{Service: "service-name", PeerName: "cluster-2"}, + }, + authz: aclAuthorizerCheckServiceNode{allowServiceWrite: true}, + expected: acl.Allow, + }, + { + name: "can't read imported csn with authz for local services and nodes", + csn: CheckServiceNode{ + Node: &Node{Node: "name", PeerName: "cluster-2"}, + Service: &NodeService{Service: "service-name", PeerName: "cluster-2"}, + }, + authz: aclAuthorizerCheckServiceNode{allowService: true, allowNode: true}, + expected: acl.Deny, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -1769,8 +1796,10 @@ func TestCheckServiceNode_CanRead(t *testing.T) { type aclAuthorizerCheckServiceNode struct { acl.Authorizer - allowNode bool - allowService bool + allowNode bool + allowService bool + allowServiceWrite bool + allowReadAllServices bool } func (a aclAuthorizerCheckServiceNode) ServiceRead(string, *acl.AuthorizerContext) acl.EnforcementDecision { @@ -1787,6 +1816,20 @@ func (a aclAuthorizerCheckServiceNode) NodeRead(string, *acl.AuthorizerContext) return acl.Deny } +func (a aclAuthorizerCheckServiceNode) ServiceReadAll(*acl.AuthorizerContext) acl.EnforcementDecision { + if a.allowReadAllServices { + return acl.Allow + } + return acl.Deny +} + +func (a aclAuthorizerCheckServiceNode) ServiceWriteAny(*acl.AuthorizerContext) acl.EnforcementDecision { + if a.allowServiceWrite { + return acl.Allow + } + return acl.Deny +} + func TestStructs_DirEntry_Clone(t *testing.T) { e := &DirEntry{ LockIndex: 5,