From 53a0755f032c159de22baf9ae4347613e98536a3 Mon Sep 17 00:00:00 2001 From: Semir Patel Date: Wed, 12 Apr 2023 16:22:44 -0500 Subject: [PATCH] Enforce ACLs on resource `Write` and `Delete` endpoints (#16956) --- .../grpc-external/services/resource/delete.go | 16 +++++-- .../services/resource/delete_test.go | 47 +++++++++++++++++++ .../grpc-external/services/resource/write.go | 15 ++++++ .../services/resource/write_test.go | 44 +++++++++++++++++ internal/resource/demo/demo.go | 4 +- internal/resource/registry.go | 4 +- internal/resource/registry_test.go | 4 +- 7 files changed, 125 insertions(+), 9 deletions(-) diff --git a/agent/grpc-external/services/resource/delete.go b/agent/grpc-external/services/resource/delete.go index 5cc4a9573..542469d42 100644 --- a/agent/grpc-external/services/resource/delete.go +++ b/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 - _ = reg + authz, err := s.getAuthorizer(tokenFromContext(ctx)) + 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 == "" { diff --git a/agent/grpc-external/services/resource/delete_test.go b/agent/grpc-external/services/resource/delete_test.go index 2cbc7a8df..6d6dec75b 100644 --- a/agent/grpc-external/services/resource/delete_test.go +++ b/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() diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go index f7c5c0378..a836b3e39 100644 --- a/agent/grpc-external/services/resource/write.go +++ b/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/") diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index 75c65f1a9..4e83cb8bb 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/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) diff --git a/internal/resource/demo/demo.go b/internal/resource/demo/demo.go index 88c749fa1..b9f8ab68d 100644 --- a/internal/resource/demo/demo.go +++ b/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 { - key := fmt.Sprintf("resource/%s/%s", resource.ToGVK(res.Id.Type), res.Id.Name) + writeACL := func(authz acl.Authorizer, id *pbresource.ID) error { + key := fmt.Sprintf("resource/%s/%s", resource.ToGVK(id.Type), id.Name) return authz.ToAllowAuthorizer().KeyWriteAllowed(key, &acl.AuthorizerContext{}) } diff --git a/internal/resource/registry.go b/internal/resource/registry.go index c8b89855f..6b49b2313 100644 --- a/internal/resource/registry.go +++ b/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{}) } } diff --git a/internal/resource/registry_test.go b/internal/resource/registry_test.go index d28ffcc78..7360e48d1 100644 --- a/internal/resource/registry_test.go +++ b/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.True(t, acl.IsErrPermissionDenied(reg.ACLs.Write(testutils.ACLNoPermissions(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.Id))) // verify default list hook requires operator:read require.NoError(t, reg.ACLs.List(testutils.ACLOperatorRead(t), artist.Id.Tenancy))