From 29cdd31572c0a10f0f698e57d6dc53d1039f0fad Mon Sep 17 00:00:00 2001
From: "gcp-cherry-pick-bot[bot]"
 <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com>
Date: Wed, 17 Apr 2024 21:12:13 -0400
Subject: [PATCH] fix(api): respect all allowed audiences, regardless of check
 order (#17876) (#17878)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
---
 util/oidc/provider.go               |  4 +++-
 util/session/sessionmanager_test.go | 37 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/util/oidc/provider.go b/util/oidc/provider.go
index d75bcf97efecd..2603f927574d3 100644
--- a/util/oidc/provider.go
+++ b/util/oidc/provider.go
@@ -135,7 +135,9 @@ func (p *providerImpl) Verify(tokenString string, argoSettings *settings.ArgoCDS
 			// to avoid logging irrelevant warnings: https://github.com/coreos/go-oidc/pull/406
 			tokenVerificationErrors[aud] = err
 		}
-		if len(tokenVerificationErrors) > 0 {
+		// If the most recent attempt encountered an error, and if we have collected multiple errors, switch to the
+		// other error type to gather more context.
+		if err != nil && len(tokenVerificationErrors) > 0 {
 			err = tokenVerificationError{errorsByAudience: tokenVerificationErrors}
 		}
 	}
diff --git a/util/session/sessionmanager_test.go b/util/session/sessionmanager_test.go
index 817966376daa3..0f399df334564 100644
--- a/util/session/sessionmanager_test.go
+++ b/util/session/sessionmanager_test.go
@@ -1137,6 +1137,43 @@ allowedAudiences: []`, oidcTestServer.URL),
 		assert.ErrorIs(t, err, common.TokenVerificationErr)
 	})
 
+	// Make sure the logic works to allow any of the allowed audiences, not just the first one.
+	t.Run("OIDC provider is external, audience is specified, actual audience isn't the first allowed audience", func(t *testing.T) {
+		config := map[string]string{
+			"url": "",
+			"oidc.config": fmt.Sprintf(`
+name: Test
+issuer: %s
+clientID: xxx
+clientSecret: yyy
+requestedScopes: ["oidc"]
+allowedAudiences: ["aud-a", "aud-b"]`, oidcTestServer.URL),
+			"oidc.tls.insecure.skip.verify": "true", // This isn't what we're testing.
+		}
+
+		// This is not actually used in the test. The test only calls the OIDC test server. But a valid cert/key pair
+		// must be set to test VerifyToken's behavior when Argo CD is configured with TLS enabled.
+		secretConfig := map[string][]byte{
+			"tls.crt": utiltest.Cert,
+			"tls.key": utiltest.PrivateKey,
+		}
+
+		settingsMgr := settings.NewSettingsManager(context.Background(), getKubeClientWithConfig(config, secretConfig), "argocd")
+		mgr := NewSessionManager(settingsMgr, getProjLister(), "", nil, NewUserStateStorage(nil))
+		mgr.verificationDelayNoiseEnabled = false
+
+		claims := jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"aud-b"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))}
+		claims.Issuer = oidcTestServer.URL
+		token := jwt.NewWithClaims(jwt.SigningMethodRS512, claims)
+		key, err := jwt.ParseRSAPrivateKeyFromPEM(utiltest.PrivateKey)
+		require.NoError(t, err)
+		tokenString, err := token.SignedString(key)
+		require.NoError(t, err)
+
+		_, _, err = mgr.VerifyToken(tokenString)
+		assert.NoError(t, err)
+	})
+
 	t.Run("OIDC provider is external, audience is not specified, token is signed with the wrong key", func(t *testing.T) {
 		config := map[string]string{
 			"url": "",