feat(5388): Allow multiple external URLs for SSO access#18927
feat(5388): Allow multiple external URLs for SSO access#18927ishitasequeira merged 29 commits intoargoproj:masterfrom
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
|
@ClifHouck make sure to adress the review in #14208 (review). You can switch the PR to draft until the test are passing and everything is implemented. Thanks for taking up the mantle! 💪 |
9c0c3ee to
9f64a90
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18927 +/- ##
==========================================
+ Coverage 50.70% 52.78% +2.08%
==========================================
Files 316 316
Lines 43496 43579 +83
==========================================
+ Hits 22054 23005 +951
+ Misses 18930 18025 -905
- Partials 2512 2549 +37 ☔ View full report in Codecov by Sentry. |
ClifHouck
left a comment
There was a problem hiding this comment.
Regarding this last comment: https://github.com/argoproj/argo-cd/pull/14208/files#r1638649274 it seems like Dex is not using the new URLs setting. So I removed that addition.
| } | ||
|
|
||
| func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) { | ||
| func (a *ClientApp) oauth2Config(request *http.Request, scopes []string) (*oauth2.Config, error) { |
There was a problem hiding this comment.
It seems like the request object is needed here. Re: https://github.com/argoproj/argo-cd/pull/14208/files#r1638640243
util/settings/settings.go
Outdated
| } | ||
| if argoCDCM.Data[settingURLsKey] != "" { | ||
| if err := yaml.Unmarshal([]byte(argoCDCM.Data[settingURLsKey]), &settings.URLs); err != nil { | ||
| log.Warnf("Failed to decode all external URLs in configmap: %v", err) |
There was a problem hiding this comment.
Corrected messages here, re: https://github.com/argoproj/argo-cd/pull/14208/files#r1638642960
|
@agaudreault I think I've addressed the comments you had on the previous PR. Except this:
Agreed. I can add some documentation around this feature.
@tjamet What do you think about the questions posed above? |
|
What do I need to about the license compliance failing? Is there a contributor agreement I need to sign? |
Hi! Yes, it should be documented, happy to help doing so. About the deprecation, this was my idea at the beginning but I faced some issues as |
|
@tjamet Any updates? |
| # Additional externally facing base URLs (optional) | ||
| urls: | | ||
| - https://argo-cd-demo2.argoproj.io | ||
|
|
There was a problem hiding this comment.
I think the feature must be documented in the ArgoCD argo-cd.readthedocs.io/en/stable/operator-manual/user-management documentation.
There was a problem hiding this comment.
Added some short documentation in the user-management doc
| } | ||
| redirectURL, err := a.settings.RedirectURLForRequest(request) | ||
| if err != nil { | ||
| redirectURL = a.redirectURI |
There was a problem hiding this comment.
Probably missing a log.Warnf("unable to find ArgoCD URL from request: %v", err)
docs/operator-manual/argocd-cm.yaml
Outdated
| url: https://argo-cd-demo.argoproj.io | ||
|
|
||
| # Additional externally facing base URLs (optional) | ||
| urls: | |
There was a problem hiding this comment.
To provide feature parity, Dex should be configured to use all the urls as valid redirectUrls in
Lines 62 to 64 in fe9aa54
oidc.config will be better.
There was a problem hiding this comment.
I think I've updated dex to at least load the additional URLs with the proper callback endpoint appended.
I've also handled config reload when settings.AdditionalURLs is modified.
docs/operator-manual/argocd-cm.yaml
Outdated
| url: https://argo-cd-demo.argoproj.io | ||
|
|
||
| # Additional externally facing base URLs (optional) | ||
| urls: | |
There was a problem hiding this comment.
Since this field does not replace the url field, I think it will be clearer if it is renamed to additionalUrls
Taking a second look, indeed, URL is used to register in dex, and the update to replace by the new proposed list is not that trivial. Eventually, a decision to deprecate the url config in favour of a list could be taken in the future |
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
codeql restricts logging fields from user input. Remove log to avoid complex escapes Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Currently, the documented configmap is invalid and raises error at the time of applying it. Ensure the configmap is valid with a unit test Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
With HTTP servers, the `url` field of `http.Requests` does not contain the host nor the protocol. To work around this, consider `request.Host` and `request.URL.RequestURI()` Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Currently, while login is correctly handled, when logging out from an alternate URL, we are redirected to the main URL. Fix this by applying the same principles as for the login part Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Clif Houck <me@clifhouck.com>
…other spots in the codebase Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Clif Houck <me@clifhouck.com>
|
I believe I've addressed everything brought up so far, and re-based on latest master e2c5a55 for good measure. Not sure what to do about the linter failing on files I haven't changed. |
|
Also need guidance on what to do about the license compliance check. |
@ClifHouck You can ignore that. |
agaudreault
left a comment
There was a problem hiding this comment.
LGTM. Added in my sprint to give it a try internally, for both dex and native oidc. I should be able to test tomorrow.
Thank you! |
Correct misspelling of additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com>
|
@ClifHouck Have you tested this image on an actual deployment? I am getting constant |
No, but I thought the included additional tests of the original PR would've covered this case. I'll setup an environment to do more in-depth testing. I may not be able to complete this before leaving for vacation though. I'll be back online on August 12th. |
Fix incorrect key for additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Fix key name Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com>
Signed-off-by: Clif Houck <me@clifhouck.com>
ishitasequeira
left a comment
There was a problem hiding this comment.
Thanks @ClifHouck for the PR!! and Thanks @agaudreault for the review!!
* Add new field to the documentation Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Fix codeql issue codeql restricts logging fields from user input. Remove log to avoid complex escapes Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Add a test to ensure validity of the documented cm Currently, the documented configmap is invalid and raises error at the time of applying it. Ensure the configmap is valid with a unit test Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * rename additionalURLs for URLs Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Allow responses redirected from SSO to alternate URLs Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Consider dex enabled when there are additional URLs configured Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Parse new URLs config from argocd-cm Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Detect additional SSO URLs from requests With HTTP servers, the `url` field of `http.Requests` does not contain the host nor the protocol. To work around this, consider `request.Host` and `request.URL.RequestURI()` Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Handle logout properly Currently, while login is correctly handled, when logging out from an alternate URL, we are redirected to the main URL. Fix this by applying the same principles as for the login part Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Fix ClientApp.oath2Config Signed-off-by: Clif Houck <me@clifhouck.com> * Fix test compile time error Signed-off-by: Clif Houck <me@clifhouck.com> * Fix TestDocumentedArgoCDConfigMapIsValid to parse in the same way as other spots in the codebase Signed-off-by: Clif Houck <me@clifhouck.com> * Fix warning messages for external URLs when attemping to load them from config map Signed-off-by: Clif Houck <me@clifhouck.com> * Revert change to IsDexConfigured since Dex does not use URLs setting Signed-off-by: Clif Houck <me@clifhouck.com> * Remove unnecessary receiver nil check in favor of fixing test Signed-off-by: Clif Houck <me@clifhouck.com> * Fix typo Signed-off-by: Clif Houck <me@clifhouck.com> * Add a unit test for RedirectURLForRequest Signed-off-by: Clif Houck <me@clifhouck.com> * Rename settings.URLs to AdditionalURLs Signed-off-by: Clif Houck <me@clifhouck.com> * Fix use of URLs in TestClientApp_HandleLogin Signed-off-by: Clif Houck <me@clifhouck.com> * Renamed urls to additionalUrls Signed-off-by: Clif Houck <me@clifhouck.com> * Integrate settings.AdditionalURLs with dex config and test settings.RedirectAdditionalURLs Signed-off-by: Clif Houck <me@clifhouck.com> * Reload ArgoCDServer when settings.AdditionalURLs changes Signed-off-by: Clif Houck <me@clifhouck.com> * Add note about additionalUrls to user-managament docs Signed-off-by: Clif Houck <me@clifhouck.com> * Add G-Research Signed-off-by: Clif Houck <me@clifhouck.com> * Change G-Research URL to point to open-source page Signed-off-by: Clif Houck <me@clifhouck.com> * Update docs/operator-manual/argocd-cm.yaml Correct misspelling of additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Update util/settings/settings.go Fix incorrect key for additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Update util/settings/settings.go Fix key name Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * fix additional urls config key in test config Signed-off-by: Clif Houck <me@clifhouck.com> --------- Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> Co-authored-by: Thibault Jamet <tjamet@users.noreply.github.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
* Add new field to the documentation Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Fix codeql issue codeql restricts logging fields from user input. Remove log to avoid complex escapes Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Add a test to ensure validity of the documented cm Currently, the documented configmap is invalid and raises error at the time of applying it. Ensure the configmap is valid with a unit test Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * rename additionalURLs for URLs Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Allow responses redirected from SSO to alternate URLs Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Consider dex enabled when there are additional URLs configured Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Parse new URLs config from argocd-cm Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Detect additional SSO URLs from requests With HTTP servers, the `url` field of `http.Requests` does not contain the host nor the protocol. To work around this, consider `request.Host` and `request.URL.RequestURI()` Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Handle logout properly Currently, while login is correctly handled, when logging out from an alternate URL, we are redirected to the main URL. Fix this by applying the same principles as for the login part Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Fix ClientApp.oath2Config Signed-off-by: Clif Houck <me@clifhouck.com> * Fix test compile time error Signed-off-by: Clif Houck <me@clifhouck.com> * Fix TestDocumentedArgoCDConfigMapIsValid to parse in the same way as other spots in the codebase Signed-off-by: Clif Houck <me@clifhouck.com> * Fix warning messages for external URLs when attemping to load them from config map Signed-off-by: Clif Houck <me@clifhouck.com> * Revert change to IsDexConfigured since Dex does not use URLs setting Signed-off-by: Clif Houck <me@clifhouck.com> * Remove unnecessary receiver nil check in favor of fixing test Signed-off-by: Clif Houck <me@clifhouck.com> * Fix typo Signed-off-by: Clif Houck <me@clifhouck.com> * Add a unit test for RedirectURLForRequest Signed-off-by: Clif Houck <me@clifhouck.com> * Rename settings.URLs to AdditionalURLs Signed-off-by: Clif Houck <me@clifhouck.com> * Fix use of URLs in TestClientApp_HandleLogin Signed-off-by: Clif Houck <me@clifhouck.com> * Renamed urls to additionalUrls Signed-off-by: Clif Houck <me@clifhouck.com> * Integrate settings.AdditionalURLs with dex config and test settings.RedirectAdditionalURLs Signed-off-by: Clif Houck <me@clifhouck.com> * Reload ArgoCDServer when settings.AdditionalURLs changes Signed-off-by: Clif Houck <me@clifhouck.com> * Add note about additionalUrls to user-managament docs Signed-off-by: Clif Houck <me@clifhouck.com> * Add G-Research Signed-off-by: Clif Houck <me@clifhouck.com> * Change G-Research URL to point to open-source page Signed-off-by: Clif Houck <me@clifhouck.com> * Update docs/operator-manual/argocd-cm.yaml Correct misspelling of additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Update util/settings/settings.go Fix incorrect key for additionalUrls Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * Update util/settings/settings.go Fix key name Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Clif Houck <me@clifhouck.com> * fix additional urls config key in test config Signed-off-by: Clif Houck <me@clifhouck.com> --------- Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com> Signed-off-by: Clif Houck <me@clifhouck.com> Co-authored-by: Thibault Jamet <tjamet@users.noreply.github.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Rhys Williams <rhys.williams@electrum.co.za>
|
Any thoughts about when this will be in a tagged release? |
|
@ishitasequeira - we could really use a tagged release that contains this change, any idea when that might happen? |
|
This will be in |
My organization has interest in getting this functionality merged, so I'm taking up the mantle. I've re-based the original PR on current (79b18e0) master. This currently builds, but tests do not pass. I'll be back online next week to fix tests, verify functionality of this change, and respond to any existing review comments from the original PR.
Preserving the original PR note here for reference:
With #4780, were introduced security measures to ensure the return_url is pointing to the current ArgoCD instance.
In several occasions, an ArgoCD isntance could be exposed through multiple network connections. Internal addresses and restricted public addresses.
Currently, a single base URL can be configured in the the argocd configmap, preventing from exposing ArgoCD on several access paths.
This change allows to define multiple hosts on which ArgoCD can be exposed, keeping backward compatibility by adding a field additionalUrls accepting a list of additional URLS on which argoCD can be exposed
Fixes #5388
Checklist: