-
-
Notifications
You must be signed in to change notification settings - Fork 665
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 client verifications #2044
Comments
In contrast to what? Static provisioning of (access) tokens (and mTLS)? |
I was thinking in the context of OAuth2, there are of course service-to-service integration scenarios where there is no need to use OAuth2, where just e g mTLS is all that is needed. Did that make it clearer? |
@TobiasAhnoff I was just wondering which other OAuth flows you are intended to disallow here. Someone might attempt to automate an authorization code flow. Is this what you are trying to disallow here? On the other hand, we would not want to disallow Token Exchange. What about something like: « Verify service-to-service authorization requests, without (human) user interaction, do not use flows which are intended to be used for interactive flows such as the authorization code grant, the implicit grant or CIBA. » |
Hi @randomstuff - as I can not tag you in the issues you are not taking part, I (mis)use this issue for that. I would like to get your feedback on every V51 tagged issue, but to set a more clear focus for getting things moving I set focus to:
Nice to have, to get issues closed and document updated:
(but should be maybe combined, see #2039 (comment)) Note that, if you agree with proposals, it is also valuable feedback to share. We also have one dedicated Slack channel for "OAuth working group" but I was not able to find your name from userlist. |
@randomstuff good point! I was thinking perhaps we try to rewrite this in a positive way, what do you think about this?
And if we introduce token-exchange here, we should also add that spec to the scope for OAuth (#2036) |
Client credentials grant has not been deprecated but it’s strongly encouraged that if you use it to use strong client authentication with it like mTLS.
Can we perhaps add something like “… with strong client authentication” at the end of this or something similar?
|
This looks like a separate verification (not only ties to machine-to-machine interaction) and probably something you would not want in L1? |
I think “… with strong client authentication” maybe does not need to be added in this verification as it applies for all levels and mandating strong client authentication methods (mTLS or private-key-twt) is addressed in #2038
Perhaps that need to be rewritten/split to more clearly capture that good (sufficient for the level of security) client authentication is a must on all levels for client-credentials (e g properly managed basic auth or mTLS etc), but for L3 strong mTLS or private-key-jwt is required. Reading this suggested verification I also think it might be too strict, not allowing public code-flow native mobile app clients on L3, I will make a comment on that in #2038, worth a discussion! |
Should it be authorization server requirement to say, that what is allowed for machine-to-machine clients and what is not? (It should not be allowed for the Client to decide)
Here seems to be 2 things - CSRF protection and minimal set of scopes. We have requirement:
Do you think it covers the case? At least I proposed the requirement keeping exactly this scenario in mind. Please provide list of updated requirements. |
ping @TobiasAhnoff - please provide latest list of proposed requirements. |
@elarlang I agree, and from #2043 we also have
I suggest to simply remove
and
since they are partly covered by #2043 and more guidance/education on when to use which flow, not mitigations for a known attack-type (the "industry standard" is part is overlap by #2047). An alternative would be add something about grants to use, perhaps like this:
|
I think 3.2.5 covers most of it, perhaps modify 3.2.5 to also state that "by default request a minimal set of scopes"?
|
Those are different problems and I prefer to not merge them to one requirement. One requires defense against CSRF, another is default behavior for assigning privileges. I think the scope question is addressed here: #2043 (comment) |
Do I understand correctly, that we don't add anything from initial 4 requirements as those are covered one way or another somewhere else and it is proposed to update current 51.2.5
|
Almost, I will try to make a summary The first two could be replaced by a modified 51.2.5
or even more specific on valid OAuth grants
The third one is not addressed by in example #2043 so I think this should be added to address that clients should not request more scopes than needed on tokens requests (even if the client is allowed to use more scopes).
The fourth one is almost covered by 3.2.5 but the aspect of secure defaults for consent pages is not addressed by #2043, since the focus is on configuration of the allowed scope for the client (not the ones selected by default on the consent page). It is a detail, but I think it is worth having this in ASVS since there are many applications that by mistake or with bad intent ask the user to consent for scopes that aren't really need for what the user wants to do (but might be needed for other user consents granted for the client). So a suggestion is to add this as separate requirement
|
I think both
and
are covered by the two requirements suggested in #2043 (comment) and #2120 As noted earlier other parts of this issue is covered by #2043 (comment) so, as far as I understand, this issue can now be closed (replaced by #2043 and #2120 discussion). |
The following verifications are suggested to address general client responsibilities.
V51.3 Client
Verify that system integrations clients, service-to-service without (human) user interaction, are required to use the 'client-credentials' grant.
Verify that clients used by (human) users are required to use the 'code' grant or, if considered more appropriate, the 'device_code' grant. Additional custom "flows", not known to be industry standard (preferably defined in RFCs), should be avoided.
Verify that clients asserts least privilege in token requests, by minimizing the set of requested scopes or any other parameter that affects permissions.
Verify that clients used by (human) users requires the users consent to initiate an authorization request. Note that the consent can be implicit or explicit, requires sufficient CSRF-protection and should (by default) request a minimal set of scopes.
Notes
Note that adding the second verification above and suggested verification for PKCE in #2041 covers the PKCE part of 51.2.2 and the other parts of 51.2.2 is OIDC which should not be part of OAuth2 verifications, so a suggestion is to remove 51.2.2 (given the scope discussion in #2039 and that PKCE verifications from #2041 are added).
Also note that if the scope will be defined as suggested in #2036 then 51.2.3 and 51.2.4 should be removed as they are not in that scope, or the scope need to include Resource indicators and RAR specs as well.
The text was updated successfully, but these errors were encountered: