Passkey local fix and improvements #8185
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 introduces several fixes and improvements for passkey authentication. The most significant change is the removal of server-side configuration for passkey user verification, simplifying the logic to rely solely on client-side settings. This is a major refactoring that removes a considerable amount of code. Additionally, the pull request includes several important bug fixes, such as preventing a potential hang in the PAM responder, fixing a buffer over-read in authtok handling, and avoiding unnecessary PIN prompts. The changes are well-reasoned and improve the robustness of the passkey authentication feature. I have not found any high or critical severity issues in this pull request.
justin-stephenson
added
backport-to-sssd-2-8
backport-to-sssd-2-9
backport-to-sssd-2-10
backport-to-sssd-2-9-4
|
Please remove |
|
A set of 'backport-to' labels is questionable, imo. |
justin-stephenson
removed
backport-to-sssd-2-8
backport-to-sssd-2-9-4
justin-stephenson
force-pushed
the
passkey_misc_improvements
branch
from
c838a72 to
028f657
Compare
Done. |
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I have one question/concern with commit passkey: Remove SYSDB_PASSKEY_USER_VERIFICATION. In the passkey kerberos case, the user-verification is still used but instead of reading it from the IPA server we now read it from ipa-otpd. Is my understanding correct?
That's correct. It is sent to the SSSD krb5 passkey plugin from ipa-otpd https://github.com/freeipa/freeipa/blob/master/daemons/ipa-otpd/passkey.c#L43 |
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 <[email protected]>
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.
Local auth functions should only be reached in AD/LDAP auth flows.
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.
justin-stephenson
force-pushed
the
passkey_misc_improvements
branch
from
028f657 to
c06768a
Compare
4 participants
This fixes local passkey auth (LDAP/AD) on the command-line when no PIN is set, it also addresses some improvements and small fixes that were found during recent passkey testing