diff --git a/common/common.go b/common/common.go index 79bcceb1c1647..5df7091330d7c 100644 --- a/common/common.go +++ b/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 diff --git a/server/cache/cache.go b/server/cache/cache.go index dbbeb37833d7d..9fe6d3f890620 100644 --- a/server/cache/cache.go +++ b/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 } diff --git a/server/cache/cache_test.go b/server/cache/cache_test.go index e92c966788fc8..6e173035aa33a 100644 --- a/server/cache/cache_test.go +++ b/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) diff --git a/server/server.go b/server/server.go index 207d09c1b56f1..a468825613876 100644 --- a/server/server.go +++ b/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) diff --git a/util/crypto/crypto.go b/util/crypto/crypto.go new file mode 100644 index 0000000000000..1b902951f22a3 --- /dev/null +++ b/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 +} diff --git a/util/crypto/crypto_test.go b/util/crypto/crypto_test.go new file mode 100644 index 0000000000000..2d982ad718295 --- /dev/null +++ b/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) +} diff --git a/util/oidc/oidc.go b/util/oidc/oidc.go index 7f7d9623601ca..1727331ec1189 100644 --- a/util/oidc/oidc.go +++ b/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) } diff --git a/util/oidc/oidc_test.go b/util/oidc/oidc_test.go index 179a575f56be2..5c94a661c4161 100644 --- a/util/oidc/oidc_test.go +++ b/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) +} diff --git a/util/settings/settings.go b/util/settings/settings.go index e05183d564008..71e731ac216ad 100644 --- a/util/settings/settings.go +++ b/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)