diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 0805561ae6..0b1469b58f 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1242,25 +1242,32 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) bool { return removed } -// filterServices is used to filter a set of services based on ACLs. -func (f *aclFilter) filterServices(services structs.Services, entMeta *structs.EnterpriseMeta) { +// filterServices is used to filter a set of services based on ACLs. Returns +// true if any elements were removed. +func (f *aclFilter) filterServices(services structs.Services, entMeta *structs.EnterpriseMeta) bool { var authzContext acl.AuthorizerContext entMeta.FillAuthzContext(&authzContext) + var removed bool + for svc := range services { if f.allowService(svc, &authzContext) { continue } f.logger.Debug("dropping service from result due to ACLs", "service", svc) + removed = true delete(services, svc) } + + return removed } // filterServiceNodes is used to filter a set of nodes for a given service -// based on the configured ACL rules. -func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) { +// based on the configured ACL rules. Returns true if any elements were removed. +func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) bool { sn := *nodes var authzContext acl.AuthorizerContext + var removed bool for i := 0; i < len(sn); i++ { node := sn[i] @@ -1269,26 +1276,30 @@ func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) { if f.allowNode(node.Node, &authzContext) && f.allowService(node.ServiceName, &authzContext) { continue } + removed = true f.logger.Debug("dropping node from result due to ACLs", "node", structs.NodeNameString(node.Node, &node.EnterpriseMeta)) sn = append(sn[:i], sn[i+1:]...) i-- } *nodes = sn + return removed } // filterNodeServices is used to filter services on a given node base on ACLs. -func (f *aclFilter) filterNodeServices(services **structs.NodeServices) { +// Returns true if any elements were removed +func (f *aclFilter) filterNodeServices(services **structs.NodeServices) bool { if *services == nil { - return + return false } var authzContext acl.AuthorizerContext (*services).Node.FillAuthzContext(&authzContext) if !f.allowNode((*services).Node.Node, &authzContext) { *services = nil - return + return true } + var removed bool for svcName, svc := range (*services).Services { svc.FillAuthzContext(&authzContext) @@ -1296,44 +1307,45 @@ func (f *aclFilter) filterNodeServices(services **structs.NodeServices) { continue } f.logger.Debug("dropping service from result due to ACLs", "service", svc.CompoundServiceID()) + removed = true delete((*services).Services, svcName) } + + return removed } // filterNodeServices is used to filter services on a given node base on ACLs. -func (f *aclFilter) filterNodeServiceList(services **structs.NodeServiceList) { - if services == nil || *services == nil { - return +// Returns true if any elements were removed. +func (f *aclFilter) filterNodeServiceList(services *structs.NodeServiceList) bool { + if services.Node == nil { + return false } var authzContext acl.AuthorizerContext - (*services).Node.FillAuthzContext(&authzContext) - if !f.allowNode((*services).Node.Node, &authzContext) { - *services = nil - return + services.Node.FillAuthzContext(&authzContext) + if !f.allowNode(services.Node.Node, &authzContext) { + *services = structs.NodeServiceList{} + return true } - svcs := (*services).Services - modified := false + var removed bool + svcs := services.Services for i := 0; i < len(svcs); i++ { svc := svcs[i] svc.FillAuthzContext(&authzContext) - if f.allowNode((*services).Node.Node, &authzContext) && f.allowService(svc.Service, &authzContext) { + if f.allowService(svc.Service, &authzContext) { continue } + f.logger.Debug("dropping service from result due to ACLs", "service", svc.CompoundServiceID()) svcs = append(svcs[:i], svcs[i+1:]...) i-- - modified = true + removed = true } + services.Services = svcs - if modified { - *services = &structs.NodeServiceList{ - Node: (*services).Node, - Services: svcs, - } - } + return removed } // filterCheckServiceNodes is used to filter nodes based on ACL rules. Returns @@ -1520,11 +1532,13 @@ func (f *aclFilter) filterServiceDump(services *structs.ServiceDump) { } // 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) { +// elements the provided ACL token cannot access. Returns true if any elements +// were removed. +func (f *aclFilter) filterNodes(nodes *structs.Nodes) bool { n := *nodes var authzContext acl.AuthorizerContext + var removed bool for i := 0; i < len(n); i++ { n[i].FillAuthzContext(&authzContext) @@ -1533,10 +1547,12 @@ func (f *aclFilter) filterNodes(nodes *structs.Nodes) { continue } f.logger.Debug("dropping node from result due to ACLs", "node", structs.NodeNameString(node, n[i].GetEnterpriseMeta())) + removed = true n = append(n[:i], n[i+1:]...) i-- } *nodes = n + return removed } // redactPreparedQueryTokens will redact any tokens unless the client has a @@ -1769,14 +1785,16 @@ func (f *aclFilter) filterAuthMethods(methods *structs.ACLAuthMethods) { *methods = ret } -func (f *aclFilter) filterServiceList(services *structs.ServiceList) { +func (f *aclFilter) filterServiceList(services *structs.ServiceList) bool { ret := make(structs.ServiceList, 0, len(*services)) + var removed bool for _, svc := range *services { var authzContext acl.AuthorizerContext svc.FillAuthzContext(&authzContext) if f.authorizer.ServiceRead(svc.Name, &authzContext) != acl.Allow { + removed = true sid := structs.NewServiceID(svc.Name, &svc.EnterpriseMeta) f.logger.Debug("dropping service from result due to ACLs", "service", sid.String()) continue @@ -1786,10 +1804,13 @@ func (f *aclFilter) filterServiceList(services *structs.ServiceList) { } *services = ret + return removed } // filterGatewayServices is used to filter gateway to service mappings based on ACL rules. -func (f *aclFilter) filterGatewayServices(mappings *structs.GatewayServices) { +// Returns true if any elements were removed. +func (f *aclFilter) filterGatewayServices(mappings *structs.GatewayServices) bool { + var removed bool ret := make(structs.GatewayServices, 0, len(*mappings)) for _, s := range *mappings { // This filter only checks ServiceRead on the linked service. @@ -1799,11 +1820,13 @@ func (f *aclFilter) filterGatewayServices(mappings *structs.GatewayServices) { if f.authorizer.ServiceRead(s.Service.Name, &authzContext) != acl.Allow { f.logger.Debug("dropping service from result due to ACLs", "service", s.Service.String()) + removed = true continue } ret = append(ret, s) } *mappings = ret + return removed } func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, subj interface{}) { @@ -1845,19 +1868,19 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub filt.filterServiceDump(&v.Dump) case *structs.IndexedNodes: - filt.filterNodes(&v.Nodes) + v.QueryMeta.ResultsFilteredByACLs = filt.filterNodes(&v.Nodes) case *structs.IndexedNodeServices: - filt.filterNodeServices(&v.NodeServices) + v.QueryMeta.ResultsFilteredByACLs = filt.filterNodeServices(&v.NodeServices) - case **structs.NodeServiceList: - filt.filterNodeServiceList(v) + case *structs.IndexedNodeServiceList: + v.QueryMeta.ResultsFilteredByACLs = filt.filterNodeServiceList(&v.NodeServices) case *structs.IndexedServiceNodes: - filt.filterServiceNodes(&v.ServiceNodes) + v.QueryMeta.ResultsFilteredByACLs = filt.filterServiceNodes(&v.ServiceNodes) case *structs.IndexedServices: - filt.filterServices(v.Services, &v.EnterpriseMeta) + v.QueryMeta.ResultsFilteredByACLs = filt.filterServices(v.Services, &v.EnterpriseMeta) case *structs.IndexedSessions: v.QueryMeta.ResultsFilteredByACLs = filt.filterSessions(&v.Sessions) @@ -1898,10 +1921,18 @@ func filterACLWithAuthorizer(logger hclog.Logger, authorizer acl.Authorizer, sub filt.filterAuthMethod(v) case *structs.IndexedServiceList: - filt.filterServiceList(&v.Services) + v.QueryMeta.ResultsFilteredByACLs = filt.filterServiceList(&v.Services) - case *structs.GatewayServices: - filt.filterGatewayServices(v) + case *structs.IndexedGatewayServices: + v.QueryMeta.ResultsFilteredByACLs = filt.filterGatewayServices(&v.Services) + + case *structs.IndexedNodesWithGateways: + if filt.filterCheckServiceNodes(&v.Nodes) { + v.QueryMeta.ResultsFilteredByACLs = true + } + if filt.filterGatewayServices(&v.Gateways) { + v.QueryMeta.ResultsFilteredByACLs = true + } default: panic(fmt.Errorf("Unhandled type passed to ACL filter: %T %#v", subj, subj)) diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 45b82d23d3..a6385bcdf2 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -2304,6 +2304,9 @@ func TestACL_filterIntentions(t *testing.T) { func TestACL_filterServices(t *testing.T) { t.Parallel() + + require := require.New(t) + // Create some services services := structs.Services{ "service1": []string{}, @@ -2313,194 +2316,339 @@ func TestACL_filterServices(t *testing.T) { // Try permissive filtering. filt := newACLFilter(acl.AllowAll(), nil) - filt.filterServices(services, nil) - if len(services) != 3 { - t.Fatalf("bad: %#v", services) - } + removed := filt.filterServices(services, nil) + require.False(removed) + require.Len(services, 3) // Try restrictive filtering. filt = newACLFilter(acl.DenyAll(), nil) - filt.filterServices(services, nil) - if len(services) != 0 { - t.Fatalf("bad: %#v", services) - } + removed = filt.filterServices(services, nil) + require.True(removed) + require.Empty(services) } func TestACL_filterServiceNodes(t *testing.T) { t.Parallel() - // Create some service nodes. - fill := func() structs.ServiceNodes { - return structs.ServiceNodes{ - &structs.ServiceNode{ - Node: "node1", - ServiceName: "foo", - }, - } - } - // Try permissive filtering. - { - nodes := fill() - filt := newACLFilter(acl.AllowAll(), nil) - filt.filterServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) - } - } + logger := hclog.NewNullLogger() - // Try restrictive filtering. - { - nodes := fill() - filt := newACLFilter(acl.DenyAll(), nil) - filt.filterServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) - } - } - - // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource(` -service "foo" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } - - // But with version 8 the node will block it. - { - nodes := fill() - filt := newACLFilter(perms, nil) - filt.filterServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) - } - } - - // Chain on access to the node. - policy, err = acl.NewPolicyFromSource(` -node "node1" { - policy = "read" -} -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } - - // Now it should go through. - { - nodes := fill() - filt := newACLFilter(perms, nil) - filt.filterServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) - } - } -} - -func TestACL_filterNodeServices(t *testing.T) { - t.Parallel() - // Create some node services. - fill := func() *structs.NodeServices { - return &structs.NodeServices{ - Node: &structs.Node{ - Node: "node1", - }, - Services: map[string]*structs.NodeService{ - "foo": { - ID: "foo", - Service: "foo", + makeList := func() *structs.IndexedServiceNodes { + return &structs.IndexedServiceNodes{ + ServiceNodes: structs.ServiceNodes{ + { + Node: "node1", + ServiceName: "foo", }, }, } } - // Try nil, which is a possible input. - { - var services *structs.NodeServices - filt := newACLFilter(acl.AllowAll(), nil) - filt.filterNodeServices(&services) - if services != nil { - t.Fatalf("bad: %#v", services) - } - } + t.Run("allowed", func(t *testing.T) { + require := require.New(t) - // Try permissive filtering. - { - services := fill() - filt := newACLFilter(acl.AllowAll(), nil) - filt.filterNodeServices(&services) - if len(services.Services) != 1 { - t.Fatalf("bad: %#v", services.Services) - } - } + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) - // Try restrictive filtering. - { - services := fill() - filt := newACLFilter(acl.DenyAll(), nil) - filt.filterNodeServices(&services) - if services != nil { - t.Fatalf("bad: %#v", *services) - } - } + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) - // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource(` -service "foo" { - policy = "read" + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Len(list.ServiceNodes, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed to read the service, but not the node", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Empty(list.ServiceNodes) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("denied", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.Empty(list.ServiceNodes) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } - // Node will block it. - { - services := fill() - filt := newACLFilter(perms, nil) - filt.filterNodeServices(&services) - if services != nil { - t.Fatalf("bad: %#v", services) +func TestACL_filterNodeServices(t *testing.T) { + t.Parallel() + + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedNodeServices { + return &structs.IndexedNodeServices{ + NodeServices: &structs.NodeServices{ + Node: &structs.Node{ + Node: "node1", + }, + Services: map[string]*structs.NodeService{ + "foo": { + ID: "foo", + Service: "foo", + }, + }, + }, } } - // Chain on access to the node. - policy, err = acl.NewPolicyFromSource(` -node "node1" { - policy = "read" + t.Run("nil input", func(t *testing.T) { + require := require.New(t) + + list := &structs.IndexedNodeServices{ + NodeServices: nil, + } + filterACLWithAuthorizer(logger, acl.AllowAll(), list) + + require.Nil(list.NodeServices) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Len(list.NodeServices.Services, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed to read the service, but not the node", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Nil(list.NodeServices) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("allowed to read the node, but not the service", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Empty(list.NodeServices.Services) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("denied", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.Nil(list.NodeServices) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } -`, acl.SyntaxLegacy, nil, nil) - if err != nil { - t.Fatalf("err %v", err) - } - perms, err = acl.NewPolicyAuthorizerWithDefaults(perms, []*acl.Policy{policy}, nil) - if err != nil { - t.Fatalf("err: %v", err) - } - // Now it should go through. - { - services := fill() - filt := newACLFilter(perms, nil) - filt.filterNodeServices(&services) - if len((*services).Services) != 1 { - t.Fatalf("bad: %#v", (*services).Services) +func TestACL_filterNodeServiceList(t *testing.T) { + t.Parallel() + + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedNodeServiceList { + return &structs.IndexedNodeServiceList{ + NodeServices: structs.NodeServiceList{ + Node: &structs.Node{ + Node: "node1", + }, + Services: []*structs.NodeService{ + {Service: "foo"}, + }, + }, } } + + t.Run("empty NodeServices", func(t *testing.T) { + require := require.New(t) + + var list structs.IndexedNodeServiceList + filterACLWithAuthorizer(logger, acl.AllowAll(), &list) + + require.Empty(list) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Len(list.NodeServices.Services, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("allowed to read the service, but not the node", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Empty(list.NodeServices) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("allowed to read the node, but not the service", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.NotEmpty(list.NodeServices.Node) + require.Empty(list.NodeServices.Services) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("denied", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.Empty(list.NodeServices) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) +} + +func TestACL_filterGatewayServices(t *testing.T) { + t.Parallel() + + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedGatewayServices { + return &structs.IndexedGatewayServices{ + Services: structs.GatewayServices{ + {Service: structs.ServiceName{Name: "foo"}}, + }, + } + } + + t.Run("allowed", func(t *testing.T) { + require := require.New(t) + + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(err) + + list := makeList() + filterACLWithAuthorizer(logger, authz, list) + + require.Len(list.Services, 1) + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) + + t.Run("denied", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.Empty(list.Services) + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) } func TestACL_filterCheckServiceNodes(t *testing.T) { @@ -2982,6 +3130,9 @@ node "node1" { func TestACL_filterNodes(t *testing.T) { t.Parallel() + + require := require.New(t) + // Create a nodes list. nodes := structs.Nodes{ &structs.Node{ @@ -2994,17 +3145,15 @@ func TestACL_filterNodes(t *testing.T) { // Try permissive filtering. filt := newACLFilter(acl.AllowAll(), nil) - filt.filterNodes(&nodes) - if len(nodes) != 2 { - t.Fatalf("bad: %#v", nodes) - } + removed := filt.filterNodes(&nodes) + require.False(removed) + require.Len(nodes, 2) // Try restrictive filtering filt = newACLFilter(acl.DenyAll(), nil) - filt.filterNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) - } + removed = filt.filterNodes(&nodes) + require.True(removed) + require.Len(nodes, 0) } func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) { @@ -3268,6 +3417,39 @@ func TestACL_filterPreparedQueries(t *testing.T) { } } +func TestACL_filterServiceList(t *testing.T) { + logger := hclog.NewNullLogger() + + makeList := func() *structs.IndexedServiceList { + return &structs.IndexedServiceList{ + Services: structs.ServiceList{ + {Name: "foo"}, + {Name: "bar"}, + }, + } + } + + t.Run("permissive filtering", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.AllowAll(), list) + + require.False(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + require.Len(list.Services, 2) + }) + + t.Run("restrictive filtering", func(t *testing.T) { + require := require.New(t) + + list := makeList() + filterACLWithAuthorizer(logger, acl.DenyAll(), list) + + require.True(list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + require.Empty(list.Services) + }) +} + func TestACL_unhandledFilterType(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 1d238dd19d..cfddeff18c 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -488,16 +488,19 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde return nil } - if err := c.srv.filterACL(args.Token, reply); err != nil { - return err - } - raw, err := filter.Execute(reply.Nodes) if err != nil { return err } reply.Nodes = raw.(structs.Nodes) + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + return c.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) }) } @@ -664,18 +667,20 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru reply.ServiceNodes = filtered } - if err := c.srv.filterACL(args.Token, reply); err != nil { - return err - } - // This is safe to do even when the filter is nil - its just a no-op then raw, err := filter.Execute(reply.ServiceNodes) if err != nil { return err } - reply.ServiceNodes = raw.(structs.ServiceNodes) + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + return c.srv.sortNodesByDistanceFrom(args.Source, reply.ServiceNodes) }) @@ -750,11 +755,7 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs if err != nil { return err } - reply.Index, reply.NodeServices = index, services - if err := c.srv.filterACL(args.Token, reply); err != nil { - return err - } if reply.NodeServices != nil { raw, err := filter.Execute(reply.NodeServices.Services) @@ -764,6 +765,13 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs reply.NodeServices.Services = raw.(map[string]*structs.NodeService) } + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + return nil }) } @@ -802,11 +810,8 @@ func (c *Catalog) NodeServiceList(args *structs.NodeSpecificRequest, reply *stru return err } - if err := c.srv.filterACL(args.Token, &services); err != nil { - return err - } - reply.Index = index + if services != nil { reply.NodeServices = *services @@ -817,6 +822,13 @@ func (c *Catalog) NodeServiceList(args *structs.NodeSpecificRequest, reply *stru reply.NodeServices.Services = raw.([]*structs.NodeService) } + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } + return nil }) } @@ -876,12 +888,11 @@ func (c *Catalog) GatewayServices(args *structs.ServiceSpecificRequest, reply *s if err != nil { return err } + reply.Index, reply.Services = index, services - if err := c.srv.filterACL(args.Token, &services); err != nil { + if err := c.srv.filterACL(args.Token, reply); err != nil { return err } - - reply.Index, reply.Services = index, services return nil }) } diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index b8b4fe2568..a1d34bf993 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -1307,42 +1307,48 @@ func TestCatalog_ListNodes_ACLFilter(t *testing.T) { testrpc.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. + token := func(policy string) string { + rules := fmt.Sprintf( + `node "%s" { policy = "%s" }`, + s1.config.NodeName, + policy, + ) + return createTokenWithPolicyName(t, codec, policy, rules, "root") + } - // The node policy should not be ignored. args := structs.DCSpecificRequest{ Datacenter: "dc1", } - { - reply := structs.IndexedNodes{} + + t.Run("deny", func(t *testing.T) { + args.Token = token("deny") + + var 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) } - } + if !reply.QueryMeta.ResultsFilteredByACLs { + t.Fatal("ResultsFilteredByACLs should be true") + } + }) - rules := fmt.Sprintf(` -node "%s" { - policy = "read" -} -`, s1.config.NodeName) - id := createToken(t, codec, rules) + t.Run("allow", func(t *testing.T) { + args.Token = token("read") - // Now try with the token and it will go through. - args.Token = id - { - reply := structs.IndexedNodes{} + var 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) } - } + if reply.QueryMeta.ResultsFilteredByACLs { + t.Fatal("ResultsFilteredByACLs should not true") + } + }) } func Benchmark_Catalog_ListNodes(t *testing.B) { @@ -1422,6 +1428,7 @@ func TestCatalog_ListServices(t *testing.T) { t.Fatalf("bad: %v", out) } require.False(t, out.QueryMeta.NotModified) + require.False(t, out.QueryMeta.ResultsFilteredByACLs) t.Run("with option AllowNotModifiedResponse", func(t *testing.T) { args.QueryOptions = structs.QueryOptions{ @@ -2474,6 +2481,7 @@ node "foo" { require.Len(t, resp.ServiceNodes, 1) v := resp.ServiceNodes[0] require.Equal(t, "foo-proxy", v.ServiceName) + require.True(t, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") } func TestCatalog_ListServiceNodes_ConnectNative(t *testing.T) { @@ -2778,6 +2786,9 @@ func TestCatalog_ListServices_FilterACL(t *testing.T) { if _, ok := reply.Services["bar"]; ok { t.Fatalf("bad: %#v", reply.Services) } + if !reply.QueryMeta.ResultsFilteredByACLs { + t.Fatal("ResultsFilteredByACLs should be true") + } } func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { @@ -2826,6 +2837,7 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", reply.ServiceNodes) } } + require.True(t, reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") // 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 @@ -2834,7 +2846,7 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { // for now until we change the sense of the version 8 ACL flag). } -func TestCatalog_NodeServices_ACLDeny(t *testing.T) { +func TestCatalog_NodeServices_ACL(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } @@ -2853,43 +2865,46 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) { testrpc.WaitForTestAgent(t, s1.RPC, "dc1", testrpc.WithToken("root")) - // The node policy should not be ignored. + token := func(policy string) string { + rules := fmt.Sprintf(` + node "%s" { policy = "%s" } + service "consul" { policy = "%s" } + `, + s1.config.NodeName, + policy, + policy, + ) + return createTokenWithPolicyName(t, codec, policy, rules, "root") + } + args := structs.NodeSpecificRequest{ Datacenter: "dc1", Node: s1.config.NodeName, } - reply := structs.IndexedNodeServices{} - if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if reply.NodeServices != nil { - t.Fatalf("should not nil") - } - rules := fmt.Sprintf(` -node "%s" { - policy = "read" -} -`, s1.config.NodeName) - id := createToken(t, codec, rules) + t.Run("deny", func(t *testing.T) { + require := require.New(t) - // Now try with the token and it will go through. - args.Token = id - 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") - } + args.Token = token("deny") - // Make sure an unknown node doesn't cause trouble. - args.Node = "nope" - if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if reply.NodeServices != nil { - t.Fatalf("should not nil") - } + var reply structs.IndexedNodeServices + err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply) + require.NoError(err) + require.Nil(reply.NodeServices) + require.True(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") + }) + + t.Run("allow", func(t *testing.T) { + require := require.New(t) + + args.Token = token("read") + + var reply structs.IndexedNodeServices + err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply) + require.NoError(err) + require.NotNil(reply.NodeServices) + require.False(reply.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") + }) } func TestCatalog_NodeServices_FilterACL(t *testing.T) { @@ -3380,6 +3395,7 @@ service "gateway" { var resp structs.IndexedGatewayServices assert.Nil(r, msgpackrpc.CallWithCodec(codec, "Catalog.GatewayServices", &req, &resp)) assert.Len(r, resp.Services, 0) + assert.True(r, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") }) rules = ` @@ -3403,6 +3419,7 @@ service "gateway" { var resp structs.IndexedGatewayServices assert.Nil(r, msgpackrpc.CallWithCodec(codec, "Catalog.GatewayServices", &req, &resp)) assert.Len(r, resp.Services, 2) + assert.True(r, resp.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") expect := structs.GatewayServices{ { diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index 27f8ba6883..e90d432003 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -130,16 +130,19 @@ func (m *Internal) ServiceDump(args *structs.ServiceDumpRequest, reply *structs. } reply.Index = maxIdx - if err := m.srv.filterACL(args.Token, &reply.Gateways); err != nil { - return err - } - raw, err := filter.Execute(reply.Nodes) if err != nil { return err } - reply.Nodes = raw.(structs.CheckServiceNodes) + + // Note: we filter the results with ACLs *after* applying the user-supplied + // bexpr filter, to ensure QueryMeta.ResultsFilteredByACLs does not include + // results that would be filtered out even if the user did have permission. + if err := m.srv.filterACL(args.Token, reply); err != nil { + return err + } + return nil }) }