-
-
Notifications
You must be signed in to change notification settings - Fork 673
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
Modify 3.5.5 - split key confusion part to a separate requirement #2360
Comments
For me it is duplicated by current V3.5.5:
It went through long journy already in #2204. If it is covered already by 3.5.5, please close this issue. If there is need to improve current 3.5.5 to cover some missing aspect, then please re-open #2204 as previous discussion is held there. If there is need for a new requirement, please provide information, what makes this new proposed requirement different and unique from 3.5.5. |
This has always felt like a V6 requirement to me.
Does this apply to more than JWTs? |
We also have V3.5.3 requirement on the topic:
Direction is to have abstract requirement (not an access-token specific). If there are no new aspects from proposed new requirement, we can re-open related issues for 3.5.3 (#2184) and/or 3.5.5 (#2204 ) if needed to finetune something there. |
The suggested changed here is related to #2361 (comment). If not "split" as suggested there, V3.5.3 (and V6) covers this as it is and this can be closed. If "split", V3.5.5 could be changed as suggested here. |
Given that we decide to split (see #2361 (comment)), should we then have V3.5.3 to only address the "strong alg" part, and rewrite it like this?
or is just add "strong" and removing "key confusion" from V3.5.3 better?
|
I try to get overview of the situation. We have current requirements that are made long journey to have those wordings:
Now we have 2 issues (#2360, #2361) with titles "add new requirements" that actually going to replace/duplicate/modify current requirements. @TobiasAhnoff - can you provide reasoning, arguments, and goals for what we should achieve here? If in practice we are going to modify existing requirements, it makes sense to declare it that way and re-open previous issues for those requirements. |
@elarlang yes, some om the "add new" are modifications of existing and it it would be better to re-open those, add comments from "add new" issues and close the "add new". |
It would be helpful to have a direction to follow. At the moment it is quite messy (at least for me). There is one split proposal involved in another issue, but list here or somewhere else the set of requirements you propose with clear differences and modifications to existing requirements. |
Given #2361 (comment) "key confusion" is suggested to be slit to a new requirement and can be removed from 3.5.5. And then this issue is only to suggests a simplification of 3.5.5, to make it more clear that strong algorithms should be used and for JWTs 'None' should not be allowed, perhaps it can be like this
So the title of this issue could be changed to "modify 3.5.5" or close this and re-open a previous issue and add comment there for the suggested modification. Does that make a good summary? |
Having read through this thread and through #2361, it feels like this got a little complicated. I think we can boil this down to:
I would therefore suggest that the following modifications to 3.5.5 and 3.5.7 would close both #2360 and #2361.
I took out the symmetric/asymmetric thing because "only usable for specifically specified algorithms" should cover that. Comments @ryarmst @TobiasAhnoff @randomstuff? |
Three small comments:
|
I think it works to address both "Only use allow listed algorithms, not included None." and "Only have strong algorithms on the allow list." in V3.5.5 and have V3.5.7 to address "Only use specific trusted keys or key sources for specific algorithms." (which includes key confusion). Perhaps modify proposals in #2360 (comment), like this?
|
Looks like Tobias's updated draft covers this.
I think we could add "for this purpose" to make it clearer.
I think once we say that keys should only be usable for specific algorithms then it solves it regardless. Updated suggestions: 3.5.5 3.5.7 final comments @randomstuff @TobiasAhnoff @elarlang ? |
3.5.5 a nice to have thing, 1st and 2nd sentence overlap maybe a bit too much. Not a critical problem but there is potential to shorten it a bit.
Should vs must?
Verify that allowlisted algorithms are used to create cryptographically secured tokens. ping @randomstuff - if I remember correctly, you proposed "create and verify", is my proposal lacking something? 3.5.7
Is the actual scope for these requirements wider ("cryptographically secured tokens") than "self-contained" tokens? |
The "verify" part may be the most important part. Even if the token issuer carefully handles the uses algorithms, a malicious attacker could try to forge malicious tokens using weaker ones. the verifier should not allow unexpected algorithms. |
Yes, if we have separate sections for these.
This probably be "must".
For |
I think it is a must for L2 and L3, but might be should for L1?
I think this is covered, the main point is to not blindly follow urls in jku, x5u or jwk |
At first sight, I like having separate requirements. On the other hand if find it difficult to separate clearly the things that must be checked: |
For the first requirement, maybe:
Wow that's long … Changes:
Question: Should it apply to decryption of the token as well? Question: Is it specific to secured token or is it valid anytime MAC or signature are used? (Should such as requirement go in V6?) |
For the second requirement, maybe:
Changes:
Things I considered but did not include:
Question:
|
I think the second requirement is good, maybe add "intended" and write in the same format as the first one, like this:
Maybe the first one could be a little "simplified" (without losing important aspects), perhaps like this (it also seems good to mention the iss claim?)
Perhaps remove "(for the intended usages)" and " (to be a preconfigured expected value)" to make it shorter?
I think maybe that could be part of V6, and requirements here are focused on the integrity of the token (even if encrypting a token is one way to assert that it can be trusted). @randomstuff do you have a requirement in mind?
I think it is good to have this in a section specific for secured "self-contained" tokens, but maybe there should also be more generic requirements addressing MAC or signature in general in V6? (maybe open a separate V6 issue for this) |
Note, some wording found in the JWT RFC which might be a source inspiration in the wording:
|
I try to summarize this, we have 3 requirements (given that we split 3.5.7 and I updated to use the JWT RFC wording), is this good? 3.5.5
3.5.7 split to
|
Verify that when the application validates a cryptographically secured token, it only uses key material that under the control of the party identified as the issuer. For JWTs and other JWS structures the iss claim must be be validated (to be a preconfigured expected value) and, if the key material is obtained from a header such as 'jku', 'x5u', 'jwk' or kid, this header can be validated using an allowlist.
When using digital signatures, the verifying key is typically the public key. Losing this key in this situation does not seem like a high impact event in some situations. Just a side note.
|
I think the intent was to say that the corresponding private key was under the control of the party :) |
Yes, maybe make this clearer like this?
or
|
I think signing key is fine.
But just to be a bit more pedantic, sometimes the integrity algorithm is a HMAC and in that case there is no signing, it’s a MAC/Hash. 👋🏽
|
True, wording is hard :) In 3.5.5 it is suggested to use the term "integrity validation", perhaps use something like that here as well?
|
I think it should be "authenticity validation" instead (?).
Validating kid against an allowlist is quite vague and possibly not that useful depending on what we intend exactly. On a second thought, I think 'kid' maye not be that relevant in this requirement after all (in general). |
Ok, I think we have the current wording. Note the change to requirement numbers as we now have a new chapter and section V52.1: 52.1.2 (previously 3.5.5) 52.1.3 (previously part of 3.5.7) 52.1.4 (previously part of 3.5.7) Comments on changes:
|
Relevant snippet:
Are there cases where allowlisting the |
To me, 52.1.2 and 52.1.3 looks good, in 52.1.4 I think 'kid' could be removed. Then one question remains: Should we use "integrity" or "authenticity"? I think both can be used, but it should/shall be the same terminology in 52.1.2 and 52.1.4, "authenticity" is good since it implies more than just integrity (that it can not be changed without validation failure), I agree with @randomstuff and vote for "authenticity" in both 52.1.2 and 52.1.4. |
Is there not a possibility of using kid without using jku? For example, this is a JWT produces by Google Identity:
So my suggestion was to side step the issue and just say validate, do we need to include "integrity" or "authenticity"? |
that works as well, given that not adding "signing" (or anything else) is ok (see #2360 (comment)).
Yes, it is in my experience the most common case. But I think @randomstuff (and I) argue that validating kid is a bit different from validating 'jku' or 'x5u' and it might be confusing to say that 'kid' should be validated against an allowlist in the same way as 'jku' or 'x5u'. But 'kid' needs to be used to look up the matching key from the key set, so in a way it is a look up from a dynamic allowlist retrieved from the authorization server. Perhaps remove 'kid', keep as is or add something like this:
Or is this too long? Must or should/can? |
It feels more complicated than necessary, I can accept that maybe there is a minor conceptual difference but I don't think it is worth overcomplicating the requirement for. So I say stick with this, with some minor tweaks (in bold): 52.1.4 (previously part of 3.5.7) |
I think what we want here is authenticity i.e. than the message was created by some identified issuer (and was not modified). This is why we must validate the key based on the issuer. Integrity would only mean that the message was not altered bot not that it was generated by the intended issuer. |
… then there is the case a proof of possession tokens. For example, in OAuth DPoP Pushed Authorization Token Request, the DPoP token is only used to prove ownership of a private key. We do not verify the binding of this private key to any owner. We do not need to validate the The requirement as currently worded arguably does not apply in this case (in particular "must be validated using an allowlist"). The "that is under the control of the token issuer" id discutable as well. (Though we might argue that it actually works in the sens that the issuer is whoever issues the authorization request …). |
Yes, allowlist validation applies to 'jku' and 'x5u'. Since we have "such as", a suggestion (to keep it easy to read, we can´t fit "all" JWT use cases etc in one clear actionable requirement), is to simply remove 'jwk' from the list of claims and if needed add a separate requirement for validating 'jwk'. Verify that when the application validates a cryptographically secured token, it only uses key material that is under the control of the token issuer. For JWTs and other JWS structures the iss claim must be be validated (to be a preconfigured expected value) and, if the key material is obtained based on a header such as 'jku', 'x5u' or 'kid', this header must be validated using an allowlist. or, also remove 'kid' ("such as" implies that the list of claims in not complete) Verify that when the application validates a cryptographically secured token, it only uses key material that is under the control of the token issuer. For JWTs and other JWS structures the iss claim must be be validated (to be a preconfigured expected value) and, if the key material is obtained based on a header such as 'jku' or 'x5u', this header must be validated using an allowlist.
This is from the RFC, I think it works, @randomstuff do you have a suggestion on how to improve this or can elaborate some more on why it needs to change? |
@randomstuff can I request that if you have a query on a proposed wording, you provide an updated wording. For my perspective, this has gotten a quite niche and complicated and I think we need to make sure we are not losing clarity in order to cover every niche case. @TobiasAhnoff I really think that jwk and kid are useful in the vast majority of cases and I don't see why we would remove them and I don't want a separate requirement on jwk. I would suggest the following which excludes DPoP and then if necessary talk about DPoP in v51: Verify that when the application validates a cryptographically secured token (except for a DPoP proof JWT), it only uses key material that is under the control of the token issuer. For JWTs and other JWS structures the iss claim must be be validated (to be a preconfigured expected value) and, if the key material is obtained based on a header such as 'jku', 'x5u', 'jwk' or 'kid', this header must be validated using an allowlist. |
Yes sorry, I wanted to raise the issue but I'm not sure whether this is clear enough already as it was and whether there is a need to have a wording with is actually accurate for this corner case. I'll try to propose something.
Yes, I agree.
I'm not so fan of adding this explicit exception ("for DPoP") though, I'd better have a general wording but I'm not yet sure how to formulate it. We can probably say it's good enough as it is. |
Some comments:
Proposition:
|
This is the 50th comment in this issue, it's a bit of a sign that the direction is not clear. If we are not close to the proposal, I would like to recommend that first there is a clear written "problem to solve" for the requirement and then we can find out how to achieve this. If there are too many different edge cases or technologies to take into account, it is worth writing separate requirements first and then merging, if those are clearly overlapping. |
I think the most recent proposal is good enough. Key points:
Opened #2431 |
Add access token requirement for strong alg and "None" to mitigating risk of using weak or "none" algorithms to protect tokens
This is part of "cleaning up 3.5" see #1917 (comment)
The text was updated successfully, but these errors were encountered: