Replace umockdev with vfido for passkey testing#237
Replace umockdev with vfido for passkey testing#237ikerexxe wants to merge 6 commits intoSSSD:masterfrom
Conversation
Update `vfido_passkey_add_register()` to return the passkey mapping, just as `umockdev_passkey_add_register()` does Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Fix authentication routing bug that prevented vfido from being used correctly Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
PASSKEY_FALLBACK_TO_PASSWORD requires a PIN, even though this might be an empty or wrong PIN. Fix this by grouping PASSKEY_FALLBACK_TO_PASSWORD with PIN requiring auth methods. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Previous implementation for the bash code was a little bit confusing and didn't make much sense from the high-level perspective. Refactored the function to follow phases, some of them are mandatory for all methods, other are only required for some of the methods. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
`vfido` feature needs `vhci_hcd` kernel module to be loaded to work, thus adding a check there Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The virtual FIDO2 authenticator has been replaced by vfido, thus update the documentation Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates the passkey testing infrastructure to use Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 migrates the passkey testing infrastructure from umockdev to vfido, which is a more modern and robust solution. The changes are well-structured, covering documentation, feature detection, and the authentication utilities themselves. I've identified a few areas for improvement, mainly around documentation consistency and simplifying some of the expect scripts for better readability and maintenance.
| time.sleep(1) | ||
|
|
||
| return result.stdout_lines[-1].strip() | ||
| return result.stdout_lines[-2].strip() |
There was a problem hiding this comment.
The output of the ipa command includes a summary line after the passkey mapping. Accessing the second to last line (-2) is fragile. A more robust approach would be to iterate through the output lines and find the one that starts with Passkey mapping:. This avoids issues if the ipa command's output format changes in the future.
for line in result.stdout_lines:
if line.strip().startswith("Passkey mapping:"):
return line.strip()
raise AssertionError("Passkey mapping not found in ipa command output")| ) -> tuple[int, int, str, str]: | ||
| """wrapper for passkey_with_output methods""" | ||
| if "virt_type" in kwargs and kwargs["virt_type"] == "vfido": | ||
| del kwargs["virt_type"] |
There was a problem hiding this comment.
Deleting virt_type from kwargs modifies the dictionary that was passed in by the caller. This can lead to unexpected side effects if the caller reuses the kwargs dictionary. It's safer to create a copy of the dictionary before modifying it.
| del kwargs["virt_type"] | |
| kwargs_copy = kwargs.copy() | |
| del kwargs_copy["virt_type"] |
| """wrapper for passkey_with_output methods""" | ||
| if "virt_type" in kwargs and kwargs["virt_type"] == "vfido": | ||
| del kwargs["virt_type"] | ||
| return self.vfido_passkey_with_output(**kwargs) |
There was a problem hiding this comment.
To avoid the side effect of modifying the original kwargs dictionary, you should pass the modified copy to the vfido_passkey_with_output method.
| return self.vfido_passkey_with_output(**kwargs) | |
| return self.vfido_passkey_with_output(**kwargs_copy) |
| del kwargs["virt_type"] | ||
| return self.vfido_passkey(**kwargs) |
There was a problem hiding this comment.
Similar to the passkey_with_output method, deleting virt_type from kwargs here can cause unexpected side effects. It's better to work with a copy of the dictionary to ensure the caller's dictionary remains unchanged.
| del kwargs["virt_type"] | |
| return self.vfido_passkey(**kwargs) | |
| kwargs_copy = kwargs.copy() | |
| del kwargs_copy["virt_type"] | |
| return self.vfido_passkey(**kwargs_copy) |
| sudo modprobe vhci-hcd | ||
|
|
||
| # Verify the module is loaded | ||
| lsmod | grep vhci |
There was a problem hiding this comment.
For consistency with the feature detection logic in sssd_test_framework/hosts/client.py, it would be better to grep for vhci_hcd instead of just vhci. This makes the verification step more specific and aligned with the actual kernel module name.
| lsmod | grep vhci | |
| lsmod | grep vhci_hcd |
| expect {{ | ||
| -i $ID_su -re "Authentication failure" {{exitmsg "Authentication failure" 1}} | ||
| -i $ID_su eof {{exitmsg "Passkey authentication successful" 0}} | ||
| -i $ID_su timeout {{exitmsg "Unexpected output" 213}} | ||
| -i $ID_touch eof {{}} | ||
| -i $ID_touch timeout {{}} | ||
| }} |
There was a problem hiding this comment.
The expect block for ID_touch can be simplified. Since you're just waiting for the vfido_touch process to finish and don't care about its output, a single expect eof is sufficient. This makes the script slightly cleaner.
| expect {{ | |
| -i $ID_su -re "Authentication failure" {{exitmsg "Authentication failure" 1}} | |
| -i $ID_su eof {{exitmsg "Passkey authentication successful" 0}} | |
| -i $ID_su timeout {{exitmsg "Unexpected output" 213}} | |
| -i $ID_touch eof {{}} | |
| -i $ID_touch timeout {{}} | |
| }} | |
| expect -i $ID_touch eof |
1 participant
Migrate the passkey testing infrastructure from
umockdevtovfido, providing a more robust and reliable solution for passkey authentication testing. The migration includes updating client feature detection to supportvfido's requirements, refactoring authentication utilities for improved integration, and updating the documentation to reflect the newvfidousage patterns. These changes enhance the overall passkey testing experience while maintaining backward compatibility where applicable.