-
-
Notifications
You must be signed in to change notification settings - Fork 668
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: Add refresh token verfications #2040
Comments
It is / replaces current 51.1.4 It is not clear now, are we saying, that refresh token rotation is not ok or it is ok, but is is recommended to use sender-contstrained tokens?
I'm not sure we need this as separate requirement, we should instead cover it in general token handling section. Till it is not clearly covered elsewhere, we can keep this requirement in.
Depends on the refresh token rotation requirement - if we kind of support refresh token rotation, it should be clearly said, that when rotating the token, the lifetime for the token must stay the same (and not be extended) related or partly duplicate to #1968 |
Yes, the intention is to modify/replace 51.1.4 to align with (as far as I now) the latest best current practices, see FAPI at https://openid.bitbucket.io/fapi/fapi-2_0-security-profile.html#section-5.3.2.1 which in Note 2 states:
Compared the old from 2022-2023 :), where refresh-token rotation was defined as an alternative to sender-contrained tokens, see https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#section-4.3
Maybe the note-part should be removed and just have:
or, since other suggested verifications in #2038 cover client authentication and sender-constrained token requirements, simply have:
This is (in my opinion) a good example of how hard it is to keep ASVS updated with OAuth/OIDC BCPs and security profiles, and the importance a well-defined scope etc (#2036) |
It should be handled together and to solve also 3.5.1 from #1917 |
@TobiasAhnoff, it seems to be a fair point, but since the details of that mechanism (Implementers need to consider a secure mechanism for clients to recover from a loss of a new refresh token on issue) is out of scope of the specification, it will read and sound like we are not sure what to recommend either. I guess this is where I see a problem with it. I would be more cautious with it as it sounds like there is no proposed solution either and it is up to the implementer to figure it out which might lead to more design/implementation flaws. Just my opinion on this, if we cannot have a bit of a solid solution for any user of ASVS that would be harder to justify using it. I am inclined to keep the current 51.1.4 unless there's a clearer path what to do with this dilemma. What do you guys think @elarlang, @TobiasAhnoff, @deleterepo? |
When I read this, the first thing I did was check what the OAuth 2.1 draft had to say about that (they mention the option of using refresh token rotation would discouraging its use). If this is to be kept, maybe something like:
|
Note: I did not know what a reference token was :) but it is mentioned in the current OAuth 2.1 draft:
However, it is not mentioned in current OAuth specifications AFAIU so maybe it might be worthwhile to add a short note explaining the term. Also we are talking about reference tokens which are access tokens, right? Or does this apply to other kind of tokens as well? Can we say "reference access token" (or would it make it more difficult to find what we are talking about?). Maybe "access tokens which are reference tokens"? |
Do we need to clarify what kind of revocation we are talking about? Revocation by the user using the AS user interface ? Revocation by the client using the token revocation API? Both? |
Maybe add something like "This can be achieved using DPoP or Certificate-Bound Access Tokens." in order to make it clearer for the layman what we are talking about? |
A clarification including both cases will be good to add. |
Yes we are talking about access tokens, good to make that clearer with in example "reference access tokens" |
Also good to make this clearer, I will modify suggested verifications to try and make things clearer, probably later today.... |
Refresh tokens is a challenging topic! I will try to address comments above from @randomstuff and @csfreak92 by suggesting 4 verifications (where the second is a modification of 51.1.4):
@elarlang this might be "related or partly duplicate" to #1968 and #1917 but perhaps it is easiest to first finish refresh token verifications for the OAuth/OIDC chapter here and then address if it is covered by #1968 and #1917 ? |
I am wondering whether this should be a "and" instead of a "or"?
So I would suggest replacing the "or" by a "and" or (better?) split it into:
Questions: Is this science-fiction? Are major authorization servers providing these features? Are there cases where there is no AS user interface? I checked how Keycloak works:
|
Not an easy task... But let's stop finetuning the technical testing guide or cheat sheet content here and state - what problem do we address with those requirements? What risk it mitigates? Maybe we can reach to not that technically detailed abstraction. Also, I'm afraid, that all the refresh_token topic things are changing so fast, that we are outdated already next day after we release it. |
@elarlang, @randomstuff, I would say @TobiasAhnoff's modifications below here are quite good and addresses the risks that we are trying to mitigate. Don't you think?
|
As each requirement needs further finetuning and discussion, I split this issue into separate issues per requirement.
|
A suggestion is to add the following verifications to address refresh tokens:
V51.2 Authorization Server
Verify that refresh tokens for public clients are sender-constrained (confidential clients must use client authentication in refresh token requests). Note that refresh token rotation to prevent e g token replay attacks is no longer recommended, but allowed. (L1, L2, L3)
Verify that refresh and reference tokens can be revoked. (L1, L2, L3)
Verify that refresh tokens have an absolute expiration. Even if sliding refresh token expiration is applied, re-authentication should be forced at some point in time. Note that how often depends on the application in scope, from hours to months. (L1, L2, L3)
The text was updated successfully, but these errors were encountered: