spring-projects / spring-boot

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

Expose property to configure configurationMetadata on an OAuth2 ClientRegistration #21375

Closed nagaraj-pattar closed 4 years ago

nagaraj-pattar commented 4 years ago

The class org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientProperties.Provider is required to have the following

    private Map<String, Object> configurationMetadata;

    public Map<String, Object> getConfigurationMetadata() {
        return configurationMetadata;
    }

    public void setConfigurationMetadata(Map<String, Object> configurationMetadata) {
        this.configurationMetadata = configurationMetadata;
    }

Due to this missing code, we are unable to configure additional metadata configuration like

"end_session_endpoint".

Request you to make this fix so that client registration defined in the application.properties / application.yml file can be configured with additional metadata and OidcClientInitiatedLogoutSuccessHandler can consume this metadata.

mbhave commented 4 years ago

I am going to revert this change. After speaking with @jgrandja, it's become clear that configurationMetadata on the provider is not meant to be set by the application. It is part of OpenID Connect's Discovery Metadata. If a issueUri is configured, Spring Security will get the metadata from the issuerUri and set it on the ClientRegistration.

@nagaraj-pattar Our guess as to why you need to explicitly set the end_session_endpoint is that the OpenID Connect provider you're using doesn't implement OpenID Connect discovery or it doesn't provide a value for end_session_endpoint in the metadata. If you must set the configurationMetadata yourself, you can do so by creating a ClientRegistration yourself and providing a ClientRegistrationRepository with it.

nagaraj-pattar commented 4 years ago

I faced few issues with issuer mismatch. If the issuer has mismatched, then those clients registrations were rejected by spring boot and application was not starting up. In such scenarios these properties will be useful to read client configuration from properties. There are already place holders for other URIs, hence, it is wise to expose these properties.

mbhave commented 4 years ago

Issuer mismatch sounds like some sort of misconfiguration. Configuring the configuration metadata manually is not what a majority of users would do and we don't want to expose a property that would let them do that. Like I said, if you must configure it manually, you can create your own ClientRegistration using the Builder yourself.

If you have any more questions please ask them on Gitter. Spring Security's Gitter might be a better place to ask anything about issuer mismatch or configurationMetadata.

dawi commented 3 years ago

Configuring the configuration metadata manually is not what a majority of users would do.

Manual metadata configuration has one advantage. The application is able to start, even if the authorization server is not reachable. I don't think that it is very likely, it's still an advantage though.

Actually this is the reason why I configured the metadata manually and stumbled over this issue.

@mbhave Is this decision final?

mbhave commented 3 years ago

@dawi As mentioned in the comment above, if you must configure the metadata manually, you can do so by creating your own ClientRegistration using the Builder.

adase11 commented 10 months ago

Coming back to this. Unless there is a strong reason for not allowing additional configuration after OpenID Connect discovery I think that Boot could offer the ability to set additional metadata properties that the provider doesn't offer configuration for. This would facilitate the case where the user (client) wants to be able to use discovery but is working with a provider that does not allow them to set the end_session_endpoint (and therefore in the current state is unable to fully rely on discovery).

For example, after working with a provider that does not offer the configuration of the end_session_endpoint property I used a variation of the solution in https://github.com/spring-projects/spring-security/issues/7695 to get things working. This works fine but it seems to me that this could be something that Spring Boot provides out of the box.

I'd propose that Spring Boot could offer a way, via properties, to specify additional configurationMetadata properties and have those specified values be added to any already provided by .well-known/openid-configuration discovery for the particular ClientRegistration.

This seems related to the following two previous issues that I came across in my search for a solution https://github.com/spring-projects/spring-security/issues/7695 https://github.com/spring-projects/spring-security/issues/10059

Also, the other way I've solved for this is to set up a LogoutSuccessHandler that calls the end_session_endpoint. This felt like more of a "hack" to me than an appropriate solution but I'm open to hearing other opinions on that.

wilkinsona commented 9 months ago

What's your take on this please, @jgrandja?

daniel-cues commented 9 months ago

@mbhave I disagree with Issuer mismatch always being a sort of misconfiguration. There are justified cases in which the issuer might mismatch. For example, I might use subdomains for each application Identity manager and this leads to a mismatch:

{
"issuer":"https://identity.mydomain.com/",
"authorization_endpoint":"https://identity-1238751009726.identity.mydomain.com/oauth2/v1/authorize",
"token_endpoint":"https://identity-1238751009726.identity.mydomain.com/oauth2/v1/token",
...
}

This setup works on other frameworks, I can't find a reason why it shouldn't on spring. I think there should be either an option to allow mismatches or at least an option to load all metadata manually.

https://github.com/spring-projects/spring-boot/issues/21375#issuecomment-1823470022 @adase11 That sounds definitely like a hack. The fact is, OidcClientInitiatedLogoutSuccessHandler exists specifically for that exact purpose but it cannot be used if end_session_endpoint is not set, which can happen for a myriad of reasons. I don't see the point on supporting this many property mappings, if they're not complete and actively make functionality unavailable.

jgrandja commented 9 months ago

@adase11

I think that Boot could offer the ability to set additional metadata properties that the provider doesn't offer configuration for

I'm curious, why doesn't the provider return the specific configuration metadata? It looks like you are referring to end_session_endpoint? If this is not being returned in the provider configuration response, then the provider does not provide this capability, which happens to be the Logout Endpoint.

However, since you're looking to set this manually, it appears that the provider does in fact support the Logout Endpoint? If this is the case, then the Provider should be configured correctly to return end_session_endpoint. This is a configuration that should be applied at the provider. Allowing the flexibility to manually set provider configuration metadata at the client application (via Boot properties) does not make sense to me, especially for this particular case.

What provider are you using? Are you able to configure it or is this managed by another team?

jgrandja commented 9 months ago

@daniel-cues

There are justified cases in which the issuer might mismatch. For example, I might use subdomains for each application Identity manager and this leads to a mismatch

Please review the spec at 4.3. OpenID Provider Configuration Validation:

The issuer value returned MUST be identical to the Issuer URL that was directly used to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

FYI, this validation check lives in Spring Security (ClientRegistrations) not Spring Boot. And as mentioned above, this validation is implemented to spec.

I understand your environment is using subdomains for each issuer (very common), however, this is still an environment related issue and should be addressed there.

adase11 commented 9 months ago

@jgrandja Here's Google's documentation around OIDC discovery which points to their .well-known/openid-configuration from accounts.google.com that doesn't contain the end_session_endpoint. I cant say definitively if there isn't some other reason that discovery file excludes the end_session_endpoint.

Another one I've seen more details on is Auth0. Auth0 doesn't allow users to configure the end_session_endpoint like they do for all the other OAuth endpoints. There, now is more complex ways to configure this with Auth0 (there are a few mentions of this on their message boards here here). What they've set up is that clients can contact support to get the endpoint added to the configuration (see docs) however I figured that if they treat this differently than others might as well.

For what it's worth, I definitely hear the argument that the Boot shouldn't have to account for providers that are non compliant to the spec

pelepelin commented 6 months ago

you can do so by creating a ClientRegistration yourself and providing a ClientRegistrationRepository with it

Missing ability to override a couple of additional properties requires copying a very big chunk of code which handles all the configuration properties under spring.security.oauth2.client.

In my case OIDC provider is compliant but it is served via a reverse proxy which is not ready at the moment of Spring app configuration and it makes impossible to fetch from issuer-uri. And that's ridiculous that it's not possible to provide the data from .well-known/openid-configuration the other way than via hardcoded RestTemplate in a final class ClientRegistrations.

wilkinsona commented 6 months ago

@pelepelin can you please describe your use case in some more detail? Specifically, it would be useful to know exactly what additional configuration you would like to be able to provide and why that's necessary when working with a compliant OIDC provider. Also, where does the reverse proxy fit in and how does that adversely affect things?

pelepelin commented 6 months ago

@wilkinsona Generally it's similar to the last linked ticket https://github.com/spring-projects/spring-security/issues/14257

So in the end I followed https://docs.spring.io/spring-security/reference/reactive/oauth2/login/core.html#webflux-oauth2-login-register-reactiveclientregistrationrepository-bean and it's not so much code but I was disappointed that I can't use spring-boot configuration properties provided out of the box (and might miss something configuring the client with my code).

wilkinsona commented 6 months ago

Thanks, @pelepelin.

WDYT of this use case, @jgrandja?

alexist commented 2 months ago

In my case : We have lot of OIDC providers, some are managed by our organization, and other are provided by our customer. Configuring issuer uri work, but : if only one provider is down, Spring application will not start. It should be great if i WE Can possible configure everything (The application should start in degraded mode and try to resolve later Dynamic URL configuration)

jgrandja commented 2 months ago

@pelepelin

Missing ability to override a couple of additional properties requires copying a very big chunk of code which handles all the configuration properties under spring.security.oauth2.client

I'd like to see the code duplication your application is requiring. Can you please put together a minimal sample demonstrating where you are needing to copy code and we'll see what we can do to help reduce it. Feel free to use the Spring Authorization Server Demo Sample as a starting point.

pelepelin commented 2 months ago

@jgrandja I'm afraid a self-containing sample for my use case would involve a docker-compose, gateway service and keycloak as an identity provider. However, for my sentence the configuration bean below is enough, companion custom properties class is obvious.

This is effectively a specialized version of OAuth2ClientPropertiesMapper with the difference that it sets some more properties, notably (up: fixed) it sets issuerUri without network validation, and it sets end_session_endpoint for which there is no setter in spring-boot.

@Configuration
@AllArgsConstructor
public class OAuth2Config {

    @NonNull
    SecurityProperties securityProperties;

    @Bean
    public ReactiveClientRegistrationRepository clientRegistrationRepository() {
        return new InMemoryReactiveClientRegistrationRepository(this.clientRegistration());
    }

    private ClientRegistration clientRegistration() {
        OAuth2Client props = securityProperties.oauth2Client();
        return ClientRegistration.withRegistrationId(props.registrationId())
            .clientId(props.clientId())
            .clientSecret(props.clientSecret())
            .clientName(props.clientName())
            .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC)
            .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
            .redirectUri("{baseUrl}" + securityProperties.authorizationCallbackPath())
            .scope(props.scope())
            .userNameAttributeName(props.userNameAttribute())
            .issuerUri(props.issuer().toASCIIString())
            .authorizationUri(props.authorizationEndpoint().toASCIIString())
            .tokenUri(props.tokenEndpoint().toASCIIString())
            .userInfoUri(props.userinfoEndpoint().toASCIIString())
            .jwkSetUri(props.jwksUri().toASCIIString())
            .providerConfigurationMetadata(Map.of(
                "end_session_endpoint", props.endSessionEndpoint().toASCIIString()
            ))
            .build();
    }
}