feature: pkce for UI argoproj#9890

Signed-off-by: mmerrill3 <jjpaacks@gmail.com>

cmd/argocd/commands/login.go

@@ -2,8 +2,6 @@ package commands
 
 import (
 	"context"
-	"crypto/sha256"
-	"encoding/base64"
 	"fmt"
 	"html"
 	"net/http"
@@ -228,14 +226,7 @@ func oauth2Login(
 		completionChan <- errMsg
 	}
 
-	// PKCE implementation of https://tools.ietf.org/html/rfc7636
-	codeVerifier, err := rand.StringFromCharset(
-		43,
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~",
-	)
-	errors.CheckError(err)
-	codeChallengeHash := sha256.Sum256([]byte(codeVerifier))
-	codeChallenge := base64.RawURLEncoding.EncodeToString(codeChallengeHash[:])
+	pkceCodes := oidcutil.GeneratePKCECodes()
 
 	// Authorization redirect callback from OAuth2 auth flow.
 	// Handles both implicit and authorization code flow
@@ -276,7 +267,7 @@ func oauth2Login(
 				handleErr(w, fmt.Sprintf("no code in request: %q", r.Form))
 				return
 			}
-			opts := []oauth2.AuthCodeOption{oauth2.SetAuthURLParam("code_verifier", codeVerifier)}
+			opts := []oauth2.AuthCodeOption{oauth2.SetAuthURLParam("code_verifier", pkceCodes.CodeVerifier)}
 			tok, err := oauth2conf.Exchange(ctx, code, opts...)
 			if err != nil {
 				handleErr(w, err.Error())
@@ -313,8 +304,8 @@ func oauth2Login(
 
 	switch grantType {
 	case oidcutil.GrantTypeAuthorizationCode:
-		opts = append(opts, oauth2.SetAuthURLParam("code_challenge", codeChallenge))
-		opts = append(opts, oauth2.SetAuthURLParam("code_challenge_method", "S256"))
+		opts = append(opts, oauth2.SetAuthURLParam("code_challenge", pkceCodes.CodeChallenge))
+		opts = append(opts, oauth2.SetAuthURLParam("code_challenge_method", pkceCodes.CodeChallengeHash))
 		url = oauth2conf.AuthCodeURL(stateNonce, opts...)
 	case oidcutil.GrantTypeImplicit:
 		url, err = oidcutil.ImplicitFlowURL(oauth2conf, stateNonce, opts...)

util/oidc/oidc.go

@@ -1,6 +1,8 @@
 package oidc
 
 import (
+	"crypto/sha256"
+	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
@@ -23,6 +25,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/server/settings/oidc"
 	"github.com/argoproj/argo-cd/v2/util/crypto"
 	"github.com/argoproj/argo-cd/v2/util/dex"
+	"github.com/argoproj/argo-cd/v2/util/errors"
 	httputil "github.com/argoproj/argo-cd/v2/util/http"
 	"github.com/argoproj/argo-cd/v2/util/rand"
 	"github.com/argoproj/argo-cd/v2/util/settings"
@@ -79,6 +82,28 @@ func GetScopesOrDefault(scopes []string) []string {
 	return scopes
 }
 
+type PKCECodes struct {
+	CodeVerifier      string
+	CodeChallenge     string
+	CodeChallengeHash string
+}
+
+func GeneratePKCECodes() *PKCECodes {
+	// PKCE implementation of https://tools.ietf.org/html/rfc7636
+	codeVerifier, err := rand.StringFromCharset(
+		43,
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~",
+	)
+	errors.CheckError(err)
+	codeChallengeHash := sha256.Sum256([]byte(codeVerifier))
+	codeChallenge := base64.RawURLEncoding.EncodeToString(codeChallengeHash[:])
+	return &PKCECodes{
+		CodeVerifier:      codeVerifier,
+		CodeChallenge:     codeChallenge,
+		CodeChallengeHash: "S256",
+	}
+}
+
 // NewClientApp will register the Argo CD client app (either via Dex or external OIDC) and return an
 // object which has HTTP handlers for handling the HTTP responses for login and callback
 func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTlsConfig *dex.DexTLSConfig, baseHRef string) (*ClientApp, error) {
@@ -150,19 +175,20 @@ func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) {
 }
 
 // generateAppState creates an app state nonce
-func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (string, error) {
+func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (string, *PKCECodes, error) {
 	// According to the spec (https://www.rfc-editor.org/rfc/rfc6749#section-10.10), this must be guessable with
 	// probability <= 2^(-128). The following call generates one of 52^24 random strings, ~= 2^136 possibilities.
 	randStr, err := rand.String(24)
 	if err != nil {
-		return "", fmt.Errorf("failed to generate app state: %w", err)
+		return "", nil, fmt.Errorf("failed to generate app state: %w", err)
 	}
+	pkceCodes := GeneratePKCECodes()
 	if returnURL == "" {
 		returnURL = a.baseHRef
 	}
-	cookieValue := fmt.Sprintf("%s:%s", randStr, returnURL)
+	cookieValue := fmt.Sprintf("%s:%s:%s", randStr, pkceCodes.CodeVerifier, returnURL)
 	if encrypted, err := crypto.Encrypt([]byte(cookieValue), a.encryptionKey); err != nil {
-		return "", err
+		return "", nil, err
 	} else {
 		cookieValue = hex.EncodeToString(encrypted)
 	}
@@ -175,38 +201,40 @@ func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (s
 		SameSite: http.SameSiteLaxMode,
 		Secure:   a.secureCookie,
 	})
-	return randStr, nil
+	return randStr, pkceCodes, nil
 }
 
-func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state string) (string, error) {
+func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state string) (string, string, error) {
 	c, err := r.Cookie(common.StateCookieName)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	val, err := hex.DecodeString(c.Value)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	val, err = crypto.Decrypt(val, a.encryptionKey)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	cookieVal := string(val)
 	redirectURL := a.baseHRef
-	parts := strings.SplitN(cookieVal, ":", 2)
-	if len(parts) == 2 && parts[1] != "" {
-		if !isValidRedirectURL(parts[1], []string{a.settings.URL, a.baseHRef}) {
-			sanitizedUrl := parts[1]
+	codeVerifier := ""
+	parts := strings.SplitN(cookieVal, ":", 3)
+	if len(parts) == 3 && parts[2] != "" {
+		if !isValidRedirectURL(parts[2], []string{a.settings.URL, a.baseHRef}) {
+			sanitizedUrl := parts[2]
 			if len(sanitizedUrl) > 100 {
 				sanitizedUrl = sanitizedUrl[:100]
 			}
 			log.Warnf("Failed to verify app state - got invalid redirectURL %q", sanitizedUrl)
-			return "", fmt.Errorf("failed to verify app state: %w", InvalidRedirectURLError)
+			return "", "", fmt.Errorf("failed to verify app state: %w", InvalidRedirectURLError)
 		}
-		redirectURL = parts[1]
+		redirectURL = parts[2]
+		codeVerifier = parts[1]
 	}
 	if parts[0] != state {
-		return "", fmt.Errorf("invalid state in '%s' cookie", common.AuthCookieName)
+		return "", "", fmt.Errorf("invalid state in '%s' cookie", common.AuthCookieName)
 	}
 	// set empty cookie to clear it
 	http.SetCookie(w, &http.Cookie{
@@ -216,7 +244,7 @@ func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state
 		SameSite: http.SameSiteLaxMode,
 		Secure:   a.secureCookie,
 	})
-	return redirectURL, nil
+	return redirectURL, codeVerifier, nil
 }
 
 // isValidRedirectURL checks whether the given redirectURL matches on of the
@@ -291,7 +319,7 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "Invalid redirect URL: the protocol and host (including port) must match and the path must be within allowed URLs if provided", http.StatusBadRequest)
 		return
 	}
-	stateNonce, err := a.generateAppState(returnURL, w)
+	stateNonce, pkceCodes, err := a.generateAppState(returnURL, w)
 	if err != nil {
 		log.Errorf("Failed to initiate login flow: %v", err)
 		http.Error(w, "Failed to initiate login flow", http.StatusInternalServerError)
@@ -301,6 +329,8 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
 	var url string
 	switch grantType {
 	case GrantTypeAuthorizationCode:
+		opts = append(opts, oauth2.SetAuthURLParam("code_challenge", pkceCodes.CodeChallenge))
+		opts = append(opts, oauth2.SetAuthURLParam("code_challenge_method", pkceCodes.CodeChallengeHash))
 		url = oauth2Config.AuthCodeURL(stateNonce, opts...)
 	case GrantTypeImplicit:
 		url, err = ImplicitFlowURL(oauth2Config, stateNonce, opts...)
@@ -337,13 +367,14 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
 		a.handleImplicitFlow(r, w, state)
 		return
 	}
-	returnURL, err := a.verifyAppState(r, w, state)
+	returnURL, codeVerifier, err := a.verifyAppState(r, w, state)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
 	ctx := gooidc.ClientContext(r.Context(), a.client)
-	token, err := oauth2Config.Exchange(ctx, code)
+	opts := []oauth2.AuthCodeOption{oauth2.SetAuthURLParam("code_verifier", codeVerifier)}
+	token, err := oauth2Config.Exchange(ctx, code, opts...)
 	if err != nil {
 		http.Error(w, fmt.Sprintf("failed to get token: %v", err), http.StatusInternalServerError)
 		return
@@ -428,7 +459,7 @@ func (a *ClientApp) handleImplicitFlow(r *http.Request, w http.ResponseWriter, s
 		CookieName: common.AuthCookieName,
 	}
 	if state != "" {
-		returnURL, err := a.verifyAppState(r, w, state)
+		returnURL, _, err := a.verifyAppState(r, w, state)
 		if err != nil {
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			return

util/oidc/oidc_test.go

@@ -192,6 +192,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
 }
 
 func Test_Login_Flow(t *testing.T) {
+
 	// Show that SSO login works when no redirect URL is provided, and we fall back to the configured base href for the
 	// Argo CD instance.
 
@@ -238,6 +239,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
 }
 
 func TestClientApp_HandleCallback(t *testing.T) {
+
 	oidcTestServer := test.GetOIDCTestServer(t)
 	t.Cleanup(oidcTestServer.Close)
 
@@ -409,7 +411,7 @@ func TestGenerateAppState(t *testing.T) {
 	app, err := NewClientApp(&settings.ArgoCDSettings{ServerSignature: signature, URL: expectedReturnURL}, "", nil, "")
 	require.NoError(t, err)
 	generateResponse := httptest.NewRecorder()
-	state, err := app.generateAppState(expectedReturnURL, generateResponse)
+	state, pkceCodes, err := app.generateAppState(expectedReturnURL, generateResponse)
 	require.NoError(t, err)
 
 	t.Run("VerifyAppState_Successful", func(t *testing.T) {
@@ -418,9 +420,10 @@ func TestGenerateAppState(t *testing.T) {
 			req.AddCookie(cookie)
 		}
 
-		returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), state)
+		returnURL, codeVerifier, err := app.verifyAppState(req, httptest.NewRecorder(), state)
 		assert.NoError(t, err)
 		assert.Equal(t, expectedReturnURL, returnURL)
+		assert.Equal(t, pkceCodes.CodeVerifier, codeVerifier)
 	})
 
 	t.Run("VerifyAppState_Failed", func(t *testing.T) {
@@ -429,7 +432,7 @@ func TestGenerateAppState(t *testing.T) {
 			req.AddCookie(cookie)
 		}
 
-		_, err := app.verifyAppState(req, httptest.NewRecorder(), "wrong state")
+		_, _, err := app.verifyAppState(req, httptest.NewRecorder(), "wrong state")
 		assert.Error(t, err)
 	})
 }
@@ -454,33 +457,35 @@ func TestGenerateAppState_XSS(t *testing.T) {
 
 		expectedReturnURL := "javascript: alert('hi')"
 		generateResponse := httptest.NewRecorder()
-		state, err := app.generateAppState(expectedReturnURL, generateResponse)
+		state, _, err := app.generateAppState(expectedReturnURL, generateResponse)
 		require.NoError(t, err)
 
 		req := httptest.NewRequest("GET", "/", nil)
 		for _, cookie := range generateResponse.Result().Cookies() {
 			req.AddCookie(cookie)
 		}
 
-		returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), state)
+		returnURL, codeVerifier, err := app.verifyAppState(req, httptest.NewRecorder(), state)
 		assert.ErrorIs(t, err, InvalidRedirectURLError)
 		assert.Empty(t, returnURL)
+		assert.Empty(t, codeVerifier)
 	})
 
 	t.Run("valid return URL succeeds", func(t *testing.T) {
 		expectedReturnURL := "https://argocd.example.com/some/path"
 		generateResponse := httptest.NewRecorder()
-		state, err := app.generateAppState(expectedReturnURL, generateResponse)
+		state, pkceCodes, err := app.generateAppState(expectedReturnURL, generateResponse)
 		require.NoError(t, err)
 
 		req := httptest.NewRequest("GET", "/", nil)
 		for _, cookie := range generateResponse.Result().Cookies() {
 			req.AddCookie(cookie)
 		}
 
-		returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), state)
+		returnURL, codeVerifier, err := app.verifyAppState(req, httptest.NewRecorder(), state)
 		assert.NoError(t, err, InvalidRedirectURLError)
 		assert.Equal(t, expectedReturnURL, returnURL)
+		assert.Equal(t, pkceCodes.CodeVerifier, codeVerifier)
 	})
 }
 
@@ -499,7 +504,8 @@ func TestGenerateAppState_NoReturnURL(t *testing.T) {
 	require.NoError(t, err)
 
 	req.AddCookie(&http.Cookie{Name: common.StateCookieName, Value: hex.EncodeToString(encrypted)})
-	returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), "123")
+	returnURL, codeVerifier, err := app.verifyAppState(req, httptest.NewRecorder(), "123")
 	assert.NoError(t, err)
 	assert.Equal(t, "/argo-cd", returnURL)
+	assert.Empty(t, codeVerifier)
 }