From 69e7c4d571279c89aba0b415ef4feb19f63c3ce0 Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Tue, 13 Feb 2024 15:35:32 -0500 Subject: [PATCH] Backport of Update ComputedTrafficPermissions ACL hooks into release/1.18.x (#20627) --- .../types/computed_traffic_permissions.go | 10 +- .../computed_traffic_permissions_test.go | 169 ++++++++---------- .../internal/types/workload_identity_test.go | 154 ++++++---------- 3 files changed, 129 insertions(+), 204 deletions(-) diff --git a/internal/auth/internal/types/computed_traffic_permissions.go b/internal/auth/internal/types/computed_traffic_permissions.go index af9a67728f..03b63c7e4b 100644 --- a/internal/auth/internal/types/computed_traffic_permissions.go +++ b/internal/auth/internal/types/computed_traffic_permissions.go @@ -63,9 +63,15 @@ func validateComputedTrafficPermissions(res *DecodedComputedTrafficPermissions) } func aclReadHookComputedTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, id *pbresource.ID, _ *pbresource.Resource) error { - return authorizer.ToAllowAuthorizer().TrafficPermissionsReadAllowed(id.Name, authzContext) + err := authorizer.ToAllowAuthorizer().TrafficPermissionsReadAllowed(id.Name, authzContext) + if err != nil { + // fallback to operator read + err = authorizer.ToAllowAuthorizer().OperatorReadAllowed(authzContext) + } + return err } func aclWriteHookComputedTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *pbresource.Resource) error { - return authorizer.ToAllowAuthorizer().TrafficPermissionsWriteAllowed(res.Id.Name, authzContext) + // Users should not be writing computed resources. + return authorizer.ToAllowAuthorizer().OperatorWriteAllowed(authzContext) } diff --git a/internal/auth/internal/types/computed_traffic_permissions_test.go b/internal/auth/internal/types/computed_traffic_permissions_test.go index e5c4379a81..8aff2400b6 100644 --- a/internal/auth/internal/types/computed_traffic_permissions_test.go +++ b/internal/auth/internal/types/computed_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/sdk/testutil" ) @@ -50,127 +47,101 @@ func TestValidateComputedTrafficPermissions_Permissions(t *testing.T) { } func TestComputedTrafficPermissionsACLs(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 + ctpData := &pbauth.ComputedTrafficPermissions{ + AllowPermissions: []*pbauth.Permission{}, + DenyPermissions: []*pbauth.Permission{}, + IsDefault: true, } - 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.ComputedTrafficPermissionsType) - require.True(t, ok) - - run := func(t *testing.T, tc testcase) { - ctpData := &pbauth.ComputedTrafficPermissions{} - res := resourcetest.Resource(pbauth.ComputedTrafficPermissionsType, "wi1"). - WithTenancy(resource.DefaultNamespacedTenancy()). - WithData(t, ctpData). - 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: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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 "test" { policy = "read" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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 "test" { policy = "read", intentions = "deny" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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 "test" { policy = "read", intentions = "read" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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, + Rules: `identity "test" { policy = "read", intentions = "write" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, // users should not write computed resources + ListOK: resourcetest.DEFAULT, }, "workload identity w1 write, deny intentions": { - rules: `identity "wi1" { policy = "write", intentions = "deny" }`, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "test" { policy = "write", intentions = "deny" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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 "test" { policy = "write", intentions = "read" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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 "test" { policy = "write", intentions = "write" }`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, // users should not write computed resources + ListOK: resourcetest.DEFAULT, + }, + "operator read": { + Rules: `operator = "read"`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, // users should not write computed resources + ListOK: resourcetest.DEFAULT, + }, + "operator write": { + Rules: `operator = "write"`, + Data: ctpData, + Typ: pbauth.ComputedTrafficPermissionsType, + 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/workload_identity_test.go b/internal/auth/internal/types/workload_identity_test.go index 19ed4cbeea..c0f3a8f921 100644 --- a/internal/auth/internal/types/workload_identity_test.go +++ b/internal/auth/internal/types/workload_identity_test.go @@ -8,139 +8,87 @@ 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" ) func TestWorkloadIdentityACLs(t *testing.T) { - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" - ) - registry := resource.NewRegistry() Register(registry) - reg, ok := registry.Resolve(pbauth.WorkloadIdentityType) - require.True(t, ok) - - type testcase struct { - rules string - check func(t *testing.T, authz acl.Authorizer, res *pbresource.Resource) - readOK string - writeOK string - listOK string - } - - 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) - } - } - - run := func(t *testing.T, tc testcase) { - wid := &pbauth.WorkloadIdentity{} - res := resourcetest.Resource(pbauth.WorkloadIdentityType, "wi1"). - WithTenancy(resource.DefaultNamespacedTenancy()). - WithData(t, wid). - 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) - }) - t.Run("errors", func(t *testing.T) { - require.ErrorIs(t, reg.ACLs.Read(authz, &acl.AuthorizerContext{}, nil, nil), resource.ErrNeedResource) - require.ErrorIs(t, reg.ACLs.Write(authz, &acl.AuthorizerContext{}, nil), resource.ErrNeedResource) - }) - } + wid := &pbauth.WorkloadIdentity{} - cases := map[string]testcase{ + cases := map[string]resourcetest.ACLTestCase{ "no rules": { - rules: ``, - readOK: DENY, - writeOK: DENY, - listOK: DEFAULT, + Rules: ``, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 read, no intentions": { - rules: `identity "wi1" { policy = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "test" { policy = "read" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 read, deny intentions has no effect": { - rules: `identity "wi1" { policy = "read", intentions = "deny" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "test" { policy = "read", intentions = "deny" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 read, intentions read has no effect": { - rules: `identity "wi1" { policy = "read", intentions = "read" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "test" { policy = "read", intentions = "read" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 write, write intentions has no effect": { - rules: `identity "wi1" { policy = "read", intentions = "write" }`, - readOK: ALLOW, - writeOK: DENY, - listOK: DEFAULT, + Rules: `identity "test" { policy = "read", intentions = "write" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 write, deny intentions has no effect": { - rules: `identity "wi1" { policy = "write", intentions = "deny" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `identity "test" { policy = "write", intentions = "deny" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 write, intentions read has no effect": { - rules: `identity "wi1" { policy = "write", intentions = "read" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `identity "test" { policy = "write", intentions = "read" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + ReadOK: resourcetest.ALLOW, + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, }, "workload identity wi1 write, intentions write": { - rules: `identity "wi1" { policy = "write", intentions = "write" }`, - readOK: ALLOW, - writeOK: ALLOW, - listOK: DEFAULT, + Rules: `identity "test" { policy = "write", intentions = "write" }`, + Typ: pbauth.WorkloadIdentityType, + Data: wid, + 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) }) } }