OIDC multiple redirect URLs by Joerger · Pull Request #12054 · gravitational/teleport

Joerger · 2022-04-19T01:46:39Z

Core changes

This PR adds support for adding multiple redirect URLs. The redirect used for a given OIDC auth request will be chosen based on the calling proxy.

If there is no redirect URL with a matching host to the proxy, the first redirect URL in the list will be used. If there is no redirect URL with a matching host AND port, then the first redirect URL with a match host will be used.

Other changes

Improved oidc provider syncing logic - waiting for the provider to start syncing now happens after releasing the lock on the server. A broken oidc provider could have resulted in the auth service being locked for every oidc validation attempt.
Unified validation of oidc connector validation into oidcConnector.CheckAndSetDefautlts. Before some of the validation was in services.ValidateOIDCConnecotor(oidcConnector), but every call path would end up calling both.
- Some tests needed to be updated to provide valid connectors.

Backwards compatibility

This PR adds a new field RedirectURLs to the oidc connector resource, and deprecates the RedirectURL field.

gRPC

In order to communicate with old clients and servers, we must do two things:

Set oidc.RedirectURLs[0] = oidc.RedirectURL when receiving an oidc connector from an old server/client
- We also set oidc.RedirectURL = "" so that the value doesn't get marshaled into the backend or client output.
Set oidc.RedirectURL = oidc.RedirectURLs[0] when sending an oidc connector to an old server/client

yaml

We can convert oidc.redirect_url to a wrapper.Strings in order to marshal from either a string or list of strings and make backwards compatibility simple from a configuration perspective.

kind: oidc
spec:
# Old
    redirect_url: https://proxy.example.com/v1/webapi/oidc/callback
# New
    redirect_url: https://proxy.example.com/v1/webapi/oidc/callback
    # OR 
    redirect_url: 
        - https://proxy.example.com/v1/webapi/oidc/callback
        - https://us.proxy.example.com/v1/webapi/oidc/callback
        - https://eu.proxy.example.com/v1/webapi/oidc/callback

However, since redirect_url is unmarshalled from the proto message field OIDCConnectorV3.Spec.RedirectURL, we can't simply change RedirectURL from a string to a wrapper.Strings - this would break backwards compatibility of gRPC requests.

However, we can hack around this by changing the json tags of RedirectURL and RedirectURLs:

// old
message OIDCConnectorSpecV3 {
    string RedirectURL  `json:"redirect_url"`
}
// new
message OIDCConnectorSpecV3 {
    // deprecated, only used in requests to/from old services
    string RedirectURL  `json:"-"`
    wrappers.StringValues RedirectURLs `json:"redirect_url"`
}

Unfortunately this approach results in non-matching json tags, but it also results in a desirable configuration experience. In the future, we can deprecate string RedirectURL, and take another deprecation cycle to fix the tag/field name. This is a bit overkill, but at least it's possible.

message OIDCConnectorSpecV3 {
    wrappers.StringValues RedirectURL `json:"redirect_url"`
    // deprecated, only used in requests to/from old services
    wrappers.StringValues RedirectURLs `json:"-"`
}

e PR - https://github.com/gravitational/teleport.e/pull/444

Closes #7042

smallinsky

Left some comments.
@Tener Could you also take a look ?

Tener · 2022-05-17T16:33:45Z

-    string RedirectURL = 4 [ (gogoproto.jsontag) = "redirect_url" ];
+    //
+    // DELETE IN 11.0.0 in favor of RedirectURLs
+    string RedirectURL = 4 [ (gogoproto.jsontag) = "redirect_url_deprecated,omitempty" ];


As per other comment: can we deprecate this field explicitly, but leave the old tag in place?

Also, for older clients, we could simply fill this field with the first value of RedirectURLs. This wouldn't be optimal (performance-wise), but would maintain backwards compatibility.

For the second part, this is already what's being done with CheckAndSetRedirectURL.

I actually agree that changing this json tag is a bit confusing. Is there a reason we can't keep the old tag, not introduce any new fields but change the type of this field to wrapper.Strings so it can be unmarshaled from both a string or an array? That would be backwards compatible, no?

Yeah, the issue is that the gRPC message will be changed, so when old clients/auth servers send String RedirectURL, the new clients/servers will return an error since they expect wrappers.Strings RedirectURL. And vice versa. So this would break backwards and forwards compatibility.

Ah yes, you're right, I mixed it up with json unmarshalling for some reason. In that case I think it makes sense. Another option we have is to just introduce a proper new RedirectURLs with tag redirect_urls and deprecate the old one.

So it looks like we have 3 options:

Try to make existing redirect_url support both strings and arrays of strings which is challenging to do in backwards compatible fashion with GRPC.

Intro new OIDC connector redirect_urls field that accepts array, and deprecate singular redirect_url in backwards compatible fashion (which means we'll likely keep it forever but update the docs to use the redirect_urls).

Let users set proxy_redirect_addr in the proxy config instead, in which case we won't need to touch the OIDC connector spec at all.

@Joerger Is that accurate? Let's check with the product team (@klizhentas @xinding33) on which they'd prefer we implement.

Correct. Note that with option 1, after the deprecation cycle is complete in v11.0.0, we can actually apply this backwards compatiblity approach again to match the json tags:

message OIDCConnectorSpecV3 { ... strings RedirectURL = 4 [ (gogoproto.jsontag) = "redirect_url_deprecated" ]; ... wrappers.Strings RedirectURLs = 14 [ (gogoproto.jsontag) = "redirect_url" ]; }

---->

message OIDCConnectorSpecV3 { ... wrappers.Strings RedirectURL = 4 [ (gogoproto.jsontag) = "redirect_url" ]; ... wrappers.Strings RedirectURLs = 14 [ (gogoproto.jsontag) = "redirect_url_deprecated" ]; }

Try to make existing redirect_url support both strings and arrays of strings which is challenging to do in backwards compatible fashion with GRPC.

Intro new OIDC connector redirect_urls field that accepts array, and deprecate singular redirect_url in backwards compatible fashion (which means we'll likely keep it forever but update the docs to use the redirect_urls).

I also should mention that the backwards compatibility steps taken for (1) and (2) will be the same - Set oidc.RedirectURL = oidc.RedirectURLs[0] when sending to old clients/servers, and read oidc.RedirectURLs[0] = oidc.RedirectURL when receiving from old clients/servers.

The only difference is that with (1), the json tags will not match the field names:

RedirectURL `json:"redirect_url_deprecated"` RedirectURLs `json:"redirect_url"`

and with (2), they will match:

RedirectURL `json:"redirect_url"` RedirectURLs `json:"redirect_urls"`

Tener · 2022-05-17T16:46:30Z

+// GetRedirectURL gets a redirect URL for the given connector. If the connector
+// has a redirect URL which matches the host of the given Proxy address, then
+// that one will be returned. Otherwise, the first URL in the list will be returned.
+func GetRedirectURL(conn types.OIDCConnector, proxyAddr string) string {


I think it is critical to test this particular function.

Also, marshalling tests in teleport.e would be a good idea too, as well as any necessary updates to tctl sso oidc configure and tctl sso test.

I've added tests for GetRedirectURL and UnmarshalOIDCConnector. It looks like tctl sso oidc configure and tctl sso test are not implemented yet, but they should not need adjustments with this PR.

Tener · 2022-05-17T17:00:17Z

Thinking about the feature in general: would it make sense to leave the old field in place (which would solve most compatibility issues) and add an additional "redirect override mapping" field? The single redirect URL would serve as a sensible default, while the mapping would allow explicit control over redirect URLs for particular proxies.

This would have several advantages:

Ease of configuration. A big one I think, given the goals we have.
Reduce complexity, unless users need it. Most organizations would not have multiple proxy URLs.
Explicit control over mapping, with no ambiguity.

Joerger · 2022-05-17T19:23:43Z

Thinking about the feature in general: would it make sense to leave the old field in place (which would solve most compatibility issues) and add an additional "redirect override mapping" field? The single redirect URL would serve as a sensible default, while the mapping would allow explicit control over redirect URLs for particular proxies.

This would have several advantages:

Ease of configuration. A big one I think, given the goals we have.

Reduce complexity, unless users need it. Most organizations would not have multiple proxy URLs.

Explicit control over mapping, with no ambiguity.

To clarify, do you mean something like the following:

redirect_url: https://proxy.example.com:3080/v1/webapi/oidc/callback
redirect_url_override:
  other.example.com:3080: https://other.example.com:3080/v1/webapi/oidc/callback
  remote.example.com:443: https://remote.example.com:443/v1/webapi/oidc/callback

And for this to work, the value on the left would need to match the proxy public addr set in the proxy's teleport.yaml exactly.

I don't think that having an override mapping field would make this any less complex or easy to configure, but the explicitly of your approach might be worth it. But first let me explain the current approach and why I did it.

My goal was to make the change to oidc yaml configuration as simple/small as possible. Since the core of the change is to enable multiple RedirectURLs, it would make sense to make redirect_url a wrapper.Strings, like many other yaml fields where we expect 1 or more values (public_addr).

IMO this creates the easiest configuration experience. All you have to do is provide the Redirect URLs that are set in your OIDC provider, and you'll get the functionality for free:

redirect_url:
 - https://proxy.example.com:3080/v1/webapi/oidc/callback
 - https://remote.example.com:3080/v1/webapi/oidc/callback

and other users can continue to use the old functionality:

redirect_url: https://proxy.example.com:3080/v1/webapi/oidc/callback

The only backwards compatibility crutch is on the gRPC layer. Other than that, we can now use the redirect url list in place of the single string used previously.

Hopefully this approach makes more sense now, but let me know if you still think it's the wrong approach.

@r0mant What do you think?

Tener · 2022-05-17T19:47:35Z

To clarify, do you mean something like the following:

redirect_url: https://proxy.example.com:3080/v1/webapi/oidc/callback
redirect_url_override:
  other.example.com:3080: https://other.example.com:3080/v1/webapi/oidc/callback
  remote.example.com:443: https://remote.example.com:443/v1/webapi/oidc/callback

Thank you, something along these lines. The explicit design would allow us to handle setup with, say, the majority of company in U.S. but also a handful of branch offices in London and Tokyo:

redirect_url: https://teleport.example.com:3080/v1/webapi/oidc/callback
redirect_url_override:
  lon.teleport.example.com:3080: https://lon.teleport.example.com:3080/v1/webapi/oidc/callback
  tok.teleport.example.com:3080: https://tok.teleport.example.com:3080/v1/webapi/oidc/callback

I'm not sure this case would be handled correctly in current code:

teleport/lib/services/oidc.go

Lines 96 to 111 in be0fef7

    
           // GetRedirectURL gets a redirect URL for the given connector. If the connector 
        
           // has a redirect URL which matches the host of the given Proxy address, then 
        
           // that one will be returned. Otherwise, the first URL in the list will be returned. 
        
           func GetRedirectURL(conn types.OIDCConnector, proxyAddr string) string { 
        
           	if len(conn.GetRedirectURLs()) == 0 { 
        
           		return "" 
        
           	} 
        
           	for _, redirectURL := range conn.GetRedirectURLs() { 
        
           		if strings.Contains(redirectURL, proxyAddr) { 
        
           			return redirectURL 
        
           		} 
        
           	} 
        
           	return conn.GetRedirectURLs()[0] 
        
           }

IMO this creates the easiest configuration experience. All you have to do is provide the Redirect URLs that are set in your OIDC provider, and you'll get the functionality for free:
redirect_url:
 - https://proxy.example.com:3080/v1/webapi/oidc/callback
 - https://remote.example.com:3080/v1/webapi/oidc/callback
and other users can continue to use the old functionality:
redirect_url: https://proxy.example.com:3080/v1/webapi/oidc/callback

We could also extend the automatic configuration code (see pending PR: https://github.com/gravitational/teleport.e/blob/d837922073dbf4ec22ec79e707f122bd8ba46962/tool/tctl/sso_configure_command/oidc.go#L314) to automatically discover all URLs.

Joerger · 2022-05-18T02:00:25Z

@xinding33 @klizhentas Can I get your guys' UX opinion on this change?

The goal of this PR is to make it possible to choose an oidc redirect url based on the calling proxy during the oidc request.
e.g. eu.proxy.example.com -> oidc provider -> eu.proxy.example.com instead of eu.proxy.example.com -> oidc provider -> proxy.exampe.com.

These are the two options in the running after talking with @r0mant. I've listed a couple pros and cons as well.

1) match redirect url with proxy from a list

First, we enable providing multiple redirect urls to the oidc connector for each proxy which you want to redirect to:

redirect_url:
 - https://proxy.example.com:3080/v1/webapi/oidc/callback
 - https://eu.remote.example.com:3080/v1/webapi/oidc/callback
 - https://au.proxy.example.com:3080/v1/webapi/oidc/callback

Then, when a user makes an oidc request - tsh login --proxy=eu.remote.example.com:3080 - the redirect url will be chosen from the list. If there's a match, it will be used. Otherwise, the first item in the list will be used.

Pros:

In an oidc provider, you must provide one or more redirect urls directly. With this approach, you would just copy that list to your teleport configuration in oidc.redirect_url

Cons:

Changing redirect_url from a string to a wrapper.Strings adds some complexity and backwards compatibility crutches

2) configure the redirect url in the proxy

We can add a proxy configuration field - proxy.sso_redirect_addr. If this field is set, then https://<sso_redirect_addr>/v1/webapi/oidc/callback will be used as the redirect url for any oidc requests on this proxy.

If it isn't set, we use oidc.redirect_url instead, the current behavior.

Pros:

If we decide to expand this functionality to SAML or other SSO providers, the same proxy.sso_redirect_addr field and approach can be used.
Setting the redirect addr in each proxy makes it easier to configure as proxies are added/removed, etc.

Cons:

There are now two configuration fields which handle oidc redirect urls
There is potentially a security concern, since an oidc redirect url would come from the proxy rather than the auth connector resource.

xinding33 · 2022-05-20T01:45:10Z

@Joerger Hey, thanks for the explanation and summary of pros and cons. Correct me if I'm wrong, but couldn't the first approach also we also extend this functionality to SAMl or other SSO providers? Personally, I prefer the first approach as there'd be a single place users can configure all proxies and there remains a single place users can configure redirect URIs. Provided that the backwards compatibility crutches aren't too severe, this seems like the better approach for users. One big question regarding this approach, potentially related to security, is how can we help users remove unused/deprecated proxies?

Joerger · 2022-05-20T19:56:44Z

@Joerger Hey, thanks for the explanation and summary of pros and cons. Correct me if I'm wrong, but couldn't the first approach also we also extend this functionality to SAMl or other SSO providers?

In the future we could apply approach 1 to make github.redirect_url and saml.acr, but with approach 2, we'd just start using proxy.sso_redirect_addr instead of adding/changing the github and saml connector fields.

However like you said, the downside of approach 2 is that you are now setting up your sso configuration across (potentially) every proxy, rather than a single resource.

Personally, I prefer the first approach as there'd be a single place users can configure all proxies and there remains a single place users can configure redirect URIs. Provided that the backwards compatibility crutches aren't too severe, this seems like the better approach for users.

I agree, keeping it in one place should make it simpler to configure.

@xinding33 If we are going with the first approach, can you take a look at this question as well? - #12054 (comment)

One big question regarding this approach, potentially related to security, is how can we help users remove unused/deprecated proxies?

First, users can remove the proxy's redirect url directly from their service provider to prevent any auth attempts from succeeding from that proxy. Second, they can remove the proxy addr from oidc.redirect_url or proxy.sso_redirect_addr.

In both approaches, the proxy would default to oidc.redirect_url[0]. If the proxy was compromised in some way and tried to use it's own redirect url, it would fail to authenticate with the service provider regardless.

Tener · 2022-05-26T08:40:32Z

-		return clientPack.client, nil
+	cachedClient, ok := a.oidcClients[clientMapKey]
+	if ok {
+		if cmp.Equal(cachedClient.connector, conn) && cachedClient.syncCtx.Err() == nil {


I see how comparing connectors instead of configs makes it possible to refactor the code more heavily, but this will also cause the clients to be recreated more often than otherwise needed. For example, any change in ClaimToRoles will cause a new client to be created needlessly: the config between old and new clients are the same, yet we force the rotation due to connector changes.

In practice, this is unlikely to cause problems, but I'm wary of introducing changes with the potential for subtle bugs.

Hmm I see what you mean, part of the reason I did this was that the provider syncing uses oidc.GetIssuerURL which isn't covered by the config, but does require a client refresh.

In general clearing the cache on any connector change seems like the safest approach to me, and should cover future changes more broadly. However I'm not opposed to making the comparison more specific to avoid needless cache clearing on changes to fields like ClaimsToRoles.

…teleport into joerger/oidc-redirect-url

* Complete deprecation for OIDC RedirectURL - #12054. * Complete deprecation for CreateSessionTracker - #12304. * Complete deprecation for SSO Auth Request http endpoints - #13073. * Complete deprecation for #12795. * Complete deprecation for http GenerateToken - #9024.

Joerger force-pushed the joerger/oidc-redirect-url branch from fab5b67 to 475df06 Compare April 19, 2022 01:47

Joerger force-pushed the joerger/oidc-redirect-url branch 8 times, most recently from 0ca0ec6 to c960157 Compare May 16, 2022 21:35

Joerger marked this pull request as ready for review May 16, 2022 21:35

github-actions Bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label May 16, 2022

github-actions Bot requested review from gabrielcorado and smallinsky May 16, 2022 21:36

Joerger force-pushed the joerger/oidc-redirect-url branch from 0622fe2 to 7c0492f Compare May 16, 2022 23:23

smallinsky reviewed May 17, 2022

View reviewed changes

Comment thread lib/auth/auth.go Outdated

Comment thread lib/services/oidc.go Outdated

smallinsky requested a review from Tener May 17, 2022 12:25

Tener reviewed May 17, 2022

View reviewed changes

Joerger added the needs-product-decision label May 18, 2022

Joerger force-pushed the joerger/oidc-redirect-url branch 4 times, most recently from 16c0f88 to fc273a2 Compare May 19, 2022 01:35

Joerger force-pushed the joerger/oidc-redirect-url branch from 1070adb to c1c778a Compare May 23, 2022 19:19

Joerger requested a review from Tener May 23, 2022 19:51

Tener reviewed May 26, 2022

View reviewed changes

smallinsky approved these changes May 26, 2022

View reviewed changes

Comment thread api/types/oidc.go Outdated

Comment thread lib/services/oidc.go

Comment thread lib/auth/oidc.go

Tener approved these changes May 26, 2022

View reviewed changes

Joerger force-pushed the joerger/oidc-redirect-url branch from 01b4908 to ddd08d8 Compare May 26, 2022 17:11

Joerger added 3 commits May 26, 2022 11:31

Only refresh oidc client on relevant connector changes.

69bc073

Merge branch 'master' into joerger/oidc-redirect-url

99e4b10

Merge branch 'master' into joerger/oidc-redirect-url

11664b9

Joerger enabled auto-merge (squash) May 26, 2022 20:46

Joerger added 4 commits May 26, 2022 13:48

Set RedirectURL on upsert.

9a98356

Merge branch 'joerger/oidc-redirect-url' of github.com:gravitational/…

d86176f

…teleport into joerger/oidc-redirect-url

Add back CheckSetRedirectURL.

21651d0

Merge branch 'master' into joerger/oidc-redirect-url

de9b752

Joerger removed the needs-product-decision label May 26, 2022

Joerger added 2 commits May 26, 2022 16:52

Merge branch 'master' into joerger/oidc-redirect-url

413016e

Update e ref.

979222c

Joerger force-pushed the joerger/oidc-redirect-url branch from e5f316e to 979222c Compare May 27, 2022 22:43

Joerger added 3 commits May 27, 2022 15:44

Merge branch 'master' into joerger/oidc-redirect-url

e93a805

Merge branch 'master' into joerger/oidc-redirect-url

1d2f76c

Merge branch 'master' into joerger/oidc-redirect-url

78c7d92

Joerger merged commit 26bad23 into master May 31, 2022

Joerger deleted the joerger/oidc-redirect-url branch May 31, 2022 18:31

Joerger added a commit that referenced this pull request May 31, 2022

OIDC multiple redirect URLs (#12054)

0413465

Joerger mentioned this pull request May 31, 2022

[v9] OIDC multiple redirect URLs #13046

Merged

Joerger mentioned this pull request Sep 1, 2022

RequireMFAType #16034

Merged

Joerger added a commit that referenced this pull request Oct 5, 2022

Complete deprecation for OIDC RedirectURL - #12054.

cc5931d

mnaser mentioned this pull request Oct 29, 2023

Allow multiple redirect_urls in an OIDC auth connector/make the redirect_url key optional #25951

Open

Joerger mentioned this pull request Mar 6, 2026

[Browser MFA] Add Browser MFA to challenge request flow #63936

Merged

4 tasks

Conversation

Joerger commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Core changes

Other changes

Backwards compatibility

gRPC

yaml

Uh oh!

smallinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tener May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Joerger May 18, 2022

Choose a reason for hiding this comment

Uh oh!

r0mant May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joerger May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r0mant May 19, 2022

Choose a reason for hiding this comment

Uh oh!

Joerger May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joerger May 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tener May 17, 2022

Choose a reason for hiding this comment

Uh oh!

Joerger May 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tener commented May 17, 2022

Uh oh!

Joerger commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tener commented May 17, 2022

Uh oh!

Joerger commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1) match redirect url with proxy from a list

2) configure the redirect url in the proxy

Uh oh!

xinding33 commented May 20, 2022

Uh oh!

Joerger commented May 20, 2022

Uh oh!

Tener May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joerger May 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Joerger commented Apr 19, 2022 •

edited

Loading

r0mant May 19, 2022 •

edited

Loading

Joerger May 19, 2022 •

edited

Loading

Joerger May 19, 2022 •

edited

Loading

Joerger commented May 17, 2022 •

edited

Loading

Joerger commented May 18, 2022 •

edited

Loading

Tener May 26, 2022 •

edited

Loading