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

V51 - OAuth - sender-contrained refresh tokens #2110

Open
elarlang opened this issue Sep 23, 2024 · 53 comments
Open

V51 - OAuth - sender-contrained refresh tokens #2110

elarlang opened this issue Sep 23, 2024 · 53 comments
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V51 Group issues related to OAuth _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented Sep 23, 2024

Spin-off from #2040 (comment)

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 proposal updates the requirement we already have as 51.2.4

Verify that refresh tokens are sender-constrained or use refresh token rotation to prevent token replay attacks. Refresh token rotation prevents usage in the event of a compromised refresh token. Sender-constrained refresh tokens cryptographically binds the refresh token to a particular Client.


Verify that refresh tokens for public clients ... (L1, L2, L3)

For how many levels we allow "public clients"?

And we have a separate discussion/requirement for confidential clients in #2109

Verify that confidential clients must use client authentication in refresh token requests. (L1, L2, L3)

@elarlang elarlang added the V51 Group issues related to OAuth label Sep 23, 2024
@randomstuff
Copy link
Contributor

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)

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.

For how many levels we allow "public clients"?

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):

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)

@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Sep 23, 2024
@elarlang
Copy link
Collaborator Author

We must be aligned with proposal from #2038

Verify that all clients are confidential and configured to use strong client authentication methods, i. e. 'mTLS' or 'private-key-jwt'.

@jmanico
Copy link
Member

jmanico commented Sep 23, 2024 via email

@TobiasAhnoff
Copy link
Contributor

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.

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

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)

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

Verify that refresh tokens for public clients are sender-constrained using DPoP or Certificate-Bound Access Tokens (mTLS) and, if appropriate, also use refresh token rotation to mitigate refresh token replay attacks. (L1, L2, L3)

@elarlang
Copy link
Collaborator Author

Verify that refresh tokens for public clients are sender-constrained using Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS) and, if appropriate, also use refresh token rotation to mitigate refresh token replay attacks.

@randomstuff - any further feedback or need for changes?

@randomstuff
Copy link
Contributor

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?

@elarlang
Copy link
Collaborator Author

I agree with that. To this direction?

Verify that the authorization server mitigates refresh token replay attacks for public clients by using (L1, L2) refresh token rotation or (L3) sender-constrained refresh tokens using Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS).

@randomstuff
Copy link
Contributor

Maybe:

Verify that the authorization server mitigates refresh token replay attacks for public clients by using (L1, L2) refresh token rotation or (L1, L2, L3) sender-constrained refresh tokens using Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS).

@randomstuff
Copy link
Contributor

randomstuff commented Sep 30, 2024

Verify that the authorization server mitigates refresh token replay attacks for public clients by using (L1, L2) refresh
token rotation or (L1, L2, L3) sender-constrained refresh tokens using Demonstrating Proof of Possession (DPoP) or
Certificate-Bound Access Tokens (mTLS).

Actually, I noticed that this conflicts with DPoP:

Refresh tokens issued to confidential clients (those having established authentication credentials with the authorization server) are not bound to the DPoP proof public key because they are already sender-constrained with a different existing mechanism.

The existing mechanism being client authentication.

@randomstuff
Copy link
Contributor

Actually, I noticed that this conflicts with DPoP

Well actually no, it does not because of "public clients" 🤦

@elarlang
Copy link
Collaborator Author

So, all good or need for changes?

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 5, 2024

Current 51.2.4:

# Description L1 L2 L3
51.2.4 [ADDED] Verify that the authorization server mitigates refresh token replay attacks for public clients by using (L1, L2) refresh token rotation or (L1, L2, L3) sender-constrained refresh tokens using Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS).

For refresh token rotation - we should require revoking previous refresh token, as decribed in:
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-11#section-4.3.1-10.2.1

@randomstuff
Copy link
Contributor

For refresh token rotation - we should require revoking previous refresh token, as decribed in:
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-11#section-4.3.1-10.2.1

I was going to say that this is implied in "refresh token rotation" but actually that's not true. So you are right! 👍

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 9, 2024

Proposal for add-on to the current requirement 51.2.4:

If the refresh token rotation is used, verify that the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if an already used and invalidated refresh token is provided.

"for that authorization" should be thought carefully - is it correct and is it understandable?

@randomstuff
Copy link
Contributor

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.

@randomstuff
Copy link
Contributor

randomstuff commented Oct 9, 2024

@elarlang, Yes, I think "authorization" if probably the better word.

See these snippets from the Oauth 2.1 draft:

The access token represents the authorization granted to the client.

[...]

An authorization grant represents the resource owner's authorization (to access its protected resources) used by the client to obtain an access token.

[...]

Access tokens are credentials used to access protected resources. An access token is a string representing an authorization issued to the client.

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).

@jmanico
Copy link
Member

jmanico commented Feb 11, 2025

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.

@elarlang
Copy link
Collaborator Author

And arguments provided why it does not suit as a requirement does not matter? :)

@jmanico
Copy link
Member

jmanico commented Feb 11, 2025 via email

@tghosth
Copy link
Collaborator

tghosth commented Feb 12, 2025

Current requirement:

# Description Level
51.4.4 [ADDED] 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. If refresh token rotation is used, verify that the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if an already used and invalidated refresh token is provided. 1

A few thoughts:

  • This is a single problem being solved, with a variety of different possible mechanisms to solve it.
  • At first glance. this does seem too long and ripe for splitting.
  • The slightly weird outcome of splitting these requirements would be a new requirement that is relevant for L1 but not relevant for L2 and L3 as the mechanism it refers to is not allowed for those levels. I don't know whether that is something we have elsewhere but it in itself is good argument for not splitting requirements.

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))."

@elarlang
Copy link
Collaborator Author

elarlang commented Feb 12, 2025

The reasons why L1 option is the last one and should stay there

  • the preferred solution is to use sender-constrained refresh tokens or mTLS as a defense - it does not make sense to start with a refresh token rotation spotlight
  • most likely applications tested against this requirement are not L1 applications - it does not make sense to start with a refresh token rotation spotlight

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".

@jmanico
Copy link
Member

jmanico commented Feb 12, 2025 via email

@elarlang
Copy link
Collaborator Author

Because for L1 it is important.

@tghosth
Copy link
Collaborator

tghosth commented Feb 12, 2025

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?

@elarlang
Copy link
Collaborator Author

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.

@jmanico
Copy link
Member

jmanico commented Feb 12, 2025 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Feb 12, 2025

What is your problem to solve here and what is the problem with the requirement to solve here?

@tghosth
Copy link
Collaborator

tghosth commented Feb 12, 2025

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."

@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 and removed _5.0 - Not blocker This issue does not block 5.0 so if it gets addressed then great, if not then fine. labels Feb 12, 2025
@jmanico
Copy link
Member

jmanico commented Feb 12, 2025 via email

@OWASP OWASP deleted a comment from elarlang Feb 12, 2025
@OWASP OWASP deleted a comment from elarlang Feb 12, 2025
@randomstuff
Copy link
Contributor

I agree that the requirement as it stands is quite long and daunting.

I've tried to come up with something like:

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 maye be used.

Verify that refresh tokens which are not sender-constrained (refresh rotation, for L1 only), 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.

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.

@randomstuff
Copy link
Contributor

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:

  1. you verify that refresh token rotation is used;
  2. then you verify than using revoked refresh tokens invalidates the current one (and ideally access tokens as well?).

@elarlang
Copy link
Collaborator Author

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)

@randomstuff
Copy link
Contributor

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).

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:

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. If refresh token rotation is used, verify that the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if an already used and invalidated refresh token is provided.

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:

[…] The authorization server cannot determine which party submitted the invalid refresh token, but it will revoke the active refresh token as well as the access authorization grant associated with it.


What about something like:

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 maye be used.

Verify that refresh tokens when refresh token rotation is used (for L1 only), the authorization server revokes all refresh tokens and all the access authorization grant (including when possible the active tokens) for that authorization if a previously invalidated refresh token is provided.


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.

@elarlang
Copy link
Collaborator Author

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:

Verify that the authorization server mitigates refresh token replay attacks for public clients

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.

@elarlang
Copy link
Collaborator Author

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:

You previously opened this discussion in #2110 (comment) - can you recall why it did not reach to the requirement?

@tghosth tghosth removed the next meeting Filter for leaders label Feb 13, 2025
@TobiasAhnoff
Copy link
Contributor

This might be a solution. If we add refresh token to 51.4.11

Verify that the authorization server issues only sender-constrained (Proof-of-Possession) access and refresh tokens, either using mTLS certificate binding or Demonstration of Proof of Possession (DPoP).

Then 51.4.4 (for level 1) could be

Verify that for public clients, if refresh tokens are not sender-constrained, then the authorization server must mitigate refresh token replay attacks. In example by using refresh token rotation.

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

If refresh token rotation is used, verify that the authorization server invalidates the refresh token after usage and revokes all refresh tokens for that authorization if an already used and invalidated refresh token is provided.

Does this work?

@jmanico
Copy link
Member

jmanico commented Feb 14, 2025

I think that is a great idea @TobiasAhnoff

@randomstuff
Copy link
Contributor

randomstuff commented Feb 14, 2025

Verify that the authorization server issues only sender-constrained (Proof-of-Possession) access and refresh tokens, either using mTLS certificate binding or Demonstration of Proof of Possession (DPoP).

This does not work as it stands because RFC 9449 (DPoP) only supports DPOP-bound refresh token for public clients:

Refresh tokens issued to confidential clients (those having established authentication credentials with the authorization server) are not bound to the DPoP proof public key because they are already sender-constrained with a different existing mechanism.

It should be something like:

Verify that the authorization server issues only sender-constrained (Proof-of-Possession) access and, for public client, refresh tokens, either using mTLS certificate binding or Demonstration of Proof of Possession (DPoP).

Also this is a downgrade in security compared to the current version which requires sender-contrains refresh tokens for public clients in L2.

@randomstuff
Copy link
Contributor

randomstuff commented Feb 14, 2025

I like @tghosth's proposition #2110 (comment). I would try to simplify it. Would something like that work?

Verify that the authorization server mitigates refresh token replay for public clients, preferably using using sender-constrained refresh tokens (i.e., Demonstrating Proof of Possession (DPoP) or Certificate-Bound Access Tokens (mTLS)) or using refresh token rotation. Refresh token rotation must only be used for L1: in this case, the authorization server must revokes all refresh tokens for that authorization if an invalidated refresh token is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V51 Group issues related to OAuth _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

5 participants