Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling OAuth2 / OIDC provider discovery explicitly #42172

Open
delbertooo opened this issue Sep 6, 2024 · 8 comments
Open

Allow disabling OAuth2 / OIDC provider discovery explicitly #42172

delbertooo opened this issue Sep 6, 2024 · 8 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@delbertooo
Copy link

I think it would be useful to disable the OAuth2 / OIDC discovery explicitly. At this moment this is possible implicitly by configuring every necessary detail of the clients registration and provider but skipping the providers issuerUri. This disables the discovery via OAuth2ClientPropertiesMapper.

Why

  • Someone wants the service to not require the IdP to be available at startup.

  • Some code may need the issuerUri to function properly.

    In fact, there already is such code: Spring Securitys OIDC back channel logout validates the providers issuerUri in OidcBackChannelLogoutTokenValidator and ends up with a NPE if you did not set an issuerUri.

    So setting the issuerUri means you are forced to use discovery.
    Leaving it null means no working back channel logout, at least not with auto configuration.

How?

I'm not quite sure. Maybe a new property:

# default (fallback) value: true
spring.security.oauth2.client.provider.MY_PROVIDER.discovery=false

Setting this to false would opt-out the discovery. The current behaviour should be kept as default.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 6, 2024
@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Sep 6, 2024
@knoobie
Copy link

knoobie commented Sep 7, 2024

+1 / We also have strong requirements that systems should not depend on other systems while starting - so we are also forced to manually configure everything. This leads to interesting problems when e.g. the oauth provider changes something and our configuration gets out of date - which in turn creates hard to troubleshoot problem at first glance.

@wilkinsona
Copy link
Member

This leads to interesting problems when e.g. the oauth provider changes something and our configuration gets out of date

How would disabling discovery help with that? I don't understand how it would help to keep your configuration in sync with that of your OAuth provider.

@knoobie
Copy link

knoobie commented Sep 9, 2024

How would disabling discovery help with that? I don't understand how it would help to keep your configuration in sync with that of your OAuth provider.

Good question, Andy! I went a little bit overboard and interpreted a bit too much into the issue, like e.g. true/false and "lazy" loading of the configuration - allowing the application to start, but retrieve the required information on first access.

@philwebb
Copy link
Member

philwebb commented Sep 9, 2024

@jgrandja do you have any thoughts on this suggested change?

@philwebb philwebb added status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Sep 9, 2024
@jgrandja
Copy link

jgrandja commented Sep 9, 2024

@delbertooo

Some code may need the issuerUri to function properly.

In fact, there already is such code: Spring Securitys OIDC back channel logout validates the providers issuerUri in OidcBackChannelLogoutTokenValidator and ends up with a NPE if you did not set an issuerUri

I can't recall, but the issuerUri may be required for the OIDC back channel logout. @jzheaux Would be able to answer this one.

Someone wants the service to not require the IdP to be available at startup.

The SupplierClientRegistrationRepository added in gh-12967 could solve this issue as the issuer lookup is deferred on first access.

@philwebb Maybe the solution is to wrap the InMemoryClientRegistrationRepository @Bean

in a SupplierClientRegistrationRepository?

@jzheaux
Copy link
Contributor

jzheaux commented Sep 9, 2024

Yes, the issuer URI is required for back-channel logout due to how ID Tokens are validated.

As a side note, it would certainly be reasonable to give a better error response. I've added spring-projects/spring-security#15771 to address that.

@delbertooo
Copy link
Author

The SupplierClientRegistrationRepository added in gh-12967 could solve this issue as the issuer lookup is deferred on first access.

@philwebb Maybe the solution is to wrap the InMemoryClientRegistrationRepository @Bean

in a SupplierClientRegistrationRepository?

@jgrandja Yes, this kind of works and is our current solution / workaround:

    @Bean
    public SupplierClientRegistrationRepository clientRegistrationRepository(OAuth2ClientProperties properties) {
        return new SupplierClientRegistrationRepository(() -> {
            log.info("Loading OAuth2 client registrations");
            final var registrations = new OAuth2ClientPropertiesMapper(properties).asClientRegistrations();
            return new InMemoryClientRegistrationRepository(registrations);
        });
    }

Works for some scenarios. Might break if one uses oauth2Login with the default generated login page, because the login page works with login links from the repository and is built at application boot. Therefore the application cannot boot without the provider.

This leads me to the question: is it desirable to configure a provider completely without the need of discovery? Or would it be more straight forward to have a local copy of the configuration metadata and use the discovery against the local file copy?

@jgrandja
Copy link

jgrandja commented Sep 9, 2024

@delbertooo

Might break if one uses oauth2Login with the default generated login page

Yes, you are right, it would break if the provider configuration endpoint was not accessible at startup time. Although the generated login page is only meant to be used at development/test time and not production, we would need to ensure it doesn't break regardless.

is it desirable to configure a provider completely without the need of discovery? Or would it be more straight forward to have a local copy of the configuration metadata and use the discovery against the local file copy?

It depends on the requirements of your application environment. The way I see it is if your application needs access to the provider configuration endpoint then it needs to ensure the provider is up and running at startup. But if it needs to be more resilient at startup and not depend on the provider then you should be explicit on the provider configuration and not depend on issuer-uri or use SupplierClientRegistrationRepository.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Sep 9, 2024
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Dec 11, 2024
@philwebb philwebb added this to the 3.x milestone Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants