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

Unable to use oAuth login with ES256 due to ReactiveOidcIdTokenDecoderFactory locked to RS256 #11799

Closed
thomasmillercf opened this issue Sep 8, 2022 · 11 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid

Comments

@thomasmillercf
Copy link

Describe the bug
The current identity provider i am using only supports ES256, and i was unable to finish the login flow as i would get unsupported algorithm of ES256. Digging further I have found the class ReactiveOidcIdTokenDecoderFactory.class locks the algo to RS256 even without checking if its supported in the jwks.

Looking into the openid spec of https://openid.net/specs/openid-financial-api-part-2-1_0.html it say ES256 should be used and RS265 should not be even though in openid spec v1 it says RS265 should be the default.
Either way i should be able to use ES256

To Reproduce

I used the cloudentity as my provider, however any provider which only supports ES256 will recreate the issue.

spring:
  security:
    oauth2:
      client:
        registration:
          cloudentity:
            provider: cloudentity
            client-authentication-method: none
            client-id: test
            authorization-grant-type: authorization_code
            scope: openid
        provider:
          cloudentity:
            token-uri: https://xxxxx.eu.authz.cloudentity.io/xxxxx/test/oauth2/token
            authorization-uri: https://xxxxx.eu.authz.cloudentity.io/xxxxx/test/oauth2/authorize
            issuer-uri: https://xxxxx.eu.authz.cloudentity.io/xxxxx/test
            jwk-set-uri: https://xxxxx.eu.authz.cloudentity.io/xxxxx/test/.well-known/jwks.json
            user-name-attribute: name
            user-info-uri: https://xxxxx.eu.authz.cloudentity.io/xxxxx/test/userinfo

Go to https://xxx.eu.authz.cloudentity.io/xxx/test/oauth2/authorize?response_type=code&client_id=test&redirect_uri=http://localhost:8080/login/oauth2/code/cloudentity

Then you will get ``nsupported algorithm of ES256`

Expected behavior

When registering the jwt decoder it should check the jwks set for which algo to use, where it defaults to RS256 then if its not found use the first one which the jwks set

Or
I should be able to configure the algo via

spring:
security:
oauth2:
client:
provider:
cloudentity:
algo: es256

@thomasmillercf thomasmillercf added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 8, 2022
@sjohnr
Copy link
Member

sjohnr commented Sep 9, 2022

@thomasmillercf, thanks for reaching out!

Take a look at the section on ID Token Signature Verification. You can provide the algorithm resolver directly as an @Bean using:

@Bean
public ReactiveJwtDecoderFactory<ClientRegistration> idTokenDecoderFactory() {
	ReactiveOidcIdTokenDecoderFactory idTokenDecoderFactory = new ReactiveOidcIdTokenDecoderFactory();
	idTokenDecoderFactory.setJwsAlgorithmResolver(clientRegistration -> SignatureAlgorithm.ES256);
	return idTokenDecoderFactory;
}

I believe this will get you on track. Having said that, I don't know (off hand) how feasible it would be to add this kind of intelligence into the implementation of ReactiveOidcIdTokenDecoderFactory, but it could be possible. For now, I'm going to close this issue as answered, but if you feel this should continue to be considered as an enhancement, feel free to let me know and we can re-open if necessary.

@sjohnr sjohnr closed this as completed Sep 9, 2022
@sjohnr sjohnr added for: stackoverflow A question that's better suited to stackoverflow.com and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 9, 2022
@sjohnr sjohnr self-assigned this Sep 9, 2022
@thomasmillercf
Copy link
Author

thomasmillercf commented Sep 9, 2022 via email

@sjohnr
Copy link
Member

sjohnr commented Sep 9, 2022

It does not look to hard if an optional algorithm could be provided on the client registration.

Since the existing algorithm resolver takes a ClientRegistration as the parameter, it does seem possible. It would require changes all the way from ClientRegistration to the Builder to Spring Boot's configuration properties to make it that easy, however.

If you feel it would be a valuable enhancement, we can re-open and discuss it further. It's worth considering that the ClientRegistration is used for more than just OIDC, so it feels like it might not make sense in all cases.

I was also wondering if you meant you thought it would be possible to "discover" the resolver from the jwk-set-uri? I haven't studied the spec deeply yet and so didn't have an answer to that question off hand. ReactiveOidcIdTokenDecoderFactory feels like a better place for this logic than pinning it to ClientRegistration since it's specific to OIDC.

@thomasmillercf
Copy link
Author

thomasmillercf commented Sep 9, 2022 via email

@sjohnr sjohnr reopened this Sep 12, 2022
@sjohnr sjohnr added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed for: stackoverflow A question that's better suited to stackoverflow.com labels Sep 12, 2022
@sjohnr
Copy link
Member

sjohnr commented Sep 12, 2022

@thomasmillercf I've reopened the issue, per your recommendation. However, I've labeled this as an enhancement.

@thomasmillercf
Copy link
Author

I have come across another issue which is related #7160
But looking into it the resources server already does what i am asking here.

But as mentioned in the ticket the reactive version is not working as expected

@jgrandja
Copy link
Contributor

@thomasmillercf

In theory its possible to work out the algo from the jwk-set-url

This is not always possible. There may be 2 or more keys that have:

"use": "sig",
"kty": "EC",
"alg": "ES256",

There may be one or more active keys and one or more passive keys depending on the key rotation strategy.

Furthermore, I haven't seen a key selection strategy defined in any of the specs so this would be custom behaviour, which is typically implemented in the consuming application. The framework will only implement to spec.

@thomasmillercf
Copy link
Author

@jgrandja

There is another issue #11812 which technically should solve this problem. As the custom bean ReactiveJwtDecoderFactory can just use that functionality.

However if we want to keep the the spec, and do not want to change default behaviour. I still think you should be able to configure a list of algo in the provider, which is something you can do in the spring resource server.

@jgrandja
Copy link
Contributor

@thomasmillercf

There is another issue #11812 which technically should solve this problem.

No, that issue is different and would not solve this one - JwtDecoders will return a JwtDecoder configured with a list of supported (allowed) algorithms which it deduces from the alg of the JwkSet.

You will still run into the same scenario as I described above

There may be one or more active keys and one or more passive keys depending on the key rotation strategy.

Regarding...

I still think you should be able to configure a list of algo in the provider, which is something you can do in the spring resource server.

Adding a property jwsAlgorithm for the client registration configuration might be an option but it's still possible to configure a different algorithm through bean configuration as such:

@Bean
public JwtDecoderFactory<ClientRegistration> idTokenDecoderFactory() {
	OidcIdTokenDecoderFactory idTokenDecoderFactory = new OidcIdTokenDecoderFactory();
	idTokenDecoderFactory.setJwsAlgorithmResolver(clientRegistration -> MacAlgorithm.HS256);
	return idTokenDecoderFactory;
}

IMO, this is a pretty straight forward configuration. However, I'd like to understand from your viewpoint what the benefit would be to use property configuration?

@thomasmillercf
Copy link
Author

@jgrandja

You are correct its straight forward if i had a single provider however I basically need to add a conditional depending on what client i am using for multi providers. At that point it feels really odd when i am configuring a provider in multiple places.

As i have to do one of the following

@Bean
public ReactiveJwtDecoderFactory<ClientRegistration> idTokenDecoderFactory() {
    ReactiveOidcIdTokenDecoderFactory idTokenDecoderFactory = new ReactiveOidcIdTokenDecoderFactory();
    idTokenDecoderFactory.setJwsAlgorithmResolver(clientRegistration -> clientRegistration.getRegistrationId().equals("cloudentity") ? SignatureAlgorithm.ES256 : SignatureAlgorithm.RS256);
    return idTokenDecoderFactory;
}

//or even

@Bean
public ReactiveJwtDecoderFactory<ClientRegistration> idTokenDecoderFactory(Map<String, SignatureAlgorithm> algoMap) {
    ReactiveOidcIdTokenDecoderFactory idTokenDecoderFactory = new ReactiveOidcIdTokenDecoderFactory();
    idTokenDecoderFactory.setJwsAlgorithmResolver(clientRegistration -> algoMap.get(clientRegistration.getRegistrationId());
    return idTokenDecoderFactory;
}

@jgrandja
Copy link
Contributor

@thomasmillercf

At that point it feels really odd when i am configuring a provider in multiple places.

The available property configuration options for a client is quite simple and minimal and if you need advanced configuration then it comes down to @Bean configuration. The ReactiveJwtDecoderFactory<ClientRegistration> is just one of many @Bean configurations available that would require similar conditional logic.

@Bean configuration provides much more flexibility compared to properties since they are static by nature.

Either way, I'm not trying to convince you otherwise but we have to be careful with the new features we introduce. There is a reason we don't provide too many properties for configuration as it would become unmanageable at some point if there are too many options available. So we decided to keep with the simple and minimal approach and if you need advanced configuration that is where @Bean configuration comes in.

I'm going to close this issue since the original issue stated that ReactiveOidcIdTokenDecoderFactory cannot be configured which is not the case.

Furthermore, I don't see us adding a new property for jwsAlgorithm at this point and even if we did this feature request would need to be added to the Spring Boot project since it would live there.

@jgrandja jgrandja assigned jgrandja and unassigned sjohnr Sep 14, 2022
@jgrandja jgrandja added status: invalid An issue that we don't feel is valid and removed type: enhancement A general enhancement labels Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants