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

Provide support for symmetric key verification via JwtDecoder #5465

Closed
sirianni opened this issue Jun 26, 2018 · 18 comments

Comments

@sirianni
Copy link

commented Jun 26, 2018

JWT signature verification for id_token fails in OidcAuthorizationCodeAuthenticationProvider for OIDC providers like Ping Federate that have alternate signature verification rules.

From Ping Identity Developers Guide:

Note: Signature validation is only required for tokens not received directly from the token endpoint (i.e. for the Implicit Client Profile). In other cases where the id_token is received directly by the client from the token endpoint over HTTPS, transport layer security should be sufficient to vouch for the integrity of the token.

And
image

These rules are valid according to the OpenID Connect specification:

If the ID Token is received via direct communication between the Client and the Token Endpoint (which it is in this flow), the TLS server validation MAY be used to validate the issuer in place of checking the token signature. The Client MUST validate the signature of all other ID Tokens according to JWS [JWS] using the algorithm specified in the JWT alg Header Parameter. The Client MUST use the keys provided by the Issuer.

There is a method within OidcAuthorizationCodeAuthenticationProvider to dynamically lookup a JwtDecoder for a given ClientRegistration:

private JwtDecoder getJwtDecoder(ClientRegistration clientRegistration) {

A few challenges here:

  1. That method is private
  2. There is no way to plug in sublcasses or alternate mappings in the .oauth2Login() configuration DSL

As such, there need to be changes to the core classes to enable such pluggability.

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

Thanks for the report @sirianni! I think being able to customize the implementation is important, but I think the first step is to fix the default implementation so it works with Ping.

@rwinch rwinch added this to the 5.1.0.M2 milestone Jun 26, 2018

@sirianni

This comment has been minimized.

Copy link
Author

commented Jun 26, 2018

OK. What approach would you suggest for that? I may be able to work on a PR.

@sirianni

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

If you don't view the Ping approach of reusing the client secret as a symmetric JWT signing key as a special case, a configuration-only approach might suffice. You could be to add another field to ProviderDetails. Something like idTokenSigningKeySource with the values:

  • jwks - resolve the public key using JWKS URL (current behavior)
  • clientSecret- use the client secret as a symmetric key
  • none - trust the id_token without signature verification since it was served directly over TLS
@sirianni

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

Looks like Auth0 allows a symmetric key to be used also:

image

However, they don't reuse the client_secret. It's a separately configured value (if I'm reading the docs correctly).
So you could make the config parameter an object with a type and a value. Something like

provider.auth0.idTokenSignature.strategy=sharedSecret
provider.auth0.idTokenSignature.secret=mySecretKey
provider.microsoft.idTokenSignature.strategy=jwks
provider.ping.idTokenSignature.straegy=trust

sirianni pushed a commit to sirianni/spring-security that referenced this issue Jun 27, 2018

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2018

@sirianni Can you please provide more details around the issue you are having.

JWT signature verification for id_token fails in OidcAuthorizationCodeAuthenticationProvider for OIDC providers like Ping Federate that have alternate signature verification rules.

Specifically, what is the alg in the JWS header that Ping is signing with? Can you also provide a stacktrace of the exception?

I agree with @rwinch, let's ensure we have things working out-of-the-box for Ping before we expose configuration points that allow for customization.

@sirianni

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

As an aside, I will say that it seems the Spring OAuth2 coding style has deviated from the standard Spring practice of coding for extensibility that has made Spring so popular. The code uses some fairly paranoid practices like marking classes as final and using private constructors and methods vs. protected ones.

let's ensure we have things working out-of-the-box for Ping

Right, I get it. I hope you can see from the comments above that I'm trying to help you assess the ability to have a generic "out of the box" solution that works for Ping.

Specifically, what is the alg in the JWS header that Ping is signing with

As I mentioned in my comment above (with screenshot of Ping docs), Ping is signing with HS256.

Right off the bat you can see that NimbusJwtDecoderJwkSupport has hardcoded the RS256 JwsAlgorithm.

	public NimbusJwtDecoderJwkSupport(String jwkSetUrl) {
		this(jwkSetUrl, JwsAlgorithms.RS256);
	}

Stacktrace:

org.springframework.security.oauth2.jwt.JwtException: An error occurred while attempting to decode the Jwt: Signed JWT rejected: Another algorithm expected, or no matching key(s) found
	at org.springframework.security.oauth2.jwt.NimbusJwtDecoderJwkSupport.decode(NimbusJwtDecoderJwkSupport.java:123) ~[spring-security-oauth2-jose-5.0.6.RELEASE.jar:5.0.6.RELEASE]
	at org.springframework.security.oauth2.client.oidc.authentication.OidcAuthorizationCodeAuthenticationProvider.authenticate(OidcAuthorizationCodeAuthenticationProvider.java:153) ~[spring-security-oauth2-client-5.0.7.BUILD-SNAPSHOT.jar:5.0.7.BUILD-SNAPSHOT]
	at org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:174) ~[spring-security-core-5.0.6.RELEASE.jar:5.0.6.RELEASE]
	at org.springframework.security.oauth2.client.web.OAuth2LoginAuthenticationFilter.attemptAuthentication(OAuth2LoginAuthenticationFilter.java:164) ~[spring-security-oauth2-client-5.0.7.BUILD-SNAPSHOT.jar:5.0.7.BUILD-SNAPSHOT]
	at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:212) ~[spring-security-web-5.0.6.RELEASE.jar:5.0.6.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.6.RELEASE.jar:5.0.6.RELEASE]
	at org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestRedirectFilter.doFilterInternal(OAuth2AuthorizationRequestRedirectFilter.java:128) ~[spring-security-oauth2-client-5.0.7.BUILD-SNAPSHOT.jar:5.0.7.BUILD-SNAPSHOT]
@sirianni

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

Note that Nimbus' native IDTokenValidator handles this case properly:

/**
 * Validator of ID tokens issued by an OpenID Provider (OP).
 *
 * <p>Supports processing of ID tokens with the following protection:
 *
 * <ul>
 *     <li>ID tokens signed (JWS) with the OP's RSA or EC key, require the
 *         OP public JWK set (provided by value or URL) to verify them.
 *     <li>ID tokens authenticated with a JWS HMAC, require the client's secret
 *         to verify them.
 *     <li>Unsecured (plain) ID tokens received at the token endpoint.
 * </ul>
 *
 * <p>Convenience static methods for creating an ID token validator from OpenID
 * Provider metadata or issuer URL, and the registered Relying Party
 * information:
 *
 * <ul>
 *     <li>{@link #create(OIDCProviderMetadata, OIDCClientInformation)}
 *     <li>{@link #create(Issuer, OIDCClientInformation)}
 * </ul>
 *
 * <p>Related specifications:
 *
 * <ul>
 *     <li>OpenID Connect Core 1.0, sections 3.1.3.7, 3.2.2.11 and 3.3.2.12.
 * </ul>
 */
@ThreadSafe
public class IDTokenValidator extends AbstractJWTValidator implements ClockSkewAware {
@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@sirianni Thank you for all your feedback!

As an aside, I will say that it seems the Spring OAuth2 coding style has deviated from the standard Spring practice of coding for extensibility that has made Spring so popular.

This is very true and quite intentional in the short term. We are adding lots of new functionality at a fast pace and so we are starting out quite cautious. As we get reasonable enhancement requests, we will be able to spend time to improve the flexibility that users are use to. If you find places you need to customize that are not easy to do so, please let us know and we will work with you to update the code.

I want to emphasize that we agree that customization of the signature verification is desirable. However, we want to prioritize fixing this to work out of the box first. It may seem very easy to expose a new API, but as a framework every API we create and expose limits our ability to refactor the code later. I hope this helps alleviate any concerns you might have.

Thanks again for working with us to make Spring Security better!

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2018

@sirianni

Right off the bat you can see that NimbusJwtDecoderJwkSupport has hardcoded the RS256 JwsAlgorithm

This was intentional to support RS256 only in the initial release of 5.0, as it was quite easy to integrate with the JWK Set endpoint.

Symmetric key verification on the other hand is going to be more involved.

Note that Nimbus' native IDTokenValidator handles this case properly

Yes it does but this is a lot more involved. The IDTokenValidator relies on Provider metadata (OpenID Connect Discovery 1.0) and Client metadata (OpenID Connect Dynamic Client Registration 1.0) in order to achieve symmetric key verification in a dynamic way.

There is a lot more work that needs to be done to have full support for OAuth/OIDC and related extension specs. We will get there but it will take some more time.

We'll prioritize symmetric key verification support so please stay tuned.

Thanks again for the feedback.

@jgrandja jgrandja changed the title Pluggable JWT signature verification for OIDC id_token Provide support for symmetric key verification via JwtDecoder Jun 28, 2018

sirianni pushed a commit to sirianni/spring-security that referenced this issue Jun 28, 2018

@jgrandja jgrandja modified the milestones: 5.1.0.M2, 5.1.0.RC1 Jul 24, 2018

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2018

@sirianni We were really hoping to resolve this issue and get it into 5.1, but unfortunately we didn't have enough resources to get to this.

However, we're still hoping to resolve #5717 for 5.1 and I believe this will still help you as it will allow you to configure a custom JwtDecoder for ID Token verification. However, you would still need to implement a JwtDecoder that provides symmetric key verification. This will at least allow you to configure it.

Would you be interested in submitting a PR for #5717 ?

@jgrandja jgrandja modified the milestones: General Backlog, 5.2.x Oct 19, 2018

@jgrandja jgrandja added this to jgrandja in Security Team Oct 23, 2018

@jgrandja jgrandja modified the milestones: 5.2.x, 5.2.0.M1 Oct 30, 2018

@jgrandja jgrandja removed this from the 5.2.0.M1 milestone Jan 14, 2019

@jgrandja jgrandja added this to the 5.2.0.M2 milestone Jan 14, 2019

@jgrandja jgrandja self-assigned this Jan 16, 2019

@jgrandja jgrandja moved this from jgrandja to In Progress in Security Team Jan 16, 2019

jgrandja added a commit to jgrandja/spring-security that referenced this issue Jan 31, 2019

jgrandja added a commit to jgrandja/spring-security that referenced this issue Apr 11, 2019

@jgrandja jgrandja closed this in bed3371 Apr 12, 2019

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

@sirianni This new feature has been added and is now in master. We're planning to release 5.2.0.M2 this coming Monday so feel free to try this out and let me know how it goes. FYI, the changes are in this commit bed3371

@jgrandja jgrandja removed this from In Progress in Security Team Apr 12, 2019

@pthorson

This comment has been minimized.

Copy link

commented May 8, 2019

@jgrandja Except in Test, how does one setJwsAlgorithmResolver? I can't seem to get the value to be anything but RS256.

@lhaidacher

This comment has been minimized.

Copy link

commented May 9, 2019

you just have to create your own JwtDecoderFactory-Bean like:

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

This comment has been minimized.

Copy link

commented May 14, 2019

Did this change include dynamically choosing the Algorithm? At ".well-known/openid-configuration" our auth server responds with multiple values: "id_token_signing_alg_values_supported":["none","HS256","HS384","HS512","RS256","RS384","RS512","ES256","ES384","ES512"]. Of course the token header has a specific value but I'm not sure if a client would know this value ahead of time.

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

@pthorson No it doesn't include this change as we cannot randomly choose the algorithm given that id_token_signing_alg_values_supported can return more than one value. However, this can easily be implemented by inspecting the resolver input clientRegistration.getProviderDetails().getConfigurationMetadata() for the id_token_signing_alg_values_supported key and determining which algorithm to return.

NOTE: getConfigurationMetadata() will only be available if the ClientRegistration is configured with the issuer-uri.

@rwinch

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@pthorson @jgrandja I think it would be nice if we could support multiple algorithms. I created gh-6883 to allow supporting any of the algorithms returned by the IdP

@compfantasy

This comment has been minimized.

Copy link

commented Jun 8, 2019

I need multiple algorithm for NimbusJwtDecoderJwkSupport constructor as well, rather than hard coded RS256, I found it's hard to override the alg, even though I create my own JwtDecoder, because it is initized in OidcAuthorizationCodeAuthenticationProvider (line 212), and this cannot be changed if we don't want to change more code, please let me know the work around for current release, thank you very much!

@jgrandja

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.