Backport of [SECVULN-8636] Filter ACL for anonymous token along with empty token into release/1.21.x (#22678)

* backport of commit 6af949c

* [SECVULN-8636] Filter ACL for anonymous token along with empty token (#22534)

* (agent/rpc) update SetQueryMetadata to check for anonymous token

* Extract functionality into original method

* Improve tests and remove regression

* Add test for anonymous token, fix test for authenticated calls

* Add changelog file

* Update test for anonymous token in intention-endpoint

* Keep anonymous variables package-private

* Restructure the maskResults function

* Fix lint issue

---------

Co-authored-by: Sujay Kumar Suman <sujaykumar.suman@hashicorp.com>

---------

Co-authored-by: Sujay Kumar Suman <sujaykumar.suman@hashicorp.com>
Co-authored-by: Sindhu Shree <sindhu20091@gmail.com>

Author: 3 people

.changelog/22534.txt

@@ -0,0 +1,3 @@
+```release-note:security
+agent: Fix a security vulnerability to filter out anonymous tokens along with empty tokens when setting the Results-Filtered-By-ACLs header
+```

agent/consul/acl_test.go

@@ -204,6 +204,26 @@ func testIdentityForToken(token string) (bool, structs.ACLIdentity, error) {
 				},
 			},
 		}, nil
+	case "authenticated":
+		return true, &structs.ACLToken{
+			AccessorID: "some-id",
+			SecretID:   "some-id",
+			Policies: []structs.ACLTokenPolicyLink{
+				{
+					ID: "node-wr",
+				},
+			},
+		}, nil
+	case "anonymous":
+		return true, &structs.ACLToken{
+			AccessorID: "00000000-0000-0000-0000-000000000002",
+			SecretID:   "anonymous",
+			Policies: []structs.ACLTokenPolicyLink{
+				{
+					ID: "node-wr",
+				},
+			},
+		}, nil
 	default:
 		return true, nil, acl.ErrNotFound
 	}

agent/consul/intention_endpoint_test.go

@@ -1773,7 +1773,7 @@ func TestIntentionList_acl(t *testing.T) {
 		var resp structs.IndexedIntentions
 		require.NoError(t, msgpackrpc.CallWithCodec(codec, "Intention.List", req, &resp))
 		require.Len(t, resp.Intentions, 0)
-		require.True(t, resp.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true")
+		require.False(t, resp.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false")
 	})
 }
 

agent/consul/rpc.go

@@ -92,6 +92,12 @@ const (
 	// value is ever reached. However, it prevents us from blocking
 	// the requesting goroutine forever.
 	enqueueLimit = 30 * time.Second
+
+	// AnonymousAccessorID is the accessor ID for anonymous token.
+	anonymousAccessorID = "00000000-0000-0000-0000-000000000002"
+
+	// AnonymousSecretID is the secret ID for anonymous token
+	anonymousSecretID = "anonymous"
 )
 
 var ErrChunkingResubmit = errors.New("please resubmit call for rechunking")
@@ -1045,7 +1051,7 @@ func (s *Server) SetQueryMeta(m blockingquery.ResponseMeta, token string) {
 		m.SetLastContact(time.Since(s.raft.LastContact()))
 		m.SetKnownLeader(s.raft.Leader() != "")
 	}
-	maskResultsFilteredByACLs(token, m)
+	maskResultsFilteredByACLs(token, m, s)
 
 	// Always set a non-zero QueryMeta.Index. Generally we expect the
 	// QueryMeta.Index to be set to structs.RaftIndex.ModifyIndex. If the query
@@ -1136,18 +1142,24 @@ func (s *Server) RPCQueryTimeout(queryTimeout time.Duration) time.Duration {
 //
 // Notes:
 //
-//   - The definition of "unauthenticated" here is incomplete, as it doesn't
-//     account for the fact that operators can modify the anonymous token with
-//     custom policies, or set namespace default policies. As these scenarios
-//     are less common and this flag is a best-effort UX improvement, we think
-//     the trade-off for reduced complexity is acceptable.
-//
 //   - This method assumes that the given token has already been validated (and
-//     will only check whether it is blank or not). It's a safe assumption because
+//     will only check whether it is blank or anonymous). It's a safe assumption because
 //     ResultsFilteredByACLs is only set to try when applying the already-resolved
 //     token's policies.
-func maskResultsFilteredByACLs(token string, meta blockingQueryResponseMeta) {
+func maskResultsFilteredByACLs(token string, m blockingQueryResponseMeta, s *Server) {
 	if token == "" {
-		meta.SetResultsFilteredByACLs(false)
+		m.SetResultsFilteredByACLs(false)
+		return
+	}
+
+	identity, err := s.resolveIdentityFromToken(token)
+	if err != nil {
+		s.rpcLogger().Error("Failed to resolve identity from token", "err", err)
+		m.SetResultsFilteredByACLs(false)
+		return
+	}
+
+	if identity.ID() == anonymousAccessorID && identity.SecretToken() == anonymousSecretID {
+		m.SetResultsFilteredByACLs(false)
 	}
 }

agent/consul/rpc_test.go

@@ -39,7 +39,6 @@ import (
 	"github.com/hashicorp/consul/agent/structs"
 	tokenStore "github.com/hashicorp/consul/agent/token"
 	"github.com/hashicorp/consul/api"
-	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/proto/private/pbsubscribe"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
@@ -234,9 +233,17 @@ func (m *MockSink) Close() error {
 // other blocking query tests reside in blockingquery_test.go in the blockingquery package.
 func TestServer_blockingQuery(t *testing.T) {
 	t.Parallel()
-	_, s := testServerWithConfig(t)
+	_, s := testServerWithConfig(t, testServerACLConfig)
+	delegate := ACLResolverTestDelegate{
+		enabled:    true,
+		datacenter: "dc1",
+	}
+	delegate.tokenReadFn = delegate.defaultTokenReadFn(errRPC)
+	r := newTestACLResolver(t, &delegate, nil)
+	s.ACLResolver = r
 
 	t.Run("ResultsFilteredByACLs is reset for unauthenticated calls", func(t *testing.T) {
+		// empty token
 		opts := structs.QueryOptions{
 			Token: "",
 		}
@@ -249,24 +256,36 @@ func TestServer_blockingQuery(t *testing.T) {
 		err := s.blockingQuery(&opts, &meta, fn)
 		require.NoError(t, err)
 		require.False(t, meta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be reset for unauthenticated calls")
-	})
 
-	t.Run("ResultsFilteredByACLs is honored for authenticated calls", func(t *testing.T) {
-		token, err := lib.GenerateUUID(nil)
+		// anonymous token
+		anonOpts := structs.QueryOptions{
+			Token: "anonymous", // secret id of anonymous token
+		}
+		var anonMeta structs.QueryMeta
+		anonFn := func(_ memdb.WatchSet, _ *state.Store) error {
+			anonMeta.ResultsFilteredByACLs = true
+			return nil
+		}
+
+		err = s.blockingQuery(&anonOpts, &anonMeta, anonFn)
 		require.NoError(t, err)
+		require.False(t, anonMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be reset for unauthenticated calls")
+	})
 
-		opts := structs.QueryOptions{
-			Token: token,
+	t.Run("ResultsFilteredByACLs is honored for authenticated calls", func(t *testing.T) {
+		delegate.localTokens = true
+		authOpts := structs.QueryOptions{
+			Token: "authenticated",
 		}
-		var meta structs.QueryMeta
-		fn := func(_ memdb.WatchSet, _ *state.Store) error {
-			meta.ResultsFilteredByACLs = true
+		var authMeta structs.QueryMeta
+		authFn := func(_ memdb.WatchSet, _ *state.Store) error {
+			authMeta.ResultsFilteredByACLs = true
 			return nil
 		}
 
-		err = s.blockingQuery(&opts, &meta, fn)
+		err := s.blockingQuery(&authOpts, &authMeta, authFn)
 		require.NoError(t, err)
-		require.True(t, meta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be honored for authenticated calls")
+		require.True(t, authMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be honored for authenticated calls")
 	})
 }