Skip to content

Passkey local fix and improvements - backport sssd-2-9#8276

Merged
ikerexxe merged 5 commits into
SSSD:sssd-2-9from
justin-stephenson:passkey_local_fix_2_9_backport
Dec 10, 2025
Merged

Passkey local fix and improvements - backport sssd-2-9#8276
ikerexxe merged 5 commits into
SSSD:sssd-2-9from
justin-stephenson:passkey_local_fix_2_9_backport

Conversation

@justin-stephenson

Copy link
Copy Markdown
Contributor

Backport of #8185

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/util/authtok.c
Comment thread src/util/authtok.c
Comment thread src/responder/pam/pamsrv_cmd.c
@alexey-tikhonov

Copy link
Copy Markdown
Member

FAILED tests/test_access_control_simple.py::test_access_control_simple__permits_user_login_based_on_group (samba) is a known issue.

@alexey-tikhonov alexey-tikhonov added no-backport This should go to target branch only. Waiting for review labels Dec 8, 2025
@sssd-bot

sssd-bot commented Dec 8, 2025

Copy link
Copy Markdown
Contributor

The pull request was accepted by @thalman with the following PR CI status:


🟢 CodeQL (success)
🟢 rpm-build:centos-stream-9-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / make-distcheck (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-9) (failure)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot
sssd-bot force-pushed the passkey_local_fix_2_9_backport branch from a527568 to fd72ce7 Compare December 8, 2025 14:13
@thalman thalman added the Blocked label Dec 8, 2025
@alexey-tikhonov

Copy link
Copy Markdown
Member

Because of #8185 (comment) I would either
(1) expect one additional patch
or
(2) the same dependency.

Personally would prefer (1)

@ikerexxe

ikerexxe commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

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

@justin-stephenson

Copy link
Copy Markdown
Contributor Author

I added the commit 3b8eada

@alexey-tikhonov

Copy link
Copy Markdown
Member

I added the commit 3b8eada

Looks like cherry picked from commit references a wrong hash?

@alexey-tikhonov

Copy link
Copy Markdown
Member

I added the commit 3b8eada

Looks like cherry picked from commit references a wrong hash?

Sorry, I misread the comment, disregard.

@justin-stephenson

Copy link
Copy Markdown
Contributor Author

@ikerexxe Please check latest changes (newly added commit) for approval

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

justin-stephenson and others added 5 commits December 10, 2025 12:20
Reviewed-by: Iker Pedrosa <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
(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 <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
(cherry picked from commit 879d073)
Local auth functions should only be reached in AD/LDAP auth flows.

Reviewed-by: Iker Pedrosa <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
(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 <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
(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 <[email protected]>
Reviewed-by: Justin Stephenson <[email protected]>
Reviewed-by: Sumit Bose <[email protected]>
(cherry picked from commit bc1460c)
Reviewed-by: Iker Pedrosa <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@sssd-bot

Copy link
Copy Markdown
Contributor

The pull request was accepted by @ikerexxe with the following PR CI status:


🟢 CodeQL (success)
🟢 rpm-build:centos-stream-9-x86_64:upstream (success)
🟢 Build / make-distcheck (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-9) (failure)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot
sssd-bot force-pushed the passkey_local_fix_2_9_backport branch from 833924f to 24b3a4c Compare December 10, 2025 12:20
@ikerexxe
ikerexxe merged commit 4c6c010 into SSSD:sssd-2-9 Dec 10, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accepted Blocked no-backport This should go to target branch only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants