catalog, mesh: implement missing ACL hooks (#19143)

This change adds ACL hooks to the remaining catalog and mesh resources, excluding any computed ones. Those will for now continue using the default operator:x permissions. It refactors a lot of the common testing functions so that they can be re-used between resources. There are also some types that we don't yet support (e.g. virtual IPs) that this change adds ACL hooks to for future-proofing.
hashicorp · Oct 14, 2023 · 0a02b1f · 0a02b1f
1 parent 41a986c
commit 0a02b1f
Show file tree

Hide file tree

Showing 50 changed files with 996 additions and 179 deletions.
diff --git a/agent/grpc-external/services/resource/read.go b/agent/grpc-external/services/resource/read.go
@@ -50,7 +50,7 @@ func (s *Server) Read(ctx context.Context, req *pbresource.ReadRequest) (*pbreso
 	authzNeedsData := false
 	err = reg.ACLs.Read(authz, authzContext, req.Id, nil)
 	switch {
-	case errors.Is(err, resource.ErrNeedData):
+	case errors.Is(err, resource.ErrNeedResource):
 		authzNeedsData = true
 		err = nil
 	case acl.IsErrPermissionDenied(err):

diff --git a/internal/auth/internal/types/computed_traffic_permissions.go b/internal/auth/internal/types/computed_traffic_permissions.go
@@ -19,7 +19,7 @@ func RegisterComputedTrafficPermission(r resource.Registry) {
 		ACLs: &resource.ACLHooks{
 			Read:  aclReadHookComputedTrafficPermissions,
 			Write: aclWriteHookComputedTrafficPermissions,
-			List:  aclListHookComputedTrafficPermissions,
+			List:  resource.NoOpACLListHook,
 		},
 		Validate: ValidateComputedTrafficPermissions,
 		Scope:    resource.ScopeNamespace,
@@ -71,9 +71,3 @@ func aclReadHookComputedTrafficPermissions(authorizer acl.Authorizer, authzConte
 func aclWriteHookComputedTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *pbresource.Resource) error {
 	return authorizer.ToAllowAuthorizer().TrafficPermissionsWriteAllowed(res.Id.Name, authzContext)
 }
-
-func aclListHookComputedTrafficPermissions(_ acl.Authorizer, _ *acl.AuthorizerContext) error {
-	// No-op List permission as we want to default to filtering resources
-	// from the list using the Read enforcement
-	return nil
-}
diff --git a/internal/auth/internal/types/traffic_permissions.go b/internal/auth/internal/types/traffic_permissions.go
@@ -19,7 +19,7 @@ func RegisterTrafficPermissions(r resource.Registry) {
 		ACLs: &resource.ACLHooks{
 			Read:  aclReadHookTrafficPermissions,
 			Write: aclWriteHookTrafficPermissions,
-			List:  aclListHookTrafficPermissions,
+			List:  resource.NoOpACLListHook,
 		},
 		Validate: ValidateTrafficPermissions,
 		Mutate:   MutateTrafficPermissions,
@@ -273,7 +273,7 @@ func isLocalPeer(p string) bool {
 
 func aclReadHookTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, _ *pbresource.ID, res *pbresource.Resource) error {
 	if res == nil {
-		return resource.ErrNeedData
+		return resource.ErrNeedResource
 	}
 	return authorizeDestination(res, func(dest string) error {
 		return authorizer.ToAllowAuthorizer().TrafficPermissionsReadAllowed(dest, authzContext)
@@ -286,12 +286,6 @@ func aclWriteHookTrafficPermissions(authorizer acl.Authorizer, authzContext *acl
 	})
 }
 
-func aclListHookTrafficPermissions(_ acl.Authorizer, _ *acl.AuthorizerContext) error {
-	// No-op List permission as we want to default to filtering resources
-	// from the list using the Read enforcement
-	return nil
-}
-
 func authorizeDestination(res *pbresource.Resource, intentionAllowed func(string) error) error {
 	tp, err := resource.Decode[*pbauth.TrafficPermissions](res)
 	if err != nil {

diff --git a/internal/auth/internal/types/workload_identity.go b/internal/auth/internal/types/workload_identity.go
@@ -18,7 +18,7 @@ func RegisterWorkloadIdentity(r resource.Registry) {
 		ACLs: &resource.ACLHooks{
 			Read:  aclReadHookWorkloadIdentity,
 			Write: aclWriteHookWorkloadIdentity,
-			List:  aclListHookWorkloadIdentity,
+			List:  resource.NoOpACLListHook,
 		},
 		Validate: nil,
 	})
@@ -36,21 +36,15 @@ func aclReadHookWorkloadIdentity(
 	if res != nil {
 		return authorizer.ToAllowAuthorizer().IdentityReadAllowed(res.Id.Name, authzCtx)
 	}
-	return resource.ErrNeedData
+	return resource.ErrNeedResource
 }
 
 func aclWriteHookWorkloadIdentity(
 	authorizer acl.Authorizer,
 	authzCtx *acl.AuthorizerContext,
 	res *pbresource.Resource) error {
 	if res == nil {
-		return resource.ErrNeedData
+		return resource.ErrNeedResource
 	}
 	return authorizer.ToAllowAuthorizer().IdentityWriteAllowed(res.Id.Name, authzCtx)
 }
-
-func aclListHookWorkloadIdentity(authorizer acl.Authorizer, context *acl.AuthorizerContext) error {
-	// No-op List permission as we want to default to filtering resources
-	// from the list using the Read enforcement
-	return nil
-}
diff --git a/internal/auth/internal/types/workload_identity_test.go b/internal/auth/internal/types/workload_identity_test.go
@@ -82,8 +82,8 @@ func TestWorkloadIdentityACLs(t *testing.T) {
 			checkF(t, tc.listOK, err)
 		})
 		t.Run("errors", func(t *testing.T) {
-			require.ErrorIs(t, reg.ACLs.Read(authz, &acl.AuthorizerContext{}, nil, nil), resource.ErrNeedData)
-			require.ErrorIs(t, reg.ACLs.Write(authz, &acl.AuthorizerContext{}, nil), resource.ErrNeedData)
+			require.ErrorIs(t, reg.ACLs.Read(authz, &acl.AuthorizerContext{}, nil, nil), resource.ErrNeedResource)
+			require.ErrorIs(t, reg.ACLs.Write(authz, &acl.AuthorizerContext{}, nil), resource.ErrNeedResource)
 		})
 	}
 

diff --git a/internal/catalog/catalogtest/helpers/acl_hooks_test_helpers.go b/internal/catalog/catalogtest/helpers/acl_hooks_test_helpers.go
@@ -0,0 +1,21 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package helpers
+
+import (
+	"testing"
+
+	"github.com/hashicorp/consul/internal/catalog"
+	"github.com/hashicorp/consul/internal/catalog/internal/testhelpers"
+	"github.com/hashicorp/consul/internal/resource"
+	pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
+	"github.com/hashicorp/consul/proto-public/pbresource"
+)
+
+func RunWorkloadSelectingTypeACLsTests[T catalog.WorkloadSelecting](t *testing.T, typ *pbresource.Type,
+	getData func(selector *pbcatalog.WorkloadSelector) T,
+	registerFunc func(registry resource.Registry),
+) {
+	testhelpers.RunWorkloadSelectingTypeACLsTests[T](t, typ, getData, registerFunc)
+}
diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go
@@ -48,6 +48,12 @@ var (
 	FailoverStatusConditionAcceptedUsingMeshDestinationPortReason  = failover.UsingMeshDestinationPortReason
 )
 
+type WorkloadSelecting = types.WorkloadSelecting
+
+func ACLHooksForWorkloadSelectingType[T WorkloadSelecting]() *resource.ACLHooks {
+	return types.ACLHooksForWorkloadSelectingType[T]()
+}
+
 // RegisterTypes adds all resource types within the "catalog" API group
 // to the given type registry
 func RegisterTypes(r resource.Registry) {

diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go
@@ -73,7 +73,7 @@ type nodeHealthControllerTestSuite struct {
 }
 
 func (suite *nodeHealthControllerTestSuite) SetupTest() {
-	suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register)
+	suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register, types.RegisterDNSPolicy)
 	suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())}
 
 	// The rest of the setup will be to prime the resource service with some data

diff --git a/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go b/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go
@@ -0,0 +1,113 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package testhelpers
+
+import (
+	"testing"
+
+	"google.golang.org/protobuf/proto"
+
+	"github.com/hashicorp/consul/internal/resource"
+	"github.com/hashicorp/consul/internal/resource/resourcetest"
+	pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
+	"github.com/hashicorp/consul/proto-public/pbresource"
+)
+
+// WorkloadSelecting denotes a resource type that uses workload selectors.
+type WorkloadSelecting interface {
+	proto.Message
+	GetWorkloads() *pbcatalog.WorkloadSelector
+}
+
+func RunWorkloadSelectingTypeACLsTests[T WorkloadSelecting](t *testing.T, typ *pbresource.Type,
+	getData func(selector *pbcatalog.WorkloadSelector) T,
+	registerFunc func(registry resource.Registry),
+) {
+	// Wire up a registry to generically invoke hooks
+	registry := resource.NewRegistry()
+	registerFunc(registry)
+
+	cases := map[string]resourcetest.ACLTestCase{
+		"no rules": {
+			Rules:   ``,
+			Data:    getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.DENY,
+			WriteOK: resourcetest.DENY,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test read": {
+			Rules:   `service "test" { policy = "read" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.DENY,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with named selectors and insufficient policy": {
+			Rules:   `service "test" { policy = "write" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.DENY,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with prefixed selectors and insufficient policy": {
+			Rules:   `service "test" { policy = "write" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.DENY,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with named selectors": {
+			Rules:   `service "test" { policy = "write" } service "workload" { policy = "read" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.ALLOW,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with prefixed selectors": {
+			Rules:   `service "test" { policy = "write" } service_prefix "workload-" { policy = "read" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload-"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.ALLOW,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with prefixed selectors and a policy with more specific than the selector": {
+			Rules:   `service "test" { policy = "write" } service_prefix "workload-" { policy = "read" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"wor"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.DENY,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with prefixed selectors and a policy with less specific than the selector": {
+			Rules:   `service "test" { policy = "write" } service_prefix "wor" { policy = "read" }`,
+			Data:    getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload-"}}),
+			Typ:     typ,
+			ReadOK:  resourcetest.ALLOW,
+			WriteOK: resourcetest.ALLOW,
+			ListOK:  resourcetest.DEFAULT,
+		},
+		"service test write with prefixed selectors and a policy with a specific service": {
+			Rules:  `service "test" { policy = "write" } service "workload" { policy = "read" }`,
+			Data:   getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload"}}),
+			Typ:    typ,
+			ReadOK: resourcetest.ALLOW,
+			// TODO (ishustava): this is wrong and should be fixed in a follow up PR. We should not allow
+			// a policy for a specific service when only prefixes are specified in the selector.
+			WriteOK: resourcetest.ALLOW,
+			ListOK:  resourcetest.DEFAULT,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			resourcetest.RunACLTestCase(t, tc, registry)
+		})
+	}
+}
diff --git a/internal/catalog/internal/types/acl_hooks.go b/internal/catalog/internal/types/acl_hooks.go
@@ -0,0 +1,56 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package types
+
+import (
+	"github.com/hashicorp/consul/acl"
+	"github.com/hashicorp/consul/internal/resource"
+	"github.com/hashicorp/consul/proto-public/pbresource"
+)
+
+func aclReadHookResourceWithWorkloadSelector(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, id *pbresource.ID, _ *pbresource.Resource) error {
+	return authorizer.ToAllowAuthorizer().ServiceReadAllowed(id.GetName(), authzContext)
+}
+
+func aclWriteHookResourceWithWorkloadSelector[T WorkloadSelecting](authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *pbresource.Resource) error {
+	if res == nil {
+		return resource.ErrNeedResource
+	}
+
+	decodedService, err := resource.Decode[T](res)
+	if err != nil {
+		return resource.ErrNeedResource
+	}
+
+	// First check service:write on the name.
+	err = authorizer.ToAllowAuthorizer().ServiceWriteAllowed(res.GetId().GetName(), authzContext)
+	if err != nil {
+		return err
+	}
+
+	// Then also check whether we're allowed to select a service.
+	for _, name := range decodedService.GetData().GetWorkloads().GetNames() {
+		err = authorizer.ToAllowAuthorizer().ServiceReadAllowed(name, authzContext)
+		if err != nil {
+			return err
+		}
+	}
+
+	for _, prefix := range decodedService.GetData().GetWorkloads().GetPrefixes() {
+		err = authorizer.ToAllowAuthorizer().ServiceReadAllowed(prefix, authzContext)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func ACLHooksForWorkloadSelectingType[T WorkloadSelecting]() *resource.ACLHooks {
+	return &resource.ACLHooks{
+		Read:  aclReadHookResourceWithWorkloadSelector,
+		Write: aclWriteHookResourceWithWorkloadSelector[T],
+		List:  resource.NoOpACLListHook,
+	}
+}
diff --git a/internal/catalog/internal/types/dns_policy.go b/internal/catalog/internal/types/dns_policy.go
@@ -19,6 +19,7 @@ func RegisterDNSPolicy(r resource.Registry) {
 		Proto:    &pbcatalog.DNSPolicy{},
 		Scope:    resource.ScopeNamespace,
 		Validate: ValidateDNSPolicy,
+		ACLs:     ACLHooksForWorkloadSelectingType[*pbcatalog.DNSPolicy](),
 	})
 }
 

diff --git a/internal/catalog/internal/types/dns_policy_test.go b/internal/catalog/internal/types/dns_policy_test.go
@@ -11,6 +11,7 @@ import (
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/types/known/anypb"
 
+	"github.com/hashicorp/consul/internal/catalog/internal/testhelpers"
 	"github.com/hashicorp/consul/internal/resource"
 	pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
 	"github.com/hashicorp/consul/proto-public/pbresource"
@@ -161,3 +162,19 @@ func TestValidateDNSPolicy_EmptySelector(t *testing.T) {
 	require.ErrorAs(t, err, &actual)
 	require.Equal(t, expected, actual)
 }
+
+func TestDNSPolicyACLs(t *testing.T) {
+	// Wire up a registry to generically invoke hooks
+	registry := resource.NewRegistry()
+	RegisterDNSPolicy(registry)
+
+	testhelpers.RunWorkloadSelectingTypeACLsTests[*pbcatalog.DNSPolicy](t, pbcatalog.DNSPolicyType,
+		func(selector *pbcatalog.WorkloadSelector) *pbcatalog.DNSPolicy {
+			return &pbcatalog.DNSPolicy{
+				Workloads: selector,
+				Weights:   &pbcatalog.Weights{Passing: 1, Warning: 0},
+			}
+		},
+		RegisterDNSPolicy,
+	)
+}
diff --git a/internal/catalog/internal/types/failover_policy.go b/internal/catalog/internal/types/failover_policy.go
@@ -25,7 +25,7 @@ func RegisterFailoverPolicy(r resource.Registry) {
 		ACLs: &resource.ACLHooks{
 			Read:  aclReadHookFailoverPolicy,
 			Write: aclWriteHookFailoverPolicy,
-			List:  aclListHookFailoverPolicy,
+			List:  resource.NoOpACLListHook,
 		},
 	})
 }
@@ -371,9 +371,3 @@ func aclWriteHookFailoverPolicy(authorizer acl.Authorizer, authzContext *acl.Aut
 	return nil
 
 }
-
-func aclListHookFailoverPolicy(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext) error {
-	// No-op List permission as we want to default to filtering resources
-	// from the list using the Read enforcement.
-	return nil
-}