Skip to content

Conversation

@spoore1
Copy link
Contributor

@spoore1 spoore1 commented Oct 18, 2025

No description provided.

@gemini-code-assist
Copy link

Summary of Changes

Hello @spoore1, 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 significantly expands the sssd_test_framework's capabilities by introducing robust support for testing virtual passkey functionality. It establishes a dedicated utility for programmatic control over virtual FIDO devices, seamlessly integrates this control into existing client and IPA roles, and provides advanced authentication methods to simulate diverse virtual passkey login scenarios. This foundational work enables automated and thorough validation of SSSD's interaction with passkey-based authentication mechanisms.

Highlights

  • Virtual FIDO Utility: Introduced a new Vfido utility class to manage virtual passkey devices and services, including starting, stopping, resetting, and simulating device touches.
  • Client Role Integration: Integrated the Vfido utility into the Client role, allowing test clients to interact with virtual passkey devices.
  • IPA Passkey Registration: Added a new method vfido_passkey_add_register to the IPAUser role, enabling the registration of virtual passkeys for users within IPA.
  • Authentication Utilities: Enhanced AuthenticationUtils with vfido_passkey_with_output and vfido_passkey methods to facilitate comprehensive testing of virtual passkey authentication flows, including PIN handling and interactive prompts.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

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

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 adds support for testing virtual passkey (vfido) functionality. It introduces a new Vfido utility class, and adds methods to IPAUser and SUAuthenticationUtils to handle passkey registration and authentication in tests. The changes are a good step forward, but I've found a few issues. There's a critical bug in vfido.py where pin_disable calls the wrong command, and another critical bug in authentication.py with incorrect match/case syntax that will prevent PIN validation from working. I've also left several medium-severity comments regarding hardcoded paths, magic strings, and inconsistencies in docstrings and messages. Please address these points to improve the robustness and maintainability of the new test utilities.

@spoore1 spoore1 force-pushed the gdm_passkey branch 3 times, most recently from 8c82ea5 to 5f65046 Compare October 21, 2025 15:48
@ikerexxe ikerexxe self-requested a review October 29, 2025 10:28
@ikerexxe ikerexxe self-assigned this Oct 29, 2025
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Some minor comments inline.

Apart from that I have one question, do we need any kernel change for vfido to run?

self._exec("remove-passkey", [passkey_mapping])
return self

def vfido_passkey_add_register(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that the method begins with vfido to differentiate it from the umockdev version. I was to propose to use method overloading, but apparently this functionality isn't available in python 🤦

Can we manage this somehow so that the method is called the same way for vfido and umockdev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into that closer. I did this here to be able to keep them separate while I was writing and testing the new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I made some changes to use the existing method names as wrappers to determine which function to run and just pass kwargs. Seems fairly straightforward but, I need to run more tests to make sure it's good with existing test_passkey.py tests.

@spoore1
Copy link
Contributor Author

spoore1 commented Oct 30, 2025

Some minor comments inline.

Apart from that I have one question, do we need any kernel change for vfido to run?

It does require the vhci-hcd kernel module. That will be taken care of in the sssd-ci-containers setup code here:
SSSD/sssd-ci-containers#151

I've updated the systemd unit file to modprobe that before starting the service. If it's not in the PR yet, it will be soon since I've updated my local repo already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants