diff --git a/agent/grpc-external/services/resource/server.go b/agent/grpc-external/services/resource/server.go index ce5b00444c..921cb62b17 100644 --- a/agent/grpc-external/services/resource/server.go +++ b/agent/grpc-external/services/resource/server.go @@ -108,4 +108,12 @@ func (s *Server) getAuthorizer(token string) (acl.Authorizer, error) { return authz, nil } +func isGRPCStatusError(err error) bool { + if err == nil { + return false + } + _, ok := status.FromError(err) + return ok +} + func clone[T proto.Message](v T) T { return proto.Clone(v).(T) } diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go index 57380f8f6b..65455adf4d 100644 --- a/agent/grpc-external/services/resource/write.go +++ b/agent/grpc-external/services/resource/write.go @@ -9,6 +9,7 @@ import ( "github.com/oklog/ulid/v2" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/internal/resource" @@ -110,10 +111,16 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre case errors.Is(err, storage.ErrNotFound): input.Id.Uid = ulid.Make().String() + // Prevent setting statuses in this endpoint. if len(input.Status) != 0 { return errUseWriteStatus } + // Enforce same tenancy for owner + if input.Owner != nil && !proto.Equal(input.Id.Tenancy, input.Owner.Tenancy) { + return status.Errorf(codes.InvalidArgument, "owner and resource tenancy must be the same") + } + // Update path. case err == nil: // Use the stored ID because it includes the Uid. @@ -143,6 +150,12 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre return storage.ErrCASFailure } + // Owner can only be set on creation. Enforce immutability. + if !proto.Equal(input.Owner, existing.Owner) { + return status.Errorf(codes.InvalidArgument, "owner cannot be changed") + } + + // Carry over status and prevent updates if input.Status == nil { input.Status = existing.Status } else if !resource.EqualStatus(input.Status, existing.Status) { @@ -163,13 +176,11 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre return nil, status.Error(codes.Aborted, err.Error()) case errors.Is(err, storage.ErrWrongUid): return nil, status.Error(codes.FailedPrecondition, err.Error()) - case err != nil: - if _, ok := status.FromError(err); !ok { - err = status.Errorf(codes.Internal, "failed to write resource: %v", err.Error()) - } + case isGRPCStatusError(err): return nil, err + case err != nil: + return nil, status.Errorf(codes.Internal, "failed to write resource: %v", err.Error()) } - return &pbresource.WriteResponse{Resource: result}, nil } diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index fa576da5d1..d1d561be9b 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/agent/grpc-external/services/resource/write_test.go @@ -379,6 +379,64 @@ func TestWrite_NonCASUpdate_Retry(t *testing.T) { require.NoError(t, <-errCh) } +func TestWrite_Owner_Immutable(t *testing.T) { + // Use of proto.Equal(..) in implementation covers all permutations + // (nil -> non-nil, non-nil -> nil, owner1 -> owner2) so only the first one + // is tested. + server := testServer(t) + client := testClient(t, server) + + demo.Register(server.Registry) + + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + rsp1, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: artist}) + require.NoError(t, err) + artist = rsp1.Resource + + // create album with no owner + album, err := demo.GenerateV2Album(rsp1.Resource.Id) + require.NoError(t, err) + album.Owner = nil + rsp2, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: album}) + require.NoError(t, err) + + // setting owner on update should fail + album = rsp2.Resource + album.Owner = artist.Id + _, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: album}) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) + require.ErrorContains(t, err, "owner cannot be changed") +} + +func TestWrite_Owner_RequireSameTenancy(t *testing.T) { + server := testServer(t) + client := testClient(t, server) + + demo.Register(server.Registry) + + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + rsp1, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: artist}) + require.NoError(t, err) + + // change album tenancy to be different from artist tenancy + album, err := demo.GenerateV2Album(rsp1.Resource.Id) + require.NoError(t, err) + album.Owner.Tenancy = &pbresource.Tenancy{ + Partition: "some", + Namespace: "other", + PeerName: "tenancy", + } + + // verify create fails + _, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: album}) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) + require.ErrorContains(t, err, "tenancy must be the same") +} + type blockOnceBackend struct { storage.Backend