Passkey local fix and improvements#8185
Conversation
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.
|
Please remove |
|
A set of 'backport-to' labels is questionable, imo. |
c838a72 to
028f657
Compare
Done. |
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 |
028f657 to
c06768a
Compare
c06768a to
7a46611
Compare
|
I rebased to include #8212 - it was a clean rebase. |
ikerexxe
left a comment
There was a problem hiding this comment.
LGTM! Thank you for fixing this
|
@alexey-tikhonov why did you remove sssd-2-9 backport label? This is a fix for passkey issue |
There would be a conflict in sssd-2-9 because #8212 was merged only in 'master'. |
|
Note: Covscan is green. |
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>
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>
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>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
7a46611 to
9a1841e
Compare
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