From bd98f690621efdf1479fde3b13f6b023692a05cc Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 25 Sep 2023 16:52:28 -0500 Subject: [PATCH 1/2] mesh: add DestinationPolicy ACL hook tenancy tests --- .../internal/types/destination_policy_test.go | 147 +++++++++++------- 1 file changed, 89 insertions(+), 58 deletions(-) diff --git a/internal/mesh/internal/types/destination_policy_test.go b/internal/mesh/internal/types/destination_policy_test.go index c2f89eeb0a9..96c897f8884 100644 --- a/internal/mesh/internal/types/destination_policy_test.go +++ b/internal/mesh/internal/types/destination_policy_test.go @@ -4,6 +4,7 @@ package types import ( + "fmt" "testing" "time" @@ -518,12 +519,27 @@ func TestDestinationPolicyACLs(t *testing.T) { registry := resource.NewRegistry() Register(registry) + newPolicy := func(t *testing.T, tenancyStr string) *pbresource.Resource { + res := resourcetest.Resource(pbmesh.DestinationPolicyType, "api"). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + WithData(t, &pbmesh.DestinationPolicy{ + PortConfigs: map[string]*pbmesh.DestinationConfig{ + "http": { + ConnectTimeout: durationpb.New(55 * time.Second), + }, + }, + }). + Build() + resourcetest.ValidateAndNormalize(t, registry, res) + 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 - listOK string } const ( @@ -532,85 +548,100 @@ func TestDestinationPolicyACLs(t *testing.T) { DEFAULT = "default" ) - checkF := func(t *testing.T, expect string, got error) { + checkF := func(t *testing.T, name string, expect string, got error) { switch expect { case ALLOW: if acl.IsErrPermissionDenied(got) { - t.Fatal("should be allowed") + t.Fatal(name + " should be allowed") } case DENY: if !acl.IsErrPermissionDenied(got) { - t.Fatal("should be denied") + t.Fatal(name + " should be denied") } case DEFAULT: - require.Nil(t, got, "expected fallthrough decision") + require.Nil(t, got, name+" expected fallthrough decision") default: - t.Fatalf("unexpected expectation: %q", expect) + t.Fatalf(name+" unexpected expectation: %q", expect) } } reg, ok := registry.Resolve(pbmesh.DestinationPolicyType) require.True(t, ok) - run := func(t *testing.T, tc testcase) { - destData := &pbmesh.DestinationPolicy{ - PortConfigs: map[string]*pbmesh.DestinationConfig{ - "http": { - ConnectTimeout: durationpb.New(55 * time.Second), - }, - }, - } - res := resourcetest.Resource(pbmesh.DestinationPolicyType, "api"). - WithTenancy(resource.DefaultNamespacedTenancy()). - WithData(t, destData). - Build() - resourcetest.ValidateAndNormalize(t, registry, res) + run := func(t *testing.T, name string, tc testcase) { + 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()}) - 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()}) + authCtx := resource.AuthorizerContext(tc.res.Id.Tenancy) - t.Run("read", func(t *testing.T) { - err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, nil) - checkF(t, tc.readOK, err) - }) - t.Run("write", func(t *testing.T) { - err := reg.ACLs.Write(authz, &acl.AuthorizerContext{}, res) - checkF(t, tc.writeOK, err) - }) - t.Run("list", func(t *testing.T) { - err := reg.ACLs.List(authz, &acl.AuthorizerContext{}) - checkF(t, tc.listOK, err) + checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, authCtx, tc.res.Id, nil)) + checkF(t, "write", tc.writeOK, reg.ACLs.Write(authz, authCtx, tc.res)) + checkF(t, "list", DEFAULT, reg.ACLs.List(authz, authCtx)) }) } - cases := map[string]testcase{ - "no rules": { - rules: ``, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, - }, - "service api read": { - rules: `service "api" { policy = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, - }, - "service api write": { - rules: `service "api" { policy = "write" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, - }, + 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(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) } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - run(t, tc) + 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, + } + run(t, name, tc) + } + + tenancies := []string{"default.default"} + if isEnterprise { + tenancies = append(tenancies, "default.foo", "alpha.default", "alpha.foo") + } + + for _, policyTenancyStr := range tenancies { + t.Run("policy tenancy: "+policyTenancyStr, 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) string { + if policyTenancyStr != aclTenancyStr { + return DENY + } + return match + } + + t.Run("no rules", func(t *testing.T) { + rules := `` + assert(t, "any", rules, newPolicy(t, policyTenancyStr), DENY, DENY) + }) + t.Run("api:read", func(t *testing.T) { + rules := serviceRead(aclTenancy.Partition, aclTenancy.Namespace, "api") + assert(t, "any", rules, newPolicy(t, policyTenancyStr), maybe(ALLOW), DENY) + }) + t.Run("api:write", func(t *testing.T) { + rules := serviceWrite(aclTenancy.Partition, aclTenancy.Namespace, "api") + assert(t, "any", rules, newPolicy(t, policyTenancyStr), maybe(ALLOW), maybe(ALLOW)) + }) + }) + } }) } } From d5526679ccd80c7f1b54d9275ecc5473af7f83f6 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 16 Oct 2023 11:25:01 -0500 Subject: [PATCH 2/2] use common plumbing --- .../internal/types/destination_policy_test.go | 62 ++++--------------- internal/resource/resourcetest/acls.go | 15 +++-- 2 files changed, 23 insertions(+), 54 deletions(-) diff --git a/internal/mesh/internal/types/destination_policy_test.go b/internal/mesh/internal/types/destination_policy_test.go index 96c897f8884..8edb51b2d83 100644 --- a/internal/mesh/internal/types/destination_policy_test.go +++ b/internal/mesh/internal/types/destination_policy_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/durationpb" - "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" @@ -534,54 +533,15 @@ func TestDestinationPolicyACLs(t *testing.T) { 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) - } - } - - reg, ok := registry.Resolve(pbmesh.DestinationPolicyType) - require.True(t, ok) - - 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()}) - - authCtx := resource.AuthorizerContext(tc.res.Id.Tenancy) - - checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, authCtx, tc.res.Id, nil)) - 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) }) } @@ -601,11 +561,13 @@ func TestDestinationPolicyACLs(t *testing.T) { } 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{ + AuthCtx: resource.AuthorizerContext(res.Id.Tenancy), + Rules: rules, + Res: res, + ReadOK: readOK, + WriteOK: writeOK, + ListOK: DEFAULT, } run(t, name, tc) } diff --git a/internal/resource/resourcetest/acls.go b/internal/resource/resourcetest/acls.go index 4ce8cc9d7ae..3f77c7fec49 100644 --- a/internal/resource/resourcetest/acls.go +++ b/internal/resource/resourcetest/acls.go @@ -41,6 +41,9 @@ var checkF = func(t *testing.T, expect string, got error) { type ACLTestCase struct { Rules string + // AuthCtx is optional. If not provided an empty one will be used. + AuthCtx *acl.AuthorizerContext + // One of either Res or Data/Owner/Typ should be set. Res *pbresource.Resource Data protoreflect.ProtoMessage @@ -92,21 +95,25 @@ func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { require.NoError(t, err) authz = acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.DenyAll()}) + if tc.AuthCtx == nil { + tc.AuthCtx = &acl.AuthorizerContext{} + } + if tc.ReadHookRequiresResource { - err = reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, nil) + err = reg.ACLs.Read(authz, tc.AuthCtx, 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) + err := reg.ACLs.Read(authz, tc.AuthCtx, res.Id, res) checkF(t, tc.ReadOK, err) }) t.Run("write", func(t *testing.T) { - err := reg.ACLs.Write(authz, &acl.AuthorizerContext{}, res) + err := reg.ACLs.Write(authz, tc.AuthCtx, res) checkF(t, tc.WriteOK, err) }) t.Run("list", func(t *testing.T) { - err := reg.ACLs.List(authz, &acl.AuthorizerContext{}) + err := reg.ACLs.List(authz, tc.AuthCtx) checkF(t, tc.ListOK, err) }) }