feat: Allow multiple extenal URLs for SSO access by tjamet · Pull Request #14208 · argoproj/argo-cd

tjamet · 2023-06-26T12:30:34Z

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

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

Please see Contribution FAQs if you have questions about your pull-request.

codecov · 2023-06-26T12:44:31Z

Codecov Report

Attention: Patch coverage is 32.55814% with 29 lines in your changes missing coverage. Please review.

Project coverage is 49.88%. Comparing base (696e6e8) to head (18b08d5).
Report is 1506 commits behind head on master.

Files with missing lines	Patch %	Lines
util/settings/settings.go	7.14%	24 Missing and 2 partials ⚠️
server/logout/logout.go	25.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14208      +/-   ##
==========================================
- Coverage   49.89%   49.88%   -0.01%     
==========================================
  Files         263      263              
  Lines       45219    45254      +35     
==========================================
+ Hits        22563    22577      +14     
- Misses      20439    20460      +21     
  Partials     2217     2217

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

util/oidc/oidc.go

tjamet · 2023-06-27T07:10:58Z

Alternatively, we can make the configuration field be URLs instead of additionalURLs and deprecate URL.
Happy to go both paths as you tell me

csantanapr · 2023-06-28T02:23:23Z

This is very useful PR @tjamet 👍

huynhsontung · 2023-07-02T07:21:02Z

+1 for using urls and deprecating url

trc-ikeskin · 2023-08-29T11:10:54Z

Any progress on this PR. We could really use this feature.
@tjamet can I somehow support with this PR?

tjamet · 2023-08-31T15:42:12Z

Hi!
I have tested this feature internally and it worked for us, I added a few fixes for log-out recently though.
It's great to see interest in this feature.
Maybe, I can push the images on my own docker hub account for you to give feedback on the feature?
Would you be interested in this?

I will post a reminder in the CNCF slack in a few days.
Here is a thread where I referenced this PR, maybe we can also use it?

With argoproj#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 argoproj#5388 Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

codeql restricts logging fields from user input. Remove log to avoid complex escapes Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.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: Thibault Jamet <tjamet@users.noreply.github.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>

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>

tjamet · 2023-09-08T15:47:23Z

@trc-ikeskin I have pushed a build of this branch on my github container registry, if you want to take a look and provide feedback:
https://github.com/tjamet/argo-cd/pkgs/container/argo-cd%2Fargocd/126159769?tag=2.9.0-1eb72892

It is built from tjamet/master that has an additional commit to allow this build

BulatSaif · 2024-03-29T13:07:02Z

Can we merge this, please?

trc-ikeskin · 2024-03-29T13:52:31Z

@BulatSaif I guess if you want to see progress on this one you will have to do some actual testing on the feature. Unfortunately we had to move on to other customers.

avosepp · 2024-05-03T20:43:20Z

How can I help test this functionality to help get it merged?

tabathad · 2024-06-10T17:23:45Z

Also eager to see this PR merged and ready to help with any additional testing or changes needed. Please let us know how we can help!

agaudreault

I think the feature should be documented in the ArgoCD https://argo-cd.readthedocs.io/en/stable/operator-manual/user-management/ documentation.

Is url deprecated? are urls valid on their own or only additional urls? Is this feature supported by all identity providers, or only by OIDC?

agaudreault · 2024-06-13T17:29:35Z

util/oidc/oidc.go

 }

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


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) {

agaudreault · 2024-06-13T17:31:30Z

util/settings/settings.go

 	}
+	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)


The error message does not seem accurate

agaudreault · 2024-06-13T17:37:00Z

util/settings/settings.go


 func (a *ArgoCDSettings) IsDexConfigured() bool {
-	if a.URL == "" {
+	if a.URL == "" && len(a.URLs) == 0 {


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

KojoRising · 2024-06-14T05:08:12Z

+1, this would be super useful. Happy to help add some testing/documentation if needed.

vinelias · 2024-06-29T16:36:27Z

Any news on that?

agaudreault · 2024-07-02T14:32:43Z

As @tjamet does not seem active, @vinelias, @KojoRising, @avo-sepp, @tabathad you can fork @tjamet's work and submit a new PR with the require code changes, tests and answers for the questions above.

ClifHouck · 2024-07-03T22:10:59Z

@tjamet @vinelias @KojoRising @avo-sepp @tabathad: I'm going to marshal this across the finish line, if I can. New PR here: #18927

agaudreault · 2024-07-04T14:22:26Z

Closing in favor of #18927

tjamet · 2024-07-05T14:52:44Z

Sorry, I missed the latest updates and comments on this PR
Let me know if I can further help. I remain super interested in getting this change through

Fsero approved these changes Jun 26, 2023

View reviewed changes

tjamet force-pushed the multiple-url branch from ea8a04c to 9e6a00f Compare June 26, 2023 12:44

tjamet changed the title ~~Allow multiple extenal URLs for SSO access~~ feat: Allow multiple extenal URLs for SSO access Jun 26, 2023

github-advanced-security bot found potential problems Jun 26, 2023

View reviewed changes

util/oidc/oidc.go Fixed Show fixed Hide fixed

tjamet force-pushed the multiple-url branch from b95115b to 1eb9378 Compare June 26, 2023 14:27

tjamet force-pushed the multiple-url branch from 1eb9378 to 5dbda02 Compare July 6, 2023 17:11

thomaspoignant approved these changes Jul 18, 2023

View reviewed changes

jeevarathinam-dhanapal approved these changes Aug 16, 2023

View reviewed changes

tjamet force-pushed the multiple-url branch from 5dbda02 to f3c6f6a Compare August 22, 2023 21:29

tjamet added 10 commits September 8, 2023 17:19

Add new field to the documentation

fc11e68

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Fix codeql issue

fa62e0f

codeql restricts logging fields from user input. Remove log to avoid complex escapes Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Add a test to ensure validity of the documented cm

6385144

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>

rename additionalURLs for URLs

7c87dbb

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Allow responses redirected from SSO to alternate URLs

b8f31d6

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Consider dex enabled when there are additional URLs configured

a2ee8bd

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

Parse new URLs config from argocd-cm

a9fd852

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>

tjamet force-pushed the multiple-url branch from f3c6f6a to 18b08d5 Compare September 8, 2023 14:19

agaudreault assigned agaudreault and jannfis Jun 13, 2024

agaudreault requested changes Jun 13, 2024

View reviewed changes

agaudreault added the lifecycle/rotten Issue/PR had no activity for a long time and should be closed soon label Jul 4, 2024

agaudreault closed this Jul 4, 2024

agaudreault mentioned this pull request Jul 4, 2024

feat(5388): Allow multiple external URLs for SSO access #18927

Merged

14 tasks

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

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

Conversation

tjamet commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

tjamet commented Jun 27, 2023

Uh oh!

csantanapr commented Jun 28, 2023

Uh oh!

huynhsontung commented Jul 2, 2023

Uh oh!

trc-ikeskin commented Aug 29, 2023

Uh oh!

tjamet commented Aug 31, 2023

Uh oh!

tjamet commented Sep 8, 2023

Uh oh!

BulatSaif commented Mar 29, 2024

Uh oh!

trc-ikeskin commented Mar 29, 2024

Uh oh!

avosepp commented May 3, 2024

Uh oh!

tabathad commented Jun 10, 2024

Uh oh!

agaudreault left a comment

Choose a reason for hiding this comment

Uh oh!

agaudreault Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

agaudreault Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

agaudreault Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

KojoRising commented Jun 14, 2024

Uh oh!

vinelias commented Jun 29, 2024

Uh oh!

agaudreault commented Jul 2, 2024

Uh oh!

ClifHouck commented Jul 3, 2024

Uh oh!

agaudreault commented Jul 4, 2024

Uh oh!

tjamet commented Jul 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

tjamet commented Jun 26, 2023 •

edited

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading