-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
V51 - OAuth - sender-contrained refresh tokens #2110
Comments
I'm wondering whether it would make sense to require the usage of sender-constrained refresh tokens (as opposed to refresh token rotation) in L3.
Maybe, this does not really matter in this verification but would be the object of another verification (?). Even if public clients are disallowed in L3, you would still want to have (because some L3 applications won't be 100% conformant from day one):
|
We must be aligned with proposal from #2038
|
Verify that refresh tokens for public clients are either sender-constrained, using DPoP or Certificate-Bound Access Tokens (mTLS), or use refresh token rotation to mitigate refresh token replay attacks. (L1, L2, L3)
The refresh-refresh pattern by itself is more of intrusion detection as opposed to attack mitigation. The other defenses force me to have full access to the client. Refresh refresh does not. If I can steal an active refresh token there is no other sender constraint that prevents me from using it. Only stealing am expired refresh token asserts the defense.
I suggest recommending (DPoP AND refresh-refresh) or (mTLS AND refresh-refresh) as opposed to recommending refresh-refresh as an alternative to the other defenses.
|
From OAuth 2.1 and FAPI 2, as far as I understand, refresh-token misuse is primary mitigated by client authentication for confidential clients. So, L3 is covered by requiring confidential clients (with strong authentication methods) and client authentication for all token requests (#2009) For public clients (which I think needs to be allowed for L1 and L2) OAuth 2.1 states: Either DPoP/mTLS or Refresh-token rotation. Which was the intent to capture with
But e g FAPI 2 notes that refresh-token rotation is a weaker mitigation (and sometimes problematic), so, as I think @jmanico suggests, ASVS should reflect this and more clearly recommend DPoP or mTLS over just rotation, correct? Perhaps like this
|
@randomstuff - any further feedback or need for changes? |
I think that requiring sender-constrained tokens even for L1 is a little drastic. Using sender-constrained access tokens all the time is a good idea but I'm not sure this matches much with the current state of software/frameworks, etc. This might therefore be a little too much for L1? |
I agree with that. To this direction?
|
Maybe:
|
Actually, I noticed that this conflicts with DPoP:
The existing mechanism being client authentication. |
Well actually no, it does not because of "public clients" 🤦 |
So, all good or need for changes? |
Current 51.2.4:
For refresh token rotation - we should require revoking previous refresh token, as decribed in: |
I was going to say that this is implied in "refresh token rotation" but actually that's not true. So you are right! 👍 |
Proposal for add-on to the current requirement 51.2.4:
"for that authorization" should be thought carefully - is it correct and is it understandable? |
I think the word we are looking for is the same we are discussing in #1968. |
@elarlang, Yes, I think "authorization" if probably the better word. See these snippets from the Oauth 2.1 draft:
Ad discussed in #1968, grant could be used to have a similar meaning of "granted authorization" but in OAuth it is always used to mean "authorization grant" which is some concrete representation of a granted authorization (such as a refresh token or an authorization code). |
Hey @randomstuff @TobiasAhnoff can I get your thoughts on this? I see two "verify" in 51.4.4 (one requirement) and really want to split it as described above. |
And arguments provided why it does not suit as a requirement does not matter? :) |
They totally matter but I do not agree. I think you are being way to strict and see no harm in splitting.
One explains what to use, another explains how to use it. We have this elsewhere in ASVS.
|
Current requirement:
A few thoughts:
I think my preference would be to try and streamline the requirement as follows: "Verify that the authorization server mitigates refresh token replay attacks for public clients. For L1 only, refresh token rotation may be used as long as the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if a previously invalidated refresh token is provided. However the prefered approach, which is required for L2 and above, is using sender-constrained refresh tokens (i.e., Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS))." |
The reasons why L1 option is the last one and should stay there
refresh token rotation is given as an acceptable option for L1. Read it as "Note that for L1 it is acceptable to use refresh token rotation". |
Here is another suggestion. Why not just drop the refresh token rotation description? That’s just describing how the RFC works.
So for 5.14.4 just use this and drop the rest.
Verify that the authorization server mitigates refresh token replay attacks for public clients, preferably using sender-constrained refresh tokens (i.e., Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS)). For L1 applications only, refresh token rotation may be used instead.
We’re already just mentioning DPoP and mTLS without describing how they work. Why do we need to do that for refresh token rotation, especially if it’s the least important?
|
Because for L1 it is important. |
Why are the implementation details for refresh token rotation important for level 1 but the implementation details of dpop and cert bound access tokens not important for level two and three? |
Have I said that mTLS details are not important? No. Correct refresh token rotation could be day-saver for L1 applications and I find it to be important. |
If they are important then why are we describing the implementation details for refresh token rotation for level 1 but not describing the implementation details of dpop and cert bound access tokens for the other levels? This seems out of sync.
|
What is your problem to solve here and what is the problem with the requirement to solve here? |
I think the inconsistency in when we provide implementation details and when we don't is a little puzzling but I don't think it is critical. How about: "Verify that the authorization server mitigates refresh token replay attacks for public clients. The preferred approach, which is required for L2 and above, is using sender-constrained refresh tokens (i.e., Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS)). For L1 only, refresh token rotation may be used as long as the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if a previously invalidated refresh token is provided." |
Here is the problem.
This requirement is mentioning three critical defenses as options and only of them is being described, the least important one. This is a discrepancy that I’d like to fix by either externalizing the descriptions or remove the refresh token description so the requirement is more in alignment with the three different defenses mentioned.
|
I agree that the requirement as it stands is quite long and daunting. I've tried to come up with something like:
The problem with that is that the second requirement is really details for the first one. In the end, I think I'd keep it in single requirement but maybe try to make it less daunting. |
What about splitting this part "revokes all refresh tokens for that authorization if a previously invalidated refresh token is provided" in a separate requirement? It might make verification simpler:
|
Requirements must be clear and only one way interpretable out of context as a stand-alone piece of text (without any other requirement and without any chapter text). I think the proposal does not meet those criteria as I listed here: #2110 (comment) |
Yes, that may be true. It might depends, on the wording though. One argument for splitting is that we actually might want to include more wording in the requirement because some things are missing. Currently we have:
What it missing here is that, in case of refresh token reuse (when using refresh token rotation), all the grant (including, if possible, the access token) should be revoked as well:
What about something like:
To clarify, I think there are valuable arguments for both positions (splitting and not splitting the requirement) and I currently don't prefer one solution of the other. Mostly experimenting with the possible solutions. |
At the moment we don't have support or any intention to use requirements that are valid for one level and are not valid for higher level. This says no to "level 1 only" solution as I pointed out in https://github.com/OWASP/ASVS/issues/2591#issuecomment-2648774593, #2110 (comment) and agreed by Josh in #2110 (comment) The point of the requirement is:
And whatever says that point, must stay in this requirement to avoid confusions that to mitigate the same problem, there are more than one solutions and those are in conflict with each other. |
You previously opened this discussion in #2110 (comment) - can you recall why it did not reach to the requirement? |
This might be a solution. If we add refresh token to 51.4.11
Then 51.4.4 (for level 1) could be
With this level 3 requires confidential clients (51.4.10) and sender-constrained tokens (51.4.11), this aligns with FAPI, for 1 and 2 this is optional, but additional refresh token protection is required if the client is public (for level 1 and 2 a confidential client is enough to protect tokens). If we want to say something about refresh token rotation, it could be a separate requirement, like this
Does this work? |
I think that is a great idea @TobiasAhnoff |
This does not work as it stands because RFC 9449 (DPoP) only supports DPOP-bound refresh token for public clients:
It should be something like:
Also this is a downgrade in security compared to the current version which requires sender-contrains refresh tokens for public clients in L2. |
I like @tghosth's proposition #2110 (comment). I would try to simplify it. Would something like that work?
|
Spin-off from #2040 (comment)
The proposal updates the requirement we already have as 51.2.4
For how many levels we allow "public clients"?
And we have a separate discussion/requirement for confidential clients in #2109
The text was updated successfully, but these errors were encountered: