spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.4k stars 40.51k forks source link

Improve minimal auto-configuration for OIDC login with Okta #25549

Open Kehrlann opened 3 years ago

Kehrlann commented 3 years ago

Spring Boot supports common OAuth2 providers (facebook, github, google and okta), and provide a set of defaults for those providers. It works really well, I can do:

spring:
  security:
    oauth2:
      client:
        registration:
          google:
            client-id: MY_CLIENT_ID.apps.googleusercontent.com
            client-secret: MY_CLIENT_SECRET

However, Okta is a bit more involved, because I get my own subdomain, so there are no "default" URIs for authorization/token/jwks, etc.

So I have to do something like:

spring:
  security:
    oauth2:
      client:
        registration:
          okta:
            client-id: MY_CLIENT_ID
            client-secret: MY_CLIENT_SECRET
        provider:
          okta:
            authorization-uri: https://MY_SUBDOMAIN.okta.com/oauth2/default/v1/authorize
            token-uri: https://MY_SUBDOMAIN.okta.com/oauth2/default/v1/token
            jwk-set-uri: https://MY_SUBDOMAIN.okta.com/oauth2/default/v1/keys

Which is the minimal configuration for a functionning OIDC login, using Spring Security's CommonOAuth2Providers.

Okta also exposes an OIDC Discovery endpoint /.well-known/openid-configuration, so I would like to do:

spring:
  security:
    oauth2:
      client:
        registration:
          okta:
            client-id: MY_CLIENT_ID
            client-secret: MY_CLIENT_SECRET
        provider:
          okta:
            issuer-uri: https://MY_SUBDOMAIN.okta.com/oauth2/default

However, by doing so, I lose the access to everything defined in org.springframework.security.config.oauth2.client.CommonOAuth2Provider.OKTA, most crucially the scopes, which are required for OAuth 2.0 login.

This is because OAuth2ClientPropertiesRegistrationAdapter#getClientRegistration first gets the configuration from the issuer-uri, and only if that's not available, it tries to get the "Common provider". Using issuer-uri overrides everything in the "Common Provider".

I'm inclined to propose a PR for this,but I'm curious what are your thoughts on this first?

The idea would be to get the "Common provider" first, and the enhance it through issuer-uri auto-discovery. I'm a bit worried about the impact this would have on the other three "common providers", as your registration would have more information, coming from the discovery endpoint.

mbhave commented 3 years ago

It sounds like there are two issues here.

  1. Simplifying Okta configuration by removing the need to configure the provider details.
  2. Values from OpenID Provider Metadata overriding common provider values.

For 1, I think having a subdomain is pretty Okta specific and it doesn't seem like something we can add that would be generally useful.

For 2, my thought was that if the is a OpenID discovery endpoint available, values from that should be used in its entirety.

@Kehrlann Is there a reason why the OP Metadata does not contain the scopes that you need?

/cc @jzheaux Do you think it would make sense to use values from the common providers even when a discovery url is provided?

Kehrlann commented 3 years ago

I totally agree with 1, subdomain is too specific and wouldn't fit in the auto-config.

re:2, what you describe is currently what is happening. If we use an issuer-uri, we don't use the common provider values at all. The values selected are the following:

  1. Values from the discovery endpoint, including some defaults from spring-security if some of the values are either null or have multiple options, see for example ClientRegistrations#getClientAuthenticationMethod which selects which token_endpoint_auth_methods_supported should be used.
  2. Whatever values the user specifies (these take precedence)

This is works very well except for the scopes.

So it feels there no "easy" way to set default scopes from an issuer-uri-defined provider.


I guess I could re-frame the problem this way:

I have a feeling there are two possibilities:

[1]: Okta seems to have a different set of scopes_supported in the OIDC Discovery Endpoint and in the OAuth 2.0 Server Metadata but I think my point is still valid.

mbhave commented 3 years ago

I don't understand why scopes will be null if Okta returns a set of scopes from the OIDC Discovery Endpoint. You mentioned that scopes_supported by the discovery endpoint are provider specific and not client specific. But isn't that also true for the scopes in CommonOAuth2Providers.OKTA? Not every client would want all those scopes provided by the default provider.

mbhave commented 3 years ago

@jzheaux pointed me to this issue about not including scopes from scopes_supported in the client registration. That explains why the scopes are empty even if the discovery endpoint returns them.

Joe's comment indicates that leaving scopes empty so that a client explicitly configures them is desired. In that case, using default scopes for every client doesn't seem to align with that.

@jgrandja Can you comment on whether you think we should use scopes from CommonOAuth2Provider instead of having the client explicitly configure them?

Kehrlann commented 3 years ago

I agree with the issue mentioned. scopes_supported represent all available scopes for that given provider, regardless of the client. There might be a lot of those, that may not be relevant to most applications.

However CommonOAuth2Provider.OKTA...scopes is an opinion by the Spring team(s) on what sensible "default scopes" should be - a very valuable opinion! The user can always override them if they have specific needs. So every "default" client, auto-configured by Boot, gets those scopes. In the case of OKTA, Spring Security tells us "a sensible default is openid + profile + email".

Today it's difficult for a user to get these defaults:

In my opinion, we should go with one of the following:

  1. To get the default scopes for Okta, users only need to specify provider.okta.issuer-uri
  2. Okta is not mentioned as one of the "providers with defaults" in Spring Boot anymore.

So I wonder if the following flow would be reasonable:

  1. Get the CommonOAuth2Provider based on registrationId or registration.provider
  2. Enhance from issuer-uri if possible
  3. Override any provider value when a value is specified by the client
  4. Override any registration value when a value is specified by the client

The drawback here is when the users had no scopes configured, we would be adding them. Having no scopes configured means "trust the authorization server to give me the correct default scopes". By default, I think Okta does not use default scopes, you have to configure them manually in your Okta account.

mbhave commented 3 years ago

Sorry for the delay @Kehrlann. I think since the scopes_supported is an exception to what gets copied over from the metadata, we could probably limit values from CommonOAuth2Provider when an issuer_uri is specified to scopes only. So probably something like:

  1. Create the builder from issuer-uri
  2. Add scopes if it is a common provider
  3. Override any provider value when a value is specified by the client
  4. Override any registration value when a value is specified by the client

@jgrandja What are your thoughts on this?

Kehrlann commented 3 years ago

Behavior seems good. If Joe greenlights this, and if it works for you, I'd be interested in submitting a PR for this.

jgrandja commented 3 years ago

@Kehrlann @mbhave Just to give some context around why CommonOAuth2Provider was created in the first place.

When we released 5.0 with the new OAuth 2.0 support, CommonOAuth2Provider was provided as a convenience for the oauth2Login() feature.

CommonOAuth2Provider.OKTA.scope defaults to openid, profile, email, which makes sense for an OAuth 2.0 Client that performs OpenID Authentication. However, not every Okta ClientRegistration needs to perform OpenID Authentication.

For example, let's say we have an application that has multiple ClientRegistration's configured with only Okta as the provider. It is most likely only one of those ClientRegistration will perform OpenID Connect and the other ones will simply perform other grant types/flows. Therefore, it doesn't make sense to default all the ClientRegistration to CommonOAuth2Provider.OKTA.scope and most likely the scope setting is overridden with the scopes the client is configured to request.

CommonOAuth2Provider.OKTA.scope is really meant as a convenience for a simple OAuth application performing oauth2Login(). But for production applications, it is likely that the ClientRegistration properties are fined tuned and this most definitely would include the scope property IMO.

As far as the provider metadata scopes_supported goes, this should not be used to default ClientRegistration.scope. This is simply what scopes the provider supports (for any client to use). However, there are custom scopes that are registered with the provider that a client may use that are NOT returned in the scopes_supported. This metadata value is more of a common (or standard) list of scopes supported.

I feel the current logic within Boot and Security is correct and I don't feel any changes are needed here. It is more secure to be in control of properties set in ClientRegistration than relying on defaults. CommonOAuth2Provider was simply provided as a convenience for the simple basic configuration cases but for production grade applications, I would advise on explicitly configuring specific properties and scope would qualify as one of those.

mbhave commented 2 years ago

Based on comments in #10308, we will make this change.