feat: Use encrypted cookie to store OAuth2 state nonce (instead of redis) (#8241)

feat: Use encrypted cookie to store OAuth2 state nonce (instead of redis) (#8241)

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

common/common.go

@@ -76,6 +76,10 @@ const (
 	ArgoCDUserAgentName = "argocd-client"
 	// AuthCookieName is the HTTP cookie name where we store our auth token
 	AuthCookieName = "argocd.token"
+	// StateCookieName is the HTTP cookie name that holds temporary nonce tokens for CSRF protection
+	StateCookieName = "argocd.oauthstate"
+	// StateCookieMaxAge is the maximum age of the oauth state cookie
+	StateCookieMaxAge = time.Minute * 5
 
 	// ChangePasswordSSOTokenMaxAge is the max token age for password change operation
 	ChangePasswordSSOTokenMaxAge = time.Minute * 5

server/cache/cache.go

@@ -13,7 +13,6 @@ import (
 	cacheutil "github.com/argoproj/argo-cd/v2/util/cache"
 	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
 	"github.com/argoproj/argo-cd/v2/util/env"
-	"github.com/argoproj/argo-cd/v2/util/oidc"
 )
 
 var ErrCacheMiss = appstatecache.ErrCacheMiss
@@ -25,8 +24,6 @@ type Cache struct {
 	loginAttemptsExpiration         time.Duration
 }
 
-var _ oidc.OIDCStateStorage = &Cache{}
-
 func NewCache(
 	cache *appstatecache.Cache,
 	connectionStatusCacheExpiration time.Duration,
@@ -91,20 +88,6 @@ func (c *Cache) SetClusterInfo(server string, res *appv1.ClusterInfo) error {
 	return c.cache.SetClusterInfo(server, res)
 }
 
-func oidcStateKey(key string) string {
-	return fmt.Sprintf("oidc|%s", key)
-}
-
-func (c *Cache) GetOIDCState(key string) (*oidc.OIDCState, error) {
-	res := oidc.OIDCState{}
-	err := c.cache.GetItem(oidcStateKey(key), &res)
-	return &res, err
-}
-
-func (c *Cache) SetOIDCState(key string, state *oidc.OIDCState) error {
-	return c.cache.SetItem(oidcStateKey(key), state, c.oidcCacheExpiration, state == nil)
-}
-
 func (c *Cache) GetCache() *cacheutil.Cache {
 	return c.cache.Cache
 }

server/cache/cache_test.go

@@ -10,7 +10,6 @@ import (
 	. "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	cacheutil "github.com/argoproj/argo-cd/v2/util/cache"
 	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
-	"github.com/argoproj/argo-cd/v2/util/oidc"
 )
 
 type fixtures struct {
@@ -46,23 +45,6 @@ func TestCache_GetRepoConnectionState(t *testing.T) {
 	assert.Equal(t, ConnectionState{Status: "my-state"}, value)
 }
 
-func TestCache_GetOIDCState(t *testing.T) {
-	cache := newFixtures().Cache
-	// cache miss
-	_, err := cache.GetOIDCState("my-key")
-	assert.Equal(t, ErrCacheMiss, err)
-	// populate cache
-	err = cache.SetOIDCState("my-key", &oidc.OIDCState{ReturnURL: "my-return-url"})
-	assert.NoError(t, err)
-	//cache miss
-	_, err = cache.GetOIDCState("other-key")
-	assert.Equal(t, ErrCacheMiss, err)
-	// cache hit
-	value, err := cache.GetOIDCState("my-key")
-	assert.NoError(t, err)
-	assert.Equal(t, &oidc.OIDCState{ReturnURL: "my-return-url"}, value)
-}
-
 func TestAddCacheFlagsToCmd(t *testing.T) {
 	cache, err := AddCacheFlagsToCmd(&cobra.Command{})()
 	assert.NoError(t, err)

server/server.go

@@ -782,7 +782,7 @@ func (a *ArgoCDServer) registerDexHandlers(mux *http.ServeMux) {
 		tlsConfig := a.settings.TLSConfig()
 		tlsConfig.InsecureSkipVerify = true
 	}
-	a.ssoClientApp, err = oidc.NewClientApp(a.settings, a.Cache, a.DexServerAddr, a.BaseHRef)
+	a.ssoClientApp, err = oidc.NewClientApp(a.settings, a.DexServerAddr, a.BaseHRef)
 	errors.CheckError(err)
 	mux.HandleFunc(common.LoginEndpoint, a.ssoClientApp.HandleLogin)
 	mux.HandleFunc(common.CallbackEndpoint, a.ssoClientApp.HandleCallback)

util/crypto/crypto.go

@@ -0,0 +1,62 @@
+package crypto
+
+import (
+	"crypto/aes"
+	"crypto/cipher"
+	"crypto/rand"
+	"crypto/sha256"
+	"errors"
+	"io"
+
+	"golang.org/x/crypto/scrypt"
+)
+
+// KeyFromPassphrase generates 32 byte key from the passphrase
+func KeyFromPassphrase(passphrase string) ([]byte, error) {
+	// salt is just a hash of a passphrase (effectively no salt)
+	salt := sha256.Sum256([]byte(passphrase))
+	// These defaults will consume approximately 16MB of memory (128 * r * N)
+	const N = 16384
+	const r = 8
+	return scrypt.Key([]byte(passphrase), salt[:], N, r, 1, 32)
+}
+
+// Encrypt encrypts the given data with the given passphrase.
+func Encrypt(data []byte, key []byte) ([]byte, error) {
+	block, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+	gcm, err := cipher.NewGCM(block)
+	if err != nil {
+		return nil, err
+	}
+	nonce := make([]byte, gcm.NonceSize())
+	if _, err = io.ReadFull(rand.Reader, nonce); err != nil {
+		return nil, err
+	}
+	ciphertext := gcm.Seal(nonce, nonce, data, nil)
+	return ciphertext, nil
+}
+
+// Decrypt decrypts the given data using the given passphrase.
+func Decrypt(data []byte, key []byte) ([]byte, error) {
+	block, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+	gcm, err := cipher.NewGCM(block)
+	if err != nil {
+		return nil, err
+	}
+	nonceSize := gcm.NonceSize()
+	if len(data) < nonceSize {
+		return nil, errors.New("data length is less than nonce size")
+	}
+	nonce, ciphertext := data[:nonceSize], data[nonceSize:]
+	plaintext, err := gcm.Open(nil, nonce, ciphertext, nil)
+	if err != nil {
+		return nil, err
+	}
+	return plaintext, nil
+}

util/crypto/crypto_test.go

@@ -0,0 +1,43 @@
+package crypto
+
+import (
+	"crypto/rand"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func newKey() ([]byte, error) {
+	b := make([]byte, 32)
+	_, err := rand.Read(b)
+	if err != nil {
+		b = nil
+	}
+	return b, err
+}
+
+func TestEncryptDecrypt_Successful(t *testing.T) {
+	key, err := newKey()
+	require.NoError(t, err)
+	encrypted, err := Encrypt([]byte("test"), key)
+	require.NoError(t, err)
+
+	decrypted, err := Decrypt(encrypted, key)
+	require.NoError(t, err)
+
+	assert.Equal(t, "test", string(decrypted))
+}
+
+func TestEncryptDecrypt_Failed(t *testing.T) {
+	key, err := newKey()
+	require.NoError(t, err)
+	encrypted, err := Encrypt([]byte("test"), key)
+	require.NoError(t, err)
+
+	wrongKey, err := newKey()
+	require.NoError(t, err)
+
+	_, err = Decrypt(encrypted, wrongKey)
+	assert.Error(t, err)
+}

util/oidc/oidc.go

@@ -1,6 +1,7 @@
 package oidc
 
 import (
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"html"
@@ -20,7 +21,7 @@ import (
 
 	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/argoproj/argo-cd/v2/server/settings/oidc"
-	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
+	"github.com/argoproj/argo-cd/v2/util/crypto"
 	"github.com/argoproj/argo-cd/v2/util/dex"
 	httputil "github.com/argoproj/argo-cd/v2/util/http"
 	"github.com/argoproj/argo-cd/v2/util/rand"
@@ -45,16 +46,6 @@ type ClaimsRequest struct {
 	IDToken map[string]*oidc.Claim `json:"id_token"`
 }
 
-type OIDCState struct {
-	// ReturnURL is the URL in which to redirect a user back to after completing an OAuth2 login
-	ReturnURL string `json:"returnURL"`
-}
-
-type OIDCStateStorage interface {
-	GetOIDCState(key string) (*OIDCState, error)
-	SetOIDCState(key string, state *OIDCState) error
-}
-
 type ClientApp struct {
 	// OAuth2 client ID of this application (e.g. argo-cd)
 	clientID string
@@ -73,11 +64,10 @@ type ClientApp struct {
 	secureCookie bool
 	// settings holds Argo CD settings
 	settings *settings.ArgoCDSettings
+	// encryptionKey holds server encryption key
+	encryptionKey []byte
 	// provider is the OIDC provider
 	provider Provider
-	// cache holds temporary nonce tokens to which hold application state values
-	// See http://tools.ietf.org/html/rfc6749#section-10.12 for more info.
-	cache OIDCStateStorage
 }
 
 func GetScopesOrDefault(scopes []string) []string {
@@ -89,18 +79,22 @@ func GetScopesOrDefault(scopes []string) []string {
 
 // 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, cache OIDCStateStorage, dexServerAddr, baseHRef string) (*ClientApp, error) {
+func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr, baseHRef string) (*ClientApp, error) {
 	redirectURL, err := settings.RedirectURL()
 	if err != nil {
 		return nil, err
 	}
+	encryptionKey, err := settings.GetServerEncryptionKey()
+	if err != nil {
+		return nil, err
+	}
 	a := ClientApp{
-		clientID:     settings.OAuth2ClientID(),
-		clientSecret: settings.OAuth2ClientSecret(),
-		redirectURI:  redirectURL,
-		issuerURL:    settings.IssuerURL(),
-		baseHRef:     baseHRef,
-		cache:        cache,
+		clientID:      settings.OAuth2ClientID(),
+		clientSecret:  settings.OAuth2ClientSecret(),
+		redirectURI:   redirectURL,
+		issuerURL:     settings.IssuerURL(),
+		baseHRef:      baseHRef,
+		encryptionKey: encryptionKey,
 	}
 	log.Infof("Creating client app (%s)", a.clientID)
 	u, err := url.Parse(settings.URL)
@@ -149,31 +143,60 @@ func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) {
 }
 
 // generateAppState creates an app state nonce
-func (a *ClientApp) generateAppState(returnURL string) string {
+func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (string, error) {
 	randStr := rand.RandString(10)
 	if returnURL == "" {
 		returnURL = a.baseHRef
 	}
-	err := a.cache.SetOIDCState(randStr, &OIDCState{ReturnURL: returnURL})
-	if err != nil {
-		// This should never happen with the in-memory cache
-		log.Errorf("Failed to set app state: %v", err)
+	cookieValue := fmt.Sprintf("%s:%s", randStr, returnURL)
+	if encrypted, err := crypto.Encrypt([]byte(cookieValue), a.encryptionKey); err != nil {
+		return "", err
+	} else {
+		cookieValue = hex.EncodeToString(encrypted)
 	}
-	return randStr
+
+	http.SetCookie(w, &http.Cookie{
+		Name:     common.StateCookieName,
+		Value:    cookieValue,
+		Expires:  time.Now().Add(common.StateCookieMaxAge),
+		HttpOnly: true,
+		SameSite: http.SameSiteLaxMode,
+		Secure:   a.secureCookie,
+	})
+	return randStr, nil
 }
 
-func (a *ClientApp) verifyAppState(state string) (*OIDCState, error) {
-	res, err := a.cache.GetOIDCState(state)
+func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state string) (string, error) {
+	c, err := r.Cookie(common.StateCookieName)
 	if err != nil {
-		if err == appstatecache.ErrCacheMiss {
-			return nil, fmt.Errorf("unknown app state %s", state)
-		} else {
-			return nil, fmt.Errorf("failed to verify app state %s: %v", state, err)
-		}
+		return "", err
 	}
-
-	_ = a.cache.SetOIDCState(state, nil)
-	return res, nil
+	val, err := hex.DecodeString(c.Value)
+	if err != nil {
+		return "", err
+	}
+	val, err = crypto.Decrypt(val, a.encryptionKey)
+	if err != nil {
+		return "", err
+	}
+	cookieVal := string(val)
+	returnURL := a.baseHRef
+	parts := strings.SplitN(cookieVal, ":", 2)
+	if len(parts) == 2 && parts[1] != "" {
+		returnURL = parts[1]
+	}
+	if parts[0] != state {
+		return "", fmt.Errorf("invalid state in '%s' cookie", common.AuthCookieName)
+	}
+	// set empty cookie to clear it
+	http.SetCookie(w, &http.Cookie{
+		Name:     common.StateCookieName,
+		Value:    "",
+		HttpOnly: true,
+		SameSite: http.SameSiteLaxMode,
+		Secure:   a.secureCookie,
+	})
+	return returnURL, nil
 }
 
 // isValidRedirectURL checks whether the given redirectURL matches on of the
@@ -248,7 +271,12 @@ 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 := a.generateAppState(returnURL)
+	stateNonce, 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)
+		return
+	}
 	grantType := InferGrantType(oidcConf)
 	var url string
 	switch grantType {
@@ -281,10 +309,10 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
 	state := r.FormValue("state")
 	if code == "" {
 		// If code was not given, it implies implicit flow
-		a.handleImplicitFlow(w, state)
+		a.handleImplicitFlow(r, w, state)
 		return
 	}
-	appState, err := a.verifyAppState(state)
+	returnURL, err := a.verifyAppState(r, w, state)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
@@ -339,7 +367,7 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
 		claimsJSON, _ := json.MarshalIndent(claims, "", "  ")
 		renderToken(w, a.redirectURI, idTokenRAW, token.RefreshToken, claimsJSON)
 	} else {
-		http.Redirect(w, r, appState.ReturnURL, http.StatusSeeOther)
+		http.Redirect(w, r, returnURL, http.StatusSeeOther)
 	}
 }
 
@@ -366,7 +394,7 @@ if (state != "" && returnURL == "") {
 // state nonce for verification, as well as looking up the return URL. Once verified, the client
 // stores the id_token from the fragment as a cookie. Finally it performs the final redirect back to
 // the return URL.
-func (a *ClientApp) handleImplicitFlow(w http.ResponseWriter, state string) {
+func (a *ClientApp) handleImplicitFlow(r *http.Request, w http.ResponseWriter, state string) {
 	type implicitFlowValues struct {
 		CookieName string
 		ReturnURL  string
@@ -375,12 +403,12 @@ func (a *ClientApp) handleImplicitFlow(w http.ResponseWriter, state string) {
 		CookieName: common.AuthCookieName,
 	}
 	if state != "" {
-		appState, err := a.verifyAppState(state)
+		returnURL, err := a.verifyAppState(r, w, state)
 		if err != nil {
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			return
 		}
-		vals.ReturnURL = appState.ReturnURL
+		vals.ReturnURL = returnURL
 	}
 	renderTemplate(w, implicitFlowTmpl, vals)
 }

util/oidc/oidc_test.go

@@ -1,17 +1,24 @@
 package oidc
 
 import (
+	"encoding/hex"
 	"encoding/json"
 	"io/ioutil"
+	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
 	gooidc "github.com/coreos/go-oidc"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
 
+	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/argoproj/argo-cd/v2/server/settings/oidc"
+	"github.com/argoproj/argo-cd/v2/util"
+	"github.com/argoproj/argo-cd/v2/util/crypto"
+	"github.com/argoproj/argo-cd/v2/util/settings"
 )
 
 func TestInferGrantType(t *testing.T) {
@@ -179,3 +186,55 @@ func TestIsValidRedirect(t *testing.T) {
 		})
 	}
 }
+
+func TestGenerateAppState(t *testing.T) {
+	signature, err := util.MakeSignature(32)
+	require.NoError(t, err)
+	expectedReturnURL := "http://argocd.example.com/"
+	app, err := NewClientApp(&settings.ArgoCDSettings{ServerSignature: signature}, "", "")
+	require.NoError(t, err)
+	generateResponse := httptest.NewRecorder()
+	state, err := app.generateAppState(expectedReturnURL, generateResponse)
+	require.NoError(t, err)
+
+	t.Run("VerifyAppState_Successful", func(t *testing.T) {
+		req := httptest.NewRequest("GET", "/", nil)
+		for _, cookie := range generateResponse.Result().Cookies() {
+			req.AddCookie(cookie)
+		}
+
+		returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), state)
+		assert.NoError(t, err)
+		assert.Equal(t, expectedReturnURL, returnURL)
+	})
+
+	t.Run("VerifyAppState_Failed", func(t *testing.T) {
+		req := httptest.NewRequest("GET", "/", nil)
+		for _, cookie := range generateResponse.Result().Cookies() {
+			req.AddCookie(cookie)
+		}
+
+		_, err := app.verifyAppState(req, httptest.NewRecorder(), "wrong state")
+		assert.Error(t, err)
+	})
+}
+
+func TestGenerateAppState_NoReturnURL(t *testing.T) {
+	signature, err := util.MakeSignature(32)
+	require.NoError(t, err)
+	cdSettings := &settings.ArgoCDSettings{ServerSignature: signature}
+	key, err := cdSettings.GetServerEncryptionKey()
+	require.NoError(t, err)
+
+	req := httptest.NewRequest("GET", "/", nil)
+	encrypted, err := crypto.Encrypt([]byte("123"), key)
+	require.NoError(t, err)
+
+	app, err := NewClientApp(cdSettings, "", "/argo-cd")
+	require.NoError(t, err)
+
+	req.AddCookie(&http.Cookie{Name: common.StateCookieName, Value: hex.EncodeToString(encrypted)})
+	returnURL, err := app.verifyAppState(req, httptest.NewRecorder(), "123")
+	assert.NoError(t, err)
+	assert.Equal(t, "/argo-cd", returnURL)
+}

util/settings/settings.go

@@ -35,6 +35,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v2/server/settings/oidc"
 	"github.com/argoproj/argo-cd/v2/util"
+	"github.com/argoproj/argo-cd/v2/util/crypto"
 	"github.com/argoproj/argo-cd/v2/util/kube"
 	"github.com/argoproj/argo-cd/v2/util/password"
 	tlsutil "github.com/argoproj/argo-cd/v2/util/tls"
@@ -1481,6 +1482,11 @@ func (a *ArgoCDSettings) IsDexConfigured() bool {
 	return len(dexCfg) > 0
 }
 
+// GetServerEncryptionKey generates a new server encryption key using the server signature as a passphrase
+func (a *ArgoCDSettings) GetServerEncryptionKey() ([]byte, error) {
+	return crypto.KeyFromPassphrase(string(a.ServerSignature))
+}
+
 func UnmarshalDexConfig(config string) (map[string]interface{}, error) {
 	var dexCfg map[string]interface{}
 	err := yaml.Unmarshal([]byte(config), &dexCfg)