added multiple providers support - take 2 #1657

hevans-dglcom · 2022-05-22T20:18:23Z

Hello,

I spent a few days in december implementing the multiple providers feature. I have finally found the time to revisit this since the structural provider changes were made. Also since the structural changes this feature is completely rewritten, so I decided to create a new PR.

You can currently test this yourself by checking out the branch and then running the following from the /contrib/local-environment/ folder:

make alpha-config-multiple-providers-build-up

Description

Added BUILDPLATFORM and VERSION args to the docker-compose build commands
Updated make file to support a multiple provider setup
Updated docker compose to support build + up
Updated docker compose alpha configs to actually work
Updated sign_in template to support multiple providers
Updated validation package to loop over the provider array and validate all providers
Created new providerMap interface built from existing provider interface
Added Provider ID to the state so callback knows which provider to use
Added dynamic oauth2/{id}/start routes for each provider ( /oauth2/start uses [0] provider, or if defaultProvider is set it will use this value - will shutdown if default provider is set but does not match an ID)
Return 404 if a /oauth2/{id}/start request does not match a providerID
Added validation to check for a provider ID in each provider - will shutdown if ID doesnt exist or two providers have the same.
Added Cookie refresh logic to select the correct provider
Update documentation for multiple provider config
Fixed Azure Provider pointer bug.

Motivation and Context

Important for environments with multiple companies etc #926
Closes #926

How Has This Been Tested?

Tested locally with Keycloak and Dex as multiple providers with alpha config
Tested locally with Dex in old style config
Have currently been running my older branch (from December) in production (with redis) with 3 azure providers for the last 3 months with no issues.

**** Other providers need to be tested to ensure pointers in the provider setup do not overwrite configs when using multiple providers from the same type **** - but this is not a breaking bug for existing 1 provider users

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

hevans-dglcom · 2022-05-22T20:19:06Z

Tests are still to be adjusted once maintainers are happy with the code

contrib/local-environment/oauth2-proxy-alpha-config-multiple-providers.yaml

bluebrown · 2022-07-01T17:44:42Z

Is there any specific reason this is stuck? Can I do something to help? This feature would be really useful.

ludydoo · 2022-07-03T09:56:49Z

I concur!

gcleaves · 2022-08-31T16:05:13Z

Let's do it!

JoelSpeed

Apologies for the delay in reviewing this, not getting a lot of time to spend on the project recently.

Thanks for putting this together, left some feedback

JoelSpeed · 2022-09-11T14:03:22Z

contrib/local-environment/docker-compose.yaml

@@ -53,7 +53,7 @@ services:
      httpbin: {}
  etcd:
    container_name: etcd
-    image: gcr.io/etcd-development/etcd:v3.4.7
+    image: gcr.io/etcd-development/etcd:v3.6.0-alpha.0


Why use an alpha image here? Can this be pinned to a stable release

contrib/local-environment/docker-compose.yaml

JoelSpeed · 2022-09-11T14:04:27Z

contrib/local-environment/oauth2-proxy-alpha-config-multiple-providers.yaml

+server:
+  BindAddress: 0.0.0.0:4180
+  SecureBindAddress: ""
+  TLS: null


Please finish this file with a new line character

JoelSpeed · 2022-09-11T14:04:46Z

contrib/local-environment/oauth2-proxy-alpha-config.cfg

 cookie_secret="OQINaROshtE9TcZkNAm-5Zs2Pv3xaWytBmc5W7sPX7w="
 email_domains="example.com"
 cookie_secure="false"
-redirect_url="http://localhost:4180/oauth2/callback"
+banner="Sign in with your specific company provider"
+cookie_refresh="0h1m0s"


Why has this change been made?

JoelSpeed · 2022-09-11T14:05:18Z

contrib/local-environment/oauth2-proxy-alpha-config.yaml

+server:
+  BindAddress: 0.0.0.0:4180
+  SecureBindAddress: ""
+  TLS: null


Please make sure to add a new line at the end of the file

JoelSpeed · 2022-09-11T14:21:47Z

pkg/app/pagewriter/templates_test.go

+		providerNameArray := make([]string, 0)
+		providerIDArray := make([]string, 0)
+		providerNameArray = append(providerNameArray, "<provider-name>")
+		providerIDArray = append(providerIDArray, "0")


Please condense

JoelSpeed · 2022-09-11T14:22:14Z

pkg/middleware/stored_session.go

@@ -36,18 +36,9 @@ const (
 // All options must be provided.
 type StoredSessionLoaderOptions struct {
 	// Session storage backend
-	SessionStore sessionsapi.SessionStore
-
-	// How often should sessions be refreshed


Why are we dropping the comments

JoelSpeed · 2022-09-11T14:23:01Z

pkg/middleware/stored_session.go

-	refreshed, err := s.sessionRefresher(req.Context(), session)
+func (s *storedSessionLoader) refreshSession(rw http.ResponseWriter, req *http.Request, providerMap providers.ProviderMap, session *sessionsapi.SessionState) error {
+
+	refreshed, err := s.providerMap[session.ProviderID].RefreshSession(req.Context(), session)


We need to get the provider from the map and check it isn't nil, in case there's an invalid ID. Every function that gets it requires this please

JoelSpeed · 2022-09-11T14:23:56Z

pkg/util/util.go

-	if len(paths) == 0 {
-		return nil, fmt.Errorf("invalid empty list of Root CAs file paths")
-	}


Why have we dropped this one?

JoelSpeed · 2022-09-11T14:24:52Z

pkg/validation/options.go

-				RootCAs:    pool,
-				MinVersion: tls.VersionTLS12,
+	} else {
+		caFiles := make([]string, 0)


No need for make here

Suggested change

caFiles := make([]string, 0)

caFiles := []string{}

github-actions · 2022-11-11T00:08:24Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

JoelSpeed · 2022-11-11T09:39:35Z

Do we have time to address the review comments?

hevans-dglcom · 2022-11-23T19:03:01Z

Hi, sorry I barely have the time to even sleep right now (new child).

I will maybe have time soon but cannot guarantee anything.

JoelSpeed · 2022-12-23T09:29:32Z

No worries @hevans-dglcom, we appreciate the time you've put in and, when you do have the time to come back to this, we can get back to it. Family and looking after yourself is more important than this project

added default provider .

.

github-actions · 2023-02-24T00:06:47Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

mteubner · 2023-07-28T16:02:13Z

Hi @hevans-dglcom, is there a way how we can support on that Change-Request. May be incorporating the review comments on your repo, or forking the master and do the changes on a different repo to bring this implementation further.

Thanks for any thoughts on that.

@JoelSpeed Or is there any other issue taking this over already that I missed?

or is #1923 more likely to be followed up ?

and if so how can we help there

hevans-dglcom · 2023-08-02T11:18:46Z

Hi @mteubner, honestly I'm not intending to try and follow this up, at least until the project gets a few more maintainers. I just don't see it even being reviewed again any time soon and as the changes touch a lot of code, It's too much work to fix all the conficts after every merge.

Feel free to fork my fork and try to bring it further, and I can help if you have any questions.

hevans-dglcom requested a review from a team as a code owner May 22, 2022 20:18

hevans-dglcom commented May 27, 2022

View reviewed changes

contrib/local-environment/oauth2-proxy-alpha-config-multiple-providers.yaml Show resolved Hide resolved

hevans-dglcom mentioned this pull request May 27, 2022

Implementation of Multiple Providers #1500

Closed

3 tasks

afirth mentioned this pull request Jun 5, 2022

Enable multiple providers #926

Open

JoelSpeed reviewed Sep 11, 2022

View reviewed changes

github-actions bot added the Stale label Nov 11, 2022

JoelSpeed removed the Stale label Nov 11, 2022

andrew-gropyus mentioned this pull request Nov 21, 2022

Support multiple Client IDs on the same proxy instance #1740

Closed

added multiple providers support

59bf792

added default provider .

hevans-dglcom force-pushed the feature/multiple-providers-v2 branch from 955336f to 59bf792 Compare December 25, 2022 22:09

updated regex to support the tag commits

c30b98e

.

salmanazmat666 mentioned this pull request Jan 3, 2023

Feature/multi tenancy #1923

Open

3 tasks

github-actions bot added the Stale label Feb 24, 2023

github-actions bot closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added multiple providers support - take 2 #1657

added multiple providers support - take 2 #1657

hevans-dglcom commented May 22, 2022 •

edited

Loading

hevans-dglcom commented May 22, 2022

bluebrown commented Jul 1, 2022

ludydoo commented Jul 3, 2022

gcleaves commented Aug 31, 2022

JoelSpeed left a comment

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

JoelSpeed Sep 11, 2022

github-actions bot commented Nov 11, 2022

JoelSpeed commented Nov 11, 2022

hevans-dglcom commented Nov 23, 2022

JoelSpeed commented Dec 23, 2022

github-actions bot commented Feb 24, 2023

mteubner commented Jul 28, 2023 •

edited

Loading

hevans-dglcom commented Aug 2, 2023 •

edited

Loading

added multiple providers support - take 2 #1657

added multiple providers support - take 2 #1657

Conversation

hevans-dglcom commented May 22, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

hevans-dglcom commented May 22, 2022

bluebrown commented Jul 1, 2022

ludydoo commented Jul 3, 2022

gcleaves commented Aug 31, 2022

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2022

JoelSpeed commented Nov 11, 2022

hevans-dglcom commented Nov 23, 2022

JoelSpeed commented Dec 23, 2022

github-actions bot commented Feb 24, 2023

mteubner commented Jul 28, 2023 • edited Loading

hevans-dglcom commented Aug 2, 2023 • edited Loading

hevans-dglcom commented May 22, 2022 •

edited

Loading

mteubner commented Jul 28, 2023 •

edited

Loading

hevans-dglcom commented Aug 2, 2023 •

edited

Loading