-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
Add requirement about usage of claims other than subject and issuer as an identifier for OpenID Connect #1826
Comments
Super excited about this. A dedicated OIDC section would be fantastic! |
Yes, that is correct - we have OAuth PR to be discussed and merged and that is why we don't have sepparate issues per requirement so far on the topic. The branch for that is located at https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md If you have in mind some wording for the proposed requirement, please share it that (we can fine-tune and help with that) and matching section from the OAuth category. |
@jsherm-fwdsec great idea! |
@csfreak92 did you see this? Can you work with @jsherm-fwdsec on this? |
For sure @tghosth, will work with @jsherm-fwdsec on this one! I think some parts of it were covered by OAuth 2.0 requirements that I wrote, but I will double/triple check to ensure we got them nailed to good detail to include OpenID stuff. |
our usual process is, that we first have agreement on the requirement wording in an issue and then we go for PR. This way it is easier to follow later, why some change was made and what where the arguments behind the change were. @jsherm-fwdsec - do you have a wording or an idea for the point for additional requirement(s)? |
How about…
Verify that applications utilizing OpenID Connect identify and utilize at least one additional claim, beyond `sub` and `iss`. This could include claims such as `email`, `preferred_username`, or a custom claim agreed upon with the identity provider (IdP).
|
For me it seems it is not correct, see https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability.
|
Hi @jmanico @elarlang! Agreed I think we need to be more specific about what the unique identifier should be and it should conform to the spec. How about something such as this: |
|
That works here's what I would suggest: Also agreed this should go under https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md#v513-resource-server. Since we're touching on ID tokens that are specific to OIDC, we might also want to add and update some terminology under this section for ID token, access token, and "Authorization Server" to include info about the OpenID Provider: https://github.com/OWASP/ASVS/blob/master-v51-oauth/5.0/en/0x51-V51-OAuth2.md#terminology. Here's my attempt:
|
Just for info, we need to solve some other open issues for the OAuth paragraph before moving this one further. |
Yeah, I'm working on the OAuth section on my local copy. I will push commits soon. |
Question for the leaders: @jmanico, @tghosth, @elarlang, Will OpenID Connect section be under the OAuth 2.0 chapter that we were working on? Or will it be on a separate chapter on its own? If it was the latter part, I would suggest @jsherm-fwdsec to create a PR on a separate branch, but if someone can point him to where to push it so that he won't be waiting too long for me on the OAuth section to be merged. |
At the moment we put all OAuth and OpenID-related requirements into one chapter. Based on the current direction, we will not have a separate section for OpenID flow. But let's see, how many OpenID-related requirements we will have, we may reorganize them into separate/different chapters. For current requirement we agreed to add it to the Resource Server chapter:
I can see the procedure, that we get the current OAuth chapter merged to the main branch, and then we go issue-by-issue with the modifications. At the moment it can be a slow-downer, as there are too many "opened issues" as separate comments in different places and it's complicated to get an overview, what is the situation and what must be done. |
I suggest that OAuth2 and OIDC are two separate chapters. They are quite
different even though they are both based on the OAuth2 Framework.
|
Are they going to be different enough to merit two chapters @csfreak92 ? |
They should probably be in different sections at least |
I haven't looked at OpenID Connect as closely with OAuth. Maybe separate sections at least for now. Can you draft something for it @jsherm-fwdsec? |
Gentlemen, please, re-read my comment: #1826 (comment) :) Let's get the OAuth chapter in, let's get the changes done, let's get OIDC requirements in and then we can shake everything to correct or better place if needed. One step at a time. |
Sounds good @elarlang. I'd be happy to help contribute to the OIDC requirements when the time comes. |
For OAuth "it depends" so a suggestion is to have a more generic requirement, perhaps like this:
For OIDC ID Tokens this is given by the specs, see https://openid.net/specs/openid-connect-core-1_0.html#IDToken which states that sub is "A locally unique and never reassigned identifier within the Issuer for the End-User, which is intended to be consumed by the Client...". So the requirement for OIDC could be something like:
|
Let's try to get the requirement for "OIDC Client" first. I made some changes to the proposal:
|
I think there are two missing things in this formulation:
Therefore, I'd use something like:
|
"are not reassigned" > "can not be reassigned"?
|
The requirement to V51.5 OIDC Client goes in via #2124
|
Now, let's move on with the requirement for the OAuth Resource Server:
We should take into account that the access token information may come from Introspection (https://datatracker.ietf.org/doc/html/rfc7662) |
Would it be worth mentioning that this is only if user identification is required (or is that clear enough as it is)? The resource server could very well receive (by design) an anonymous access token. |
Yes, conditional requirement makes sense. |
A suggestion is
Or just
|
@TobiasAhnoff, both wordings use "uniquely identified" which I think is not clear enough (the important feature being "not reattributable"). |
I am wondering whether it it would be worth adding a requirement stating that if multiple identity providers are used, we should verify that each the identities of each issuer are properly partitioned from each other i.e. that IdP1 cannot spoof the identity of a user from IdP2 (unless this is desired as a consequence of some functional requirement). |
What if the same user uses 2 different IdP's but those should give the same user in the client? |
For the access token requirement, I propose one alternative direction:
|
Yes, that's why there was "unless this is desired as a consequence of some functional requirement". The goal being to say that this must not happen "by mistake" but only by design. Something in the line of:
|
On the other hand, this is not specific to OAuth but applicable to other SSO / authentication systems. I'll check which verifications (if any) in the authentication chapter (or somewhere else) already covers this. |
I feel like here are 3 topics mixed:
I there is need to improve requirement 1, let's open a new issue. Let's have agreement - PR-ready for requirement 2. If there is material for requirement 3, it is material for a separate issue. |
The second requirement goes to V51.4 OAuth Resource Server via #2140
|
We have now two requirements in, but feels like one topic requires some spin-off from here. @randomstuff - please recheck it, and if the "user idenfitication" issue requires a separate issue, please open it. |
It has it's own issue now: #2150 and we can close this one out. |
Usage of claims other than the subject and issuer identifier to uniquely identify an end user in OpenID Connect is non-compliant with the framework. As per a recent Microsoft report, there is a false identifier anti-pattern being followed and exploited in the wild resulting in account takeover.
To keep it simple, the sub (subject) and iss (issuer) claims, when used together, are considered to be a unique primary identifier for OIDC, as the uniqueness across users is guaranteed. Other claims such as email, username or phone number should not be used, as they can change over time and an attacker can falsify these claims.
I'm interested in contributing and happy to create a PR for this, as well as adding other OIDC requirements. Perhaps this is something I can help with as part of the OAuth2 changes being discussed here ?
The text was updated successfully, but these errors were encountered: