From eb7a11e5b012c04312475667c54961c4f9a53569 Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Tue, 13 Feb 2024 12:49:43 -0500 Subject: [PATCH] Backport of Refactor xTP tests into release/1.18.x (#20616) * backport of commit e484c3c7dcd6562992c27510016e4f6e2e629a48 * backport of commit 76afe081a5f5fc051862b4446b25d45c28e2f76d * backport of commit cb93adba7969e894ef4dcfed240be4bf9b39f339 * backport of commit a23ea51c82cd5a5fffead4c0f3658971d60bc83b --------- Co-authored-by: Chris S. Kim --- .../trafficpermissions/controller_test.go | 81 +- .../types/namespace_traffic_permissions.go | 2 +- .../namespace_traffic_permissions_test.go | 664 +--------- .../types/partition_traffic_permissions.go | 5 +- .../partition_traffic_permissions_test.go | 664 +--------- .../internal/types/traffic_permissions.go | 201 +--- .../types/traffic_permissions_ce_test.go | 74 -- .../types/traffic_permissions_test.go | 1070 ++++++++--------- internal/auth/internal/types/validate.go | 203 ++++ ...issions_validator_ce.go => validate_ce.go} | 13 +- 10 files changed, 857 insertions(+), 2120 deletions(-) delete mode 100644 internal/auth/internal/types/traffic_permissions_ce_test.go create mode 100644 internal/auth/internal/types/validate.go rename internal/auth/internal/types/{traffic_permissions_validator_ce.go => validate_ce.go} (57%) diff --git a/internal/auth/internal/controllers/trafficpermissions/controller_test.go b/internal/auth/internal/controllers/trafficpermissions/controller_test.go index 08a29c30f7..c5a05def9d 100644 --- a/internal/auth/internal/controllers/trafficpermissions/controller_test.go +++ b/internal/auth/internal/controllers/trafficpermissions/controller_test.go @@ -133,9 +133,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_NoReferencingTrafficPermis Write(suite.T(), suite.client) wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(suite.T(), suite.client) - require.NotNil(suite.T(), wi) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - require.NotNil(suite.T(), id) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(suite.T(), err) @@ -151,12 +149,9 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi suite.Run("trafperms and namespace trafperms exist", func() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { t := suite.T() - // Workload identity ID == CTP ID - wi1ID := &pbresource.ID{ - Name: "wi1", - Type: pbauth.ComputedTrafficPermissionsType, - Tenancy: tenancy, - } + // Create the workload identity to be referenced + wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) perm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -176,7 +171,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(tenancy). Write(t, suite.client) - suite.requireTrafficPermissionsTracking(tp1, wi1ID) + suite.requireTrafficPermissionsTracking(tp1, id) nsPerm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -194,10 +189,6 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(tenancy). Write(t, suite.client) - // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(t, err) @@ -210,6 +201,10 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi suite.Run("only namespace trafperms exist", func() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { t := suite.T() + + // create the workload identity to reference + wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) // Write allow namespace trafperms nsPerm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ @@ -228,10 +223,6 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(tenancy). Write(t, suite.client) - // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - require.NoError(t, suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id})) // Ensure that the CTP was created @@ -243,6 +234,10 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi suite.Run("only partition trafperms exist", func() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { t := suite.T() + // create the workload identity to reference + wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) + // Write allow partition trafperms ptPerm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ @@ -261,10 +256,6 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(&pbresource.Tenancy{Partition: tenancy.GetPartition()}). Write(t, suite.client) - // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - require.NoError(t, suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id})) // Ensure that the CTP was created @@ -276,12 +267,11 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi suite.Run("trafperms and partition trafperms exist", func() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { t := suite.T() - // Workload identity ID == CTP ID - wi1ID := &pbresource.ID{ - Name: "wi1", - Type: pbauth.ComputedTrafficPermissionsType, - Tenancy: tenancy, - } + + // create the workload identity to reference + wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) + perm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -301,7 +291,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(tenancy). Write(t, suite.client) - suite.requireTrafficPermissionsTracking(tp1, wi1ID) + suite.requireTrafficPermissionsTracking(tp1, id) ptPerm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -319,10 +309,6 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(&pbresource.Tenancy{Partition: tenancy.GetPartition()}). Write(t, suite.client) - // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(t, err) @@ -335,12 +321,11 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi suite.Run("trafperms, partition and namespace trafperms exist", func() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { t := suite.T() - // Workload identity ID == CTP ID - wi1ID := &pbresource.ID{ - Name: "wi1", - Type: pbauth.ComputedTrafficPermissionsType, - Tenancy: tenancy, - } + + // create the workload identity to reference + wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) + perm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -360,7 +345,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(tenancy). Write(t, suite.client) - suite.requireTrafficPermissionsTracking(tp1, wi1ID) + suite.requireTrafficPermissionsTracking(tp1, id) nsPerm1 := &pbauth.Permission{ Sources: []*pbauth.Source{ { @@ -395,10 +380,6 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi WithTenancy(&pbresource.Tenancy{Partition: tenancy.GetPartition()}). Write(t, suite.client) - // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(t, suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() - err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(t, err) @@ -458,8 +439,8 @@ func (suite *controllerSuite) TestReconcile_WorkloadIdentityDelete_ReferencingTr suite.requireTrafficPermissionsTracking(tp2, wi1ID) // create the workload identity that they reference - wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(suite.T(), suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() + wi := rtest.ResourceID(wi1ID).Write(suite.T(), suite.client) + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi1ID) err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(suite.T(), err) @@ -475,9 +456,9 @@ func (suite *controllerSuite) TestReconcile_WorkloadIdentityDelete_ReferencingTr func (suite *controllerSuite) TestReconcile_WorkloadIdentityDelete_NoReferencingTrafficPermissionsExist() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - // create the workload identity that they reference + // create the workload identity to be referenced wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(suite.T(), suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(suite.T(), err) @@ -499,7 +480,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // create the workload identity to be referenced wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(suite.T(), suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(suite.T(), err) @@ -600,7 +581,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_Destination suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // create the workload identity to be referenced wi := rtest.Resource(pbauth.WorkloadIdentityType, "wi1").WithTenancy(tenancy).Write(suite.T(), suite.client) - id := rtest.Resource(pbauth.ComputedTrafficPermissionsType, wi.Id.Name).WithTenancy(tenancy).WithOwner(wi.Id).ID() + id := resource.ReplaceType(pbauth.ComputedTrafficPermissionsType, wi.Id) err := suite.reconciler.Reconcile(suite.ctx, suite.rt, controller.Request{ID: id}) require.NoError(suite.T(), err) diff --git a/internal/auth/internal/types/namespace_traffic_permissions.go b/internal/auth/internal/types/namespace_traffic_permissions.go index 9eeeafb0d1..694c6daa03 100644 --- a/internal/auth/internal/types/namespace_traffic_permissions.go +++ b/internal/auth/internal/types/namespace_traffic_permissions.go @@ -41,7 +41,7 @@ var ValidateNamespaceTrafficPermissions = resource.DecodeAndValidate(validateNam func validateNamespaceTrafficPermissions(res *DecodedNamespaceTrafficPermissions) error { var merr error - if err := v.ValidateAction(res.Data); err != nil { + if err := validateAction(res.Data); err != nil { merr = multierror.Append(merr, err) } if err := validatePermissions(res.Id, res.Data); err != nil { diff --git a/internal/auth/internal/types/namespace_traffic_permissions_test.go b/internal/auth/internal/types/namespace_traffic_permissions_test.go index 8b1a93b2e3..faf6d60e0f 100644 --- a/internal/auth/internal/types/namespace_traffic_permissions_test.go +++ b/internal/auth/internal/types/namespace_traffic_permissions_test.go @@ -8,12 +8,9 @@ import ( "github.com/stretchr/testify/require" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" - "github.com/hashicorp/consul/proto-public/pbresource" "github.com/hashicorp/consul/proto/private/prototest" "github.com/hashicorp/consul/sdk/testutil" ) @@ -21,7 +18,7 @@ import ( func TestValidateNamespaceTrafficPermissions_ParseError(t *testing.T) { data := &pbauth.ComputedTrafficPermissions{AllowPermissions: nil} - res := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "tp"). + res := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "ntp"). WithData(t, data). Build() @@ -30,221 +27,6 @@ func TestValidateNamespaceTrafficPermissions_ParseError(t *testing.T) { require.ErrorAs(t, err, &resource.ErrDataParse{}) } -// todo: this test is copy-pasted from traffic permissions tests. -// would be nice to refator this to keep them in sync. -func TestValidateNamespaceTrafficPermissions(t *testing.T) { - cases := map[string]struct { - id *pbresource.ID - ntp *pbauth.NamespaceTrafficPermissions - expectErr string - }{ - "ok-minimal": { - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - }, - }, - "unspecified-action": { - // Any type other than the TrafficPermissions type would work - // to cause the error we are expecting - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_UNSPECIFIED, - }, - expectErr: `invalid "data.action" field`, - }, - "invalid-action": { - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action(50), - }, - expectErr: `invalid "data.action" field`, - }, - "source-tenancy": { - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "cl1", - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-same-tenancy-as-tp": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-peer-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: "peer", - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-sameness-group-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - }, - "source-has-peer-and-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-sameness-group-and-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-sameness-group-and-partition-peer-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ntp: &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - } - - for n, tc := range cases { - t.Run(n, func(t *testing.T) { - resBuilder := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "ntp"). - WithData(t, tc.ntp) - if tc.id != nil { - resBuilder = resBuilder.WithTenancy(tc.id.Tenancy) - } - res := resBuilder.Build() - - err := ValidateNamespaceTrafficPermissions(res) - if tc.expectErr == "" { - require.NoError(t, err) - } else { - testutil.RequireErrorContains(t, err, tc.expectErr) - } - }) - } -} - func TestValidateNamespaceTrafficPermissions_Permissions(t *testing.T) { for n, tc := range permissionsTestCases() { t.Run(n, func(t *testing.T) { @@ -272,327 +54,32 @@ func TestValidateNamespaceTrafficPermissions_Permissions(t *testing.T) { } func TestMutateNamespaceTrafficPermissions(t *testing.T) { - type testcase struct { - policyTenancy *pbresource.Tenancy - tp *pbauth.NamespaceTrafficPermissions - expect *pbauth.NamespaceTrafficPermissions - } - - run := func(t *testing.T, tc testcase) { - tenancy := tc.policyTenancy + run := func(t *testing.T, tc mutationTestCase) { + tenancy := tc.tenancy if tenancy == nil { tenancy = resource.DefaultNamespacedTenancy() } res := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "ntp"). WithTenancy(tenancy). - WithData(t, tc.tp). + WithData(t, &pbauth.NamespaceTrafficPermissions{ + Action: pbauth.Action_ACTION_ALLOW, + Permissions: tc.permissions, + }). Build() err := MutateNamespaceTrafficPermissions(res) got := resourcetest.MustDecode[*pbauth.NamespaceTrafficPermissions](t, res) - require.NoError(t, err) - prototest.AssertDeepEqual(t, tc.expect, got.Data) + + if tc.expectErr == "" { + require.NoError(t, err) + prototest.AssertDeepEqual(t, tc.expect, got.Data.Permissions) + } else { + testutil.RequireErrorContains(t, err, tc.expectErr) + } } - cases := map[string]testcase{ - "empty-1": { - tp: &pbauth.NamespaceTrafficPermissions{}, - expect: &pbauth.NamespaceTrafficPermissions{}, - }, - "kitchen-sink-default-partition": { - tp: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - }, - }, - }, - }, - expect: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - }, - }, - }, - }, - }, - "kitchen-sink-excludes-default-partition": { - tp: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - expect: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - }, - "kitchen-sink-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ - Partition: "ap1", - Namespace: "ns3", - }, - tp: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, - { - IdentityName: "i2", - Partition: "non-default", - }, - }, - }, - }, - }, - expect: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "default", - Partition: "non-default", - Peer: "local", - }, - }, - }, - }, - }, - }, - "kitchen-sink-excludes-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ - Partition: "ap1", - Namespace: "ns3", - }, - tp: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, - }, - }, - }, - }, - }, - }, - expect: &pbauth.NamespaceTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - }, - } - - for name, tc := range cases { + for name, tc := range mutationTestCases() { t.Run(name, func(t *testing.T) { run(t, tc) }) @@ -600,110 +87,59 @@ func TestMutateNamespaceTrafficPermissions(t *testing.T) { } func TestNamespaceTrafficPermissionsACLs(t *testing.T) { - // Wire up a registry to generically invoke hooks registry := resource.NewRegistry() Register(registry) - type testcase struct { - rules string - readOK string - writeOK string - listOK string + ntpData := &pbauth.NamespaceTrafficPermissions{ + Action: pbauth.Action_ACTION_ALLOW, } - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" - ) - - checkF := func(t *testing.T, expect string, got error) { - switch expect { - case ALLOW: - if acl.IsErrPermissionDenied(got) { - t.Fatal("should be allowed") - } - case DENY: - if !acl.IsErrPermissionDenied(got) { - t.Fatal("should be denied") - } - case DEFAULT: - require.Nil(t, got, "expected fallthrough decision") - default: - t.Fatalf("unexpected expectation: %q", expect) - } - } - - reg, ok := registry.Resolve(pbauth.NamespaceTrafficPermissionsType) - require.True(t, ok) - - run := func(t *testing.T, tc testcase) { - tpData := &pbauth.NamespaceTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - } - res := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "ntp1"). - WithTenancy(resource.DefaultNamespacedTenancy()). - WithData(t, tpData). - Build() - resourcetest.ValidateAndNormalize(t, registry, res) - - 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()}) - - t.Run("read", func(t *testing.T) { - err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, res) - 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) - }) - } - - cases := map[string]testcase{ + cases := map[string]resourcetest.ACLTestCase{ "no rules": { - rules: ``, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: ``, + Data: ntpData, + Typ: pbauth.NamespaceTrafficPermissionsType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "operator read": { - rules: `operator = "read"`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `operator = "read"`, + Data: ntpData, + Typ: pbauth.NamespaceTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "operator write": { - rules: `operator = "write"`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `operator = "write"`, + Data: ntpData, + Typ: pbauth.NamespaceTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, "mesh read": { - rules: `mesh = "read"`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `mesh = "read"`, + Data: ntpData, + Typ: pbauth.NamespaceTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, - "namespace write": { - rules: `mesh = "write"`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + "mesh write": { + Rules: `mesh = "write"`, + Data: ntpData, + Typ: pbauth.NamespaceTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - run(t, tc) + resourcetest.RunACLTestCase(t, tc, registry) }) } } diff --git a/internal/auth/internal/types/partition_traffic_permissions.go b/internal/auth/internal/types/partition_traffic_permissions.go index d9cd1cb6e7..d257548197 100644 --- a/internal/auth/internal/types/partition_traffic_permissions.go +++ b/internal/auth/internal/types/partition_traffic_permissions.go @@ -4,10 +4,11 @@ package types import ( + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/internal/resource" pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" - "github.com/hashicorp/go-multierror" ) type DecodedPartitionTrafficPermissions = resource.DecodedResource[*pbauth.PartitionTrafficPermissions] @@ -40,7 +41,7 @@ var ValidatePartitionTrafficPermissions = resource.DecodeAndValidate(validatePar func validatePartitionTrafficPermissions(res *DecodedPartitionTrafficPermissions) error { var merr error - if err := v.ValidateAction(res.Data); err != nil { + if err := validateAction(res.Data); err != nil { merr = multierror.Append(merr, err) } if err := validatePermissions(res.Id, res.Data); err != nil { diff --git a/internal/auth/internal/types/partition_traffic_permissions_test.go b/internal/auth/internal/types/partition_traffic_permissions_test.go index eb38024745..b0e8a33719 100644 --- a/internal/auth/internal/types/partition_traffic_permissions_test.go +++ b/internal/auth/internal/types/partition_traffic_permissions_test.go @@ -6,15 +6,13 @@ package types import ( "testing" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/structs" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" - "github.com/hashicorp/consul/proto-public/pbresource" "github.com/hashicorp/consul/proto/private/prototest" "github.com/hashicorp/consul/sdk/testutil" - "github.com/stretchr/testify/require" ) func TestValidatePartitionTrafficPermissions_ParseError(t *testing.T) { @@ -29,218 +27,6 @@ func TestValidatePartitionTrafficPermissions_ParseError(t *testing.T) { require.ErrorAs(t, err, &resource.ErrDataParse{}) } -func TestValidatePartitionTrafficPermissions(t *testing.T) { - // TODO: refactor test cases as these are similar to namespace traffic permissions - cases := map[string]struct { - id *pbresource.ID - ptp *pbauth.PartitionTrafficPermissions - expectErr string - }{ - "ok-minimal": { - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - }, - }, - "unspecified-action": { - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_UNSPECIFIED, - }, - expectErr: `invalid "data.action" field`, - }, - "invalid-action": { - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action(50), - }, - expectErr: `invalid "data.action" field`, - }, - "source-tenancy": { - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "cl1", - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-same-tenancy-as-tp": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "pt1", - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-peer-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: "peer", - SamenessGroup: "", - }, - }, - }, - }, - }, - }, - "source-has-sameness-group-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - }, - "source-has-peer-and-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-sameness-group-and-partition-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - "source-has-sameness-group-and-partition-peer-set": { - id: &pbresource.ID{ - Tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - }, - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "sg1", - }, - }, - }, - }, - }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, - }, - } - - for n, tc := range cases { - t.Run(n, func(t *testing.T) { - resBuilder := resourcetest.Resource(pbauth.PartitionTrafficPermissionsType, "ptp"). - WithData(t, tc.ptp) - if tc.id != nil { - resBuilder = resBuilder.WithTenancy(tc.id.Tenancy) - } - res := resBuilder.Build() - - err := ValidatePartitionTrafficPermissions(res) - if tc.expectErr == "" { - require.NoError(t, err) - } else { - testutil.RequireErrorContains(t, err, tc.expectErr) - } - }) - } -} - func TestValidatePartitionTrafficPermissions_Permissions(t *testing.T) { for n, tc := range permissionsTestCases() { t.Run(n, func(t *testing.T) { @@ -268,327 +54,32 @@ func TestValidatePartitionTrafficPermissions_Permissions(t *testing.T) { } func TestMutatePartitionTrafficPermissions(t *testing.T) { - type testcase struct { - policyTenancy *pbresource.Tenancy - ptp *pbauth.PartitionTrafficPermissions - expect *pbauth.PartitionTrafficPermissions - } - - run := func(t *testing.T, tc testcase) { - tenancy := tc.policyTenancy + run := func(t *testing.T, tc mutationTestCase) { + tenancy := tc.tenancy if tenancy == nil { tenancy = resource.DefaultPartitionedTenancy() } - res := resourcetest.Resource(pbauth.PartitionTrafficPermissionsType, "ptp"). + res := resourcetest.Resource(pbauth.NamespaceTrafficPermissionsType, "ntp"). WithTenancy(tenancy). - WithData(t, tc.ptp). + WithData(t, &pbauth.PartitionTrafficPermissions{ + Action: pbauth.Action_ACTION_ALLOW, + Permissions: tc.permissions, + }). Build() err := MutatePartitionTrafficPermissions(res) got := resourcetest.MustDecode[*pbauth.PartitionTrafficPermissions](t, res) - require.NoError(t, err) - prototest.AssertDeepEqual(t, tc.expect, got.Data) + + if tc.expectErr == "" { + require.NoError(t, err) + prototest.AssertDeepEqual(t, tc.expect, got.Data.Permissions) + } else { + testutil.RequireErrorContains(t, err, tc.expectErr) + } } - cases := map[string]testcase{ - "empty-1": { - ptp: &pbauth.PartitionTrafficPermissions{}, - expect: &pbauth.PartitionTrafficPermissions{}, - }, - "kitchen-sink-default-partition": { - ptp: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - }, - }, - }, - }, - expect: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - }, - }, - }, - }, - }, - "kitchen-sink-excludes-default-partition": { - ptp: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - expect: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - }, - "kitchen-sink-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ - Partition: "ap1", - Namespace: "ns3", - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, - { - IdentityName: "i2", - Partition: "non-default", - }, - }, - }, - }, - }, - expect: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "default", - Partition: "non-default", - Peer: "local", - }, - }, - }, - }, - }, - }, - "kitchen-sink-excludes-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ - Partition: "ap1", - Namespace: "ns3", - }, - ptp: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, - }, - }, - }, - }, - }, - }, - expect: &pbauth.PartitionTrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, - }, - }, - }, - }, - }, - }, - }, - } - - for name, tc := range cases { + for name, tc := range mutationTestCases() { t.Run(name, func(t *testing.T) { run(t, tc) }) @@ -596,110 +87,59 @@ func TestMutatePartitionTrafficPermissions(t *testing.T) { } func TestPartitionTrafficPermissionsACLs(t *testing.T) { - // Wire up a registry to generically invoke hooks registry := resource.NewRegistry() Register(registry) - type testcase struct { - rules string - readOK string - writeOK string - listOK string + ptpData := &pbauth.PartitionTrafficPermissions{ + Action: pbauth.Action_ACTION_ALLOW, } - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" - ) - - checkF := func(t *testing.T, expect string, got error) { - switch expect { - case ALLOW: - if acl.IsErrPermissionDenied(got) { - t.Fatal("should be allowed") - } - case DENY: - if !acl.IsErrPermissionDenied(got) { - t.Fatal("should be denied") - } - case DEFAULT: - require.Nil(t, got, "expected fallthrough decision") - default: - t.Fatalf("unexpected expectation: %q", expect) - } - } - - reg, ok := registry.Resolve(pbauth.PartitionTrafficPermissionsType) - require.True(t, ok) - - run := func(t *testing.T, tc testcase) { - tpData := &pbauth.PartitionTrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - } - res := resourcetest.Resource(pbauth.PartitionTrafficPermissionsType, "ptp1"). - WithTenancy(resource.DefaultPartitionedTenancy()). - WithData(t, tpData). - Build() - resourcetest.ValidateAndNormalize(t, registry, res) - - 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()}) - - t.Run("read", func(t *testing.T) { - err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, res) - 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) - }) - } - - cases := map[string]testcase{ + cases := map[string]resourcetest.ACLTestCase{ "no rules": { - rules: ``, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: ``, + Data: ptpData, + Typ: pbauth.PartitionTrafficPermissionsType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "operator read": { - rules: `operator = "read"`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `operator = "read"`, + Data: ptpData, + Typ: pbauth.PartitionTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "operator write": { - rules: `operator = "write"`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `operator = "write"`, + Data: ptpData, + Typ: pbauth.PartitionTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, "mesh read": { - rules: `mesh = "read"`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `mesh = "read"`, + Data: ptpData, + Typ: pbauth.PartitionTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, - "partition write": { - rules: `mesh = "write"`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + "mesh write": { + Rules: `mesh = "write"`, + Data: ptpData, + Typ: pbauth.PartitionTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - run(t, tc) + resourcetest.RunACLTestCase(t, tc, registry) }) } } diff --git a/internal/auth/internal/types/traffic_permissions.go b/internal/auth/internal/types/traffic_permissions.go index 71aa1b7d24..440c964931 100644 --- a/internal/auth/internal/types/traffic_permissions.go +++ b/internal/auth/internal/types/traffic_permissions.go @@ -4,8 +4,6 @@ package types import ( - "golang.org/x/exp/slices" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/acl" @@ -123,8 +121,7 @@ type validator interface { func validateTrafficPermissions(res *DecodedTrafficPermissions) error { var merr error - err := v.ValidateAction(res.Data) - if err != nil { + if err := validateAction(res.Data); err != nil { merr = multierror.Append(merr, err) } @@ -142,202 +139,6 @@ func validateTrafficPermissions(res *DecodedTrafficPermissions) error { return merr } -func validatePermissions(id *pbresource.ID, data interface{ GetPermissions() []*pbauth.Permission }) error { - var merr error - for i, permission := range data.GetPermissions() { - wrapErr := func(err error) error { - return resource.ErrInvalidListElement{ - Name: "permissions", - Index: i, - Wrapped: err, - } - } - if err := validatePermission(permission, id, wrapErr); err != nil { - merr = multierror.Append(merr, err) - } - } - return merr -} - -func validatePermission(p *pbauth.Permission, id *pbresource.ID, wrapErr func(error) error) error { - var merr error - - if len(p.Sources) == 0 { - merr = multierror.Append(merr, wrapErr(resource.ErrInvalidField{ - Name: "sources", - Wrapped: resource.ErrEmpty, - })) - } - - for s, src := range p.Sources { - wrapSrcErr := func(err error) error { - return wrapErr(resource.ErrInvalidListElement{ - Name: "sources", - Index: s, - Wrapped: err, - }) - } - if sourceHasIncompatibleTenancies(src, id) { - merr = multierror.Append(merr, wrapSrcErr(resource.ErrInvalidListElement{ - Name: "source", - Wrapped: errSourcesTenancy, - })) - } - - if src.Namespace == "" && src.IdentityName != "" { - merr = multierror.Append(merr, wrapSrcErr(resource.ErrInvalidField{ - Name: "source", - Wrapped: errSourceWildcards, - })) - } - - // Excludes are only valid for wildcard sources. - if src.IdentityName != "" && len(src.Exclude) > 0 { - merr = multierror.Append(merr, wrapSrcErr(resource.ErrInvalidField{ - Name: "exclude_sources", - Wrapped: errSourceExcludes, - })) - continue - } - - for e, d := range src.Exclude { - wrapExclSrcErr := func(err error) error { - return wrapErr(resource.ErrInvalidListElement{ - Name: "exclude_sources", - Index: e, - Wrapped: err, - }) - } - if sourceHasIncompatibleTenancies(d, id) { - merr = multierror.Append(merr, wrapExclSrcErr(resource.ErrInvalidField{ - Name: "exclude_source", - Wrapped: errSourcesTenancy, - })) - } - - if d.Namespace == "" && d.IdentityName != "" { - merr = multierror.Append(merr, wrapExclSrcErr(resource.ErrInvalidField{ - Name: "source", - Wrapped: errSourceWildcards, - })) - } - } - } - for d, dest := range p.DestinationRules { - wrapDestRuleErr := func(err error) error { - return wrapErr(resource.ErrInvalidListElement{ - Name: "destination_rules", - Index: d, - Wrapped: err, - }) - } - if (len(dest.PathExact) > 0 && len(dest.PathPrefix) > 0) || - (len(dest.PathRegex) > 0 && len(dest.PathExact) > 0) || - (len(dest.PathRegex) > 0 && len(dest.PathPrefix) > 0) { - merr = multierror.Append(merr, wrapDestRuleErr(resource.ErrInvalidListElement{ - Name: "destination_rule", - Wrapped: errInvalidPrefixValues, - })) - } - if len(dest.Headers) > 0 { - for h, hdr := range dest.Headers { - wrapHeaderErr := func(err error) error { - return wrapDestRuleErr(resource.ErrInvalidListElement{ - Name: "destination_header_rules", - Index: h, - Wrapped: err, - }) - } - if len(hdr.Name) == 0 { - merr = multierror.Append(merr, wrapHeaderErr(resource.ErrInvalidListElement{ - Name: "destination_header_rule", - Wrapped: errHeaderRulesInvalid, - })) - } - } - } - if dest.IsEmpty() { - merr = multierror.Append(merr, wrapDestRuleErr(resource.ErrInvalidListElement{ - Name: "destination_rule", - Wrapped: errInvalidRule, - })) - } - if len(dest.Exclude) > 0 { - for e, excl := range dest.Exclude { - wrapExclPermRuleErr := func(err error) error { - return wrapDestRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_rules", - Index: e, - Wrapped: err, - }) - } - if (len(excl.PathExact) > 0 && len(excl.PathPrefix) > 0) || - (len(excl.PathRegex) > 0 && len(excl.PathExact) > 0) || - (len(excl.PathRegex) > 0 && len(excl.PathPrefix) > 0) { - merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_rule", - Wrapped: errInvalidPrefixValues, - })) - } - for eh, hdr := range excl.Headers { - wrapExclHeaderErr := func(err error) error { - return wrapDestRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_header_rules", - Index: eh, - Wrapped: err, - }) - } - if len(hdr.Name) == 0 { - merr = multierror.Append(merr, wrapExclHeaderErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_header_rule", - Wrapped: errHeaderRulesInvalid, - })) - } - } - for _, m := range excl.Methods { - if len(dest.Methods) != 0 && !slices.Contains(dest.Methods, m) { - merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_header_rule", - Wrapped: errExclValuesMustBeSubset, - })) - } - } - for _, port := range excl.PortNames { - if len(dest.PortNames) != 0 && !slices.Contains(dest.PortNames, port) { - merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_header_rule", - Wrapped: errExclValuesMustBeSubset, - })) - } - } - if excl.IsEmpty() { - merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ - Name: "exclude_permission_rule", - Wrapped: errInvalidRule, - })) - } - } - } - } - - return merr -} - -func sourceHasIncompatibleTenancies(src pbauth.SourceToSpiffe, id *pbresource.ID) bool { - if id.Tenancy == nil { - id.Tenancy = &pbresource.Tenancy{} - } - peerSet := !isLocalPeer(src.GetPeer()) - apSet := src.GetPartition() != id.Tenancy.Partition - sgSet := src.GetSamenessGroup() != "" - - return (apSet && peerSet) || (apSet && sgSet) || (peerSet && sgSet) -} - -func isLocalPeer(p string) bool { - return p == "local" || p == "" -} - func aclReadHookTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *DecodedTrafficPermissions) error { return authorizer.ToAllowAuthorizer().TrafficPermissionsReadAllowed(res.Data.Destination.IdentityName, authzContext) } diff --git a/internal/auth/internal/types/traffic_permissions_ce_test.go b/internal/auth/internal/types/traffic_permissions_ce_test.go deleted file mode 100644 index d1ba5d3bb4..0000000000 --- a/internal/auth/internal/types/traffic_permissions_ce_test.go +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build !consulent - -package types - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/hashicorp/consul/internal/resource/resourcetest" - pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" - "github.com/hashicorp/consul/sdk/testutil" -) - -func TestValidateTrafficPermissionsActionCE(t *testing.T) { - cases := map[string]struct { - tp *pbauth.TrafficPermissions - expectErr string - }{ - "ok-minimal": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{IdentityName: "wi-1"}, - Action: pbauth.Action_ACTION_ALLOW, - }, - }, - "unspecified-action": { - // Any type other than the TrafficPermissions type would work - // to cause the error we are expecting - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "wi1", - }, - Action: pbauth.Action_ACTION_UNSPECIFIED, - Permissions: nil, - }, - expectErr: `invalid "data.action" field: action must be allow`, - }, - "deny-action": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{IdentityName: "wi-1"}, - Action: pbauth.Action_ACTION_DENY, - }, - expectErr: `invalid "data.action" field: action must be allow`, - }, - "invalid-action": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "wi1", - }, - Action: pbauth.Action(50), - Permissions: nil, - }, - expectErr: `invalid "data.action" field: action must be allow`, - }, - } - - for n, tc := range cases { - t.Run(n, func(t *testing.T) { - res := resourcetest.Resource(pbauth.TrafficPermissionsType, "tp"). - WithData(t, tc.tp). - Build() - - err := ValidateTrafficPermissions(res) - if tc.expectErr == "" { - require.NoError(t, err) - } else { - testutil.RequireErrorContains(t, err, tc.expectErr) - } - }) - } -} diff --git a/internal/auth/internal/types/traffic_permissions_test.go b/internal/auth/internal/types/traffic_permissions_test.go index 9d614809b8..6c113d81f7 100644 --- a/internal/auth/internal/types/traffic_permissions_test.go +++ b/internal/auth/internal/types/traffic_permissions_test.go @@ -7,15 +7,15 @@ import ( "testing" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" "github.com/hashicorp/consul/proto-public/pbresource" "github.com/hashicorp/consul/proto/private/prototest" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/version/versiontest" ) func TestValidateTrafficPermissions_ParseError(t *testing.T) { @@ -31,285 +31,302 @@ func TestValidateTrafficPermissions_ParseError(t *testing.T) { } func TestValidateTrafficPermissions(t *testing.T) { + const ( + TrafficPermissions = 1 << iota + NamespaceTrafficPermissions + PartitionTrafficPermissions + ) + all := TrafficPermissions | NamespaceTrafficPermissions | PartitionTrafficPermissions + cases := map[string]struct { - tp *pbauth.TrafficPermissions - id *pbresource.ID - expectErr string + // bitmask of what xTrafficPermissions to test + xTPs int + + // following fields will be used to construct all the xTrafficPermissions + destination *pbauth.Destination // used only by TrafficPermissions + action pbauth.Action + permissions []*pbauth.Permission + + id *pbresource.ID + expectErr string + enterprise bool }{ "ok-minimal": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{IdentityName: "wi-1"}, - Action: pbauth.Action_ACTION_ALLOW, - }, + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, }, "ok-permissions": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{IdentityName: "wi-1"}, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - IdentityName: "wi-2", - Namespace: "default", - Partition: "default", - }, - { - IdentityName: "wi-1", - Namespace: "default", - Partition: "ap1", - }, + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + IdentityName: "wi-2", + Namespace: "default", + Partition: "default", }, - DestinationRules: []*pbauth.DestinationRule{ - { - PathPrefix: "/", - Methods: []string{"GET"}, - Headers: []*pbauth.DestinationRuleHeader{{Name: "X-Consul-Token", Present: false, Invert: true}}, - PortNames: []string{"https"}, - Exclude: []*pbauth.ExcludePermissionRule{{PathExact: "/admin"}}, - }, + { + IdentityName: "wi-1", + Namespace: "default", + Partition: "ap1", + }, + }, + DestinationRules: []*pbauth.DestinationRule{ + { + PathPrefix: "/", + Methods: []string{"GET"}, + Headers: []*pbauth.DestinationRuleHeader{{Name: "X-Consul-Token", Present: false, Invert: true}}, + PortNames: []string{"https"}, + Exclude: []*pbauth.ExcludePermissionRule{{PathExact: "/admin"}}, }, }, }, }, }, "unspecified-action": { - // Any type other than the TrafficPermissions type would work - // to cause the error we are expecting - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "wi1", - }, - Action: pbauth.Action_ACTION_UNSPECIFIED, - Permissions: nil, - }, - expectErr: `invalid "data.action" field`, + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_UNSPECIFIED, + expectErr: `invalid "data.action" field`, }, "invalid-action": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "wi1", - }, - Action: pbauth.Action(50), - Permissions: nil, - }, - expectErr: `invalid "data.action" field`, + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action(50), + expectErr: `invalid "data.action" field`, + }, + "deny-action": { + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_DENY, + expectErr: `invalid "data.action" field`, + enterprise: true, }, "no-destination": { - tp: &pbauth.TrafficPermissions{ - Action: pbauth.Action_ACTION_ALLOW, - Permissions: nil, - }, - expectErr: `invalid "data.destination" field: cannot be empty`, + xTPs: TrafficPermissions, + destination: nil, + action: pbauth.Action_ACTION_ALLOW, + expectErr: `invalid "data.destination" field: cannot be empty`, }, "source-tenancy": { - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "cl1", - SamenessGroup: "sg1", - }, + xTPs: all, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "ap1", + Peer: "cl1", + SamenessGroup: "sg1", }, - DestinationRules: nil, }, + DestinationRules: nil, }, }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, + expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": permissions sources may not specify partitions, peers, and sameness_groups together`, }, "source-has-same-tenancy-as-tp": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultPartitionName, - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: resource.DefaultPartitionName, + Peer: resource.DefaultPeerName, + SamenessGroup: "", }, }, }, }, }, "source-has-partition-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: resource.DefaultPeerName, - SamenessGroup: "", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "part", + Peer: resource.DefaultPeerName, + SamenessGroup: "", }, }, }, }, }, "source-has-peer-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultNamespaceName, - Peer: "peer", - SamenessGroup: "", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: resource.DefaultPartitionName, + Peer: "peer", + SamenessGroup: "", }, }, }, }, }, "source-has-sameness-group-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: resource.DefaultNamespaceName, - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: resource.DefaultPartitionName, + Peer: resource.DefaultPeerName, + SamenessGroup: "sg1", }, }, }, }, }, "source-has-peer-and-partition-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "part", + Peer: "peer", + SamenessGroup: "", }, }, }, }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, + expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": permissions sources may not specify partitions, peers, and sameness_groups together`, }, "source-has-sameness-group-and-partition-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: resource.DefaultPeerName, - SamenessGroup: "sg1", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "part", + Peer: resource.DefaultPeerName, + SamenessGroup: "sg1", }, }, }, }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, + expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": permissions sources may not specify partitions, peers, and sameness_groups together`, }, "source-has-sameness-group-and-partition-peer-set": { + xTPs: all, id: &pbresource.ID{ Tenancy: &pbresource.Tenancy{ Partition: resource.DefaultPartitionName, }, }, - tp: &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{ - IdentityName: "w1", - }, - Action: pbauth.Action_ACTION_ALLOW, - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "part", - Peer: "peer", - SamenessGroup: "sg1", - }, + destination: &pbauth.Destination{IdentityName: "wi-1"}, + action: pbauth.Action_ACTION_ALLOW, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "part", + Peer: "peer", + SamenessGroup: "sg1", }, }, }, }, - expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": invalid element at index 0 of list "source": permissions sources may not specify partitions, peers, and sameness_groups together`, + expectErr: `invalid element at index 0 of list "permissions": invalid element at index 0 of list "sources": permissions sources may not specify partitions, peers, and sameness_groups together`, }, } for n, tc := range cases { t.Run(n, func(t *testing.T) { - resBuilder := resourcetest.Resource(pbauth.TrafficPermissionsType, "tp"). - WithData(t, tc.tp) - if tc.id != nil { - resBuilder = resBuilder.WithTenancy(tc.id.Tenancy) + check := func(t *testing.T, typ *pbresource.Type, data protoreflect.ProtoMessage, validateFunc resource.ValidationHook) { + resBuilder := resourcetest.Resource(typ, "tp"). + WithData(t, data) + if tc.id != nil { + resBuilder = resBuilder.WithTenancy(tc.id.Tenancy) + } + res := resBuilder.Build() + err := validateFunc(res) + if tc.expectErr == "" { + require.NoError(t, err) + } else if tc.enterprise && versiontest.IsEnterprise() { + require.NoError(t, err) + } else { + // Expect error in CE, not ENT + testutil.RequireErrorContains(t, err, tc.expectErr) + } } - res := resBuilder.Build() - - err := ValidateTrafficPermissions(res) - if tc.expectErr == "" { - require.NoError(t, err) - } else { - testutil.RequireErrorContains(t, err, tc.expectErr) + if tc.xTPs&TrafficPermissions != 0 { + t.Run("TrafficPermissions", func(t *testing.T) { + tp := &pbauth.TrafficPermissions{ + Destination: tc.destination, + Action: tc.action, + Permissions: tc.permissions, + } + check(t, pbauth.TrafficPermissionsType, tp, ValidateTrafficPermissions) + }) + } + if tc.xTPs&NamespaceTrafficPermissions != 0 { + t.Run("NamespaceTrafficPermissions", func(t *testing.T) { + ntp := &pbauth.NamespaceTrafficPermissions{ + Action: tc.action, + Permissions: tc.permissions, + } + check(t, pbauth.NamespaceTrafficPermissionsType, ntp, ValidateNamespaceTrafficPermissions) + }) + } + if tc.xTPs&PartitionTrafficPermissions != 0 { + t.Run("PartitionTrafficPermissions", func(t *testing.T) { + ptp := &pbauth.PartitionTrafficPermissions{ + Action: tc.action, + Permissions: tc.permissions, + } + check(t, pbauth.PartitionTrafficPermissionsType, ptp, ValidatePartitionTrafficPermissions) + }) } }) } @@ -580,164 +597,134 @@ func TestValidateTrafficPermissions_Permissions(t *testing.T) { } } -func TestMutateTrafficPermissions(t *testing.T) { - type testcase struct { - policyTenancy *pbresource.Tenancy - tp *pbauth.TrafficPermissions - expect *pbauth.TrafficPermissions - expectErr string - } +type mutationTestCase struct { + tenancy *pbresource.Tenancy + permissions []*pbauth.Permission + expect []*pbauth.Permission + expectErr string +} - run := func(t *testing.T, tc testcase) { - tenancy := tc.policyTenancy - if tenancy == nil { - tenancy = resource.DefaultNamespacedTenancy() - } - res := resourcetest.Resource(pbauth.TrafficPermissionsType, "api"). - WithTenancy(tenancy). - WithData(t, tc.tp). - Build() - - err := MutateTrafficPermissions(res) - - got := resourcetest.MustDecode[*pbauth.TrafficPermissions](t, res) - - if tc.expectErr == "" { - require.NoError(t, err) - prototest.AssertDeepEqual(t, tc.expect, got.Data) - } else { - testutil.RequireErrorContains(t, err, tc.expectErr) - } - } - - cases := map[string]testcase{ - "empty-1": { - tp: &pbauth.TrafficPermissions{}, - expect: &pbauth.TrafficPermissions{}, +func mutationTestCases() map[string]mutationTestCase { + return map[string]mutationTestCase{ + "empty": { + permissions: nil, + expect: nil, }, "kitchen-sink-default-partition": { - tp: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + {}, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Peer: "local", }, }, }, }, - expect: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - // TODO(peering/v2) revisit peer defaulting - // Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, + expect: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "default", + Peer: "local", + }, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + Partition: "default", + Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + // TODO(peering/v2) revisit peer defaulting + // Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "default", + Peer: "local", }, }, }, }, }, "kitchen-sink-excludes-default-partition": { - tp: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Exclude: []*pbauth.ExcludeSource{ + {}, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Peer: "local", }, }, }, }, }, }, - expect: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "default", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "default", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - // TODO(peering/v2) revisit peer defaulting - // Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "default", - Peer: "local", - }, + expect: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "default", + Peer: "local", + Exclude: []*pbauth.ExcludeSource{ + { + Partition: "default", + Peer: "local", + }, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + Partition: "default", + Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + // TODO(peering/v2) revisit peer defaulting + // Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "default", + Peer: "local", }, }, }, @@ -746,166 +733,158 @@ func TestMutateTrafficPermissions(t *testing.T) { }, }, "kitchen-sink-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ + tenancy: &pbresource.Tenancy{ Partition: "ap1", Namespace: "ns3", }, - tp: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, - { - IdentityName: "i2", - Partition: "non-default", - }, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + {}, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap5", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Peer: "local", + }, + { + IdentityName: "i2", + }, + { + IdentityName: "i2", + Partition: "non-default", }, }, }, }, - expect: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - // TODO(peering/v2) revisit to figure out defaulting - // Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "default", - Partition: "non-default", - Peer: "local", - }, + expect: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "ap1", + Namespace: "", + Peer: "local", + }, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + Partition: "ap1", + Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap5", + // TODO(peering/v2) revisit to figure out defaulting + // Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + Peer: "local", + }, + { + IdentityName: "i2", + Namespace: "ns3", + Partition: "ap1", + Peer: "local", + }, + { + IdentityName: "i2", + Namespace: "default", + Partition: "non-default", + Peer: "local", }, }, }, }, }, "kitchen-sink-excludes-non-default-partition": { - policyTenancy: &pbresource.Tenancy{ + tenancy: &pbresource.Tenancy{ Partition: "ap1", Namespace: "ns3", }, - tp: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Exclude: []*pbauth.ExcludeSource{ - {}, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Peer: "local", - }, - { - IdentityName: "i2", - }, + permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Exclude: []*pbauth.ExcludeSource{ + {}, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap5", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Peer: "local", + }, + { + IdentityName: "i2", }, }, }, }, }, }, - expect: &pbauth.TrafficPermissions{ - Permissions: []*pbauth.Permission{ - { - Sources: []*pbauth.Source{ - { - Partition: "ap1", - Peer: "local", - Exclude: []*pbauth.ExcludeSource{ - { - Partition: "ap1", - Namespace: "", - Peer: "local", - }, - { - Peer: "not-default", - }, - { - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap5", - // TODO(peering/v2) revisit peer defaulting - // Peer: "local", - }, - { - IdentityName: "i1", - Namespace: "ns1", - Partition: "ap1", - Peer: "local", - }, - { - IdentityName: "i2", - Namespace: "ns3", - Partition: "ap1", - Peer: "local", - }, + expect: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + Partition: "ap1", + Peer: "local", + Exclude: []*pbauth.ExcludeSource{ + { + Partition: "ap1", + Namespace: "", + Peer: "local", + }, + { + Peer: "not-default", + }, + { + Namespace: "ns1", + Partition: "ap1", + Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap5", + // TODO(peering/v2) revisit peer defaulting + // Peer: "local", + }, + { + IdentityName: "i1", + Namespace: "ns1", + Partition: "ap1", + Peer: "local", + }, + { + IdentityName: "i2", + Namespace: "ns3", + Partition: "ap1", + Peer: "local", }, }, }, @@ -914,8 +893,36 @@ func TestMutateTrafficPermissions(t *testing.T) { }, }, } +} - for name, tc := range cases { +func TestMutateTrafficPermissions(t *testing.T) { + run := func(t *testing.T, tc mutationTestCase) { + tenancy := tc.tenancy + if tenancy == nil { + tenancy = resource.DefaultNamespacedTenancy() + } + res := resourcetest.Resource(pbauth.TrafficPermissionsType, "api"). + WithTenancy(tenancy). + WithData(t, &pbauth.TrafficPermissions{ + Destination: &pbauth.Destination{IdentityName: "wi1"}, + Action: pbauth.Action_ACTION_ALLOW, + Permissions: tc.permissions, + }). + Build() + + err := MutateTrafficPermissions(res) + + got := resourcetest.MustDecode[*pbauth.TrafficPermissions](t, res) + + if tc.expectErr == "" { + require.NoError(t, err) + prototest.AssertDeepEqual(t, tc.expect, got.Data.Permissions) + } else { + testutil.RequireErrorContains(t, err, tc.expectErr) + } + } + + for name, tc := range mutationTestCases() { t.Run(name, func(t *testing.T) { run(t, tc) }) @@ -923,130 +930,83 @@ func TestMutateTrafficPermissions(t *testing.T) { } func TestTrafficPermissionsACLs(t *testing.T) { - // Wire up a registry to generically invoke hooks registry := resource.NewRegistry() Register(registry) - type testcase struct { - rules string - check func(t *testing.T, authz acl.Authorizer, res *pbresource.Resource) - readOK string - writeOK string - listOK string + tpData := &pbauth.TrafficPermissions{ + Destination: &pbauth.Destination{IdentityName: "wi1"}, + Action: pbauth.Action_ACTION_ALLOW, } - - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" - ) - - checkF := func(t *testing.T, expect string, got error) { - switch expect { - case ALLOW: - if acl.IsErrPermissionDenied(got) { - t.Fatal("should be allowed") - } - case DENY: - if !acl.IsErrPermissionDenied(got) { - t.Fatal("should be denied") - } - case DEFAULT: - require.Nil(t, got, "expected fallthrough decision") - default: - t.Fatalf("unexpected expectation: %q", expect) - } - } - - reg, ok := registry.Resolve(pbauth.TrafficPermissionsType) - require.True(t, ok) - - run := func(t *testing.T, tc testcase) { - tpData := &pbauth.TrafficPermissions{ - Destination: &pbauth.Destination{IdentityName: "wi1"}, - Action: pbauth.Action_ACTION_ALLOW, - } - res := resourcetest.Resource(pbauth.TrafficPermissionsType, "tp1"). - WithTenancy(resource.DefaultNamespacedTenancy()). - WithData(t, tpData). - Build() - resourcetest.ValidateAndNormalize(t, registry, res) - - 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()}) - - t.Run("read", func(t *testing.T) { - err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, res) - 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) - }) - } - - cases := map[string]testcase{ + cases := map[string]resourcetest.ACLTestCase{ "no rules": { - rules: ``, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: ``, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 read, no intentions": { - rules: `identity "wi1" { policy = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "read" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 read, deny intentions": { - rules: `identity "wi1" { policy = "read", intentions = "deny" }`, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "read", intentions = "deny" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 read, intentions read": { - rules: `identity "wi1" { policy = "read", intentions = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "read", intentions = "read" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, - "workload identity w1 write, write intentions": { - rules: `identity "wi1" { policy = "read", intentions = "write" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + "workload identity w1 read, intentions write": { + Rules: `identity "wi1" { policy = "read", intentions = "write" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 write, deny intentions": { - rules: `identity "wi1" { policy = "write", intentions = "deny" }`, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "write", intentions = "deny" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 write, intentions read": { - rules: `identity "wi1" { policy = "write", intentions = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "write", intentions = "read" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity w1 write, intentions write": { - rules: `identity "wi1" { policy = "write", intentions = "write" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `identity "wi1" { policy = "write", intentions = "write" }`, + Data: tpData, + Typ: pbauth.TrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - run(t, tc) + resourcetest.RunACLTestCase(t, tc, registry) }) } } diff --git a/internal/auth/internal/types/validate.go b/internal/auth/internal/types/validate.go new file mode 100644 index 0000000000..d7cd01d67b --- /dev/null +++ b/internal/auth/internal/types/validate.go @@ -0,0 +1,203 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package types + +import ( + "github.com/hashicorp/go-multierror" + "golang.org/x/exp/slices" + + "github.com/hashicorp/consul/internal/resource" + pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +func validatePermissions(id *pbresource.ID, data interface{ GetPermissions() []*pbauth.Permission }) error { + var merr error + for i, permission := range data.GetPermissions() { + wrapErr := func(err error) error { + return resource.ErrInvalidListElement{ + Name: "permissions", + Index: i, + Wrapped: err, + } + } + if err := validatePermission(permission, id, wrapErr); err != nil { + merr = multierror.Append(merr, err) + } + } + return merr +} + +func validatePermission(p *pbauth.Permission, id *pbresource.ID, wrapErr func(error) error) error { + var merr error + + if len(p.Sources) == 0 { + merr = multierror.Append(merr, wrapErr(resource.ErrInvalidField{ + Name: "sources", + Wrapped: resource.ErrEmpty, + })) + } + + for s, src := range p.Sources { + wrapSrcErr := func(err error) error { + return wrapErr(resource.ErrInvalidListElement{ + Name: "sources", + Index: s, + Wrapped: err, + }) + } + if sourceHasIncompatibleTenancies(src, id) { + merr = multierror.Append(merr, wrapSrcErr(errSourcesTenancy)) + } + + if src.Namespace == "" && src.IdentityName != "" { + merr = multierror.Append(merr, wrapSrcErr(errSourceWildcards)) + } + + // Excludes are only valid for wildcard sources. + if src.IdentityName != "" && len(src.Exclude) > 0 { + merr = multierror.Append(merr, wrapSrcErr(resource.ErrInvalidField{ + Name: "exclude_sources", + Wrapped: errSourceExcludes, + })) + continue + } + + for e, d := range src.Exclude { + wrapExclSrcErr := func(err error) error { + return wrapErr(resource.ErrInvalidListElement{ + Name: "exclude_sources", + Index: e, + Wrapped: err, + }) + } + if sourceHasIncompatibleTenancies(d, id) { + merr = multierror.Append(merr, wrapExclSrcErr(resource.ErrInvalidField{ + Name: "exclude_source", + Wrapped: errSourcesTenancy, + })) + } + + if d.Namespace == "" && d.IdentityName != "" { + merr = multierror.Append(merr, wrapExclSrcErr(resource.ErrInvalidField{ + Name: "source", + Wrapped: errSourceWildcards, + })) + } + } + } + for d, dest := range p.DestinationRules { + wrapDestRuleErr := func(err error) error { + return wrapErr(resource.ErrInvalidListElement{ + Name: "destination_rules", + Index: d, + Wrapped: err, + }) + } + if (len(dest.PathExact) > 0 && len(dest.PathPrefix) > 0) || + (len(dest.PathRegex) > 0 && len(dest.PathExact) > 0) || + (len(dest.PathRegex) > 0 && len(dest.PathPrefix) > 0) { + merr = multierror.Append(merr, wrapDestRuleErr(resource.ErrInvalidListElement{ + Name: "destination_rule", + Wrapped: errInvalidPrefixValues, + })) + } + if len(dest.Headers) > 0 { + for h, hdr := range dest.Headers { + wrapHeaderErr := func(err error) error { + return wrapDestRuleErr(resource.ErrInvalidListElement{ + Name: "destination_header_rules", + Index: h, + Wrapped: err, + }) + } + if len(hdr.Name) == 0 { + merr = multierror.Append(merr, wrapHeaderErr(resource.ErrInvalidListElement{ + Name: "destination_header_rule", + Wrapped: errHeaderRulesInvalid, + })) + } + } + } + if dest.IsEmpty() { + merr = multierror.Append(merr, wrapDestRuleErr(resource.ErrInvalidListElement{ + Name: "destination_rule", + Wrapped: errInvalidRule, + })) + } + if len(dest.Exclude) > 0 { + for e, excl := range dest.Exclude { + wrapExclPermRuleErr := func(err error) error { + return wrapDestRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_rules", + Index: e, + Wrapped: err, + }) + } + if (len(excl.PathExact) > 0 && len(excl.PathPrefix) > 0) || + (len(excl.PathRegex) > 0 && len(excl.PathExact) > 0) || + (len(excl.PathRegex) > 0 && len(excl.PathPrefix) > 0) { + merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_rule", + Wrapped: errInvalidPrefixValues, + })) + } + for eh, hdr := range excl.Headers { + wrapExclHeaderErr := func(err error) error { + return wrapDestRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_header_rules", + Index: eh, + Wrapped: err, + }) + } + if len(hdr.Name) == 0 { + merr = multierror.Append(merr, wrapExclHeaderErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_header_rule", + Wrapped: errHeaderRulesInvalid, + })) + } + } + for _, m := range excl.Methods { + if len(dest.Methods) != 0 && !slices.Contains(dest.Methods, m) { + merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_header_rule", + Wrapped: errExclValuesMustBeSubset, + })) + } + } + for _, port := range excl.PortNames { + if len(dest.PortNames) != 0 && !slices.Contains(dest.PortNames, port) { + merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_header_rule", + Wrapped: errExclValuesMustBeSubset, + })) + } + } + if excl.IsEmpty() { + merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ + Name: "exclude_permission_rule", + Wrapped: errInvalidRule, + })) + } + } + } + } + + return merr +} + +func sourceHasIncompatibleTenancies(src pbauth.SourceToSpiffe, id *pbresource.ID) bool { + if id.Tenancy == nil { + id.Tenancy = &pbresource.Tenancy{} + } + peerSet := !isLocalPeer(src.GetPeer()) + apSet := src.GetPartition() != id.Tenancy.Partition + sgSet := src.GetSamenessGroup() != "" + + return (apSet && peerSet) || (apSet && sgSet) || (peerSet && sgSet) +} + +func isLocalPeer(p string) bool { + return p == "local" || p == "" +} diff --git a/internal/auth/internal/types/traffic_permissions_validator_ce.go b/internal/auth/internal/types/validate_ce.go similarity index 57% rename from internal/auth/internal/types/traffic_permissions_validator_ce.go rename to internal/auth/internal/types/validate_ce.go index 574d15fe95..b953cfc8a2 100644 --- a/internal/auth/internal/types/traffic_permissions_validator_ce.go +++ b/internal/auth/internal/types/validate_ce.go @@ -12,18 +12,9 @@ import ( pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1" ) -var v validator = &actionValidator{} - -type actionValidator struct{} - -func (v *actionValidator) ValidateAction(data interface{ GetAction() pbauth.Action }) error { - // enumcover:pbauth.Action +func validateAction(data interface{ GetAction() pbauth.Action }) error { switch data.GetAction() { case pbauth.Action_ACTION_ALLOW: - case pbauth.Action_ACTION_UNSPECIFIED: - fallthrough - case pbauth.Action_ACTION_DENY: - fallthrough default: return resource.ErrInvalidField{ Name: "data.action", @@ -32,5 +23,3 @@ func (v *actionValidator) ValidateAction(data interface{ GetAction() pbauth.Acti } return nil } - -var _ validator = (*actionValidator)(nil)