Enforce ACLs on resource Write and Delete endpoints (#16956)

agent/grpc-external/services/resource/delete.go

@@ -7,6 +7,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/internal/storage"
 	"github.com/hashicorp/consul/proto-public/pbresource"
 )
@@ -18,14 +19,23 @@ import (
 // Deletes of previously deleted or non-existent resource are no-ops.
 // Returns an Aborted error if the requested Version does not match the stored Version.
 func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pbresource.DeleteResponse, error) {
-	// check type registered
 	reg, err := s.resolveType(req.Id.Type)
 	if err != nil {
 		return nil, err
 	}
 
-	// TODO(spatel): reg will be used for ACL hooks
+	authz, err := s.getAuthorizer(tokenFromContext(ctx))
-	_ = reg
+	if err != nil {
+		return nil, err
+	}
+
+	err = reg.ACLs.Write(authz, req.Id)
+	switch {
+	case acl.IsErrPermissionDenied(err):
+		return nil, status.Error(codes.PermissionDenied, err.Error())
+	case err != nil:
+		return nil, status.Errorf(codes.Internal, "failed write acl: %v", err)
+	}
 
 	versionToDelete := req.Version
 	if versionToDelete == "" {

agent/grpc-external/services/resource/delete_test.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"testing"
 
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
+	"github.com/hashicorp/consul/acl/resolver"
 	"github.com/hashicorp/consul/internal/resource/demo"
 	"github.com/hashicorp/consul/internal/storage"
 	"github.com/hashicorp/consul/proto-public/pbresource"
@@ -26,6 +28,51 @@ func TestDelete_TypeNotRegistered(t *testing.T) {
 	require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
 }
 
+func TestDelete_ACLs(t *testing.T) {
+	type testCase struct {
+		authz       resolver.Result
+		assertErrFn func(error)
+	}
+	testcases := map[string]testCase{
+		"delete denied": {
+			authz: AuthorizerFrom(t, demo.ArtistV1WritePolicy),
+			assertErrFn: func(err error) {
+				require.Error(t, err)
+				require.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+			},
+		},
+		"delete allowed": {
+			authz: AuthorizerFrom(t, demo.ArtistV2WritePolicy),
+			assertErrFn: func(err error) {
+				require.NoError(t, err)
+			},
+		},
+	}
+
+	for desc, tc := range testcases {
+		t.Run(desc, func(t *testing.T) {
+			server := testServer(t)
+			client := testClient(t, server)
+
+			mockACLResolver := &MockACLResolver{}
+			mockACLResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
+				Return(tc.authz, nil)
+			server.ACLResolver = mockACLResolver
+			demo.Register(server.Registry)
+
+			artist, err := demo.GenerateV2Artist()
+			require.NoError(t, err)
+
+			artist, err = server.Backend.WriteCAS(context.Background(), artist)
+			require.NoError(t, err)
+
+			// exercise ACL
+			_, err = client.Delete(testContext(t), &pbresource.DeleteRequest{Id: artist.Id})
+			tc.assertErrFn(err)
+		})
+	}
+}
+
 func TestDelete_Success(t *testing.T) {
 	t.Parallel()
 

agent/grpc-external/services/resource/write.go

@@ -10,6 +10,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/internal/storage"
 	"github.com/hashicorp/consul/lib/retry"
@@ -42,6 +43,20 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 		return nil, err
 	}
 
+	authz, err := s.getAuthorizer(tokenFromContext(ctx))
+	if err != nil {
+		return nil, err
+	}
+
+	// check acls
+	err = reg.ACLs.Write(authz, req.Resource.Id)
+	switch {
+	case acl.IsErrPermissionDenied(err):
+		return nil, status.Error(codes.PermissionDenied, err.Error())
+	case err != nil:
+		return nil, status.Errorf(codes.Internal, "failed write acl: %v", err)
+	}
+
 	// Check the user sent the correct type of data.
 	if !req.Resource.Data.MessageIs(reg.Proto) {
 		got := strings.TrimPrefix(req.Resource.Data.TypeUrl, "type.googleapis.com/")

agent/grpc-external/services/resource/write_test.go

@@ -6,11 +6,13 @@ import (
 	"testing"
 
 	"github.com/oklog/ulid/v2"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/anypb"
 
+	"github.com/hashicorp/consul/acl/resolver"
 	"github.com/hashicorp/consul/internal/resource/demo"
 	"github.com/hashicorp/consul/internal/storage"
 	"github.com/hashicorp/consul/proto-public/pbresource"
@@ -70,6 +72,48 @@ func TestWrite_TypeNotFound(t *testing.T) {
 	require.Contains(t, err.Error(), "resource type demo.v2.artist not registered")
 }
 
+func TestWrite_ACLs(t *testing.T) {
+	type testCase struct {
+		authz       resolver.Result
+		assertErrFn func(error)
+	}
+	testcases := map[string]testCase{
+		"write denied": {
+			authz: AuthorizerFrom(t, demo.ArtistV1WritePolicy),
+			assertErrFn: func(err error) {
+				require.Error(t, err)
+				require.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
+			},
+		},
+		"write allowed": {
+			authz: AuthorizerFrom(t, demo.ArtistV2WritePolicy),
+			assertErrFn: func(err error) {
+				require.NoError(t, err)
+			},
+		},
+	}
+
+	for desc, tc := range testcases {
+		t.Run(desc, func(t *testing.T) {
+			server := testServer(t)
+			client := testClient(t, server)
+
+			mockACLResolver := &MockACLResolver{}
+			mockACLResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
+				Return(tc.authz, nil)
+			server.ACLResolver = mockACLResolver
+			demo.Register(server.Registry)
+
+			artist, err := demo.GenerateV2Artist()
+			require.NoError(t, err)
+
+			// exercise ACL
+			_, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: artist})
+			tc.assertErrFn(err)
+		})
+	}
+}
+
 func TestWrite_ResourceCreation_Success(t *testing.T) {
 	server := testServer(t)
 	client := testClient(t, server)

internal/resource/demo/demo.go

@@ -74,8 +74,8 @@ func Register(r resource.Registry) {
 		return authz.ToAllowAuthorizer().KeyReadAllowed(key, &acl.AuthorizerContext{})
 	}
 
-	writeACL := func(authz acl.Authorizer, res *pbresource.Resource) error {
+	writeACL := func(authz acl.Authorizer, id *pbresource.ID) error {
-		key := fmt.Sprintf("resource/%s/%s", resource.ToGVK(res.Id.Type), res.Id.Name)
+		key := fmt.Sprintf("resource/%s/%s", resource.ToGVK(id.Type), id.Name)
 		return authz.ToAllowAuthorizer().KeyWriteAllowed(key, &acl.AuthorizerContext{})
 	}
 

internal/resource/registry.go

@@ -49,7 +49,7 @@ type ACLHooks struct {
 	// Write is used to authorize Write and Delete RPCs.
 	//
 	// If it is omitted, `operator:write` permission is assumed.
-	Write func(acl.Authorizer, *pbresource.Resource) error
+	Write func(acl.Authorizer, *pbresource.ID) error
 
 	// List is used to authorize List RPCs.
 	//
@@ -94,7 +94,7 @@ func (r *TypeRegistry) Register(registration Registration) {
 		}
 	}
 	if registration.ACLs.Write == nil {
-		registration.ACLs.Write = func(authz acl.Authorizer, resource *pbresource.Resource) error {
+		registration.ACLs.Write = func(authz acl.Authorizer, id *pbresource.ID) error {
 			return authz.ToAllowAuthorizer().OperatorWriteAllowed(&acl.AuthorizerContext{})
 		}
 	}

internal/resource/registry_test.go

@@ -76,8 +76,8 @@ func TestRegister_Defaults(t *testing.T) {
 	require.True(t, acl.IsErrPermissionDenied(reg.ACLs.Read(testutils.ACLNoPermissions(t), artist.Id)))
 
 	// verify default write hook requires operator:write
-	require.NoError(t, reg.ACLs.Write(testutils.ACLOperatorWrite(t), artist))
+	require.NoError(t, reg.ACLs.Write(testutils.ACLOperatorWrite(t), artist.Id))
-	require.True(t, acl.IsErrPermissionDenied(reg.ACLs.Write(testutils.ACLNoPermissions(t), artist)))
+	require.True(t, acl.IsErrPermissionDenied(reg.ACLs.Write(testutils.ACLNoPermissions(t), artist.Id)))
 
 	// verify default list hook requires operator:read
 	require.NoError(t, reg.ACLs.List(testutils.ACLOperatorRead(t), artist.Id.Tenancy))