-
Notifications
You must be signed in to change notification settings - Fork 33
Check key usage #282
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
Merged
Merged
Check key usage #282
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d6c503b
Check_key_usage initial
MarcusJGStreets 9926a4e
check key usage
MarcusJGStreets 1f3403b
it is 2025
MarcusJGStreets a3ab679
Response to comments
MarcusJGStreets 836b05a
Add files via upload
MarcusJGStreets c4813f5
Copy-editor update
athoelke 171f374
Clarifications and fixes to text following review
athoelke eb68214
Update header file
athoelke ea64cc4
Rework the description of psa_check_key_usage()
athoelke 09044b4
FIxing typos and resolving todo.
athoelke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change that to allow either
PSA_KEY_USAGE_DERIVEorPSA_KEY_USAGE_DERIVE_PUBLIC?The situation is actually somewhat similar to
PSA_KEY_USAGE_SIGN_MESSAGEvsPSA_KEY_USAGE_SIGN_HASH, in that we had keys originally created with only theSIGN_HASHpermission and we wanted to allow them for an operation that is naturally controlled bySIGN_MESSAGE. What we did then was to say that any key created with theSIGN_MESSAGEflag also gains theSIGN_HASHflag (reported if you query the key's usage). So maybe we should do the same here, namely say that any key created withPSA_KEY_USAGE_DERIVEalso gainsPSA_KEY_USAGE_DERIVE_PUBLIC(if the crypto core complies with spec 1.4), in addition to changing the requirement inpsa_pake_setupto requireDERIVE_PUBLICwhen doing an operation in the verifier role?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the other way around: setting
PSA_KEY_USAGE_SIGN_HASHcausesPSA_KEY_USAGE_SIGN_MESSAGEto also be set for the key. But the pattern fits in this case as well.As well as source code compatibility, we probably ought to consider an existing key store in which a SPAKE2+ verifier (public) key only has DERIVE usage, and for which the policy is not updatable. In this case, we would want the updated implementation to accept a SPAKE2+ public key with DERIVE usage. Perhaps the wording should be that keys created with DERIVE usage, implicitly also have DERIVE_PUBLIC usage - and it is undefined whether that happens when the key is stored, or when the usage flags are inspected?
For newly created/derived keys (if we introduce this change), then the ('connected registration' flow in Figure 4 of SPAKE2+ registration) would set DERIVE_PUBLIC on the key imported into the Verifier key store, instead of DERIVE.
If the verifier key is not transferred from the Prover during the registration phase, and both roles start from the shared password ('independent registration' in Figure 4 of SPAKE2+ registration) - then the Verifier will derive the SPAKE2+ key pair from the password, and just provide the key-pair to the PAKE operation for the Verifier role. In this case the verifier could just set DERIVE_PUBLIC usage on the key-pair, which restricts it to only be valid in the Verifier role (and makes the Prover part of the key-pair unusable in that key object).
In both cases, DERIVE usage should be permitted, for source code compatibility for applications written against the current spec - this utilises the 'implicit DERIVE_PUBLIC' rule described above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that adopting
PSA_KEY_USAGE_DERIVE_PUBLICas a 1st-class policy flag, as described here, is not necessary for this particular PR that introduces the flag to enable the desiredpsa_check_key_usage()behavior.I am going to split this idea, and its discussion, into a separate Issue, that can be pursued as a follow-up to this PR.