-
Notifications
You must be signed in to change notification settings - Fork 827
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
Fix/user token #2193
Fix/user token #2193
Conversation
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/184416787 The labels on this github issue will be updated when the story is started. |
So you want prevent the flow from Test: |
Right. With knowledge of that well known client with secret "", anyone can effectively use the user_token grant to get a new token from an existing one, essentially avoid expiration forever. This is because the user_token grant allows you to get a refresh token for another client. And since the client has a well known secret of "", anyone could use that refresh token to get a new access token. And then you could do it again and again to avoid the expiration on the original token. |
If you have a refresh token from a "cf login" / "cf login --sso" then you can also refresh this for ever because the token stored in ~/.cf/config.json is from cf client and you know the secret to refresh it. Ok, so I agree that it is a problem, user_token should not be used at all. Until know we found no time to deprecate it. It was from a time before JWT - Bearer but exchanging a token without secret on receiver side should not be possible. So from my side we can limit user_token and then we have to discuss howto deprecate user_token flow. If someone needs a replacement then we can do the replacment with jwt-bearer or https://www.rfc-editor.org/rfc/rfc8693.html FYI @bruce-ricard / @Tallicia , we should discuss this next meeting |
Yes, the only thing I would add is that presumably the token in ~/.cf/config.json would be protected by file system level permissions and not network vulnerable. |
yes, but my point was. If you have a refresh token from cf client then you can always refresh it. The others dont see it because the refresh token is always the same. Therefore you should activate jwt.token.refresh.rotate |
/easycla |
@mikeroda Is this issue still open ? There is a situation where you can have a public clients and for them there is a request #2138 where we should create refresh tokens. The request for #2138 only is valid for authorization_code grant type created tokens so it does not relate to this topic, however maybe your fix should halso have an option to check the allow public option for a client so that you have an option to allow/disallow refresh tokens |
Apologies, I forgot to look at this. I'll take a look now. |
@strehle yes this is still open. I don't see how this should be related to the allow public option since that is specific to the authorization code flow. That would essentially expand the meaning of that option to include the user_token grant. In other words, I want to be able to support public clients with the authorization code flow but not get refresh tokens for them with the user_token grant. |
@Tallicia we should discuss this in next meeting(s) because I agree to fix this but it would mean a breaking change and we may have to increase to 77.x.x version |
This could be considered a vulnerability fix, in which case it doesn't require a major bump. |
Yes I'm arguing this is a vulnerability fix. |
- avoid logging or echoing unsantized input from the request - this mirrors the change made to AuthorizationEndpoint in spring-security-oauth2 2.5.2.RELEASE, see: spring-attic/spring-security-oauth@2b58aaf Change-Id: Id93034bc69355fcf988c56827fa65c70338694cf
- apparently the whitespace is being trimmed off by spring in the xml so the request matcher isn't doing a case insensitive comparison when the header value is Bearer Change-Id: I0f93cc2a0ebf364560687c4e57887a100753dd2d
UsernamePasswordAuthenticationToken clientAuth = | ||
new UsernamePasswordAuthenticationToken(request.getRequestParameters().get(CLIENT_ID), ""); | ||
clientAuthenticationProvider.authenticate(clientAuth); | ||
throw new InvalidClientException("Grant type is not allowed for this client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is very difficult to understand. It seems that an exception is expected in order for the grant to be validated. Could this (and perhaps the authenticate method that's called here) be restructured so that the requirement this is validating is easier to understand from looking at the code?
@Test(expected = InvalidClientException.class) | ||
public void test_client_no_secret() { | ||
when(clientAuthenticationProvider.authenticate(any(UsernamePasswordAuthenticationToken.class))) | ||
.thenReturn(null); | ||
missing_parameter("non existent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this method was already there, but can we improve the name of missing_parameter()? Maybe validate_with_missing_parameter()?
@@ -188,6 +194,15 @@ protected Authentication validateRequest(TokenRequest request) { | |||
|
|||
@Test | |||
public void happy_day() { | |||
when(clientAuthenticationProvider.authenticate(any(UsernamePasswordAuthenticationToken.class))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this test? It was a little bit confusing before this change , but now with a mock that throws an exception but the name "happy_day" I can't tell if this is a happy path test or not.
The grant is similar to refresh but always without a secret. The 4 examples are for the discussion about issue #2193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in general that we should restrict the user_token grant type so that exchanges from normal clients (with non-empty secret) are not allowed to do with public clients (empty secret) unless this is not allowed explicitly (therefore my comment to allowpublic). The refresh flow in meanwhile allows the public usage, see #2402 . So I would allow user_token grant in same way if existing token was from public client,
But I would disallow the flow if an exchange from normal client to client with empty secret (like cf) is requested.
I created the ITs to demonstrate current behaviour: #2194
@@ -97,6 +104,16 @@ protected Authentication validateRequest(TokenRequest request) { | |||
ClientDetails receiving = clientDetailsService.loadClientByClientId(request.getRequestParameters().get(CLIENT_ID), IdentityZoneHolder.get().getId()); | |||
super.validateGrantType(GRANT_TYPE_REFRESH_TOKEN, receiving); | |||
|
|||
//7. receiving client must have a client secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote to perform the check in extra method, means, check if secret "" is set to client. I assume you agree if I provide a PR which would perform the check for you?
@@ -43,7 +44,8 @@ public void setup() { | |||
mock(AuthorizationServerTokenServices.class), | |||
mock(MultitenantClientServices.class), | |||
mock(OAuth2RequestFactory.class), | |||
mock(RevocableTokenProvisioning.class) | |||
mock(RevocableTokenProvisioning.class), | |||
mock(ClientDetailsAuthenticationProvider.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would not be needed then
|
||
userAuthentication = mock(UaaAuthentication.class); | ||
granter = new UserTokenGranter( | ||
tokenServices, | ||
clientDetailsService, | ||
requestFactory, | ||
tokenStore | ||
tokenStore, | ||
clientAuthenticationProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would not be needed then
* Add IT for user_token grant * add more examples for user_token grant The grant is similar to refresh but always without a secret. The 4 examples are for the discussion about issue #2193 * refactor test cases * test renamed * review
@mikeroda I see this also as security issue, but I would allow the exchange if first token was from a public client (for me public is no secret or empty secret) so an exchange from a client with secret "" to cf with secret "" is not an issue from my point of view. However, I created integration tests, therefore request you to rebase this PR, then at least 1 test should fail. We discuss then a fix |
* Add IT for user_token grant * add more examples for user_token grant The grant is similar to refresh but always without a secret. The 4 examples are for the discussion about issue #2193 * refactor test cases * test renamed * review Co-authored-by: Bruce Ricard <bricard@vmware.com>
Don't allow the user_token grant to give a refresh token to a client that has no secret, such as those that may be bootstrapped without a secret, like perhaps "cf" for example. Change-Id: Iefb5505a5597d02e0ccf29f4c16bb3bcdb5990e2
b619a1b
to
07ad972
Compare
expected failed tests UserTokenGrantIT > testExchangeFromConfidentialClientWithCfClientWithEmptySecret FAILED We should discuss about a solution. I see 2 options
Because with either 1) or 2) we could allow testExchangeFromPublicClientWithPublicClient (because the client here has allowpulic:true) |
I agree that If so, then the problem statement is more like:
|
I agree with the first and third points but the client having |
Let me attempt to give a precise "exploitation" path based on what you said:
@mikeroda @strehle Please validate above. Please provide your version of the "exploitation" path that if that works better. |
@peterhaochen47 please see integration test In This PR now 2 failures
For me the problematic point is, client cf is well known in CF community and it is always the same. (name and secret) a) could be allowed if cf client would have allowpublic attribute, which is currently not the case. |
I still don't think it should be allowed on clients with allowpublic=true because that option I think was more to allow the authorization code flow with PKCE and this would be a significant expansion of the meaning. Furthermore I can't imagine anyone ever wants to allow it. |
but what if first token was generated from a public usage ?, e.g. from a client without secret (or empty secret) then the second token should be allowed as well (at least to me) |
The first token still requires an authorization grant so even though the client does not authenticate with a secret, its ability to get tokens is constrained by the user. The problem is that the second, third, fourth, etc. tokens can be obtained infinitely. |
Assuming that "exploitation" path I mapped out is accurate (again, please correct me, if these steps are not the issue you are describing), I don't think there's a way to fix its fundamental flaws without completely changing this flow or just removing this flow. In my opinion, the "fundamental flaws" that have enabled this "exploitation" path are:
But overall, perhaps it's not worthwhile to try to fix the fundamental flaws of a grant flow that we don't have a use case for anymore (is anyone using it?). Why don't we just deprecate it (and warn operators against it) and remove it soon. I don't think the short-term fixes would help all that much since they don't address the more fundamental illogical, pattern-breaking things about this flow. |
I vote in meanwhile also for this because the grant type is such special that it might not worth to support it any longer. We have jwt-bearer and if needed we can also add RFC 8693 , https://oauth.net/2/token-exchange/ and with these options, user_token is no longer needed |
@peterhaochen47 yes I agree with the exploitation path you laid out. client-a could also be a misbehaving client and acquire tokens for client-public-empty-secret indefinitely as you described. For the latter, the JWT bearer token grant would also have the same vulnerability and perhaps we should be protecting that as well. |
Given that:
Instead of mildly limiting the power of this grant (which is still a breaking change) without address its flaws, I propose that we just mark this grant as deprecated & close this PR. The operator can ensure that this exploitation path is not possible by not allowing any of UAA clients to use this grant. |
Note that client-a's credentials are not needed, only the user access token is needed for this grant type. |
Yes indeed, performing the user_token grant type once does NOT require |
FYI, the only conclusion we (uaa-team) have is, we want deprecate user_token flow. Currently there is jwt-bearer already as replacement. |
Closing due to inactivity (and a solution, aka deprecation, has been proposed). Please feel free to request to reopen this PR if there are more to discuss. |
Prevent the user_token grant from being used to obtain a refresh token for a client that has no secret, such as those that may be bootstrapped, like perhaps "cf" for example.