ClientRegistrations.fromIssuerLocation for Oauth2 AuthorizationServer requires jwks url even though jwks is not required in the metadata spec

knutejoh commented 5 years ago

Summary

When using ClientRegistrations.fromIssuerLocation for setting up Oauth2 AuthorizationServer the code requires jwks url to be a part of the returned metadata in the .well-known/oauth-authorization-server even though this is not required in the metadata spec (see https://tools.ietf.org/html/rfc8414)

Actual Behavior

Nullpointer thrown from line 222 (.jwkSetUri(metadata.getJWKSetURI().toASCIIString())) in org.springframework.security.oauth2.client.registration.ClientRegistrations

Expected Behavior

No nullpointer and the client configured correctly with the provided metadata

Configuration

Here is an example metadata file that should work { "issuer": "https://issuerurl:port", "authorization_endpoint": "https://issuerurl:port/oauth/authorize", "token_endpoint": "https://issuerurl:port/oauth/token", "scopes_supported": [ "user:check-access", "user:full", "user:info", "user:list-projects", "user:list-scoped-projects" ], "response_types_supported": [ "code", "token" ], "grant_types_supported": [ "authorization_code", "implicit" ], "code_challenge_methods_supported": [ "plain", "S256" ] }

Version

Spring Securiy Oauth2 Client 5.2.0.RELEASE

Sample

rhamedy commented 5 years ago

I remember working on ClientRegistrations.fromIssuerLocation with @jzheaux help, however, the part that throws NPE wasn't touched.

Both JwtDecodersTests.java and ClientRegistrationsTests.java have a DEFAULT_RESPONSE_TEMPLATE that includes a value for jwks_uri field. The specification definitely marked jwks_uri as OPTIONAL.

I would love to make a pull request should we decide to refactor/add null-check 🙂

jzheaux commented 5 years ago

Thanks for the report @knutejoh. Adding a null-check makes sense.

@rhamedy sounds great, it's yours!

Since OIDC Discovery does require jwks_url, let's make sure to give a more informative error in the case of OIDC, but simply allow it to be null for OAuth2 metadata endpoints.

Let's also please address what is probably a similar issue in JwtDecoders and ReactiveJwtDecoders. Certainly, these must have a JWK Set Uri to be correctly constructed, but the classes can give a better error than NullPointerException.

rhamedy commented 5 years ago

@jzheaux Thank you for the info. A question before I get started

What's your suggestion when it comes to checking whether it's OIDC or OAUTH2 request?

Assuming that validation could go in ClientRegistrations.withProviderConfiguration, Can we rely on returned metadata object to decide if it's OIDC or OAUTH2? If yes, then

The fields that both specifications REQUIRE is response_types_supported and id_token seem to be specific to OIDC.
Not sure if we could rely on ClientRegistrations.getScopes method even though scopes_supported is not REQUIRED but, RECOMMENDED. If null, the method does seem to default to openid.

If we cannot rely on response metadata to decide then we could also rely on status of the rest call in ClientRegistrations.getConfiguration based on passed uris.

We might have to refactor the ClientRegistrationsTests since same DEFAULT_RESPONSE is used for both OIDC and OAUTH2 🤔

jzheaux commented 4 years ago

@rhamedy Good questions.

Can we rely on returned metadata object to decide if it's OIDC or OAUTH2?

No, I don't think we should sniff the response to try and detect the type of request.

Instead, it'd probably be best to validate at the time the request is made since we know the type of endpoint at that time.

Something like this might work:

Change:

private static URI oidc(URI issuer) {

to

private static Supplier<ClientRegistration.Builder> oidc(URI issuer) {

It would return a Supplier that would make the invocation and parse the response.

There are probably other ways to do it, too, but that would hopefully place the majority of the custom code for a given endpoint type in one spot.

rhamedy commented 4 years ago

@jzheaux With regards to your proposed suggestion for use of Supplier. I am curious how are we going to handle the getConfiguration call below from fromIssuerLocation

Map<String, Object> configuration = getConfiguration(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri));

I am struggling to understand how your proposed solution is going to work? Could you please give a little bit more insights into your vision for switch to Supplier?

The solution I have in mind is as follow:

Assumption

The OpenID Connect Discovery endpoint will always have openid-configuration in the endpoint URL.

Code changes

Update the getConfiguration method to replace the following

return rest.exchange(request, typeReference).getBody();

with

Map<String, Object> configuration = rest.exchange(request, typeReference).getBody();
validateJwksUri(uri, configuration);
return configuration;

Add a new method that conditionally validates jwks_uri if the discovery endpoint URL contains openid-configuration and throws exception if the response does not have jwks_uri key OR the value of jwks_uri is null (not sure whether to account for empty string as well or not).

private static void validateJwksUri(URI uri, Map<String, Object> configuration) {
    String JWKS_URI = "jwks_uri";

    if(uri.toASCIIString().indexOf("openid-configuration") != -1 && (!configuration.containsKey(JWKS_URI)
            || configuration.get(JWKS_URI) == null)) {
        throw new IllegalArgumentException("The '" + JWKS_URI + "' is a required field in OpenId protocol.");
    }
}

and finally, update the withProviderConfiguration to replace

.jwkSetUri(metadata.getJWKSetURI().toASCIIString())

with

.jwkSetUri(metadata.getJWKSetURI() != null ? metadata.getJWKSetURI().toASCIIString() : null)

With the above changes, in the tests, we just have to remove the jwks_uri from the response for some requests to test whether it's throwing an appropriate exception or not

System.out.println(this.response.remove("jwks_uri"));

Concern

The if(uri.toASCIIString().indexOf("openid-configuration") != -1 a little strange but, seem to work 🤔

Not sure if this is an ideal solution and whether it covers some edges cases or not. Regardless, curious to hear more about Supplier approach unless you think this is better.

Sorry for the long reply 😐

rhamedy commented 4 years ago

Hi, @jzheaux created a draft pull request since we are still in discussions around an ideal solution. The draft pull request showcases the changes for the fix I had in mind, however, I would be happy to re-purpose the pull request once I get a little more clarity in the Supplier

jzheaux commented 4 years ago

I believe this:

Map<String, Object> configuration = getConfiguration(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri));

would change to:

ClientRegistration.Builder clientRegistration = getConfiguration(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri));

The idea here is that we already know it is an OIDC call when we are inside the oidc method, so let's try and place the custom code there, eliminating the need for guessing based on the URI contents.

I think the oidc method, in turn, might look something like:

private static Supplier<ClientRegistration.Builder> oidc(URI issuer) {
    URI uri = UriComponentsBuilder.fromUri(issuer)
            .replacePath(issuer.getPath() + OIDC_METADATA_PATH).build(Collections.emptyMap());
    return () -> {
        RequestEntity<Void> request = RequestEntity.get(uri).build();
        Map<String, Object> response = rest.exchange(request, typeReference).getBody();
        OIDCProviderMetadata metadata = parse(response, OIDCProviderMetadata::parse);
        return withProviderConfiguration(metadata, issuer.toASCIIString())
                .jwkSetUri(metadata.getJWKSetURI().toASCIIString())
                .userInfoUri(metadata.getUserInfoEndpointURI().toASCIIString());
    };
}

rhamedy commented 4 years ago

Thanks, @jzheaux, I have pushed my changes up. Looking forward to hearing your thoughts 👍

spring-projects / spring-security