grpc/acl: fix bug where ACL token was required even if disabled (#15904)

Fixes a bug introduced by #15346 where we'd always require an ACL token even if ACLs were disabled because we were erroneously treating `nil` identity as anonymous.
hashicorp · Jan 5, 2023 · b78de5a · b78de5a
1 parent ee2d47d
commit b78de5a
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 11 deletions.
diff --git 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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(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.ACLAllowAll(t), nil)
+		Return(testutils.ACLsDisabled(t), nil)
 
 	caManager := &MockCAManager{}
 	caManager.On("AuthorizeAndSignCertificate", mock.Anything, mock.Anything).

diff --git a/agent/grpc-external/services/dataplane/get_supported_features_test.go b/agent/grpc-external/services/dataplane/get_supported_features_test.go
@@ -53,6 +53,25 @@ func TestSupportedDataplaneFeatures_Success(t *testing.T) {
 	}
 }
 
+func TestSupportedDataplaneFeatures_ACLsDisabled(t *testing.T) {
+	aclResolver := &MockACLResolver{}
+	aclResolver.On("ResolveTokenAndDefaultMeta", "", mock.Anything, mock.Anything).
+		Return(testutils.ACLsDisabled(t), nil)
+
+	options := structs.QueryOptions{Token: ""}
+	ctx, err := external.ContextWithQueryOptions(context.Background(), options)
+	require.NoError(t, err)
+
+	server := NewServer(Config{
+		Logger:      hclog.NewNullLogger(),
+		ACLResolver: aclResolver,
+	})
+	client := testClient(t, server)
+	resp, err := client.GetSupportedDataplaneFeatures(ctx, &pbdataplane.GetSupportedDataplaneFeaturesRequest{})
+	require.NoError(t, err)
+	require.Equal(t, 3, len(resp.SupportedDataplaneFeatures))
+}
+
 func TestSupportedDataplaneFeatures_InvalidACLToken(t *testing.T) {
 	// Mock the ACL resolver to return ErrNotFound.
 	aclResolver := &MockACLResolver{}

diff --git a/agent/grpc-external/testutils/acl.go b/agent/grpc-external/testutils/acl.go
@@ -23,12 +23,11 @@ func ACLAnonymous(t *testing.T) resolver.Result {
 	}
 }
 
-func ACLAllowAll(t *testing.T) resolver.Result {
+func ACLsDisabled(t *testing.T) resolver.Result {
 	t.Helper()
 
 	return resolver.Result{
-		Authorizer:  acl.AllowAll(),
-		ACLIdentity: randomACLIdentity(t),
+		Authorizer: acl.ManageAll(),
 	}
 }
 

diff --git a/agent/grpc-external/utils.go b/agent/grpc-external/utils.go
@@ -36,7 +36,7 @@ func RequireAnyValidACLToken(resolver ACLResolver, token string) error {
 		return status.Error(codes.Unauthenticated, err.Error())
 	}
 
-	if id := authz.ACLIdentity; id == nil || id.ID() == structs.ACLTokenAnonymousID {
+	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")
 	}