Skip to content

Smartcard multi token tests#8519

Draft
krishnavema wants to merge 2 commits intoSSSD:masterfrom
krishnavema:smartcard-multi-token-tests
Draft

Smartcard multi token tests#8519
krishnavema wants to merge 2 commits intoSSSD:masterfrom
krishnavema:smartcard-multi-token-tests

Conversation

@krishnavema
Copy link
Contributor

No description provided.

@krishnavema krishnavema requested a review from spoore1 March 15, 2026 16:12
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 introduces system tests for multi-token smartcard authentication. It includes new helper functions for setting up tokens and authenticating, along with several test cases covering different scenarios with two tokens. A key change is updating the sssd-test-framework dependency to a personal fork to support these new tests. My review focuses on the risk associated with this dependency and on improving the maintainability and robustness of the new test code by addressing magic numbers and polling logic.

git+https://github.com/next-actions/pytest-tier
git+https://github.com/next-actions/pytest-output
git+https://github.com/SSSD/sssd-test-framework
git+https://github.com/krishnavema/sssd-test-framework@multi-token-smart-card-support

Choose a reason for hiding this comment

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

critical

This change introduces a dependency on a personal fork (krishnavema/sssd-test-framework). While this might be acceptable for development, it poses a security and maintenance risk for the main branch. The changes from this fork should be merged into the upstream SSSD/sssd-test-framework repository, and the dependency should point to an official release or commit from the upstream repository before this pull request is merged.

git+https://github.com/SSSD/sssd-test-framework

Comment on lines +16 to +18
TOKEN1_LABEL = "SC_Token_1"
TOKEN2_LABEL = "SC_Token_2"
TOKEN_PIN = "123456"

Choose a reason for hiding this comment

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

high

To improve readability and maintainability of the polling loop in authenticate_with_smartcard, it's good practice to define the magic numbers used there as constants here.

TOKEN1_LABEL = "SC_Token_1"
TOKEN2_LABEL = "SC_Token_2"
TOKEN_PIN = "123456"

# Constants for the user resolvability polling loop
USER_RESOLVABLE_ATTEMPTS = 15
USER_RESOLVABLE_INTERVAL_S = 2
USER_RESOLVABLE_CACHE_EXPIRY_ATTEMPT = 3

Comment on lines +114 to +120
for attempt in range(15):
time.sleep(2)
cached = client.tools.getent.passwd(username)
if cached is not None:
break
if attempt == 3:
client.host.conn.run("sss_cache -E", raise_on_error=False)

Choose a reason for hiding this comment

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

high

As suggested in another comment, please use the newly defined constants for the polling loop parameters. This will make the code more readable and easier to maintain.

Additionally, using a fixed time.sleep() can make tests either slow or flaky. For future improvements, consider creating a more robust waiting utility that polls for a condition with a timeout instead of a fixed sleep.

    for attempt in range(USER_RESOLVABLE_ATTEMPTS):
        time.sleep(USER_RESOLVABLE_INTERVAL_S)
        cached = client.tools.getent.passwd(username)
        if cached is not None:
            break
        if attempt == USER_RESOLVABLE_CACHE_EXPIRY_ATTEMPT:
            client.host.conn.run("sss_cache -E", raise_on_error=False)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this to the SUAuthenticationUtils, it may make sense to follow this suggestion there. If we have reasonable settings we could use globally, you could add globals to misc/globals.py.

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

This is a good start. I've run into a couple snags with testing but basics are working so here's a start for review.

enroll_to_token(client, ipa, token2_username, token_label=TOKEN2_LABEL)


def configure_sssd_for_smartcard(client: Client) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be handled in a new method in the SSSDCommonConfiguration class in the sssd util module. Then you can call something like this in the test:

client.sssd.common.smartcard_with_softhsm()

client.sssd.start()


def authenticate_with_smartcard(client: Client, username: str, pin: str, *, num_certs: int = 1) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can move to the authentication utils module in the SUAuthenticationUtils class. Maybe something like smartcard_with_su? you should probably also consider following existing conventions there of having one basic method for simple assertions (like password()) and one that returns output for use within the test case if needed (like password_with_output()).

Comment on lines +114 to +120
for attempt in range(15):
time.sleep(2)
cached = client.tools.getent.passwd(username)
if cached is not None:
break
if attempt == 3:
client.host.conn.run("sss_cache -E", raise_on_error=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this to the SUAuthenticationUtils, it may make sense to follow this suggestion there. If we have reasonable settings we could use globally, you could add globals to misc/globals.py.

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.

3 participants