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

Client JwtBearer grant type should support non Jwt principal #9812

Closed
sclorng opened this issue May 26, 2021 · 17 comments
Closed

Client JwtBearer grant type should support non Jwt principal #9812

sclorng opened this issue May 26, 2021 · 17 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Milestone

Comments

@sclorng
Copy link

sclorng commented May 26, 2021

Expected Behavior

Given a client registration with authorization-grant-type: urn:ietf:params:oauth:grant-type:jwt-bearer and an arbitrary authentication, an implementation of OAuth2AuthorizedClientProvider should build a Jwt bearer with claim sub =authentication.getName(), sign it the same way Jwt Bearer client authentication does and use it as the assertion of the JwtBearer grant request.

It should be as simple as

Authentication authentication = new MyAuthentication(new MyPrincipal("john"));
String body = webClient
    .get()
    .attributes(authentication(authentication).andThen(clientRegistrationId("client-jwt-bearer")))
    .retrieve()
    .bodyToMono(String.class)
    .block();

Current Behavior

JwtBearerOAuth2AuthorizedClientProvider works only if the given authentication principal is a Jwt.
Which means when using this grant type, we must add a lot of code (which is for a most part the same as building the Jwt Bearer client assertion.

ClientRegistration clientRegistration = ...;
JWK jwk = jwkResolver.apply(clientRegistration);
NimbusJwsEncoder jwsEncoder = new NimbusJwsEncoder(new ImmutableJWKSet<>(new JWKSet(jwk)));
JoseHeader joseHeader = ...;
JwtClaimsSet.Builder claimsBuilder = JwtClaimsSet.builder()
  .issuer(clientRegistration.getClientId())
  .subject("john")
  .audience(Collections.singletonList(clientRegistration.getProviderDetails().getIssuerUri())) // audience is the issuer uri
  .id(UUID.randomUUID().toString())
  .issuedAt(issuedAt)
  .expiresAt(expiresAt);
JwtClaimsSet jwtClaimsSet = claimsBuilder.build();
Jwt jws = jwsEncoder.encode(joseHeader, jwtClaimsSet);

Authentication authentication = new JwtAuthenticationToken(jws);
String body = webClient
    .get()
    .attributes(authentication(authentication).andThen(clientRegistrationId("client-jwt-bearer")))
    .retrieve()
    .bodyToMono(String.class)
    .block();

Is it the way we should use it ?

Context

JwtBearer grant type is not for exchanging the request bearer token but to allow a client to build/sign an Assertion of type Jwt with a subject specified by the client.

@sclorng sclorng added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels May 26, 2021
@jzheaux jzheaux removed the status: waiting-for-triage An issue we've not yet triaged label May 26, 2021
@jgrandja
Copy link
Contributor

jgrandja commented May 26, 2021

@scrocquesel

JwtBearer grant type is not for exchanging the request bearer token but to allow a client to build/sign an Assertion of type Jwt with a subject specified by the client.

This is not correct. The JwtBearer grant type IS for exchanging the request bearer token. It is NOT used to build/sign an Assertion of type Jwt. It seems you are referring to gh-8175?

Please review the spec in Section 2.1. Using JWTs as Authorization Grants.

I'll close this as invalid as JwtBearerOAuth2AuthorizedClientProvider and associated DefaultJwtBearerTokenResponseClient has been implemented correctly as per spec.

@jgrandja jgrandja added 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 and removed type: enhancement A general enhancement labels May 26, 2021
@sclorng
Copy link
Author

sclorng commented May 27, 2021

Read section 3.3 about audience. Then, you will understand that it cannot work.

The JWT MUST contain an "aud" (audience) claim containing a
value that identifies the authorization server as an intended
audience.

But, the token provided in as a bearer in a request to a service MUST contain an audience claim containing a value that identifies the service as an intended audience.
It is not common to have a token emit to access a service to have an audience of the authorization server as it is not the purpose of such a token.
Which means that, ultimately, the JWT will be build and signed by the client with such a claim value and issuer claim will be the client id (or the client will need to request a token with such an audience value to another service.) as described in Section 3 of the spec. See https://datatracker.ietf.org/doc/html/rfc7521#section-3

What you describe is the rfc8693 with a subject_token_type of urn:ietf:params:oauth:token-type:access_token

See OAuth 2.0 Token Exchange

urn:ietf:params:oauth:token-type:access_token
Indicates that the token is an OAuth 2.0 access token issued by
the given authorization server.

@sclorng
Copy link
Author

sclorng commented May 27, 2021

Although this document does not define the processes by which the
client obtains the assertion (prior to sending it to the
authorization server), there are two common patterns described below.

I think a good way to be compliant with the spec would be to add a Converter<OAuth2AuthorizationContext, Jwt> to the JwtBearerOAuth2AuthorizedClientProvider.
That way, the implementation will not drive the way the JWT is obtained. Default implementations could be :

  • use the principal if its a JWT and documentation should clearly states that the token shoud not be the Authorization: Bearer token received in the current request. It will allow to easily drop in a Jwt obtained from another STS and keep the current behavior.
  • use the current principal name to build/and sign a JWT issued by the client itself. This one could use something similar to the NimbusJwtClientAuthenticationParametersConverter to allow to resolve the JWK used to sign as it the exact same needs.

@jgrandja
Copy link
Contributor

@scrocquesel

The JWT MUST contain an "aud" (audience) claim containing a
value that identifies the authorization server as an intended
audience.

But, the token provided in as a bearer in a request to a service MUST contain an audience claim containing a value that identifies the service as an intended audience.

The point you reference (Section 3 - No.3) is referring to Client Authentication processing NOT Authorization Grant processing.

Below is the full spec:

The JWT MUST contain an "aud" (audience) claim containing a
value that identifies the authorization server as an intended
audience. The token endpoint URL of the authorization server
MAY be used as a value for an "aud" element to identify the
authorization server as an intended audience of the JWT. The
authorization server MUST reject any JWT that does not contain
its own identity as the intended audience.

Take note of the bold highlight. This clearly indicates that the aud of the assertion is the Authorization Server's token endpoint URL, which is used for client authentication. It further states..." the authorization server MUST reject any JWT that does not contain its own identity as the intended audience", which further validates that this spec is for Client Authentication processing.

FYI, the current implementation has been tested and works to spec. Please see this comment.

use the current principal name to build/and sign a JWT issued by the client itself.

Where exactly in the spec does it state that this behaviour should be implemented? This applies to Client Authentication only and does not apply to Authorization Grant processing. If you believe it does, please refer me to the section in the spec that states this.

At this point, if you still feel the current implementation is incorrect, please provide a minimal reproducible example with the provider you're using to integrate this feature with. Also provide a link to the reference of the provider's documentation for urn:ietf:params:oauth:grant-type:jwt-bearer.

@sclorng
Copy link
Author

sclorng commented May 28, 2021

You exactly point out the problem in the bold section :

The
authorization server MUST reject any JWT that does not contain
its own identity as the intended audience.

This is exaclty the issue I have when reusing the bearer token given to the service which audience is the service resource id.

Find out a sample use case of this grant flow : https://developer.atlassian.com/cloud/jira/software/user-impersonation-for-connect-apps/

See some implementation https://curity.io/resources/learn/jwt-assertion/#user-authentication-using-jwts

These checks are like JWTs used for client authentication. In particular, the JWT must contain an audience (aud) claim. The value must be the OAuth profile’s token endpoint.

Note that the issue you refer to state that it doesn't work due to missing requirement (aud claim value is not good) #6053 (comment)

Note also that the spec says

Authentication of the client is optional, as described in
Section 3.2.1 of OAuth 2.0 [RFC6749], and consequently, the
"client_id" is only needed when a form of client authentication that
relies on the parameter is used.

Rephrased this states that the assertion is by itself sufficient to authenticate the client as it is signed by it.

Is is sufficient to reconsider ?

@jgrandja
Copy link
Contributor

@scrocquesel I did not receive a response to:

Where exactly in the spec does it state that this behaviour should be implemented? ... please refer me to the section in the spec that states this.

Both Atlassian and Curity specify what the claims should be in the Jwt assertion for the jwt-bearer grant flow. These are specific to these providers and don't necessarily port to other providers, as the spec does not detail what the claims should be in a jwt-bearer authorization grant. It ONLY details what the claims should be in a Jwt assertion for Client Authentication. Our main priority is to implement to spec and non-standard behaviour is the responsibility of the application.

The JwtBearerOAuth2AuthorizedClientProvider is a basic implementation of the jwt-bearer authorization grant type and it's designed with this intent. All it needs is a Jwt as input. It is not designed (or intended) to create a Jwt from other inputs, as it would complicate the implementation and no where in the spec it states that this behaviour must be implemented.

The part that you are missing with the Atlassian integration is "2. JWT token exchange: The app creates an assertion...". Please add your requirements (or vote) in the JwtEncoder PR #9208 as this is what you need to create your Jwt assertion. Then you could easily create any instance of Authentication that returns the created Jwt via Authentication.getPrincipal() and ensure OAuth2AuthorizationContext.getPrincipal() returns this Authentication you need to create.

@sclorng
Copy link
Author

sclorng commented May 28, 2021

@scrocquesel I did not receive a response to:

Where exactly in the spec does it state that this behaviour should be implemented? ... please refer me to the section in the spec that states this.

Both Atlassian and Curity specify what the claims should be in the Jwt assertion for the jwt-bearer grant flow. These are specific to these providers and don't necessarily port to other providers, as the spec does not detail what the claims should be in a jwt-bearer authorization grant. It ONLY details what the claims should be in a Jwt assertion for Client Authentication. Our main priority is to implement to spec and non-standard behaviour is the responsibility of the application.

The JwtBearerOAuth2AuthorizedClientProvider is a basic implementation of the jwt-bearer authorization grant type and it's designed with this intent. All it needs is a Jwt as input. It is not designed (or intended) to create a Jwt from other inputs, as it would complicate the implementation and no where in the spec it states that this behaviour must be implemented.

NO, the spec describe what the JWT should be for all scenario. Nowhere, in the spec it is said that it only concern one scenario. If it has been the case, the spec would have been organized differently and certainly been separated in two different RFC. This is not the case, because the JWT is very similar and it is the whole purpose of having an Assertion Framework for OAuth 2.0.

As I said, the spec doesn't specifiy a way about how to get the Jwt and just give two use cases for the authorization grant flow. One of them is building and signing the Jwt by the client like the client authentication assertion.
BUT the spec is clear about what the content of this Jwt must be (and give some sample scenario described section 6) and this is what AS implementation like Atlassian, Curity, Keycloak check for.

If you don't want to implement such scenario, well. I'm not requesting to implement them directly in Spring Security but at least provide a way to inject into it more easily having an open closed JwtBearer Provider.

The part that you are missing with the Atlassian integration is "2. JWT token exchange: The app creates an assertion...". Please add your requirements (or vote) in the JwtEncoder PR #9208 as this is what you need to create your Jwt assertion. Then you could easily create any instance of Authentication that returns the created Jwt via Authentication.getPrincipal() and ensure OAuth2AuthorizationContext.getPrincipal() returns this Authentication you need to create.

I am greatful to see that we are now in phase that the token provided in request to the resource provider IS NOT the jwt bearer assertion and that the current implementation require a specific authentication instance with a specific Jwt targetted to the AS. This is one of the building rule every party involved in oauth2 token MUST enforce and it is recall every time in every RFC Spec related to oauth. You may have test against permissive implementation.
Without custom code, it cannot work out of the box as it will use the current authentication context which is built from the request token targeting the resource provider.

It leads to the core of this issue : We must get or build a Jwt and it is not as easy as it seems to do it right.
Having the JwtEncoder is a must have I am waiting for.

BUT this will partly answer the issue

  • it will require caller code to know/inject every intrinsics already used by the provider, for example the client registration to access the private key, etc... which is clearly not what I want my code to have to deal with. I want the security thingies to be as transparent as possible when making web client call.
  • it will require to sign a JWT or request such a JWT to another STS (second spec use case) every time , which is very costly to provide such an authentication instance, even if the oauth client is already registered. Allowing to inject into the authorize provider will allow to do that only when required. This can certainly be mitigated by having kind of cache but it makes caller code utterly complicated.
  • it will not allow for client to authenticate for itself (use case describe in the spec) when using a default client registration.

Could you review this gist to see what I am currently asking for ? As you can see, I have a working solution which can satisfy our both visions. I can stay with a custom provider implementation but extending spring security to add custom grant type support means a lot of code duplication. For eg. OAuth2AccessTokenResponseClient, as per access token response rfc it will the same logic for a lot of access token request but due to generic parameter, we must duplicate code. Having an abstract base class would help a lot.

@sclorng
Copy link
Author

sclorng commented May 31, 2021

Below is the full spec:

The JWT MUST contain an "aud" (audience) claim containing a
value that identifies the authorization server as an intended
audience. The token endpoint URL of the authorization server
MAY be used as a value for an "aud" element to identify the
authorization server as an intended audience of the JWT. The
authorization server MUST reject any JWT that does not contain
its own identity as the intended audience.

Take note of the bold highlight. This clearly indicates that the aud of the assertion is the Authorization Server's token endpoint URL, which is used for client authentication. It further states..." the authorization server MUST reject any JWT that does not contain its own identity as the intended audience", which further validates that this spec is for Client Authentication processing.

I don't see in the text that it refer specifically to client authentification. Let me guide you through the spec.

This specification provides a framework for the use of assertions
with OAuth 2.0
in the form of a new client authentication mechanism
and a new authorization grant type. Mechanisms are specified for
transporting assertions during interactions with a token endpoint;
general processing rules are also specified.

This specification provides a general framework for the use of
assertions as authorization grants
with OAuth 2.0. It also provides
a framework for assertions to be used for client authentication.

  1. Framework (no plural form)

An assertion is a package of information that allows identity and
security information
to be shared across security domains. An
assertion typically contains information about a subject or
principal, information about the party that issued the assertion and
when was it issued, and the conditions under which the assertion is
to be considered valid, such as when and where it can be used.

The entity that creates and signs or integrity-protects the assertion
is typically known as the "Issuer", and the entity that consumes the
assertion and relies on its information is typically known as the
"Relying Party". In the context of this document, the authorization
server acts as a relying party
.

  1. Assertion Content and Processing
    This section provides a general content and processing model for the
    use of assertions in OAuth 2.0
    [RFC6749].

which clearly makes echo to previous bolded text This specification provides a **general framework** for **the use of assertions as authorization grants** with OAuth 2.0.

And Section 5 describe the content of what is an oauth assertion with no relation to which use case it will used. Nowhere, in the text, it is said that section 5 only refer to client authentication. And there is no other section that may described what would be the content of an assertion in the jwt bearer flow.
Plain as day. Nothing to interpolate.
It is even more clear if you knew the spec before 2013 where the details of the content was given for each use case instead of grouping it.
You will also find some example in https://datatracker.ietf.org/doc/html/rfc7523#section-4 which is just implementation of the framework.

So, https://docs.spring.io/spring-security/site/docs/current/reference/html5/#using-the-access-token-3 is wrong per the RFC. It may be easier to reuse a given access token for a pet sample and to show how it is wrongly easy to use this flow but from a security point of view, it will just not work.

BTW, another implementation of this not so standard assertion https://is.docs.wso2.com/en/latest/learn/jwt-grant/, Azure AD, plus a lot of others all respecting the RFC, point by point.

I don't know what to show you more to convince you that the current implementation would need some improvement to be the best fit when using Jwt Bearer grant flow.

I am also interested to know which AS implementation you make your tests with. Okta seems the one but Jwt Bearer grant flow is currently not supported as per the doc.

@jgrandja
Copy link
Contributor

@sclorng Apologies for the long delay in my response.

I finally circled back to this and after re-reading all the comments, I agree that some changes are required.

The Atlassian and Curity flows validates the use case where the client creates the assertion and exchanges it via a urn:ietf:params:oauth:grant-type:jwt-bearer grant flow.

From my initial understanding of the 2 specs, where the client acts as the issuer, this flow is used for client authentication only. Now I see, that the authorization grant flow applies here as well.

I think your suggested improvement in this comment (exposing a Converter) makes sense. Would you be interested in submitting a PR for this?

Let's start with this improvement first and then see what else we can improve to make things easier on your end. Sound good?

@scrocquesel
Copy link

Let's start with this improvement first and then see what else we can improve to make things easier on your end. Sound good?

@jgrandja I may not have as much time as before.

I quickly pushed a branch to discuss the design with a converter. (quickly as in 'it may not compile'). Let me know if you prefer I create a draft PR.

DefaultJwtBearerAuthContextJwtConverter is required to be non breaking and be compatible with current behavior.

As the Jwt building/signing logic will be the same for everyone, I opt for an abstract converter AbstractClientSignedJwtBearerConverter with one abstract method to get the subject, which is the only claim that should differ. As you will see, as we are building and signing a Jwt Bearer, the code is very similar to the NimbusJwtClientAuthenticationParametersConverter. The only difference is the subject and the audience (issuerUri instead of tokenUri). Maybe it can be factored.

One could then implements its own logic like below.

@Override
String getSubject(OAuth2AuthorizationContext authorizationContext){
    return authorizationContext.getClientRegistration().getClientId();
}

@Override
String getSubject(OAuth2AuthorizationContext authorizationContext){
    return authorizationContext.getPrincipal().getName();
}

These could be default implementations, either separated or merged but if merged, the converter will need a marker to know which path to go and as far as I know it is not standardized. In my own code, I test for a scope user_impersonation because the AS need this scope to allow to impersonate. So if requested, I know I should use the principal name to impersonate it, otherwise I use the clientId.

Maybe, spring security can be a bit opiniated here and test for a specific Authentication implementation for the client id path and otherwise use the principal name. So that one can add it to the attributes with a helper like

String body = webClient
    .get()
    .attributes(clientAuthentication()) // or impersonatedAuthentication("john doe")
    .retrieve()
    .bodyToMono(String.class)
    .block(); 
 }

IMHO, this will be easier to config. One client can impersonate and self authenticate depending on the oauth request context only. Otherwise, it will be tricky to configure the OAuth2AuthorizedClientManager.

This is nearly the code I used in production, hope it will make sense.

@jgrandja
Copy link
Contributor

Thanks for the details @scrocquesel.

I may not have as much time as before

We just released 5.6 on Monday so we have plenty of time to get this enhancement in for 5.7.

I'm quite busy now until Dec 1, as we're releasing Spring Authorization Server 0.2.1 Dec 1.

I'll circle back to this early Dec and we can work through a design that will help with your setup.

the code is very similar to the NimbusJwtClientAuthenticationParametersConverter

Good to know. We'll see if it makes sense to re-factor and reuse.

I'll get back to you early Dec. Thanks.

@jgrandja jgrandja modified the milestones: 5.7.x, 5.7.0-M1 Dec 2, 2021
@jgrandja
Copy link
Contributor

@scrocquesel I took a look at your branch and the one required enhancement would be similar to what you proposed.

In JwtBearerOAuth2AuthorizedClientProvider, we would introduce field:

// TODO Check type to avoid CCE
private Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver = (context) -> (Jwt) context.getPrincipal().getPrincipal();

The default jwtAssertionResolver would preserve the current behaviour and applications can override the default behaviour via:

public void setJwtAssertionResolver(Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver)

The setter would allow implementors to realize either of the 2 common patterns:

  1. Client obtains assertion from STS
  2. Client creates assertion

As far as introducing something similar to AbstractClientSignedJwtBearerConverter, I'm not sure if this makes sense at this point. As you mentioned, the sub and aud may differ but there may be custom claims and/or headers required for other implementors, which now would require a customizer extension of some kind. The abstract implementation now becomes more complicated and without a more standard way of building I'm not sure we can get this right.

Now that NimbusJwtEncoder is available in 5.6, full control is given to the implementor to customize the Jwt however they choose.

Thoughts?

@scrocquesel
Copy link

@scrocquesel I took a look at your branch and the one required enhancement would be similar to what you proposed.

In JwtBearerOAuth2AuthorizedClientProvider, we would introduce field:

// TODO Check type to avoid CCE
private Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver = (context) -> (Jwt) context.getPrincipal().getPrincipal();

The default jwtAssertionResolver would preserve the current behaviour and applications can override the default behaviour via:

public void setJwtAssertionResolver(Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver)

The setter would allow implementors to realize either of the 2 common patterns:

  1. Client obtains assertion from STS
  2. Client creates assertion

As far as introducing something similar to AbstractClientSignedJwtBearerConverter, I'm not sure if this makes sense at this point. As you mentioned, the sub and aud may differ but there may be custom claims and/or headers required for other implementors, which now would require a customizer extension of some kind. The abstract implementation now becomes more complicated and without a more standard way of building I'm not sure we can get this right.

Now that NimbusJwtEncoder is available in 5.6, full control is given to the implementor to customize the Jwt however they choose.

Thoughts?

Make sense to me. Now that we have NimbusJwtEncoder and authorizationContext.getClientRegistration(), we should be good to implement the logic in our own custom class.

@jgrandja
Copy link
Contributor

Excellent ! Let me know if you would like to submit a PR for this enhancement. If you don't have time, I can take it on.

@scrocquesel
Copy link

Excellent ! Let me know if you would like to submit a PR for this enhancement. If you don't have time, I can take it on.

I let you handle this. There isn't much of code change and you'll be quicker with doc and respecting guildelines.

@jgrandja
Copy link
Contributor

Sounds good @scrocquesel

jgrandja added a commit that referenced this issue Jan 10, 2022
@jgrandja
Copy link
Contributor

@scrocquesel This enhancement is now merged via 214cfe8. Please give it a try and let me know if there are more improvements you would like to see. Thanks for all your feedback.

jgrandja added a commit that referenced this issue Jan 10, 2022
jgrandja added a commit that referenced this issue Mar 17, 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) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants