Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions docs/operator-manual/argocd-cm.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ data:
# Argo CD's externally facing base URL (optional). Required when configuring SSO
url: https://argo-cd-demo.argoproj.io

# Additional externally facing base URLs (optional)
urls: |
- https://argo-cd-demo2.argoproj.io

# Enables application status badge feature
statusbadge.enabled: "true"

Expand Down Expand Up @@ -245,9 +249,9 @@ data:

# A set of settings that allow enabling or disabling the config management tool.
# If unset, each defaults to "true".
kustomize.enabled: true
jsonnet.enabled: true
helm.enabled: true
kustomize.enabled: "true"
jsonnet.enabled: "true"
helm.enabled: "true"

# Build options/parameters to use with `kustomize build` (optional)
kustomize.buildOptions: --load_restrictor none
Expand Down
5 changes: 4 additions & 1 deletion server/logout/logout.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
return
}

argoURL := argoCDSettings.URL
argoURL, err := argoCDSettings.ArgoURLForRequest(r)
if err != nil {
log.Warnf("unable to find ArgoCD URL from config: %v", err)
}
if argoURL == "" {
// golang does not provide any easy way to determine scheme of current request
// so redirecting ot http which will auto-redirect too https if necessary
Expand Down
56 changes: 56 additions & 0 deletions server/logout/logout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ var (
oidcToken = "eyJraWQiOiJYQi1MM3ZFdHhYWXJLcmRSQnVEV0NwdnZsSnk3SEJVb2d5N253M1U1Z1ZZIiwiYWxnIjoiUlMyNTYifQ.eyJzdWIiOiIwMHVqNnM1NDVyNU5peVNLcjVkNSIsIm5hbWUiOiJqZCByIiwiZW1haWwiOiJqYWlkZWVwMTdydWx6QGdtYWlsLmNvbSIsInZlciI6MSwiaXNzIjoiaHR0cHM6Ly9kZXYtNTY5NTA5OC5va3RhLmNvbSIsImF1ZCI6IjBvYWowM2FmSEtqN3laWXJwNWQ1IiwiaWF0IjoxNjA1NTcyMzU5LCJleHAiOjE2MDU1NzU5NTksImp0aSI6IklELl9ORDJxVG5iREFtc3hIZUt2U2ZHeVBqTXRicXFEQXdkdlRQTDZCTnpfR3ciLCJhbXIiOlsicHdkIl0sImlkcCI6IjAwb2lnaGZmdkpRTDYzWjhoNWQ1IiwicHJlZmVycmVkX3VzZXJuYW1lIjoiamFpZGVlcDE3cnVsekBnbWFpbC5jb20iLCJhdXRoX3RpbWUiOjE2MDU1NzIzNTcsImF0X2hhc2giOiJqZVEwRml2ak9nNGI2TUpXRDIxOWxnIn0.GHkqwXgW-lrAhJdypW7SVjW0YdNLFQiRL8iwgT6DHJxP9Nb0OtkH2NKcBYAA5N6bTPLRQUHgYwWcgm5zSXmvqa7ciIgPF3tiQI8UmJA9VFRRDR-x9ExX15nskCbXfiQ67MriLslUrQUyzSCfUrSjXKwnDxbKGQncrtmRsh5asfCzJFb9excn311W9HKbT3KA0Ot7eOMnVS6V7SGfXxnKs6szcXIEMa_FhB4zDAVLr-dnxvSG_uuWcHrAkLTUVhHbdQQXF7hXIEfyr5lkMJN-drjdz-bn40GaYulEmUvO1bjcL9toCVQ3Ismypyr0b8phj4w3uRsLDZQxTxK7jAXlyQ"
nonOidcToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2MDU1NzQyMTIsImlzcyI6ImFyZ29jZCIsIm5iZiI6MTYwNTU3NDIxMiwic3ViIjoiYWRtaW4ifQ.zDJ4piwWnwsHON-oPusHMXWINlnrRDTQykYogT7afeE"
expectedNonOIDCLogoutURL = "http://localhost:4000"
expectedNonOIDCLogoutURLOnSecondHost = "http://argocd.my-corp.tld"
expectedOIDCLogoutURL = "https://dev-5695098.okta.com/oauth2/v1/logout?id_token_hint=" + oidcToken + "&post_logout_redirect_uri=" + baseURL
expectedOIDCLogoutURLWithRootPath = "https://dev-5695098.okta.com/oauth2/v1/logout?id_token_hint=" + oidcToken + "&post_logout_redirect_uri=" + baseURL + "/" + rootPath
)
Expand Down Expand Up @@ -180,6 +181,34 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
},
},
)
kubeClientWithoutOIDCAndMultipleURLs := fake.NewSimpleClientset(
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: common.ArgoCDConfigMapName,
Namespace: "default",
Labels: map[string]string{
"app.kubernetes.io/part-of": "argocd",
},
},
Data: map[string]string{
"url": "http://localhost:4000",
"urls": "- http://argocd.my-corp.tld",
},
},
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: common.ArgoCDSecretName,
Namespace: "default",
Labels: map[string]string{
"app.kubernetes.io/part-of": "argocd",
},
},
Data: map[string][]byte{
"admin.password": nil,
"server.secretkey": nil,
},
},
)
kubeClientWithoutOIDCConfig := fake.NewSimpleClientset(
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -211,6 +240,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
settingsManagerWithOIDCConfig := settings.NewSettingsManager(context.Background(), kubeClientWithOIDCConfig, "default")
settingsManagerWithoutOIDCConfig := settings.NewSettingsManager(context.Background(), kubeClientWithoutOIDCConfig, "default")
settingsManagerWithOIDCConfigButNoLogoutURL := settings.NewSettingsManager(context.Background(), kubeClientWithOIDCConfigButNoLogoutURL, "default")
settingsManagerWithoutOIDCAndMultipleURLs := settings.NewSettingsManager(context.Background(), kubeClientWithoutOIDCAndMultipleURLs, "default")
settingsManagerWithOIDCConfigButNoURL := settings.NewSettingsManager(context.Background(), kubeClientWithOIDCConfigButNoURL, "default")

sessionManager := session.NewSessionManager(settingsManagerWithOIDCConfig, test.NewFakeProjLister(), "", nil, session.NewUserStateStorage(nil))
Expand All @@ -236,6 +266,13 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
}
return &jwt.RegisteredClaims{Issuer: "okta"}, "", nil
}
nonoidcHandlerWithMultipleURLs := NewHandler(appclientset.NewSimpleClientset(), settingsManagerWithoutOIDCAndMultipleURLs, sessionManager, "", baseHRef, "default")
nonoidcHandlerWithMultipleURLs.verifyToken = func(tokenString string) (jwt.Claims, string, error) {
if !validJWTPattern.MatchString(tokenString) {
return nil, "", errors.New("invalid jwt")
}
return &jwt.RegisteredClaims{Issuer: "okta"}, "", nil
}

oidcHandlerWithoutBaseURL := NewHandler(appclientset.NewSimpleClientset(), settingsManagerWithOIDCConfigButNoURL, sessionManager, "argocd", baseHRef, "default")
oidcHandlerWithoutBaseURL.verifyToken = func(tokenString string) (jwt.Claims, string, error) {
Expand All @@ -257,6 +294,9 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
nonoidcRequest, err := http.NewRequest(http.MethodGet, "http://localhost:4000/api/logout", nil)
assert.NoError(t, err)
nonoidcRequest.Header = nonOidcTokenHeader
nonoidcRequestOnSecondHost, err := http.NewRequest(http.MethodGet, "http://argocd.my-corp.tld/api/logout", nil)
assert.NoError(t, err)
nonoidcRequestOnSecondHost.Header = nonOidcTokenHeader
assert.NoError(t, err)
requestWithInvalidToken, err := http.NewRequest(http.MethodGet, "http://localhost:4000/api/logout", nil)
assert.NoError(t, err)
Expand Down Expand Up @@ -321,6 +361,22 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
expectedLogoutURL: expectedNonOIDCLogoutURL,
wantErr: false,
},
{
name: "Case:non-OIDC Logout request on the first supported URL",
handler: nonoidcHandlerWithMultipleURLs,
request: nonoidcRequest,
responseRecorder: httptest.NewRecorder(),
expectedLogoutURL: expectedNonOIDCLogoutURL,
wantErr: false,
},
{
name: "Case:non-OIDC Logout request on the second supported URL",
handler: nonoidcHandlerWithMultipleURLs,
request: nonoidcRequestOnSecondHost,
responseRecorder: httptest.NewRecorder(),
expectedLogoutURL: expectedNonOIDCLogoutURLOnSecondHost,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
17 changes: 11 additions & 6 deletions util/oidc/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,21 @@ func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTl
return &a, nil
}

func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) {
func (a *ClientApp) oauth2Config(r *http.Request, scopes []string) (*oauth2.Config, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If only the url is necessary to find the config, pass only the URL instead of the whole request object.

Suggested change
func (a *ClientApp) oauth2Config(r *http.Request, scopes []string) (*oauth2.Config, error) {
func (a *ClientApp) oauth2Config(request *url.URL, scopes []string) (*oauth2.Config, error) {

endpoint, err := a.provider.Endpoint()
if err != nil {
return nil, err
}
redirectURL, err := a.settings.RedirectURLForRequest(r)
if err != nil {
redirectURL = a.redirectURI
}
return &oauth2.Config{
ClientID: a.clientID,
ClientSecret: a.clientSecret,
Endpoint: *endpoint,
Scopes: scopes,
RedirectURL: a.redirectURI,
RedirectURL: redirectURL,
}, nil
}

Expand Down Expand Up @@ -195,7 +199,7 @@ func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state
redirectURL := a.baseHRef
parts := strings.SplitN(cookieVal, ":", 2)
if len(parts) == 2 && parts[1] != "" {
if !isValidRedirectURL(parts[1], []string{a.settings.URL, a.baseHRef}) {
if !isValidRedirectURL(parts[1], append([]string{a.settings.URL, a.baseHRef}, a.settings.URLs...)) {
sanitizedUrl := parts[1]
if len(sanitizedUrl) > 100 {
sanitizedUrl = sanitizedUrl[:100]
Expand Down Expand Up @@ -280,14 +284,15 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
scopes = config.RequestedScopes
opts = AppendClaimsAuthenticationRequestParameter(opts, config.RequestedIDTokenClaims)
}
oauth2Config, err := a.oauth2Config(GetScopesOrDefault(scopes))
oauth2Config, err := a.oauth2Config(r, GetScopesOrDefault(scopes))
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
returnURL := r.FormValue("return_url")
// Check if return_url is valid, otherwise abort processing (see https://github.com/argoproj/argo-cd/pull/4780)
if !isValidRedirectURL(returnURL, []string{a.settings.URL}) {

if !isValidRedirectURL(returnURL, append([]string{a.settings.URL}, a.settings.URLs...)) {
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
}
Expand Down Expand Up @@ -319,7 +324,7 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {

// HandleCallback is the callback handler for an OAuth2 login flow
func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
oauth2Config, err := a.oauth2Config(nil)
oauth2Config, err := a.oauth2Config(r, nil)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
Expand Down
99 changes: 99 additions & 0 deletions util/oidc/oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,105 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL),
assert.NotContains(t, w.Body.String(), "certificate is not trusted")
assert.NotContains(t, w.Body.String(), "certificate signed by unknown authority")
})

t.Run("with additional base URL", func(t *testing.T) {
cdSettings := &settings.ArgoCDSettings{
URL: "https://argocd.example.com",
URLs: []string{"https://localhost:8080", "https://other.argocd.example.com"},
OIDCTLSInsecureSkipVerify: true,
DexConfig: `connectors:
- type: github
name: GitHub
config:
clientID: aabbccddeeff00112233
clientSecret: aabbccddeeff00112233`,
OIDCConfigRAW: fmt.Sprintf(`
name: Test
issuer: %s
clientID: xxx
clientSecret: yyy
requestedScopes: ["oidc"]`, oidcTestServer.URL),
}
cert, err := tls.X509KeyPair(test.Cert, test.PrivateKey)
require.NoError(t, err)
cdSettings.Certificate = &cert

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

t.Run("should accept login redirecting on the main domain", func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, "https://argocd.example.com/auth/login", nil)

req.URL.RawQuery = url.Values{
"return_url": []string{"https://argocd.example.com/applications"},
}.Encode()

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)
assert.Equal(t, fmt.Sprintf("%s://%s", location.Scheme, location.Host), oidcTestServer.URL)
assert.Equal(t, "/auth", location.Path)
assert.Equal(t, "https://argocd.example.com/auth/callback", location.Query().Get("redirect_uri"))
})

t.Run("should accept login redirecting on the alternative domains", func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, "https://localhost:8080/auth/login", nil)

req.URL.RawQuery = url.Values{
"return_url": []string{"https://localhost:8080/applications"},
}.Encode()

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)
assert.Equal(t, fmt.Sprintf("%s://%s", location.Scheme, location.Host), oidcTestServer.URL)
assert.Equal(t, "/auth", location.Path)
assert.Equal(t, "https://localhost:8080/auth/callback", location.Query().Get("redirect_uri"))
})

t.Run("should accept login redirecting on the alternative domains", func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, "https://other.argocd.example.com/auth/login", nil)

req.URL.RawQuery = url.Values{
"return_url": []string{"https://other.argocd.example.com/applications"},
}.Encode()

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)
assert.Equal(t, fmt.Sprintf("%s://%s", location.Scheme, location.Host), oidcTestServer.URL)
assert.Equal(t, "/auth", location.Path)
assert.Equal(t, "https://other.argocd.example.com/auth/callback", location.Query().Get("redirect_uri"))
})

t.Run("should deny login redirecting on the alternative domains", func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, "https://not-argocd.example.com/auth/login", nil)

req.URL.RawQuery = url.Values{
"return_url": []string{"https://not-argocd.example.com/applications"},
}.Encode()

w := httptest.NewRecorder()

app.HandleLogin(w, req)

assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Empty(t, w.Header().Get("Location"))
})

})
}

func Test_Login_Flow(t *testing.T) {
Expand Down
44 changes: 43 additions & 1 deletion util/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/base64"
"errors"
"fmt"
"math/big"
"net/http"
"net/url"
"path"
"reflect"
Expand Down Expand Up @@ -47,6 +49,9 @@ type ArgoCDSettings struct {
// URL is the externally facing URL users will visit to reach Argo CD.
// The value here is used when configuring SSO. Omitting this value will disable SSO.
URL string `json:"url,omitempty"`
// URLs is a list of externally facing URLs users will visit to reach Argo CD.
// The value here is used when configuring SSO reachable from multiple domains.
URLs []string `json:"urls,omitempty"`
// Indicates if status badge is enabled or not.
StatusBadgeEnabled bool `json:"statusBadgeEnable"`
// Indicates if status badge custom root URL should be used.
Expand Down Expand Up @@ -392,6 +397,8 @@ const (
settingServerPrivateKey = "tls.key"
// settingURLKey designates the key where Argo CD's external URL is set
settingURLKey = "url"
// settingURLsKey designates the key where Argo CD's external URLs is set
settingURLsKey = "urls"
// repositoriesKey designates the key where ArgoCDs repositories list is set
repositoriesKey = "repositories"
// repositoryCredentialsKey designates the key where ArgoCDs repositories credentials list is set
Expand Down Expand Up @@ -1422,6 +1429,16 @@ func updateSettingsFromConfigMap(settings *ArgoCDSettings, argoCDCM *apiv1.Confi
if err := validateExternalURL(argoCDCM.Data[settingUiBannerURLKey]); err != nil {
log.Warnf("Failed to validate UI banner URL in configmap: %v", err)
}
if argoCDCM.Data[settingURLsKey] != "" {
if err := yaml.Unmarshal([]byte(argoCDCM.Data[settingURLsKey]), &settings.URLs); err != nil {
log.Warnf("Failed decode all UI banner URLs in configmap: %v", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message does not seem accurate

}
}
for _, url := range settings.URLs {
if err := validateExternalURL(url); err != nil {
log.Warnf("Failed to validate UI banner URL in configmap: %v", err)
}
}
settings.UiBannerURL = argoCDCM.Data[settingUiBannerURLKey]
settings.UserSessionDuration = time.Hour * 24
if userSessionDurationStr, ok := argoCDCM.Data[userSessionDurationKey]; ok {
Expand Down Expand Up @@ -1740,7 +1757,7 @@ func (a *ArgoCDSettings) IsSSOConfigured() bool {
}

func (a *ArgoCDSettings) IsDexConfigured() bool {
if a.URL == "" {
if a.URL == "" && len(a.URLs) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Dex using this new URLs setting? It seems unclear if Dex should be valid if there are only URLs configured.

return false
}
dexCfg, err := UnmarshalDexConfig(a.DexConfig)
Expand Down Expand Up @@ -1925,6 +1942,31 @@ func (a *ArgoCDSettings) RedirectURL() (string, error) {
return appendURLPath(a.URL, common.CallbackEndpoint)
}

func (a *ArgoCDSettings) ArgoURLForRequest(r *http.Request) (string, error) {
if a == nil {
return "", errors.New("nil argocd settings")
}

for _, candidateURL := range append([]string{a.URL}, a.URLs...) {
u, err := url.Parse(candidateURL)
if err != nil {
return "", err
}
if u.Host == r.Host && strings.HasPrefix(r.URL.RequestURI(), u.RequestURI()) {
return candidateURL, nil
}
}
return a.URL, nil
}

func (a *ArgoCDSettings) RedirectURLForRequest(r *http.Request) (string, error) {
base, err := a.ArgoURLForRequest(r)
if err != nil {
return "", err
}
return appendURLPath(base, common.CallbackEndpoint)
}

func (a *ArgoCDSettings) DexRedirectURL() (string, error) {
return appendURLPath(a.URL, common.DexCallbackEndpoint)
}
Expand Down
Loading