fix: add CLI client IDs to default OIDC allowed audiences (argoproj#1…

…2170) (argoproj#12179) * fix(settings): add CLI client ID in default OAuth2 allowed audiences Signed-off-by: Yann Soubeyrand <yann.soubeyrand@camptocamp.com> * fix: add CLI client IDs to default OIDC allowed audiences (argoproj#12170) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * docs Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * handle expired token properly Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Yann Soubeyrand <yann.soubeyrand@camptocamp.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Yann Soubeyrand <yann.soubeyrand@camptocamp.com> Signed-off-by: schakrad <chakradari.sindhu@gmail.com>
schakrad · Mar 14, 2023 · 96ec8ea · 96ec8ea
1 parent cfe3b45
commit 96ec8ea
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 4 deletions.
diff --git a/docs/operator-manual/user-management/index.md b/docs/operator-manual/user-management/index.md
@@ -302,8 +302,9 @@ data:
     clientID: aaaabbbbccccddddeee
     clientSecret: $oidc.okta.clientSecret
     
-    # Optional list of allowed aud claims. If omitted or empty, defaults to the clientID value above. If you specify a 
-    # list and want the clientD to be allowed, you must explicitly include it in the list.
+    # Optional list of allowed aud claims. If omitted or empty, defaults to the clientID value above (and the 
+    # cliCientID, if that is also specified). If you specify a list and want the clientID to be allowed, you must 
+    # explicitly include it in the list.
     # Token verification will pass if any of the token's audiences matches any of the audiences in this list.
     allowedAudiences:
     - aaaabbbbccccddddeee

diff --git a/util/oidc/provider.go b/util/oidc/provider.go
@@ -107,6 +107,13 @@ func (p *providerImpl) Verify(tokenString string, argoSettings *settings.ArgoCDS
 		// Token must be verified for at least one allowed audience
 		for _, aud := range allowedAudiences {
 			idToken, err = p.verify(aud, tokenString, false)
+			tokenExpiredError := &gooidc.TokenExpiredError{}
+			if errors.As(err, &tokenExpiredError) {
+				// If the token is expired, we won't bother checking other audiences. It's important to return a
+				// TokenExpiredError instead of an error related to an incorrect audience, because the caller may
+				// have specific behavior to handle expired tokens.
+				break
+			}
 			if err == nil {
 				break
 			}

diff --git a/util/settings/settings.go b/util/settings/settings.go
@@ -1743,12 +1743,16 @@ func (a *ArgoCDSettings) OAuth2ClientID() string {
 func (a *ArgoCDSettings) OAuth2AllowedAudiences() []string {
 	if config := a.oidcConfig(); config != nil {
 		if len(config.AllowedAudiences) == 0 {
-			return []string{config.ClientID}
+			allowedAudiences := []string{config.ClientID}
+			if config.CLIClientID != "" {
+				allowedAudiences = append(allowedAudiences, config.CLIClientID)
+			}
+			return allowedAudiences
 		}
 		return config.AllowedAudiences
 	}
 	if a.DexConfig != "" {
-		return []string{common.ArgoCDClientAppID}
+		return []string{common.ArgoCDClientAppID, common.ArgoCDCLIClientAppID}
 	}
 	return nil
 }

diff --git a/util/settings/settings_test.go b/util/settings/settings_test.go
@@ -1343,3 +1343,68 @@ rootCA: "invalid"`},
 		})
 	}
 }
+
+func Test_OAuth2AllowedAudiences(t *testing.T) {
+	testCases := []struct {
+		name     string
+		settings *ArgoCDSettings
+		expected []string
+	}{
+		{
+			name:     "Empty",
+			settings: &ArgoCDSettings{},
+			expected: []string{},
+		},
+		{
+			name: "OIDC configured, no audiences specified, clientID used",
+			settings: &ArgoCDSettings{OIDCConfigRAW: `name: Test
+issuer: aaa
+clientID: xxx
+clientSecret: yyy
+requestedScopes: ["oidc"]`},
+			expected: []string{"xxx"},
+		},
+		{
+			name: "OIDC configured, no audiences specified, clientID and cliClientID used",
+			settings: &ArgoCDSettings{OIDCConfigRAW: `name: Test
+issuer: aaa
+clientID: xxx
+cliClientID: cli-xxx
+clientSecret: yyy
+requestedScopes: ["oidc"]`},
+			expected: []string{"xxx", "cli-xxx"},
+		},
+		{
+			name: "OIDC configured, audiences specified",
+			settings: &ArgoCDSettings{OIDCConfigRAW: `name: Test
+issuer: aaa
+clientID: xxx
+clientSecret: yyy
+requestedScopes: ["oidc"]
+allowedAudiences: ["aud1", "aud2"]`},
+			expected: []string{"aud1", "aud2"},
+		},
+		{
+			name: "Dex configured",
+			settings: &ArgoCDSettings{DexConfig: `connectors:
+  - type: github
+    id: github
+    name: GitHub
+    config:
+      clientID: aabbccddeeff00112233
+      clientSecret: $dex.github.clientSecret
+      orgs:
+      - name: your-github-org
+`},
+			expected: []string{common.ArgoCDClientAppID, common.ArgoCDCLIClientAppID},
+		},
+	}
+
+	for _, tc := range testCases {
+		tcc := tc
+		t.Run(tcc.name, func(t *testing.T) {
+			t.Parallel()
+			assert.ElementsMatch(t, tcc.expected, tcc.settings.OAuth2AllowedAudiences())
+		})
+	}
+}