fix(dex): always request federated:id scope (#17908) (#21726)

Signed-off-by: Alexandre Gaudreault <[email protected]>

common/common.go

@@ -158,6 +158,8 @@ const (
 	ArgoCDCLIClientAppName = "Argo CD CLI"
 	// ArgoCDCLIClientAppID is the Oauth client ID we will use when registering our CLI to dex
 	ArgoCDCLIClientAppID = "argo-cd-cli"
+	// DexFederatedScope allows to receive the federated_claims from Dex. https://dexidp.io/docs/configuration/custom-scopes-claims-clients/
+	DexFederatedScope = "federated:id"
 )
 
 // Resource metadata labels and annotations (keys and values) used by Argo CD components

docs/operator-manual/upgrading/2.14-3.0.md

@@ -25,9 +25,9 @@ Read the [RBAC documentation](../rbac.md#fine-grained-permissions-for-updatedele
 
 ### Removal of `argocd_app_sync_status`, `argocd_app_health_status` and `argocd_app_created_time` Metrics
 
-The `argocd_app_sync_status`, `argocd_app_health_status` and `argocd_app_created_time`, deprecated and disabled by 
+The `argocd_app_sync_status`, `argocd_app_health_status` and `argocd_app_created_time`, deprecated and disabled by
 default since 1.5.0, have been removed. The information previously provided by these metrics is now available as labels
-on the  `argocd_app_info` metric.
+on the `argocd_app_info` metric.
 
 #### Detection
 
@@ -39,6 +39,38 @@ If it is not set to true, you can safely upgrade with no changes.
 If you are using these metrics, you will need to update your monitoring dashboards and alerts to use the new metric and
 labels before upgrading.
 
+### Changes to RBAC with Dex SSO Authentication
+
+When using Dex, the `sub` claim returned in the authentication was used as the subject for RBAC. That value depends on
+the Dex internal implementation and should not be considered an immutable value that represent the subject.
+
+The new behavior will request the `federated:id` [scope](https://dexidp.io/docs/configuration/custom-scopes-claims-clients/) from Dex, and the new value used as the RBAC subject will be based
+on the `federated_claims.user_id` claim instead of the `sub` claim.
+
+If you were using the Dex sub claim in RBAC policies, you will need to update them to maintain the same access.
+
+You can know the correct `user_id` to use by decoding the current `sub` claims defined in your policies. You can also configure which
+value is used as `user_id` for some [connectors](https://dexidp.io/docs/connectors/).
+
+```sh
+$> echo "ChdleGFtcGxlQGFyZ29wcm9qLmlvEgJkZXhfY29ubl9pZA" | base64 -d
+
+[email protected]_conn_i%
+```
+
+```yaml
+# Policies based on the Dex sub claim (wrong)
+- g, ChdleGFtcGxlQGFyZ29wcm9qLmlvEgJkZXhfY29ubl9pZA, role:example
+- p, ChdleGFtcGxlQGFyZ29wcm9qLmlvEgJkZXhfY29ubl9pZA, applications, *, *, allow
+
+# Policies now based on federated_claims.user_id claim (correct)
+- g, [email protected], role:example
+- p, [email protected], applications, *, *, allow
+```
+
+If authenticating with the CLI, make sure to use the new version as well to obtain an authentication token with the
+appropriate claims.
+
 ## Other changes
 
 ### Using `cluster.inClusterEnabled: "false"`

pkg/apiclient/apiclient.go

@@ -327,9 +327,10 @@ func (c *client) OIDCConfig(ctx context.Context, set *settingspkg.Settings) (*oa
 			clientID = set.OIDCConfig.ClientID
 		}
 		issuerURL = set.OIDCConfig.Issuer
-		scopes = set.OIDCConfig.Scopes
+		scopes = oidcutil.GetScopesOrDefault(set.OIDCConfig.Scopes)
 	case set.DexConfig != nil && len(set.DexConfig.Connectors) > 0:
 		clientID = common.ArgoCDCLIClientAppID
+		scopes = append(oidcutil.GetScopesOrDefault(nil), common.DexFederatedScope)
 		issuerURL = fmt.Sprintf("%s%s", set.URL, common.DexAPIEndpoint)
 	default:
 		return nil, nil, fmt.Errorf("%s is not configured with SSO", c.ServerAddr)
@@ -342,7 +343,6 @@ func (c *client) OIDCConfig(ctx context.Context, set *settingspkg.Settings) (*oa
 	if err != nil {
 		return nil, nil, fmt.Errorf("Failed to parse provider config: %w", err)
 	}
-	scopes = oidcutil.GetScopesOrDefault(scopes)
 	if oidcutil.OfflineAccess(oidcConf.ScopesSupported) {
 		scopes = append(scopes, oidc.ScopeOfflineAccess)
 	}

util/oidc/oidc.go

@@ -314,10 +314,13 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
 	scopes := make([]string, 0)
 	var opts []oauth2.AuthCodeOption
 	if config := a.settings.OIDCConfig(); config != nil {
-		scopes = config.RequestedScopes
+		scopes = GetScopesOrDefault(config.RequestedScopes)
 		opts = AppendClaimsAuthenticationRequestParameter(opts, config.RequestedIDTokenClaims)
+	} else if a.settings.IsDexConfigured() {
+		scopes = append(GetScopesOrDefault(nil), common.DexFederatedScope)
 	}
-	oauth2Config, err := a.oauth2Config(r, GetScopesOrDefault(scopes))
+
+	oauth2Config, err := a.oauth2Config(r, scopes)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return

util/oidc/oidc_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
+	"sigs.k8s.io/yaml"
 
 	"github.com/argoproj/argo-cd/v3/common"
 	"github.com/argoproj/argo-cd/v3/server/settings/oidc"
@@ -204,6 +205,108 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
 		assert.NotContains(t, w.Body.String(), "certificate signed by unknown authority")
 	})
 
+	t.Run("OIDC auth", func(t *testing.T) {
+		cdSettings := &settings.ArgoCDSettings{
+			URL:                       "https://argocd.example.com",
+			OIDCTLSInsecureSkipVerify: true,
+		}
+		oidcConfig := settings.OIDCConfig{
+			Name:         "Test",
+			Issuer:       oidcTestServer.URL,
+			ClientID:     "xxx",
+			ClientSecret: "yyy",
+		}
+		oidcConfigRaw, err := yaml.Marshal(oidcConfig)
+		require.NoError(t, err)
+		cdSettings.OIDCConfigRAW = string(oidcConfigRaw)
+
+		app, err := NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour))
+		require.NoError(t, err)
+
+		req := httptest.NewRequest(http.MethodGet, "https://argocd.example.com/auth/login", nil)
+		w := httptest.NewRecorder()
+		app.HandleLogin(w, req)
+
+		assert.Equal(t, http.StatusSeeOther, w.Code)
+		location, err := url.Parse(w.Header().Get("Location"))
+		require.NoError(t, err)
+		values, err := url.ParseQuery(location.RawQuery)
+		require.NoError(t, err)
+		assert.Equal(t, []string{"openid", "profile", "email", "groups"}, strings.Split(values.Get("scope"), " "))
+		assert.Equal(t, "xxx", values.Get("client_id"))
+		assert.Equal(t, "code", values.Get("response_type"))
+	})
+
+	t.Run("OIDC auth with custom scopes", func(t *testing.T) {
+		cdSettings := &settings.ArgoCDSettings{
+			URL:                       "https://argocd.example.com",
+			OIDCTLSInsecureSkipVerify: true,
+		}
+		oidcConfig := settings.OIDCConfig{
+			Name:            "Test",
+			Issuer:          oidcTestServer.URL,
+			ClientID:        "xxx",
+			ClientSecret:    "yyy",
+			RequestedScopes: []string{"oidc"},
+		}
+		oidcConfigRaw, err := yaml.Marshal(oidcConfig)
+		require.NoError(t, err)
+		cdSettings.OIDCConfigRAW = string(oidcConfigRaw)
+
+		app, err := NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour))
+		require.NoError(t, err)
+
+		req := httptest.NewRequest(http.MethodGet, "https://argocd.example.com/auth/login", nil)
+		w := httptest.NewRecorder()
+		app.HandleLogin(w, req)
+
+		assert.Equal(t, http.StatusSeeOther, w.Code)
+		location, err := url.Parse(w.Header().Get("Location"))
+		require.NoError(t, err)
+		values, err := url.ParseQuery(location.RawQuery)
+		require.NoError(t, err)
+		assert.Equal(t, []string{"oidc"}, strings.Split(values.Get("scope"), " "))
+		assert.Equal(t, "xxx", values.Get("client_id"))
+		assert.Equal(t, "code", values.Get("response_type"))
+	})
+
+	t.Run("Dex auth", func(t *testing.T) {
+		cdSettings := &settings.ArgoCDSettings{
+			URL: dexTestServer.URL,
+		}
+		dexConfig := map[string]any{
+			"connectors": []map[string]any{
+				{
+					"type": "github",
+					"name": "GitHub",
+					"config": map[string]any{
+						"clientId":     "aabbccddeeff00112233",
+						"clientSecret": "aabbccddeeff00112233",
+					},
+				},
+			},
+		}
+		dexConfigRaw, err := yaml.Marshal(dexConfig)
+		require.NoError(t, err)
+		cdSettings.DexConfig = string(dexConfigRaw)
+
+		app, err := NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour))
+		require.NoError(t, err)
+
+		req := httptest.NewRequest(http.MethodGet, "https://argocd.example.com/auth/login", nil)
+		w := httptest.NewRecorder()
+		app.HandleLogin(w, req)
+
+		assert.Equal(t, http.StatusSeeOther, w.Code)
+		location, err := url.Parse(w.Header().Get("Location"))
+		require.NoError(t, err)
+		values, err := url.ParseQuery(location.RawQuery)
+		require.NoError(t, err)
+		assert.Equal(t, []string{"openid", "profile", "email", "groups", common.DexFederatedScope}, strings.Split(values.Get("scope"), " "))
+		assert.Equal(t, common.ArgoCDClientAppID, values.Get("client_id"))
+		assert.Equal(t, "code", values.Get("response_type"))
+	})
+
 	t.Run("with additional base URL", func(t *testing.T) {
 		cdSettings := &settings.ArgoCDSettings{
 			URL:                       "https://argocd.example.com",