-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Comments
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 |
Read section 3.3 about audience. Then, you will understand that it cannot work.
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. What you describe is the rfc8693 with a subject_token_type of
|
I think a good way to be compliant with the spec would be to add a
|
The point you reference (Section 3 - No.3) is referring to Client Authentication processing NOT Authorization Grant processing. Below is the full spec:
Take note of the bold highlight. This clearly indicates that the FYI, the current implementation has been tested and works to spec. Please see this comment.
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 |
You exactly point out the problem in the bold section :
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
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
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 ? |
@scrocquesel I did not receive a response to:
Both Atlassian and Curity specify what the claims should be in the The 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 |
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. 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.
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. 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. BUT this will partly answer the issue
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. |
I don't see in the text that it refer specifically to client authentification. Let me guide you through the spec.
which clearly makes echo to previous bolded text 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. 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. |
@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 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 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.
As the Jwt building/signing logic will be the same for everyone, I opt for an abstract converter One could then implements its own logic like below.
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 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
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. |
Thanks for the details @scrocquesel.
We just released I'm quite busy now until Dec 1, as we're releasing Spring Authorization Server I'll circle back to this early Dec and we can work through a design that will help with your setup.
Good to know. We'll see if it makes sense to re-factor and reuse. I'll get back to you early Dec. Thanks. |
@scrocquesel I took a look at your branch and the one required enhancement would be similar to what you proposed. In // TODO Check type to avoid CCE
private Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver = (context) -> (Jwt) context.getPrincipal().getPrincipal(); The default public void setJwtAssertionResolver(Function<OAuth2AuthorizationContext, Jwt> jwtAssertionResolver) The setter would allow implementors to realize either of the 2 common patterns:
As far as introducing something similar to Now that Thoughts? |
Make sense to me. Now that we have |
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. |
Sounds good @scrocquesel |
@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. |
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 claimsub =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
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.
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.
The text was updated successfully, but these errors were encountered: