Passkey local fix and improvements - backport sssd-2-9 #8276
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request backports fixes and improvements for passkey authentication. The changes include removing the dependency on the IPA server for user verification policy, simplifying the logic to rely on local configuration. It also introduces several important fixes, such as preventing a potential double authentication flow when using Kerberos with passkeys, and fixing critical memory safety issues related to PIN handling that could lead to crashes or buffer over-reads. The overall changes improve the robustness and maintainability of the passkey feature.
|
|
alexey-tikhonov
added
no-backport
|
The pull request was accepted by @thalman with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
sssd-bot
force-pushed
the
passkey_local_fix_2_9_backport
branch
from
a527568 to
fd72ce7
Compare
|
Because of #8185 (comment) I would either Personally would prefer (1) |
|
Since this is a backport and we don't need to maintain any order option 1 seems like the better approach, but I'm fine with both option |
|
I added the commit 3b8eada |
Looks like |
Sorry, I misread the comment, disregard. |
justin-stephenson
force-pushed
the
passkey_local_fix_2_9_backport
branch
from
3b8eada to
833924f
Compare
|
@ikerexxe Please check latest changes (newly added commit) for approval |
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com> (cherry picked from commit be5df34)
Remove SYSDB_PASSKEY_USER_VERIFICATION and related functions. In phase 1 of passkey implementation we read passkey user verification from IPA LDAP tree, however now user verification is sent to the SSSD krb5 plugin from ipa-otpd. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com> (cherry picked from commit 879d073)
Local auth functions should only be reached in AD/LDAP auth flows. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com> (cherry picked from commit 304f298)
Remove support of ambiguous "unset" state of passkey user verification. pam_sss prompting is binary, either on or off. The use of 'unset' passkey user verification state allows for ambiguous behavior in SSSD. For example, passkey_child may perform undefined behavior when '--user-verification' argument is not set, now SSSD will always send '--user-verification=false/true' to passkey_child. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com> (cherry picked from commit e9216fc)
When authenticating with a passkey, different PAM code paths within SSSD
can result in the `authtok` containing data even when the user did not
enter a PIN. Depending on the flow (e.g., triggered by `gdm` vs. `su`),
this data might be an empty string or non-printable characters like `^L`
(form feed).
The previous code had two issues:
1. It only checked if the `authtok` was non-empty
(`sss_authtok_get_type(...) != SSS_AUTHTOK_TYPE_EMPTY`). If user
verification was disabled, this check would incorrectly pass for
these 'junk' `authtok` values. This caused SSSD to prepare and send
an erroneous PIN to the passkey helper.
2. In the case where the `authtok` *was* correctly empty, the check
would fail, `write_buf_len` would remain 0, and the `if
(write_buf_len != 0)` block containing the `write_pipe_send` call
would be skipped. This stalled the authentication flow, as the
callback to continue the process was never set.
This patch fixes both issues:
1. The `user_verification` setting is now stored in the state struct.
The logic is updated to only prepare the PIN buffer if the `authtok`
is non-empty *and* user verification is required
(`state->user_verification != PAM_PASSKEY_VERIFICATION_OFF`).
2. The `write_pipe_send` call is moved outside the conditional block so
it always runs. This ensures that the asynchronous child
communication (via `passkey_child_write_done`) is always triggered,
even if the write buffer is empty (0-length).
This resolves both failure modes: junk PINs are no longer sent when
verification is off, and the auth flow no longer stalls when no PIN is
present.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
(cherry picked from commit bc1460c)
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
|
The pull request was accepted by @ikerexxe with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
sssd-bot
force-pushed
the
passkey_local_fix_2_9_backport
branch
from
833924f to
24b3a4c
Compare
5 participants
Backport of #8185