From 067a0112e2ab618b1a3477a6ea7924c6909870b7 Mon Sep 17 00:00:00 2001 From: Semir Patel Date: Thu, 24 Aug 2023 10:49:46 -0500 Subject: [PATCH] resource: Make resource listbyowner tenancy aware (#18566) --- .../services/resource/list_by_owner.go | 89 +++++++--- .../services/resource/list_by_owner_test.go | 155 +++++++++++++++--- internal/resource/decode_test.go | 6 +- internal/resource/demo/demo.go | 7 - internal/resource/http/http_test.go | 8 +- 5 files changed, 209 insertions(+), 56 deletions(-) diff --git a/agent/grpc-external/services/resource/list_by_owner.go b/agent/grpc-external/services/resource/list_by_owner.go index 8f1b4216a3..3c0ccbda65 100644 --- a/agent/grpc-external/services/resource/list_by_owner.go +++ b/agent/grpc-external/services/resource/list_by_owner.go @@ -10,39 +10,71 @@ import ( "google.golang.org/grpc/status" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/proto-public/pbresource" ) func (s *Server) ListByOwner(ctx context.Context, req *pbresource.ListByOwnerRequest) (*pbresource.ListByOwnerResponse, error) { - if err := validateListByOwnerRequest(req); err != nil { - return nil, err - } - - _, err := s.resolveType(req.Owner.Type) + reg, err := s.validateListByOwnerRequest(req) if err != nil { return nil, err } + // Convert v2 request tenancy to v1 for ACL subsystem. + entMeta := v2TenancyToV1EntMeta(req.Owner.Tenancy) + token := tokenFromContext(ctx) + + // Fill entMeta with token tenancy when empty. + authz, authzContext, err := s.getAuthorizer(token, entMeta) + if err != nil { + return nil, err + } + + // Handle defaulting empty tenancy units from request. + v1EntMetaToV2Tenancy(reg, entMeta, req.Owner.Tenancy) + + // Check list ACL before verifying tenancy exists to not leak tenancy existence. + err = reg.ACLs.List(authz, authzContext) + switch { + case acl.IsErrPermissionDenied(err): + return nil, status.Error(codes.PermissionDenied, err.Error()) + case err != nil: + return nil, status.Errorf(codes.Internal, "failed list acl: %v", err) + } + + // Check v1 tenancy exists for the v2 resource. + if err = v1TenancyExists(reg, s.V1TenancyBridge, req.Owner.Tenancy, codes.InvalidArgument); err != nil { + return nil, err + } + + // Get owned resources. children, err := s.Backend.ListByOwner(ctx, req.Owner) if err != nil { return nil, status.Errorf(codes.Internal, "failed list by owner: %v", err) } - // TODO(spatel): Refactor _ and entMeta in NET-4917 - authz, authzContext, err := s.getAuthorizer(tokenFromContext(ctx), acl.DefaultEnterpriseMeta()) - if err != nil { - return nil, err - } - result := make([]*pbresource.Resource, 0) for _, child := range children { - reg, err := s.resolveType(child.Id.Type) + // Retrieve child type's registration to access read ACL hook. + childReg, err := s.resolveType(child.Id.Type) if err != nil { return nil, err } - // ACL filter - err = reg.ACLs.Read(authz, authzContext, child.Id) + // Rebuild authorizer if tenancy not identical between owner and child (child scope + // may be narrower). + childAuthz := authz + childAuthzContext := authzContext + if !resource.EqualTenancy(req.Owner.Tenancy, child.Id.Tenancy) { + childEntMeta := v2TenancyToV1EntMeta(child.Id.Tenancy) + childAuthz, childAuthzContext, err = s.getAuthorizer(token, childEntMeta) + if err != nil { + return nil, err + } + } + + // Filter out children that fail real ACL. + err = childReg.ACLs.Read(childAuthz, childAuthzContext, child.Id) switch { case acl.IsErrPermissionDenied(err): continue @@ -55,17 +87,36 @@ func (s *Server) ListByOwner(ctx context.Context, req *pbresource.ListByOwnerReq return &pbresource.ListByOwnerResponse{Resources: result}, nil } -func validateListByOwnerRequest(req *pbresource.ListByOwnerRequest) error { +func (s *Server) validateListByOwnerRequest(req *pbresource.ListByOwnerRequest) (*resource.Registration, error) { if req.Owner == nil { - return status.Errorf(codes.InvalidArgument, "owner is required") + return nil, status.Errorf(codes.InvalidArgument, "owner is required") } if err := validateId(req.Owner, "owner"); err != nil { - return err + return nil, err } if req.Owner.Uid == "" { - return status.Errorf(codes.InvalidArgument, "owner uid is required") + return nil, status.Errorf(codes.InvalidArgument, "owner uid is required") } - return nil + + reg, err := s.resolveType(req.Owner.Type) + if err != nil { + return nil, err + } + + // Lowercase + resource.Normalize(req.Owner.Tenancy) + + // Error when partition scoped and namespace not empty. + if reg.Scope == resource.ScopePartition && req.Owner.Tenancy.Namespace != "" { + return nil, status.Errorf( + codes.InvalidArgument, + "partition scoped type %s cannot have a namespace. got: %s", + resource.ToGVK(req.Owner.Type), + req.Owner.Tenancy.Namespace, + ) + } + + return reg, nil } diff --git a/agent/grpc-external/services/resource/list_by_owner_test.go b/agent/grpc-external/services/resource/list_by_owner_test.go index 5b2fe63975..4a60e3ac94 100644 --- a/agent/grpc-external/services/resource/list_by_owner_test.go +++ b/agent/grpc-external/services/resource/list_by_owner_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/demo" "github.com/hashicorp/consul/proto-public/pbresource" "github.com/hashicorp/consul/proto/private/prototest" @@ -17,41 +18,49 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" ) func TestListByOwner_InputValidation(t *testing.T) { server := testServer(t) client := testClient(t, server) - demo.RegisterTypes(server.Registry) - testCases := map[string]func(*pbresource.ListByOwnerRequest){ - "no owner": func(req *pbresource.ListByOwnerRequest) { req.Owner = nil }, - "no type": func(req *pbresource.ListByOwnerRequest) { req.Owner.Type = nil }, - "no tenancy": func(req *pbresource.ListByOwnerRequest) { req.Owner.Tenancy = nil }, - "no name": func(req *pbresource.ListByOwnerRequest) { req.Owner.Name = "" }, - "no uid": func(req *pbresource.ListByOwnerRequest) { req.Owner.Uid = "" }, - // clone necessary to not pollute DefaultTenancy - "tenancy partition not default": func(req *pbresource.ListByOwnerRequest) { - req.Owner.Tenancy = clone(req.Owner.Tenancy) - req.Owner.Tenancy.Partition = "" + testCases := map[string]func(artistId, recordlabelId *pbresource.ID) *pbresource.ID{ + "no owner": func(artistId, recordLabelId *pbresource.ID) *pbresource.ID { + return nil }, - "tenancy namespace not default": func(req *pbresource.ListByOwnerRequest) { - req.Owner.Tenancy = clone(req.Owner.Tenancy) - req.Owner.Tenancy.Namespace = "" + "no type": func(artistId, _ *pbresource.ID) *pbresource.ID { + artistId.Type = nil + return artistId }, - "tenancy peername not local": func(req *pbresource.ListByOwnerRequest) { - req.Owner.Tenancy = clone(req.Owner.Tenancy) - req.Owner.Tenancy.PeerName = "" + "no tenancy": func(artistId, _ *pbresource.ID) *pbresource.ID { + artistId.Tenancy = nil + return artistId + }, + "no name": func(artistId, _ *pbresource.ID) *pbresource.ID { + artistId.Name = "" + return artistId + }, + "no uid": func(artistId, _ *pbresource.ID) *pbresource.ID { + artistId.Uid = "" + return artistId + }, + "partition scope with non-empty namespace": func(_, recordLabelId *pbresource.ID) *pbresource.ID { + recordLabelId.Tenancy.Namespace = "ishouldnothaveanamespace" + return recordLabelId }, } for desc, modFn := range testCases { t.Run(desc, func(t *testing.T) { - res, err := demo.GenerateV2Artist() + artist, err := demo.GenerateV2Artist() require.NoError(t, err) - req := &pbresource.ListByOwnerRequest{Owner: res.Id} - modFn(req) + recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes") + require.NoError(t, err) + + // Each test case picks which resource to use based on the resource type's scope. + req := &pbresource.ListByOwnerRequest{Owner: modFn(artist.Id, recordLabel.Id)} _, err = client.ListByOwner(testContext(t), req) require.Error(t, err) @@ -67,7 +76,7 @@ func TestListByOwner_TypeNotRegistered(t *testing.T) { _, err := client.ListByOwner(context.Background(), &pbresource.ListByOwnerRequest{ Owner: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Uid: "bogus", Name: "bogus", }, @@ -125,8 +134,108 @@ func TestListByOwner_Many(t *testing.T) { prototest.AssertElementsMatch(t, albums, rsp3.Resources) } +func TestListByOwner_OwnerTenancyDoesNotExist(t *testing.T) { + tenancyCases := map[string]func(artistId, recordlabelId *pbresource.ID) *pbresource.ID{ + "partition not found when namespace scoped": func(artistId, _ *pbresource.ID) *pbresource.ID { + id := clone(artistId) + id.Uid = "doesnotmatter" + id.Tenancy.Partition = "boguspartition" + return id + }, + "namespace not found when namespace scoped": func(artistId, _ *pbresource.ID) *pbresource.ID { + id := clone(artistId) + id.Uid = "doesnotmatter" + id.Tenancy.Namespace = "bogusnamespace" + return id + }, + "partition not found when partition scoped": func(_, recordLabelId *pbresource.ID) *pbresource.ID { + id := clone(recordLabelId) + id.Uid = "doesnotmatter" + id.Tenancy.Partition = "boguspartition" + return id + }, + } + for desc, modFn := range tenancyCases { + t.Run(desc, func(t *testing.T) { + server := testServer(t) + demo.RegisterTypes(server.Registry) + client := testClient(t, server) + + recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes") + require.NoError(t, err) + recordLabel, err = server.Backend.WriteCAS(testContext(t), recordLabel) + require.NoError(t, err) + + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + artist, err = server.Backend.WriteCAS(testContext(t), artist) + require.NoError(t, err) + + // Verify non-existant tenancy units in owner err with not found. + _, err = client.ListByOwner(testContext(t), &pbresource.ListByOwnerRequest{Owner: modFn(artist.Id, recordLabel.Id)}) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) + require.Contains(t, err.Error(), "resource not found") + }) + } +} + +func TestListByOwner_Tenancy_Defaults_And_Normalization(t *testing.T) { + for tenancyDesc, modFn := range tenancyCases() { + t.Run(tenancyDesc, func(t *testing.T) { + server := testServer(t) + demo.RegisterTypes(server.Registry) + client := testClient(t, server) + + // Create partition scoped recordLabel. + recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes") + require.NoError(t, err) + rsp1, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: recordLabel}) + require.NoError(t, err) + recordLabel = rsp1.Resource + + // Create namespace scoped artist. + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + rsp2, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: artist}) + require.NoError(t, err) + artist = rsp2.Resource + + // Owner will be either partition scoped (recordLabel) or namespace scoped (artist) based on testcase. + moddedOwnerId := modFn(artist.Id, recordLabel.Id) + var ownerId *pbresource.ID + + // Avoid using the modded id when linking owner to child. + switch { + case proto.Equal(moddedOwnerId.Type, demo.TypeV2Artist): + ownerId = artist.Id + case proto.Equal(moddedOwnerId.Type, demo.TypeV1RecordLabel): + ownerId = recordLabel.Id + default: + require.Fail(t, "unexpected resource type") + } + + // Link owner to child. + album, err := demo.GenerateV2Album(ownerId) + require.NoError(t, err) + rsp3, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: album}) + require.NoError(t, err) + album = rsp3.Resource + + // Test + listRsp, err := client.ListByOwner(testContext(t), &pbresource.ListByOwnerRequest{ + Owner: moddedOwnerId, + }) + require.NoError(t, err) + + // Verify child album always returned. + prototest.AssertDeepEqual(t, album, listRsp.Resources[0]) + }) + } +} + func TestListByOwner_ACL_PerTypeDenied(t *testing.T) { - authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "deny" }`) + authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "deny" }`, demo.ArtistV2ListPolicy) _, rsp, err := roundTripListByOwner(t, authz) // verify resource filtered out, hence no results @@ -135,7 +244,7 @@ func TestListByOwner_ACL_PerTypeDenied(t *testing.T) { } func TestListByOwner_ACL_PerTypeAllowed(t *testing.T) { - authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "read" }`) + authz := AuthorizerFrom(t, `key_prefix "resource/demo.v2.Album/" { policy = "read" }`, demo.ArtistV2ListPolicy) album, rsp, err := roundTripListByOwner(t, authz) // verify resource not filtered out diff --git a/internal/resource/decode_test.go b/internal/resource/decode_test.go index 17c1bd7f1b..a516673e43 100644 --- a/internal/resource/decode_test.go +++ b/internal/resource/decode_test.go @@ -29,7 +29,7 @@ func TestGetDecodedResource(t *testing.T) { babypantsID := &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "babypants", } @@ -75,7 +75,7 @@ func TestDecode(t *testing.T) { foo := &pbresource.Resource{ Id: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "babypants", }, Data: any, @@ -95,7 +95,7 @@ func TestDecode(t *testing.T) { foo := &pbresource.Resource{ Id: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "babypants", }, Data: &anypb.Any{ diff --git a/internal/resource/demo/demo.go b/internal/resource/demo/demo.go index c47d7e31e6..0a8f2e4400 100644 --- a/internal/resource/demo/demo.go +++ b/internal/resource/demo/demo.go @@ -22,13 +22,6 @@ import ( ) var ( - // TenancyDefault contains the default values for all tenancy units. - TenancyDefault = &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - PeerName: "local", - Namespace: resource.DefaultNamespaceName, - } - // TypeV1RecordLabel represents a record label which artists are signed to. // Used specifically as a resource to test partition only scoped resources. TypeV1RecordLabel = &pbresource.Type{ diff --git a/internal/resource/http/http_test.go b/internal/resource/http/http_test.go index 1edef161fb..93c458ecbf 100644 --- a/internal/resource/http/http_test.go +++ b/internal/resource/http/http_test.go @@ -165,7 +165,7 @@ func TestResourceWriteHandler(t *testing.T) { readRsp, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{ Id: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "keith-urban", }, }) @@ -264,7 +264,7 @@ func TestResourceWriteHandler(t *testing.T) { readRsp, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{ Id: &pbresource.ID{ Type: demo.TypeV1Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "keith-urban-v1", }, }) @@ -430,7 +430,7 @@ func TestResourceDeleteHandler(t *testing.T) { _, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{ Id: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "keith-urban", }, }) @@ -453,7 +453,7 @@ func TestResourceDeleteHandler(t *testing.T) { _, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{ Id: &pbresource.ID{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), Name: "keith-urban", }, })