-
-
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 access token requirement for preventing "key confusion" #2361
Comments
I don't think it is JWT or token specific requirement - those who have worked with V6 maybe can hint is this covered there already. Also it is on the limit with the scope. You need to have access to issuer setup that is a bit out of scope from application point of view. And if you have access, then I think it is more a question of handling keys in general? (ping @randomstuff @danielcuthbert ) For tokens we additionally have or will have requirement for validating the 'iss' claim. |
"key confusion" is mentioned at the moment in current version of 3.5.5:
The requirement itself is discussed in the issue #2204 . Is this a recommendation to split the current 3.5.5, is it a duplicate of current 3.5.5 and should be closed or it provides some new aspect that is not covered yet? |
The suggestion was to split split the current 3.5.5 and have "key confusion" together with "Verify that the cryptographic keys for tokens belong to the token issuer" and to provide an example for JWTs with "For JWTs the 'iss' claim must be validated to be a preconfigured expected value". I think is is good to split, since "using strong alg" and "key confusion" are different things, you could use strong algs and still confuse keys. |
Given that we decide to split, is this good as a requirement for the "key confusion" part?
|
Would it be "needs to be supported" or "are supported"? Maybe we need to say that this is for token verification (not generation)?
Nitpick, there is a missing comma:
|
Perhaps this?
|
Some more clarifications to document:
|
3.5.7 is about using the correct cryptographic material for validating the token. The one we are discussing here is supposed to be about key confusion but actually covers:
We should probably better focus on (1) and remove (2) from this requirement. Alternatively or in addition, this might be part of a more general cryptography requirement about verifying that each key is used with its intended cryptographic usage:
|
Maybe 3.5.7 could include issuer claim, like this?
I think this would address the "key confusion" scenario as well. It is also probably good to open a new issue for a "more general cryptography requirement about verifying that each key is used with its intended cryptographic usage" in V6 |
Just focusing on key confusion, if we look at the key confusion example from the WSTG, there are potentially two issues:
In theory, key confusion could still occur with (2) in place if an application needed to accept both symmetric and asymmetric algorithms. The issue can also occur even if the application uses a trusted key from a trusted source, so I think it should be separate from 3.5.7. @randomstuff I like the idea of abstracting this to ensure that keys are only used for intended purposes, but I think abstracting it will make it more challenging to implement and test as a requirement, so here is a first attempt just focusing on key confusion with JWTs as an example:
Concerning 3.5.5:
I think it is worthwhile to have a more precise requirement instead of "additional controls should". As is, the core of 3.5.5 addresses (2) from my points above and then appends that last phrase to address (1). |
Is this necessary? Would it be OK if I'm using Should we say "For example, for JWTs, …"? |
Good point, how about something more specific to key confusion:
|
Please see my comment here: #2360 (comment) |
Add access token requirement for preventing "key confusion".
This is part of "cleaning up 3.5" see #1917 (comment)
The text was updated successfully, but these errors were encountered: