Skip to content

Commit

Permalink
Backport of catalog, mesh: implement missing ACL hooks into release/1…
Browse files Browse the repository at this point in the history
….17.x (#19212)

catalog, mesh: implement missing ACL hooks (#19143)

This change adds ACL hooks to the remaining catalog and mesh resources, excluding any computed ones. Those will for now continue using the default operator:x permissions.

It refactors a lot of the common testing functions so that they can be re-used between resources.

There are also some types that we don't yet support (e.g. virtual IPs) that this change adds ACL hooks to for future-proofing.

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
  • Loading branch information
hc-github-team-consul-core and ishustava authored Oct 14, 2023
1 parent 41a986c commit 689f32c
Show file tree
Hide file tree
Showing 50 changed files with 996 additions and 179 deletions.
2 changes: 1 addition & 1 deletion agent/grpc-external/services/resource/read.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (s *Server) Read(ctx context.Context, req *pbresource.ReadRequest) (*pbreso
authzNeedsData := false
err = reg.ACLs.Read(authz, authzContext, req.Id, nil)
switch {
case errors.Is(err, resource.ErrNeedData):
case errors.Is(err, resource.ErrNeedResource):
authzNeedsData = true
err = nil
case acl.IsErrPermissionDenied(err):
Expand Down
8 changes: 1 addition & 7 deletions internal/auth/internal/types/computed_traffic_permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func RegisterComputedTrafficPermission(r resource.Registry) {
ACLs: &resource.ACLHooks{
Read: aclReadHookComputedTrafficPermissions,
Write: aclWriteHookComputedTrafficPermissions,
List: aclListHookComputedTrafficPermissions,
List: resource.NoOpACLListHook,
},
Validate: ValidateComputedTrafficPermissions,
Scope: resource.ScopeNamespace,
Expand Down Expand Up @@ -71,9 +71,3 @@ func aclReadHookComputedTrafficPermissions(authorizer acl.Authorizer, authzConte
func aclWriteHookComputedTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *pbresource.Resource) error {
return authorizer.ToAllowAuthorizer().TrafficPermissionsWriteAllowed(res.Id.Name, authzContext)
}

func aclListHookComputedTrafficPermissions(_ acl.Authorizer, _ *acl.AuthorizerContext) error {
// No-op List permission as we want to default to filtering resources
// from the list using the Read enforcement
return nil
}
10 changes: 2 additions & 8 deletions internal/auth/internal/types/traffic_permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func RegisterTrafficPermissions(r resource.Registry) {
ACLs: &resource.ACLHooks{
Read: aclReadHookTrafficPermissions,
Write: aclWriteHookTrafficPermissions,
List: aclListHookTrafficPermissions,
List: resource.NoOpACLListHook,
},
Validate: ValidateTrafficPermissions,
Mutate: MutateTrafficPermissions,
Expand Down Expand Up @@ -273,7 +273,7 @@ func isLocalPeer(p string) bool {

func aclReadHookTrafficPermissions(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, _ *pbresource.ID, res *pbresource.Resource) error {
if res == nil {
return resource.ErrNeedData
return resource.ErrNeedResource
}
return authorizeDestination(res, func(dest string) error {
return authorizer.ToAllowAuthorizer().TrafficPermissionsReadAllowed(dest, authzContext)
Expand All @@ -286,12 +286,6 @@ func aclWriteHookTrafficPermissions(authorizer acl.Authorizer, authzContext *acl
})
}

func aclListHookTrafficPermissions(_ acl.Authorizer, _ *acl.AuthorizerContext) error {
// No-op List permission as we want to default to filtering resources
// from the list using the Read enforcement
return nil
}

func authorizeDestination(res *pbresource.Resource, intentionAllowed func(string) error) error {
tp, err := resource.Decode[*pbauth.TrafficPermissions](res)
if err != nil {
Expand Down
12 changes: 3 additions & 9 deletions internal/auth/internal/types/workload_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func RegisterWorkloadIdentity(r resource.Registry) {
ACLs: &resource.ACLHooks{
Read: aclReadHookWorkloadIdentity,
Write: aclWriteHookWorkloadIdentity,
List: aclListHookWorkloadIdentity,
List: resource.NoOpACLListHook,
},
Validate: nil,
})
Expand All @@ -36,21 +36,15 @@ func aclReadHookWorkloadIdentity(
if res != nil {
return authorizer.ToAllowAuthorizer().IdentityReadAllowed(res.Id.Name, authzCtx)
}
return resource.ErrNeedData
return resource.ErrNeedResource
}

func aclWriteHookWorkloadIdentity(
authorizer acl.Authorizer,
authzCtx *acl.AuthorizerContext,
res *pbresource.Resource) error {
if res == nil {
return resource.ErrNeedData
return resource.ErrNeedResource
}
return authorizer.ToAllowAuthorizer().IdentityWriteAllowed(res.Id.Name, authzCtx)
}

func aclListHookWorkloadIdentity(authorizer acl.Authorizer, context *acl.AuthorizerContext) error {
// No-op List permission as we want to default to filtering resources
// from the list using the Read enforcement
return nil
}
4 changes: 2 additions & 2 deletions internal/auth/internal/types/workload_identity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ func TestWorkloadIdentityACLs(t *testing.T) {
checkF(t, tc.listOK, err)
})
t.Run("errors", func(t *testing.T) {
require.ErrorIs(t, reg.ACLs.Read(authz, &acl.AuthorizerContext{}, nil, nil), resource.ErrNeedData)
require.ErrorIs(t, reg.ACLs.Write(authz, &acl.AuthorizerContext{}, nil), resource.ErrNeedData)
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)
})
}

Expand Down
21 changes: 21 additions & 0 deletions internal/catalog/catalogtest/helpers/acl_hooks_test_helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package helpers

import (
"testing"

"github.com/hashicorp/consul/internal/catalog"
"github.com/hashicorp/consul/internal/catalog/internal/testhelpers"
"github.com/hashicorp/consul/internal/resource"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
"github.com/hashicorp/consul/proto-public/pbresource"
)

func RunWorkloadSelectingTypeACLsTests[T catalog.WorkloadSelecting](t *testing.T, typ *pbresource.Type,
getData func(selector *pbcatalog.WorkloadSelector) T,
registerFunc func(registry resource.Registry),
) {
testhelpers.RunWorkloadSelectingTypeACLsTests[T](t, typ, getData, registerFunc)
}
6 changes: 6 additions & 0 deletions internal/catalog/exports.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ var (
FailoverStatusConditionAcceptedUsingMeshDestinationPortReason = failover.UsingMeshDestinationPortReason
)

type WorkloadSelecting = types.WorkloadSelecting

func ACLHooksForWorkloadSelectingType[T WorkloadSelecting]() *resource.ACLHooks {
return types.ACLHooksForWorkloadSelectingType[T]()
}

// RegisterTypes adds all resource types within the "catalog" API group
// to the given type registry
func RegisterTypes(r resource.Registry) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ type nodeHealthControllerTestSuite struct {
}

func (suite *nodeHealthControllerTestSuite) SetupTest() {
suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register)
suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register, types.RegisterDNSPolicy)
suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())}

// The rest of the setup will be to prime the resource service with some data
Expand Down
113 changes: 113 additions & 0 deletions internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package testhelpers

import (
"testing"

"google.golang.org/protobuf/proto"

"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/internal/resource/resourcetest"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
"github.com/hashicorp/consul/proto-public/pbresource"
)

// WorkloadSelecting denotes a resource type that uses workload selectors.
type WorkloadSelecting interface {
proto.Message
GetWorkloads() *pbcatalog.WorkloadSelector
}

func RunWorkloadSelectingTypeACLsTests[T WorkloadSelecting](t *testing.T, typ *pbresource.Type,
getData func(selector *pbcatalog.WorkloadSelector) T,
registerFunc func(registry resource.Registry),
) {
// Wire up a registry to generically invoke hooks
registry := resource.NewRegistry()
registerFunc(registry)

cases := map[string]resourcetest.ACLTestCase{
"no rules": {
Rules: ``,
Data: getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.DENY,
WriteOK: resourcetest.DENY,
ListOK: resourcetest.DEFAULT,
},
"service test read": {
Rules: `service "test" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.DENY,
ListOK: resourcetest.DEFAULT,
},
"service test write with named selectors and insufficient policy": {
Rules: `service "test" { policy = "write" }`,
Data: getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.DENY,
ListOK: resourcetest.DEFAULT,
},
"service test write with prefixed selectors and insufficient policy": {
Rules: `service "test" { policy = "write" }`,
Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.DENY,
ListOK: resourcetest.DEFAULT,
},
"service test write with named selectors": {
Rules: `service "test" { policy = "write" } service "workload" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Names: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.ALLOW,
ListOK: resourcetest.DEFAULT,
},
"service test write with prefixed selectors": {
Rules: `service "test" { policy = "write" } service_prefix "workload-" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload-"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.ALLOW,
ListOK: resourcetest.DEFAULT,
},
"service test write with prefixed selectors and a policy with more specific than the selector": {
Rules: `service "test" { policy = "write" } service_prefix "workload-" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"wor"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.DENY,
ListOK: resourcetest.DEFAULT,
},
"service test write with prefixed selectors and a policy with less specific than the selector": {
Rules: `service "test" { policy = "write" } service_prefix "wor" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload-"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
WriteOK: resourcetest.ALLOW,
ListOK: resourcetest.DEFAULT,
},
"service test write with prefixed selectors and a policy with a specific service": {
Rules: `service "test" { policy = "write" } service "workload" { policy = "read" }`,
Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload"}}),
Typ: typ,
ReadOK: resourcetest.ALLOW,
// TODO (ishustava): this is wrong and should be fixed in a follow up PR. We should not allow
// a policy for a specific service when only prefixes are specified in the selector.
WriteOK: resourcetest.ALLOW,
ListOK: resourcetest.DEFAULT,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
resourcetest.RunACLTestCase(t, tc, registry)
})
}
}
56 changes: 56 additions & 0 deletions internal/catalog/internal/types/acl_hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package types

import (
"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/proto-public/pbresource"
)

func aclReadHookResourceWithWorkloadSelector(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, id *pbresource.ID, _ *pbresource.Resource) error {
return authorizer.ToAllowAuthorizer().ServiceReadAllowed(id.GetName(), authzContext)
}

func aclWriteHookResourceWithWorkloadSelector[T WorkloadSelecting](authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, res *pbresource.Resource) error {
if res == nil {
return resource.ErrNeedResource
}

decodedService, err := resource.Decode[T](res)
if err != nil {
return resource.ErrNeedResource
}

// First check service:write on the name.
err = authorizer.ToAllowAuthorizer().ServiceWriteAllowed(res.GetId().GetName(), authzContext)
if err != nil {
return err
}

// Then also check whether we're allowed to select a service.
for _, name := range decodedService.GetData().GetWorkloads().GetNames() {
err = authorizer.ToAllowAuthorizer().ServiceReadAllowed(name, authzContext)
if err != nil {
return err
}
}

for _, prefix := range decodedService.GetData().GetWorkloads().GetPrefixes() {
err = authorizer.ToAllowAuthorizer().ServiceReadAllowed(prefix, authzContext)
if err != nil {
return err
}
}

return nil
}

func ACLHooksForWorkloadSelectingType[T WorkloadSelecting]() *resource.ACLHooks {
return &resource.ACLHooks{
Read: aclReadHookResourceWithWorkloadSelector,
Write: aclWriteHookResourceWithWorkloadSelector[T],
List: resource.NoOpACLListHook,
}
}
1 change: 1 addition & 0 deletions internal/catalog/internal/types/dns_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func RegisterDNSPolicy(r resource.Registry) {
Proto: &pbcatalog.DNSPolicy{},
Scope: resource.ScopeNamespace,
Validate: ValidateDNSPolicy,
ACLs: ACLHooksForWorkloadSelectingType[*pbcatalog.DNSPolicy](),
})
}

Expand Down
17 changes: 17 additions & 0 deletions internal/catalog/internal/types/dns_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/known/anypb"

"github.com/hashicorp/consul/internal/catalog/internal/testhelpers"
"github.com/hashicorp/consul/internal/resource"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
"github.com/hashicorp/consul/proto-public/pbresource"
Expand Down Expand Up @@ -161,3 +162,19 @@ func TestValidateDNSPolicy_EmptySelector(t *testing.T) {
require.ErrorAs(t, err, &actual)
require.Equal(t, expected, actual)
}

func TestDNSPolicyACLs(t *testing.T) {
// Wire up a registry to generically invoke hooks
registry := resource.NewRegistry()
RegisterDNSPolicy(registry)

testhelpers.RunWorkloadSelectingTypeACLsTests[*pbcatalog.DNSPolicy](t, pbcatalog.DNSPolicyType,
func(selector *pbcatalog.WorkloadSelector) *pbcatalog.DNSPolicy {
return &pbcatalog.DNSPolicy{
Workloads: selector,
Weights: &pbcatalog.Weights{Passing: 1, Warning: 0},
}
},
RegisterDNSPolicy,
)
}
8 changes: 1 addition & 7 deletions internal/catalog/internal/types/failover_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func RegisterFailoverPolicy(r resource.Registry) {
ACLs: &resource.ACLHooks{
Read: aclReadHookFailoverPolicy,
Write: aclWriteHookFailoverPolicy,
List: aclListHookFailoverPolicy,
List: resource.NoOpACLListHook,
},
})
}
Expand Down Expand Up @@ -371,9 +371,3 @@ func aclWriteHookFailoverPolicy(authorizer acl.Authorizer, authzContext *acl.Aut
return nil

}

func aclListHookFailoverPolicy(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext) error {
// No-op List permission as we want to default to filtering resources
// from the list using the Read enforcement.
return nil
}
Loading

0 comments on commit 689f32c

Please sign in to comment.