cryptenroll: Use CTAP2.1 credProtect extension #32295
Merged
Conversation
This file contains 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
github-actions
bot
added
util-lib
please-review
poettering
reviewed
Apr 16, 2024
poettering
added
cryptsetup
fido2
good-to-merge/with-minor-suggestions
and removed
please-review
poettering
reviewed
Apr 16, 2024
poettering
added
reviewed/needs-rework 🔨
BryanJacobs
force-pushed
the
use_credprotect
branch
from
60e2f61
to
b7234f6
Compare
github-actions
bot
added
please-review
poettering
reviewed
Apr 16, 2024
|
looks good, just one minor nitpick, see above. |
poettering
added
reviewed/needs-rework 🔨
github-actions
bot
added
please-review
Addressed, thank you. |
|
please squash. ant log_warning, not log_notice, please |
BryanJacobs
force-pushed
the
use_credprotect
branch
from
8d4a5b4
to
72a0b4c
Compare
Done. Apologies for wasting your time. |
poettering
reviewed
Apr 16, 2024
np. but one mor fix please |
BryanJacobs
force-pushed
the
use_credprotect
branch
from
72a0b4c
to
6476553
Compare
When enrolling a new FIDO2 token with a client PIN, this tells the authenticator to require the PIN on all uses. It also collects a PIN before attempting to create a credential. Works around #31443 in most (not all) scenarios.
BryanJacobs
force-pushed
the
use_credprotect
branch
from
4c1621b
to
7ec00f2
Compare
poettering
added
good-to-merge/waiting-for-ci 👍
|
thanks! lgtm! |
github-actions
bot
removed
the
good-to-merge/waiting-for-ci 👍
kszczek
added a commit
to kszczek/systemd
that referenced
this pull request
Apr 27, 2024
The recently merged PR systemd#32295 introduced support for the credProtect extension, but in doing so, it broke the discoverability of credentials by setting the policy to "userVerificationRequired". This policy would require us to pass the PIN to the token in the pre-flight request to be able to discover it, which defeats the purpose of the pre-flight request as it's supposed to be non-interactive. This commit relaxes the protection policy, so that the credential can be discovered if we provide it's ID, which we do.
kszczek
added a commit
to kszczek/systemd
that referenced
this pull request
Apr 27, 2024
The recently merged PR systemd#32295 introduced support for the credProtect extension, but in doing so, it broke the discoverability of credentials by setting the policy to FIDO_CRED_PROT_UV_REQUIRED. This policy would require us to pass the PIN to the token in the pre-flight request to be able to discover it, which defeats the purpose of pre-flight requests as they're supposed to be non-interactive. While relaxing the policy to FIDO_CRED_PROT_UV_OPTIONAL_WITH_ID would solve this problem in most cases, some edge cases would remain broken. One of those edge cases is outlined in the CTAP 2.1 specification, which states: Note: Some authenticators for high-security environments may be configured to always set credProtect 3 for all created credentials regardless of what the platform requests. This commit removes the support for the credProtect extension.
kszczek
added a commit
to kszczek/systemd
that referenced
this pull request
Apr 27, 2024
The recently merged PR systemd#32295 introduced support for the credProtect extension, but in doing so, it broke the discoverability of credentials by setting the policy to FIDO_CRED_PROT_UV_REQUIRED for UV-less, PIN-protected credentials. This policy would require us to pass the PIN to the token in the pre-flight request to be able to discover it, which defeats the purpose of pre-flight requests as they're supposed to be non-interactive. This commit restricts the usage of credProtect to UV credentials only.
poettering
pushed a commit
that referenced
this pull request
May 2, 2024
The recently merged PR #32295 introduced support for the credProtect extension, but in doing so, it broke the discoverability of credentials by setting the policy to FIDO_CRED_PROT_UV_REQUIRED for UV-less, PIN-protected credentials. This policy would require us to pass the PIN to the token in the pre-flight request to be able to discover it, which defeats the purpose of pre-flight requests as they're supposed to be non-interactive. This commit restricts the usage of credProtect to UV credentials only.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When enrolling a new FIDO2 token with a client PIN, this tells the authenticator to require the PIN on all uses.
Works around #31443 in most (not all) scenarios.
A full fix would require checking the
uvflag of the result from thegetAssertioncall and comparing with the JSON token info for whether a PIN or UV was requested.Nonetheless, this should be an improvement.