From 584b6563d495faf80ce40cb499d201559de178a3 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 25 Sep 2023 16:51:53 -0500 Subject: [PATCH 1/2] mesh: add xRoute ACL hook tenancy tests --- internal/mesh/internal/types/xroute_test.go | 196 +++++++++++++------- 1 file changed, 126 insertions(+), 70 deletions(-) diff --git a/internal/mesh/internal/types/xroute_test.go b/internal/mesh/internal/types/xroute_test.go index cd6d6d766327..fcbd61dd3ad7 100644 --- a/internal/mesh/internal/types/xroute_test.go +++ b/internal/mesh/internal/types/xroute_test.go @@ -114,6 +114,7 @@ func getXRouteParentRefTestCases() map[string]xRouteParentRefTestcase { Port: port, } } + return map[string]xRouteParentRefTestcase{ "no parent refs": { routeTenancy: resource.DefaultNamespacedTenancy(), @@ -372,7 +373,10 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare userNewRoute := newRoute newRoute = func(t *testing.T, parentRefs, backendRefs []*pbresource.Reference) *pbresource.Resource { + require.NotEmpty(t, parentRefs) + require.NotEmpty(t, backendRefs) res := userNewRoute(t, parentRefs, backendRefs) + res.Id.Tenancy = parentRefs[0].Tenancy resourcetest.ValidateAndNormalize(t, registry, res) return res } @@ -408,42 +412,54 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare } } - resOneParentOneBackend := newRoute(t, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "api1"), - }, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "backend1"), - }, - ) - resTwoParentsOneBackend := newRoute(t, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "api1"), - newRef(pbcatalog.ServiceType, "api2"), - }, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "backend1"), - }, - ) - resOneParentTwoBackends := newRoute(t, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "api1"), - }, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "backend1"), - newRef(pbcatalog.ServiceType, "backend2"), - }, - ) - resTwoParentsTwoBackends := newRoute(t, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "api1"), - newRef(pbcatalog.ServiceType, "api2"), - }, - []*pbresource.Reference{ - newRef(pbcatalog.ServiceType, "backend1"), - newRef(pbcatalog.ServiceType, "backend2"), - }, - ) + serviceRef := func(tenancy, name string) *pbresource.Reference { + return newRefWithTenancy(pbcatalog.ServiceType, tenancy, name) + } + + resOneParentOneBackend := func(parentTenancy, backendTenancy string) *pbresource.Resource { + return newRoute(t, + []*pbresource.Reference{ + serviceRef(parentTenancy, "api1"), + }, + []*pbresource.Reference{ + serviceRef(backendTenancy, "backend1"), + }, + ) + } + resTwoParentsOneBackend := func(parentTenancy, backendTenancy string) *pbresource.Resource { + return newRoute(t, + []*pbresource.Reference{ + serviceRef(parentTenancy, "api1"), + serviceRef(parentTenancy, "api2"), + }, + []*pbresource.Reference{ + serviceRef(backendTenancy, "backend1"), + }, + ) + } + resOneParentTwoBackends := func(parentTenancy, backendTenancy string) *pbresource.Resource { + return newRoute(t, + []*pbresource.Reference{ + serviceRef(parentTenancy, "api1"), + }, + []*pbresource.Reference{ + serviceRef(backendTenancy, "backend1"), + serviceRef(backendTenancy, "backend2"), + }, + ) + } + resTwoParentsTwoBackends := func(parentTenancy, backendTenancy string) *pbresource.Resource { + return newRoute(t, + []*pbresource.Reference{ + serviceRef(parentTenancy, "api1"), + serviceRef(parentTenancy, "api2"), + }, + []*pbresource.Reference{ + serviceRef(backendTenancy, "backend1"), + serviceRef(backendTenancy, "backend2"), + }, + ) + } run := func(t *testing.T, name string, tc testcase) { t.Run(name, func(t *testing.T) { @@ -457,19 +473,29 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare reg, ok := registry.Resolve(tc.res.Id.GetType()) require.True(t, ok) - err = reg.ACLs.Read(authz, &acl.AuthorizerContext{}, tc.res.Id, nil) + authCtx := &acl.AuthorizerContext{} // unused + + err = reg.ACLs.Read(authz, authCtx, tc.res.Id, nil) require.ErrorIs(t, err, resource.ErrNeedResource, "read hook should require the data payload") - checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, &acl.AuthorizerContext{}, tc.res.Id, tc.res)) - checkF(t, "write", tc.writeOK, reg.ACLs.Write(authz, &acl.AuthorizerContext{}, tc.res)) - checkF(t, "list", DEFAULT, reg.ACLs.List(authz, &acl.AuthorizerContext{})) + checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, authCtx, tc.res.Id, tc.res)) + checkF(t, "write", tc.writeOK, reg.ACLs.Write(authz, authCtx, tc.res)) + checkF(t, "list", DEFAULT, reg.ACLs.List(authz, authCtx)) }) } - serviceRead := func(name string) string { + isEnterprise := (structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default") + + serviceRead := func(partition, namespace, name string) string { + if isEnterprise { + return fmt.Sprintf(` partition %q { namespace %q { service %q { policy = "read" } } }`, partition, namespace, name) + } return fmt.Sprintf(` service %q { policy = "read" } `, name) } - serviceWrite := func(name string) string { + serviceWrite := func(partition, namespace, name string) string { + if isEnterprise { + return fmt.Sprintf(` partition %q { namespace %q { service %q { policy = "write" } } }`, partition, namespace, name) + } return fmt.Sprintf(` service %q { policy = "write" } `, name) } @@ -483,34 +509,64 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare run(t, name, tc) } - t.Run("no rules", func(t *testing.T) { - rules := `` - assert(t, "1parent 1backend", rules, resOneParentOneBackend, DENY, DENY) - assert(t, "1parent 2backends", rules, resOneParentTwoBackends, DENY, DENY) - assert(t, "2parents 1backend", rules, resTwoParentsOneBackend, DENY, DENY) - assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends, DENY, DENY) - }) - t.Run("api1:read", func(t *testing.T) { - rules := serviceRead("api1") - assert(t, "1parent 1backend", rules, resOneParentOneBackend, ALLOW, DENY) - assert(t, "1parent 2backends", rules, resOneParentTwoBackends, ALLOW, DENY) - assert(t, "2parents 1backend", rules, resTwoParentsOneBackend, DENY, DENY) - assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends, DENY, DENY) - }) - t.Run("api1:write", func(t *testing.T) { - rules := serviceWrite("api1") - assert(t, "1parent 1backend", rules, resOneParentOneBackend, ALLOW, DENY) - assert(t, "1parent 2backends", rules, resOneParentTwoBackends, ALLOW, DENY) - assert(t, "2parents 1backend", rules, resTwoParentsOneBackend, DENY, DENY) - assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends, DENY, DENY) - }) - t.Run("api1:write backend1:read", func(t *testing.T) { - rules := serviceWrite("api1") + serviceRead("backend1") - assert(t, "1parent 1backend", rules, resOneParentOneBackend, ALLOW, ALLOW) - assert(t, "1parent 2backends", rules, resOneParentTwoBackends, ALLOW, DENY) - assert(t, "2parents 1backend", rules, resTwoParentsOneBackend, DENY, DENY) - assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends, DENY, DENY) - }) + tenancies := []string{"default.default"} + if isEnterprise { + tenancies = append(tenancies, "default.foo", "alpha.default", "alpha.foo") + } + + for _, parentTenancyStr := range tenancies { + t.Run("route tenancy: "+parentTenancyStr, func(t *testing.T) { + for _, backendTenancyStr := range tenancies { + t.Run("backend tenancy: "+backendTenancyStr, func(t *testing.T) { + for _, aclTenancyStr := range tenancies { + t.Run("acl tenancy: "+aclTenancyStr, func(t *testing.T) { + aclTenancy := resourcetest.Tenancy(aclTenancyStr) + + maybe := func(match string, parentOnly bool) string { + if parentTenancyStr != aclTenancyStr { + return DENY + } + if !parentOnly && backendTenancyStr != aclTenancyStr { + return DENY + } + return match + } + + t.Run("no rules", func(t *testing.T) { + rules := `` + assert(t, "1parent 1backend", rules, resOneParentOneBackend(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "1parent 2backends", rules, resOneParentTwoBackends(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "2parents 1backend", rules, resTwoParentsOneBackend(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends(parentTenancyStr, backendTenancyStr), DENY, DENY) + }) + t.Run("api1:read", func(t *testing.T) { + rules := serviceRead(aclTenancy.Partition, aclTenancy.Namespace, "api1") + assert(t, "1parent 1backend", rules, resOneParentOneBackend(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), DENY) + assert(t, "1parent 2backends", rules, resOneParentTwoBackends(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), DENY) + assert(t, "2parents 1backend", rules, resTwoParentsOneBackend(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends(parentTenancyStr, backendTenancyStr), DENY, DENY) + }) + t.Run("api1:write", func(t *testing.T) { + rules := serviceWrite(aclTenancy.Partition, aclTenancy.Namespace, "api1") + assert(t, "1parent 1backend", rules, resOneParentOneBackend(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), DENY) + assert(t, "1parent 2backends", rules, resOneParentTwoBackends(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), DENY) + assert(t, "2parents 1backend", rules, resTwoParentsOneBackend(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends(parentTenancyStr, backendTenancyStr), DENY, DENY) + }) + t.Run("api1:write backend1:read", func(t *testing.T) { + rules := serviceWrite(aclTenancy.Partition, aclTenancy.Namespace, "api1") + + serviceRead(aclTenancy.Partition, aclTenancy.Namespace, "backend1") + assert(t, "1parent 1backend", rules, resOneParentOneBackend(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), maybe(ALLOW, false)) + assert(t, "1parent 2backends", rules, resOneParentTwoBackends(parentTenancyStr, backendTenancyStr), maybe(ALLOW, true), DENY) + assert(t, "2parents 1backend", rules, resTwoParentsOneBackend(parentTenancyStr, backendTenancyStr), DENY, DENY) + assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends(parentTenancyStr, backendTenancyStr), DENY, DENY) + }) + }) + } + }) + } + }) + } } func newRef(typ *pbresource.Type, name string) *pbresource.Reference { From 761090d96d0ab2d8fbd6dfca787d86596f03e24c Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 16 Oct 2023 11:19:46 -0500 Subject: [PATCH 2/2] use common plumbing --- internal/mesh/internal/types/xroute_test.go | 65 ++++----------------- internal/resource/resourcetest/acls.go | 53 ++++++++++++----- 2 files changed, 52 insertions(+), 66 deletions(-) diff --git a/internal/mesh/internal/types/xroute_test.go b/internal/mesh/internal/types/xroute_test.go index fcbd61dd3ad7..5ba39e24cc07 100644 --- a/internal/mesh/internal/types/xroute_test.go +++ b/internal/mesh/internal/types/xroute_test.go @@ -12,7 +12,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/wrapperspb" - "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" @@ -381,37 +380,12 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare return res } - type testcase struct { - res *pbresource.Resource - rules string - check func(t *testing.T, authz acl.Authorizer, res *pbresource.Resource) - readOK string - writeOK string - } - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" + DENY = resourcetest.DENY + ALLOW = resourcetest.ALLOW + DEFAULT = resourcetest.DEFAULT ) - checkF := func(t *testing.T, name string, expect string, got error) { - switch expect { - case ALLOW: - if acl.IsErrPermissionDenied(got) { - t.Fatal(name + " should be allowed") - } - case DENY: - if !acl.IsErrPermissionDenied(got) { - t.Fatal(name + " should be denied") - } - case DEFAULT: - require.Nil(t, got, name+" expected fallthrough decision") - default: - t.Fatalf(name+" unexpected expectation: %q", expect) - } - } - serviceRef := func(tenancy, name string) *pbresource.Reference { return newRefWithTenancy(pbcatalog.ServiceType, tenancy, name) } @@ -461,26 +435,9 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare ) } - run := func(t *testing.T, name string, tc testcase) { + run := func(t *testing.T, name string, tc resourcetest.ACLTestCase) { t.Run(name, func(t *testing.T) { - config := acl.Config{ - WildcardName: structs.WildcardSpecifier, - } - authz, err := acl.NewAuthorizerFromRules(tc.rules, &config, nil) - require.NoError(t, err) - authz = acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.DenyAll()}) - - reg, ok := registry.Resolve(tc.res.Id.GetType()) - require.True(t, ok) - - authCtx := &acl.AuthorizerContext{} // unused - - err = reg.ACLs.Read(authz, authCtx, tc.res.Id, nil) - require.ErrorIs(t, err, resource.ErrNeedResource, "read hook should require the data payload") - - checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, authCtx, tc.res.Id, tc.res)) - checkF(t, "write", tc.writeOK, reg.ACLs.Write(authz, authCtx, tc.res)) - checkF(t, "list", DEFAULT, reg.ACLs.List(authz, authCtx)) + resourcetest.RunACLTestCase(t, tc, registry) }) } @@ -500,11 +457,13 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare } assert := func(t *testing.T, name string, rules string, res *pbresource.Resource, readOK, writeOK string) { - tc := testcase{ - res: res, - rules: rules, - readOK: readOK, - writeOK: writeOK, + tc := resourcetest.ACLTestCase{ + Rules: rules, + Res: res, + ReadOK: readOK, + WriteOK: writeOK, + ListOK: DEFAULT, + ReadHookRequiresResource: true, } run(t, name, tc) } diff --git a/internal/resource/resourcetest/acls.go b/internal/resource/resourcetest/acls.go index 4aff9e30327b..4ce8cc9d7ae0 100644 --- a/internal/resource/resourcetest/acls.go +++ b/internal/resource/resourcetest/acls.go @@ -39,27 +39,49 @@ var checkF = func(t *testing.T, expect string, got error) { } type ACLTestCase struct { - Rules string - Data protoreflect.ProtoMessage - Owner *pbresource.ID - Typ *pbresource.Type + Rules string + + // One of either Res or Data/Owner/Typ should be set. + Res *pbresource.Resource + Data protoreflect.ProtoMessage + Owner *pbresource.ID + Typ *pbresource.Type + ReadOK string WriteOK string ListOK string + + ReadHookRequiresResource bool } func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { - reg, ok := registry.Resolve(tc.Typ) - require.True(t, ok) + var ( + typ *pbresource.Type + res *pbresource.Resource + ) + if tc.Res != nil { + require.Nil(t, tc.Data) + require.Nil(t, tc.Owner) + require.Nil(t, tc.Typ) + typ = tc.Res.Id.GetType() + res = tc.Res + } else { + require.NotNil(t, tc.Data) + require.NotNil(t, tc.Typ) + typ = tc.Typ - resolvedType, ok := registry.Resolve(tc.Typ) - require.True(t, ok) + resolvedType, ok := registry.Resolve(typ) + require.True(t, ok) - res := Resource(tc.Typ, "test"). - WithTenancy(DefaultTenancyForType(t, resolvedType)). - WithOwner(tc.Owner). - WithData(t, tc.Data). - Build() + res = Resource(tc.Typ, "test"). + WithTenancy(DefaultTenancyForType(t, resolvedType)). + WithOwner(tc.Owner). + WithData(t, tc.Data). + Build() + } + + reg, ok := registry.Resolve(typ) + require.True(t, ok) ValidateAndNormalize(t, registry, res) @@ -70,6 +92,11 @@ func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { require.NoError(t, err) authz = acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.DenyAll()}) + if tc.ReadHookRequiresResource { + err = reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, nil) + require.ErrorIs(t, err, resource.ErrNeedResource, "read hook should require the data payload") + } + t.Run("read", func(t *testing.T) { err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, res) checkF(t, tc.ReadOK, err)