Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: pkce for UI #9890 #11325

Closed
wants to merge 1 commit into from
Closed

feature: pkce for UI #9890 #11325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions cmd/argocd/commands/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package commands

import (
"context"
"crypto/sha256"
"encoding/base64"
"fmt"
"html"
"net/http"
Expand All @@ -30,6 +28,7 @@ import (
jwtutil "github.com/argoproj/argo-cd/v2/util/jwt"
"github.com/argoproj/argo-cd/v2/util/localconfig"
oidcutil "github.com/argoproj/argo-cd/v2/util/oidc"
"github.com/argoproj/argo-cd/v2/util/oidc/pkce"
"github.com/argoproj/argo-cd/v2/util/rand"
)

Expand Down Expand Up @@ -228,14 +227,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 := pkce.GeneratePKCECodes()

// Authorization redirect callback from OAuth2 auth flow.
// Handles both implicit and authorization code flow
Expand Down Expand Up @@ -276,7 +268,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())
Expand Down Expand Up @@ -313,8 +305,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...)
Expand Down
7 changes: 6 additions & 1 deletion server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ import (
"github.com/argoproj/argo-cd/v2/util/notification/k8s"
settings_notif "github.com/argoproj/argo-cd/v2/util/notification/settings"
"github.com/argoproj/argo-cd/v2/util/oidc"
"github.com/argoproj/argo-cd/v2/util/oidc/pkce"
"github.com/argoproj/argo-cd/v2/util/rbac"
util_session "github.com/argoproj/argo-cd/v2/util/session"
settings_util "github.com/argoproj/argo-cd/v2/util/settings"
Expand Down Expand Up @@ -172,6 +173,7 @@ type ArgoCDServer struct {
log *log.Entry
sessionMgr *util_session.SessionManager
settingsMgr *settings_util.SettingsManager
pkceMgr *pkce.PKCEManager
enf *rbac.Enforcer
projInformer cache.SharedIndexInformer
projLister applisters.AppProjectNamespaceLister
Expand Down Expand Up @@ -264,6 +266,8 @@ func NewServer(ctx context.Context, opts ArgoCDServerOpts) *ArgoCDServer {
appsetLister := appFactory.Argoproj().V1alpha1().ApplicationSets().Lister().ApplicationSets(opts.Namespace)

userStateStorage := util_session.NewUserStateStorage(opts.RedisClient)
pkceStateStorage := pkce.NewPKCEStateStorage(opts.RedisClient)
pkceManager := pkce.NewPKCEManager(pkceStateStorage)
sessionMgr := util_session.NewSessionManager(settingsMgr, projLister, opts.DexServerAddr, opts.DexTLSConfig, userStateStorage)
enf := rbac.NewEnforcer(opts.KubeClientset, opts.Namespace, common.ArgoCDRBACConfigMapName, nil)
enf.EnableEnforce(!opts.DisableAuth)
Expand Down Expand Up @@ -307,6 +311,7 @@ func NewServer(ctx context.Context, opts ArgoCDServerOpts) *ArgoCDServer {
apiFactory: apiFactory,
secretInformer: secretInformer,
configMapInformer: configMapInformer,
pkceMgr: pkceManager,
}
}

Expand Down Expand Up @@ -989,7 +994,7 @@ func (a *ArgoCDServer) registerDexHandlers(mux *http.ServeMux) {
// Run dex OpenID Connect Identity Provider behind a reverse proxy (served at /api/dex)
var err error
mux.HandleFunc(common.DexAPIEndpoint+"/", dexutil.NewDexHTTPReverseProxy(a.DexServerAddr, a.BaseHRef, a.DexTLSConfig))
a.ssoClientApp, err = oidc.NewClientApp(a.settings, a.DexServerAddr, a.DexTLSConfig, a.BaseHRef)
a.ssoClientApp, err = oidc.NewClientApp(a.settings, a.DexServerAddr, a.DexTLSConfig, a.BaseHRef, a.pkceMgr)
errorsutil.CheckError(err)
mux.HandleFunc(common.LoginEndpoint, a.ssoClientApp.HandleLogin)
mux.HandleFunc(common.CallbackEndpoint, a.ssoClientApp.HandleCallback)
Expand Down
17 changes: 15 additions & 2 deletions util/oidc/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package oidc

import (
"context"
"encoding/hex"
"encoding/json"
"fmt"
Expand All @@ -24,6 +25,7 @@ import (
"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/oidc/pkce"
"github.com/argoproj/argo-cd/v2/util/rand"
"github.com/argoproj/argo-cd/v2/util/settings"
)
Expand Down Expand Up @@ -70,6 +72,8 @@ type ClientApp struct {
encryptionKey []byte
// provider is the OIDC provider
provider Provider
// pkce manager for PKCE entries
pkceManager *pkce.PKCEManager
}

func GetScopesOrDefault(scopes []string) []string {
Expand All @@ -81,7 +85,7 @@ 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, dexServerAddr string, dexTlsConfig *dex.DexTLSConfig, baseHRef string) (*ClientApp, error) {
func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTlsConfig *dex.DexTLSConfig, baseHRef string, pkceManager *pkce.PKCEManager) (*ClientApp, error) {
redirectURL, err := settings.RedirectURL()
if err != nil {
return nil, err
Expand All @@ -97,6 +101,7 @@ func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTl
issuerURL: settings.IssuerURL(),
baseHRef: baseHRef,
encryptionKey: encryptionKey,
pkceManager: pkceManager,
}
log.Infof("Creating client app (%s)", a.clientID)
u, err := url.Parse(settings.URL)
Expand Down Expand Up @@ -301,6 +306,12 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
var url string
switch grantType {
case GrantTypeAuthorizationCode:
ctx := context.Background()
pkceCodes := pkce.GeneratePKCECodes()
pkceCodes.Nonce = stateNonce
a.pkceManager.StorePKCEEntry(ctx, pkceCodes, time.Hour)
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...)
Expand Down Expand Up @@ -343,7 +354,9 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
return
}
ctx := gooidc.ClientContext(r.Context(), a.client)
token, err := oauth2Config.Exchange(ctx, code)
codeVerifier := a.pkceManager.RetrieveVerifierCode(state)
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
Expand Down
47 changes: 35 additions & 12 deletions util/oidc/oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@ import (
"github.com/argoproj/argo-cd/v2/util"
"github.com/argoproj/argo-cd/v2/util/crypto"
"github.com/argoproj/argo-cd/v2/util/dex"
"github.com/argoproj/argo-cd/v2/util/oidc/pkce"
"github.com/argoproj/argo-cd/v2/util/settings"
"github.com/argoproj/argo-cd/v2/util/test"

redis_test "github.com/argoproj/argo-cd/v2/test"
)

func TestInferGrantType(t *testing.T) {
Expand Down Expand Up @@ -110,6 +113,9 @@ func TestHandleCallback(t *testing.T) {
}

func TestClientApp_HandleLogin(t *testing.T) {
redis, closer := redis_test.NewInMemoryRedis()
defer closer()

oidcTestServer := test.GetOIDCTestServer(t)
t.Cleanup(oidcTestServer.Close)

Expand All @@ -126,7 +132,7 @@ clientID: xxx
clientSecret: yyy
requestedScopes: ["oidc"]`, oidcTestServer.URL),
}
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

req := httptest.NewRequest("GET", "https://argocd.example.com/auth/login", nil)
Expand All @@ -141,7 +147,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),

cdSettings.OIDCTLSInsecureSkipVerify = true

app, err = NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err = NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

w = httptest.NewRecorder()
Expand All @@ -166,7 +172,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
require.NoError(t, err)
cdSettings.Certificate = &cert

app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

req := httptest.NewRequest("GET", "https://argocd.example.com/auth/login", nil)
Expand All @@ -179,7 +185,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
t.Fatal("did not receive expected certificate verification failure error")
}

app, err = NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com")
app, err = NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

w = httptest.NewRecorder()
Expand All @@ -192,6 +198,10 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
}

func Test_Login_Flow(t *testing.T) {

redis, closer := redis_test.NewInMemoryRedis()
defer closer()

// 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.

Expand All @@ -211,7 +221,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),

// The base href (the last argument for NewClientApp) is what HandleLogin will fall back to when no explicit
// redirect URL is given.
app, err := NewClientApp(cdSettings, "", nil, "/")
app, err := NewClientApp(cdSettings, "", nil, "/", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

w := httptest.NewRecorder()
Expand All @@ -238,6 +248,10 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
}

func TestClientApp_HandleCallback(t *testing.T) {

redis, closer := redis_test.NewInMemoryRedis()
defer closer()

oidcTestServer := test.GetOIDCTestServer(t)
t.Cleanup(oidcTestServer.Close)

Expand All @@ -254,7 +268,7 @@ clientID: xxx
clientSecret: yyy
requestedScopes: ["oidc"]`, oidcTestServer.URL),
}
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

req := httptest.NewRequest("GET", "https://argocd.example.com/auth/callback", nil)
Expand All @@ -269,7 +283,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),

cdSettings.OIDCTLSInsecureSkipVerify = true

app, err = NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err = NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

w = httptest.NewRecorder()
Expand All @@ -294,7 +308,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
require.NoError(t, err)
cdSettings.Certificate = &cert

app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com")
app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

req := httptest.NewRequest("GET", "https://argocd.example.com/auth/callback", nil)
Expand All @@ -307,7 +321,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
t.Fatal("did not receive expected certificate verification failure error")
}

app, err = NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com")
app, err = NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

w = httptest.NewRecorder()
Expand Down Expand Up @@ -403,10 +417,13 @@ func TestIsValidRedirect(t *testing.T) {
}

func TestGenerateAppState(t *testing.T) {
redis, closer := redis_test.NewInMemoryRedis()
defer closer()

signature, err := util.MakeSignature(32)
require.NoError(t, err)
expectedReturnURL := "http://argocd.example.com/"
app, err := NewClientApp(&settings.ArgoCDSettings{ServerSignature: signature, URL: expectedReturnURL}, "", nil, "")
app, err := NewClientApp(&settings.ArgoCDSettings{ServerSignature: signature, URL: expectedReturnURL}, "", nil, "", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)
generateResponse := httptest.NewRecorder()
state, err := app.generateAppState(expectedReturnURL, generateResponse)
Expand Down Expand Up @@ -435,6 +452,9 @@ func TestGenerateAppState(t *testing.T) {
}

func TestGenerateAppState_XSS(t *testing.T) {
redis, closer := redis_test.NewInMemoryRedis()
defer closer()

signature, err := util.MakeSignature(32)
require.NoError(t, err)
app, err := NewClientApp(
Expand All @@ -443,7 +463,7 @@ func TestGenerateAppState_XSS(t *testing.T) {
URL: "https://argocd.example.com",
ServerSignature: signature,
},
"", nil, "",
"", nil, "", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)),
)
require.NoError(t, err)

Expand Down Expand Up @@ -485,6 +505,9 @@ func TestGenerateAppState_XSS(t *testing.T) {
}

func TestGenerateAppState_NoReturnURL(t *testing.T) {
redis, closer := redis_test.NewInMemoryRedis()
defer closer()

signature, err := util.MakeSignature(32)
require.NoError(t, err)
cdSettings := &settings.ArgoCDSettings{ServerSignature: signature}
Expand All @@ -495,7 +518,7 @@ func TestGenerateAppState_NoReturnURL(t *testing.T) {
encrypted, err := crypto.Encrypt([]byte("123"), key)
require.NoError(t, err)

app, err := NewClientApp(cdSettings, "", nil, "/argo-cd")
app, err := NewClientApp(cdSettings, "", nil, "/argo-cd", pkce.NewPKCEManager(pkce.NewPKCEStateStorage(redis)))
require.NoError(t, err)

req.AddCookie(&http.Cookie{Name: common.StateCookieName, Value: hex.EncodeToString(encrypted)})
Expand Down
56 changes: 56 additions & 0 deletions util/oidc/pkce/pkcemanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package pkce

import (
"context"
"crypto/sha256"
"encoding/base64"
"time"

"github.com/argoproj/argo-cd/v2/util/errors"
"github.com/argoproj/argo-cd/v2/util/rand"
)

// PKCEManager handles code verifications and their associated auth tokens
type PKCEManager struct {
storage PKCEStateStorage
}

type PKCECodes struct {
CodeVerifier string
CodeChallenge string
AuthCode string
Nonce 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",
}
}

// PKCEManager creates a new pkce manager
func NewPKCEManager(storage PKCEStateStorage) *PKCEManager {
s := PKCEManager{
storage: storage,
}
return &s
}

func (mgr *PKCEManager) StorePKCEEntry(ctx context.Context, pkceCodes *PKCECodes, expiringAt time.Duration) error {
return mgr.storage.StorePKCEEntry(ctx, pkceCodes, expiringAt)
}

func (mgr *PKCEManager) RetrieveVerifierCode(nonce string) string {
return mgr.storage.RetrieveCodeVerifier(nonce)
}
Loading