Add support for multi-token management in SmartCardUtils by krishnavema · Pull Request #239 · SSSD/sssd-test-framework

krishnavema · 2026-03-15T15:39:21Z

No description provided.

gemini-code-assist · 2026-03-15T15:39:34Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 extends the SmartCardUtils class to support comprehensive multi-token management for SoftHSM. The changes enable the simulation of complex smart card environments with multiple tokens, which is crucial for testing scenarios that require distinct PKCS#11 slots. This enhancement improves the flexibility and realism of smart card related testing within the framework.

Highlights

Multi-token Initialization: Added a new method, initialize_additional_token, to allow for the creation of multiple SoftHSM tokens without removing existing ones, facilitating multi-token test environments.
Targeted Certificate/Key Addition: Enhanced the add_cert and add_key methods with token_label and label parameters, enabling the addition of certificates and private keys to specific PKCS#11 tokens.
p11-kit Integration: Introduced register_for_p11_child to configure SoftHSM as a system-wide p11-kit module, ensuring p11_child can discover and interact with multiple PKCS#11 slots.

🧠 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

sssd_test_framework/utils/smartcard.py
- Added initialize_additional_token method to support creating additional SoftHSM tokens.
- Modified add_cert method to accept token_label and label for targeted certificate addition.
- Modified add_key method to pass token_label and label to the underlying add_cert call.
- Added register_for_p11_child method to configure SoftHSM for system-wide p11-kit module visibility.

Activity

No human activity has been recorded on this pull request yet.

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.

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. ↩

gemini-code-assist

Code Review

This pull request adds support for managing multiple smart card tokens by introducing initialize_additional_token to create new tokens without wiping existing ones, and register_for_p11_child to make them visible to p11_child. The add_cert and add_key methods are also updated to allow targeting a specific token. The changes are logical, but I have a couple of suggestions to improve maintainability and security. I've identified some code duplication that could be refactored and an instance of overly permissive file permissions that should be tightened.

gemini-code-assist · 2026-03-15T15:40:32Z

+        args: CLIBuilderArgs = {
+            "label": (self.cli.option.VALUE, label),
+            "free": (self.cli.option.SWITCH, True),
+            "so-pin": (self.cli.option.VALUE, so_pin),
+            "pin": (self.cli.option.VALUE, user_pin),
+        }
+        self.host.conn.run(
+            self.cli.command("softhsm2-util --init-token", args), env={"SOFTHSM2_CONF": self.SOFTHSM2_CONF_PATH}
+        )


The logic for building arguments and calling softhsm2-util --init-token in this new method is identical to the one in the initialize_card method (lines 69-77). This code duplication should be avoided by extracting the common logic into a private helper method. This will improve code maintainability.

I could be missing something here but, I don't see any code duplication. I think this can be ignored unless someone sees the duplicate code.

gemini-code-assist · 2026-03-15T15:40:32Z

+            f"> /etc/systemd/system/sssd.service.d/softhsm.conf"
+        )
+        self.host.conn.run("systemctl daemon-reload")
+        self.host.conn.run("chmod -R o+rX /opt/test_ca/")


The command chmod -R o+rX /opt/test_ca/ grants read permissions to all files under /opt/test_ca/ to all users on the system, which is overly permissive. While this is a test environment, it's better to apply the principle of least privilege. A more precise command would only grant the necessary permissions, for example by only allowing traversal of the directory and read access to the configuration file.

Suggested change

self.host.conn.run("chmod -R o+rX /opt/test_ca/")

self.host.conn.run(f"chmod o+x /opt/test_ca && chmod o+r {conf}")

spoore1

I'll have more comments in the test PR but this is a start.

spoore1

I have a few questions and suggestions. Mostly changing some of the shell commands to fs calls for the file edits/manipulations.

spoore1 · 2026-04-22T20:46:04Z

        rc, _, _, _ = self.vfido_passkey_with_output(username=username, pin=pin, command=command)
        return rc == 0

+    def smartcard_with_su_output(self, username: str, pin: str, *, num_certs: int = 1) -> ProcessResult:


I would rename this to smartcard_with_output to better match the other functions in the class. And since we'll call it with client.auth.su.smartcard_with_output I don't think we need the "su" in the method name.

spoore1 · 2026-04-22T20:47:42Z

+        for attempt in range(USER_RESOLVABLE_ATTEMPTS):
+            time.sleep(USER_RESOLVABLE_INTERVAL_S)
+            check = self.host.conn.run(f"getent passwd {username}", raise_on_error=False)
+            if check.rc == 0:
+                break
+            if attempt == USER_RESOLVABLE_CACHE_EXPIRY_ATTEMPT:
+                self.host.conn.run("sss_cache -E", raise_on_error=False)
+        else:
+            raise AssertionError(
+                f"User '{username}' was not resolvable by SSSD after {USER_RESOLVABLE_ATTEMPTS} attempts"
+            )


Did you run into issues resolving the users when you wrote this? We haven't usually needed this type of check in the past. I don't have anything against the extra checks but, I'm wondering if they were added because of a particular issue we might need to fix elsewhere.

spoore1 · 2026-04-22T20:51:26Z

+                f"User '{username}' was not resolvable by SSSD after {USER_RESOLVABLE_ATTEMPTS} attempts"
+            )
+
+        su_input = f"1\n{pin}" if num_certs > 1 else pin


While this may work as expected with multiple tokens (I'm not 100% sure ATM), won't this force selection of the first certificate from a token that has multiple certificates? We might want an optional argument for certificate selection with 1 being the default.

spoore1 · 2026-04-22T20:52:04Z

+            raise_on_error=False,
+        )
+
+    def smartcard_with_su(self, username: str, pin: str, *, num_certs: int = 1) -> bool:


Like the other method, I would suggest renaming this one to just smartcard to closer match the other methods in the class.

spoore1 · 2026-04-22T20:58:02Z

+        args: CLIBuilderArgs = {
+            "label": (self.cli.option.VALUE, label),
+            "free": (self.cli.option.SWITCH, True),
+            "so-pin": (self.cli.option.VALUE, so_pin),
+            "pin": (self.cli.option.VALUE, user_pin),
+        }
+        self.host.conn.run(
+            self.cli.command("softhsm2-util --init-token", args), env={"SOFTHSM2_CONF": self.SOFTHSM2_CONF_PATH}
+        )


I could be missing something here but, I don't see any code duplication. I think this can be ignored unless someone sees the duplicate code.

spoore1 · 2026-04-22T21:20:21Z

+        smartcard.host.conn.run(f"grep -q 'slots.removable' {conf} || echo 'slots.removable = true' >> {conf}")
+        smartcard.host.conn.run(f"cp {conf} /etc/softhsm2.conf")


I believe we want to try to use python native solutions when possible over these types of file edits. I think you can do something like:

softhsm_conf = smartcard.fs.read(conf) if "slots.removable" not in softhsm_conf: smartcard.fs.append(conf, "\nslots.removable = true")

If you want to work in a temp file before copying to the real one, you could also make use of the fs.copy() method.

spoore1 · 2026-04-22T21:20:47Z

+
+        smartcard.host.conn.run(f"grep -q 'slots.removable' {conf} || echo 'slots.removable = true' >> {conf}")
+        smartcard.host.conn.run(f"cp {conf} /etc/softhsm2.conf")
+        smartcard.host.conn.run(f'echo "module: {module}" > /etc/pkcs11/modules/softhsm2.module')


Same thing here. Use the fs methods where possible.

spoore1 · 2026-04-22T21:21:24Z

+        smartcard.host.conn.run(
+            f'printf "[Service]\\nEnvironment=SOFTHSM2_CONF={conf}\\n" '
+            f"> /etc/systemd/system/sssd.service.d/softhsm.conf"


I think you can do this one with a simple fs.write()

spoore1 · 2026-04-22T21:23:48Z

+            f'printf "[Service]\\nEnvironment=SOFTHSM2_CONF={conf}\\n" '
+            f"> /etc/systemd/system/sssd.service.d/softhsm.conf"
+        )
+        smartcard.host.conn.run("systemctl daemon-reload")


Use smartcard.svc.reload_daemon() here I think.

spoore1 · 2026-04-22T21:24:40Z

+        smartcard.host.conn.run("chmod -R o+rX /opt/test_ca/")
+        smartcard.host.conn.run(f"chown -R sssd:sssd {token_storage}/ && chmod -R 770 {token_storage}/")


I think you should be able to use fs.chown() and fs.chmod() here.

krishnavema requested a review from spoore1 March 15, 2026 15:39

gemini-code-assist Bot reviewed Mar 15, 2026

View reviewed changes

krishnavema requested a review from sumit-bose March 17, 2026 07:51

krishnavema assigned sumit-bose and spoore1 Mar 17, 2026

spoore1 requested changes Mar 18, 2026

View reviewed changes

Comment thread sssd_test_framework/utils/smartcard.py Outdated

Comment thread sssd_test_framework/utils/smartcard.py Outdated

Add support for multi-token management in SmartCardUtils

4823cbb

krishnavema force-pushed the multi-token-smart-card-support branch from bcdf5e8 to 4823cbb Compare March 20, 2026 07:58

krishnavema marked this pull request as ready for review March 20, 2026 07:59

krishnavema requested a review from spoore1 March 20, 2026 08:04

jakub-vavra-cz mentioned this pull request Apr 22, 2026

Extend framework for sudo use-cases #242

Open

spoore1 requested changes Apr 22, 2026

View reviewed changes

spoore1 mentioned this pull request Apr 22, 2026

Smartcard multi token tests SSSD/sssd#8519

Draft

	self.host.conn.run("chmod -R o+rX /opt/test_ca/")
	self.host.conn.run(f"chmod o+x /opt/test_ca && chmod o+r {conf}")

		smartcard.host.conn.run(f"grep -q 'slots.removable' {conf} \|\| echo 'slots.removable = true' >> {conf}")
		smartcard.host.conn.run(f"cp {conf} /etc/softhsm2.conf")

		smartcard.host.conn.run("chmod -R o+rX /opt/test_ca/")
		smartcard.host.conn.run(f"chown -R sssd:sssd {token_storage}/ && chmod -R 770 {token_storage}/")

Conversation

krishnavema commented Mar 15, 2026

Uh oh!

gemini-code-assist Bot commented Mar 15, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants