Skip to content

Commit

Permalink
grpc/acl: relax permissions required for "core" endpoints
Browse files Browse the repository at this point in the history
Previously, these endpoints required `service:write` permission on _any_
service as a sort of proxy for "is the caller allowed to participate in
the mesh?".

Now, they're called as part of the process of establishing a server
connection by any consumer of the consul-server-connection-manager
library, which will include non-mesh workloads (e.g. Consul KV as a
storage backend for Vault) as well as ancillary components such as
consul-k8s' acl-init process, which likely won't have `service:write`
permission.

So this commit relaxes those requirements to accept *any* valid ACL token
on the following gRPC endpoints:

- `hashicorp.consul.dataplane.DataplaneService/GetSupportedDataplaneFeatures`
- `hashicorp.consul.serverdiscovery.ServerDiscoveryService/WatchServers`
- `hashicorp.consul.connectca.ConnectCAService/WatchRoots`
  • Loading branch information
boxofrad committed Nov 14, 2022
1 parent 931cec4 commit ae9b6a9
Show file tree
Hide file tree
Showing 11 changed files with 126 additions and 66 deletions.
3 changes: 3 additions & 0 deletions .changelog/15346.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:enhancement
acl: relax permissions on the `WatchServers`, `WatchRoots` and `GetSupportedDataplaneFeatures` gRPC endpoints to accept *any* valid ACL token
```
14 changes: 7 additions & 7 deletions agent/grpc-external/services/connectca/sign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestSign_ConnectDisabled(t *testing.T) {
func TestSign_Validation(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

server := NewServer(Config{
Logger: hclog.NewNullLogger(),
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestSign_Unauthenticated(t *testing.T) {
func TestSign_PermissionDenied(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand All @@ -116,7 +116,7 @@ func TestSign_PermissionDenied(t *testing.T) {
func TestSign_InvalidCSR(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand All @@ -142,7 +142,7 @@ func TestSign_InvalidCSR(t *testing.T) {
func TestSign_RateLimited(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand All @@ -168,7 +168,7 @@ func TestSign_RateLimited(t *testing.T) {
func TestSign_InternalError(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand All @@ -194,7 +194,7 @@ func TestSign_InternalError(t *testing.T) {
func TestSign_Success(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand All @@ -220,7 +220,7 @@ func TestSign_Success(t *testing.T) {
func TestSign_RPCForwarding(t *testing.T) {
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerAllowAll(t), nil)
Return(testutils.ACLAllowAll(t), nil)

caManager := &MockCAManager{}
caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).
Expand Down
2 changes: 1 addition & 1 deletion agent/grpc-external/services/connectca/watch_roots.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (s *Server) serveRoots(
serverStream pbconnectca.ConnectCAService_WatchRootsServer,
logger hclog.Logger,
) (uint64, error) {
if err := s.authorize(token); err != nil {
if err := external.RequireAnyValidACLToken(s.ACLResolver, token); err != nil {
return 0, err
}

Expand Down
12 changes: 6 additions & 6 deletions agent/grpc-external/services/connectca/watch_roots_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestWatchRoots_Success(t *testing.T) {
// Mock the ACL Resolver to return an authorizer with `service:write`.
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceWriteAny(t), nil)
Return(testutils.ACLNoPermissions(t), nil)

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down Expand Up @@ -144,7 +144,7 @@ func TestWatchRoots_ACLTokenInvalidated(t *testing.T) {
// first two times it is called (initial connect and first re-auth).
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceWriteAny(t), nil).Twice()
Return(testutils.ACLNoPermissions(t), nil).Twice()

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down Expand Up @@ -184,9 +184,9 @@ func TestWatchRoots_ACLTokenInvalidated(t *testing.T) {
// Expect the stream to remain open and to receive the new roots.
mustGetRoots(t, rspCh)

// Simulate removing the `service:write` permission.
// Simulate deleting the ACL token.
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerDenyAll(t), nil)
Return(resolver.Result{}, acl.ErrNotFound)

// Update the ACL token to cause the subscription to be force-closed.
err = fsm.GetStore().ACLTokenSet(1, &structs.ACLToken{
Expand All @@ -197,7 +197,7 @@ func TestWatchRoots_ACLTokenInvalidated(t *testing.T) {

// Expect the stream to be terminated.
err = mustGetError(t, rspCh)
require.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
require.Equal(t, codes.Unauthenticated.String(), status.Code(err).String())
}

func TestWatchRoots_StateStoreAbandoned(t *testing.T) {
Expand All @@ -214,7 +214,7 @@ func TestWatchRoots_StateStoreAbandoned(t *testing.T) {
// Mock the ACL Resolver to return an authorizer with `service:write`.
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceWriteAny(t), nil)
Return(testutils.ACLNoPermissions(t), nil)

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func TestGetEnvoyBootstrapParams_Success(t *testing.T) {

aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceRead(t, tc.registerReq.Service.ID), nil)
Return(testutils.ACLServiceRead(t, tc.registerReq.Service.ID), nil)

options := structs.QueryOptions{Token: testToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down Expand Up @@ -233,7 +233,7 @@ func TestGetEnvoyBootstrapParams_Error(t *testing.T) {
aclResolver := &MockACLResolver{}

aclResolver.On("ResolveTokenAndDefaultMeta", testToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceRead(t, proxyServiceID), nil)
Return(testutils.ACLServiceRead(t, proxyServiceID), nil)

options := structs.QueryOptions{Token: testToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down Expand Up @@ -329,7 +329,7 @@ func TestGetEnvoyBootstrapParams_PermissionDenied(t *testing.T) {
// Mock the ACL resolver to return a deny all authorizer
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerDenyAll(t), nil)
Return(testutils.ACLNoPermissions(t), nil)

options := structs.QueryOptions{Token: testToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down
14 changes: 2 additions & 12 deletions agent/grpc-external/services/dataplane/get_supported_features.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

acl "github.com/hashicorp/consul/acl"
external "github.com/hashicorp/consul/agent/grpc-external"
structs "github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/proto-public/pbdataplane"
)

Expand All @@ -18,20 +16,12 @@ func (s *Server) GetSupportedDataplaneFeatures(ctx context.Context, req *pbdatap
logger.Trace("Started processing request")
defer logger.Trace("Finished processing request")

// Require the given ACL token to have `service:write` on any service
options, err := external.QueryOptionsFromContext(ctx)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}

var authzContext acl.AuthorizerContext
entMeta := structs.WildcardEnterpriseMetaInPartition(structs.WildcardSpecifier)
authz, err := s.ACLResolver.ResolveTokenAndDefaultMeta(options.Token, entMeta, &authzContext)
if err != nil {
return nil, status.Error(codes.Unauthenticated, err.Error())
}
if err := authz.ToAllowAuthorizer().ServiceWriteAnyAllowed(&authzContext); err != nil {
return nil, status.Error(codes.PermissionDenied, err.Error())
if err := external.RequireAnyValidACLToken(s.ACLResolver, options.Token); err != nil {
return nil, err
}

supportedFeatures := []*pbdataplane.DataplaneFeatureSupport{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func TestSupportedDataplaneFeatures_Success(t *testing.T) {
// Mock the ACL Resolver to return an authorizer with `service:write`.
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceWriteAny(t), nil)
Return(testutils.ACLServiceWriteAny(t), nil)

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand Down Expand Up @@ -53,7 +53,7 @@ func TestSupportedDataplaneFeatures_Success(t *testing.T) {
}
}

func TestSupportedDataplaneFeatures_Unauthenticated(t *testing.T) {
func TestSupportedDataplaneFeatures_InvalidACLToken(t *testing.T) {
// Mock the ACL resolver to return ErrNotFound.
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Expand All @@ -74,11 +74,11 @@ func TestSupportedDataplaneFeatures_Unauthenticated(t *testing.T) {
require.Nil(t, resp)
}

func TestSupportedDataplaneFeatures_PermissionDenied(t *testing.T) {
// Mock the ACL resolver to return a deny all authorizer
func TestSupportedDataplaneFeatures_AnonymousACLToken(t *testing.T) {
// Mock the ACL resolver to return ErrNotFound.
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerDenyAll(t), nil)
aclResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.ACLAnonymous(t), nil)

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
Expand All @@ -91,6 +91,25 @@ func TestSupportedDataplaneFeatures_PermissionDenied(t *testing.T) {
client := testClient(t, server)
resp, err := client.GetSupportedDataplaneFeatures(ctx, &pbdataplane.GetSupportedDataplaneFeaturesRequest{})
require.Error(t, err)
require.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
require.Equal(t, codes.Unauthenticated.String(), status.Code(err).String())
require.Nil(t, resp)
}

func TestSupportedDataplaneFeatures_NoPermissions(t *testing.T) {
// Mock the ACL resolver to return a deny all authorizer
aclResolver := &MockACLResolver{}
aclResolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.ACLNoPermissions(t), nil)

options := structs.QueryOptions{Token: testACLToken}
ctx, err := external.ContextWithQueryOptions(context.Background(), options)
require.NoError(t, err)

server := NewServer(Config{
Logger: hclog.NewNullLogger(),
ACLResolver: aclResolver,
})
client := testClient(t, server)
_, err = client.GetSupportedDataplaneFeatures(ctx, &pbdataplane.GetSupportedDataplaneFeaturesRequest{})
require.NoError(t, err)
}
18 changes: 1 addition & 17 deletions agent/grpc-external/services/serverdiscovery/watch_servers.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/agent/consul/autopilotevents"
"github.com/hashicorp/consul/agent/consul/stream"
external "github.com/hashicorp/consul/agent/grpc-external"
Expand Down Expand Up @@ -46,7 +45,7 @@ func (s *Server) WatchServers(req *pbserverdiscovery.WatchServersRequest, server
}

func (s *Server) serveReadyServers(token string, index uint64, req *pbserverdiscovery.WatchServersRequest, serverStream pbserverdiscovery.ServerDiscoveryService_WatchServersServer, logger hclog.Logger) (uint64, error) {
if err := s.authorize(token); err != nil {
if err := external.RequireAnyValidACLToken(s.ACLResolver, token); err != nil {
return 0, err
}

Expand Down Expand Up @@ -105,21 +104,6 @@ func (s *Server) serveReadyServers(token string, index uint64, req *pbserverdisc
}
}

func (s *Server) authorize(token string) error {
// Require the given ACL token to have `service:write` on any service (in any
// partition and namespace).
var authzContext acl.AuthorizerContext
entMeta := structs.WildcardEnterpriseMetaInPartition(structs.WildcardSpecifier)
authz, err := s.ACLResolver.ResolveTokenAndDefaultMeta(token, entMeta, &authzContext)
if err != nil {
return status.Error(codes.Unauthenticated, err.Error())
}
if err := authz.ToAllowAuthorizer().ServiceWriteAnyAllowed(&authzContext); err != nil {
return status.Error(codes.PermissionDenied, err.Error())
}
return nil
}

func eventToResponse(req *pbserverdiscovery.WatchServersRequest, event stream.Event) (*pbserverdiscovery.WatchServersResponse, error) {
readyServers, err := autopilotevents.ExtractEventPayload(event)
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func TestWatchServers_StreamLifeCycle(t *testing.T) {
// 2 times authorization should succeed and the third should fail.
resolver := newMockACLResolver(t)
resolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerServiceWriteAny(t), nil).Twice()
Return(testutils.ACLNoPermissions(t), nil).Twice()

// add the token to the requests context
options := structs.QueryOptions{Token: testACLToken}
Expand Down Expand Up @@ -192,13 +192,13 @@ func TestWatchServers_StreamLifeCycle(t *testing.T) {
prototest.AssertDeepEqual(t, threeServerResponse, rsp)
}

func TestWatchServers_ACLToken_PermissionDenied(t *testing.T) {
func TestWatchServers_ACLToken_AnonymousToken(t *testing.T) {
// setup the event publisher and snapshot handler
_, publisher := setupPublisher(t)

resolver := newMockACLResolver(t)
resolver.On("ResolveTokenAndDefaultMeta", testACLToken, mock.Anything, mock.Anything).
Return(testutils.TestAuthorizerDenyAll(t), nil).Once()
Return(testutils.ACLAnonymous(t), nil).Once()

// add the token to the requests context
options := structs.QueryOptions{Token: testACLToken}
Expand All @@ -222,7 +222,7 @@ func TestWatchServers_ACLToken_PermissionDenied(t *testing.T) {

// Expect to get an Unauthenticated error immediately.
err = mustGetError(t, rspCh)
require.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
require.Equal(t, codes.Unauthenticated.String(), status.Code(err).String())
}

func TestWatchServers_ACLToken_Unauthenticated(t *testing.T) {
Expand Down
49 changes: 41 additions & 8 deletions agent/grpc-external/testutils/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,43 @@ import (

"github.com/stretchr/testify/require"

"github.com/hashicorp/go-uuid"

"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/acl/resolver"
"github.com/hashicorp/consul/agent/structs"
)

func TestAuthorizerAllowAll(t *testing.T) resolver.Result {
func ACLAnonymous(t *testing.T) resolver.Result {
t.Helper()

return resolver.Result{Authorizer: acl.AllowAll()}
return resolver.Result{
Authorizer: acl.DenyAll(),
ACLIdentity: &structs.ACLToken{
AccessorID: structs.ACLTokenAnonymousID,
},
}
}

func TestAuthorizerDenyAll(t *testing.T) resolver.Result {
func ACLAllowAll(t *testing.T) resolver.Result {
t.Helper()

return resolver.Result{Authorizer: acl.DenyAll()}
return resolver.Result{
Authorizer: acl.AllowAll(),
ACLIdentity: randomACLIdentity(t),
}
}

func TestAuthorizerServiceWriteAny(t *testing.T) resolver.Result {
func ACLNoPermissions(t *testing.T) resolver.Result {
t.Helper()

return resolver.Result{
Authorizer: acl.DenyAll(),
ACLIdentity: randomACLIdentity(t),
}
}

func ACLServiceWriteAny(t *testing.T) resolver.Result {
t.Helper()

policy, err := acl.NewPolicyFromSource(`
Expand All @@ -34,10 +54,13 @@ func TestAuthorizerServiceWriteAny(t *testing.T) resolver.Result {
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
require.NoError(t, err)

return resolver.Result{Authorizer: authz}
return resolver.Result{
Authorizer: authz,
ACLIdentity: randomACLIdentity(t),
}
}

func TestAuthorizerServiceRead(t *testing.T, serviceName string) resolver.Result {
func ACLServiceRead(t *testing.T, serviceName string) resolver.Result {
t.Helper()

aclRule := &acl.Policy{
Expand All @@ -53,5 +76,15 @@ func TestAuthorizerServiceRead(t *testing.T, serviceName string) resolver.Result
authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{aclRule}, nil)
require.NoError(t, err)

return resolver.Result{Authorizer: authz}
return resolver.Result{
Authorizer: authz,
ACLIdentity: randomACLIdentity(t),
}
}

func randomACLIdentity(t *testing.T) structs.ACLIdentity {
id, err := uuid.GenerateUUID()
require.NoError(t, err)

return &structs.ACLToken{AccessorID: id}
}

0 comments on commit ae9b6a9

Please sign in to comment.