diff --git a/.changelog/15346.txt b/.changelog/15346.txt new file mode 100644 index 000000000..fe0e833c2 --- /dev/null +++ b/.changelog/15346.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +acl: relax permissions on the `WatchServers`, `WatchRoots` and `GetSupportedDataplaneFeatures` gRPC endpoints to accept *any* valid ACL token +``` diff --git a/agent/grpc-external/services/connectca/sign_test.go b/agent/grpc-external/services/connectca/sign_test.go index efd27c53b..8ccaad4df 100644 --- a/agent/grpc-external/services/connectca/sign_test.go +++ b/agent/grpc-external/services/connectca/sign_test.go @@ -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(), @@ -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). @@ -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). @@ -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). @@ -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). @@ -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). @@ -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). diff --git a/agent/grpc-external/services/connectca/watch_roots.go b/agent/grpc-external/services/connectca/watch_roots.go index b2ee9f491..09dfd9572 100644 --- a/agent/grpc-external/services/connectca/watch_roots.go +++ b/agent/grpc-external/services/connectca/watch_roots.go @@ -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 } diff --git a/agent/grpc-external/services/connectca/watch_roots_test.go b/agent/grpc-external/services/connectca/watch_roots_test.go index f5c65a620..1f7b8e896 100644 --- a/agent/grpc-external/services/connectca/watch_roots_test.go +++ b/agent/grpc-external/services/connectca/watch_roots_test.go @@ -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) @@ -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) @@ -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{ @@ -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) { @@ -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) diff --git a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go b/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go index 55aeca0e3..a96ef9254 100644 --- a/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go +++ b/agent/grpc-external/services/dataplane/get_envoy_bootstrap_params_test.go @@ -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) @@ -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) @@ -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) diff --git a/agent/grpc-external/services/dataplane/get_supported_features.go b/agent/grpc-external/services/dataplane/get_supported_features.go index 4d3abc0ed..a37d381b2 100644 --- a/agent/grpc-external/services/dataplane/get_supported_features.go +++ b/agent/grpc-external/services/dataplane/get_supported_features.go @@ -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" ) @@ -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{ diff --git a/agent/grpc-external/services/dataplane/get_supported_features_test.go b/agent/grpc-external/services/dataplane/get_supported_features_test.go index 52ff3f30a..31667e71d 100644 --- a/agent/grpc-external/services/dataplane/get_supported_features_test.go +++ b/agent/grpc-external/services/dataplane/get_supported_features_test.go @@ -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) @@ -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). @@ -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) @@ -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) +} diff --git a/agent/grpc-external/services/serverdiscovery/watch_servers.go b/agent/grpc-external/services/serverdiscovery/watch_servers.go index de3977be8..a672560b9 100644 --- a/agent/grpc-external/services/serverdiscovery/watch_servers.go +++ b/agent/grpc-external/services/serverdiscovery/watch_servers.go @@ -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" @@ -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 } @@ -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 { diff --git a/agent/grpc-external/services/serverdiscovery/watch_servers_test.go b/agent/grpc-external/services/serverdiscovery/watch_servers_test.go index a57c0f985..91570e395 100644 --- a/agent/grpc-external/services/serverdiscovery/watch_servers_test.go +++ b/agent/grpc-external/services/serverdiscovery/watch_servers_test.go @@ -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} @@ -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} @@ -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) { diff --git a/agent/grpc-external/testutils/acl.go b/agent/grpc-external/testutils/acl.go index fab3646e7..18fbd2446 100644 --- a/agent/grpc-external/testutils/acl.go +++ b/agent/grpc-external/testutils/acl.go @@ -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(` @@ -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{ @@ -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} } diff --git a/agent/grpc-external/utils.go b/agent/grpc-external/utils.go index c2c77ace6..898c53feb 100644 --- a/agent/grpc-external/utils.go +++ b/agent/grpc-external/utils.go @@ -1,6 +1,14 @@ package external -import "github.com/hashicorp/go-uuid" +import ( + "github.com/hashicorp/go-uuid" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/acl/resolver" + "github.com/hashicorp/consul/agent/structs" +) // We tag logs with a unique identifier to ease debugging. In the future this // should probably be a real Open Telemetry trace ID. @@ -11,3 +19,26 @@ func TraceID() string { } return id } + +type ACLResolver interface { + ResolveTokenAndDefaultMeta(string, *acl.EnterpriseMeta, *acl.AuthorizerContext) (resolver.Result, error) +} + +// RequireAnyValidACLToken checks that the caller provided a valid ACL token +// without requiring any specific permissions. This is useful for endpoints +// that are used by all/most consumers of our API, such as those called by the +// consul-server-connection-manager library when establishing a new connection. +// +// Note: no token is required if ACLs are disabled. +func RequireAnyValidACLToken(resolver ACLResolver, token string) error { + authz, err := resolver.ResolveTokenAndDefaultMeta(token, nil, nil) + if err != nil { + return status.Error(codes.Unauthenticated, err.Error()) + } + + if id := authz.ACLIdentity; id == nil || id.ID() == structs.ACLTokenAnonymousID { + return status.Error(codes.Unauthenticated, "An ACL token must be provided (via the `x-consul-token` metadata field) to call this endpoint") + } + + return nil +}