Skip to content

Conversation

@krishnavema
Copy link
Contributor

The system should prompt the user for a PIN when attempting to authenticate via smart card. This ensures that the smart card is being used as the primary authentication method.

@krishnavema krishnavema changed the title tests:Added IPA Certificate Authority Tests Tests:Added IPA Certificate Authority Tests Oct 27, 2025
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 a new test case to verify smart card authentication with nested 'su' commands for IPA users. The test sets up an IPA user, requests a smart card certificate, configures SSSD for smart card authentication, and then executes nested 'su' commands to validate the authentication flow. The review focuses on ensuring the test case is reliable and correctly validates the intended functionality.

@krishnavema krishnavema requested a review from danlavu October 27, 2025 17:53
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

A few minor details, but overall, it looks very good. Thankyou.

@krishnavema krishnavema force-pushed the ipa-certificate-authority-test branch from f260683 to d8ebff0 Compare October 31, 2025 10:22
@krishnavema krishnavema requested a review from danlavu October 31, 2025 16:16
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

This looks great, thank you. One comment, nothing related to the test code.

@danlavu danlavu self-requested a review November 1, 2025 02:11
@danlavu danlavu self-assigned this Nov 1, 2025
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

This looks great, thank you. I'm going to approve this since I'm on PTO this week, but you need to add pytest.mark.importance. Critical or High will run with each PRCI run, I think this test case is important enough to execute with each PR.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Vema

@krishnavema krishnavema force-pushed the ipa-certificate-authority-test branch 2 times, most recently from bcf380a to 8714ac2 Compare November 4, 2025 14:04
@pbrezina pbrezina force-pushed the master branch 3 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27
@krishnavema krishnavema force-pushed the ipa-certificate-authority-test branch from 8714ac2 to bbedf8b Compare November 4, 2025 16:40
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the changes, test is still working fine and I have no further comments, ACK.

bye,
Sumit

@sssd-bot sssd-bot force-pushed the ipa-certificate-authority-test branch from bbedf8b to e564d17 Compare November 5, 2025 14:34
@SSSD SSSD deleted a comment from sssd-bot Nov 5, 2025
@sssd-bot
Copy link

sssd-bot commented Nov 6, 2025

The pull request was accepted by @sumit-bose with the following PR CI status:


🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / All tests are successful (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / All tests are successful (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / All tests are successful (success)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sumit-bose
Copy link
Contributor

Hi,

I came a across a race-condition. It looks like after re-starting virt_cacard.service the certificates might not be available in the virtual Smartcard immediately which makes the test fail. Adding a time.sleep(1) after the restart makes it pass again.

Not sure if a sleep is the best fix here, maybe checking with pkcs11-tool from the OpenSC package or some other utilities before running the test would be better?

Since the test always passed in the PR CI it might be possible to delay fixing this and merge the patch as is. But this might be risky and I would suggest to at least add the sleep. What do you think?

bye,
Sumit

@danlavu
Copy link

danlavu commented Nov 10, 2025

Not sure if a sleep is the best fix here, maybe checking with pkcs11-tool from the OpenSC package or some other utilities before running the test would be better?

This can probably use wait_for_condition() ?

            @pytest.mark.topology(KnownTopology.LDAP)
            def test_just_condition(client: Client):
                client.sssd.domain["ldap_uri"] = "ldap://typo"
                client.sssd.start(debug_level=None, raise_on_error=False)

                assert client.sssd.default_domain
                r = client.tools.wait_for_condition(condition=f"sssctl domain-status {client.sssd.default_domain}")
                assert r.rc == 0
                assert "LDAP: not connected" in r.stdout

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.

One minor update to add a builtwith() marker. This can be added at a later time if this PR is merged before updated.

Reviewed-by: Dan Lavu <dlavu@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
@krishnavema krishnavema force-pushed the ipa-certificate-authority-test branch from aacaefd to 7fb3721 Compare November 12, 2025 16:22
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.

LGTM

@krishnavema krishnavema merged commit 0d2b75f into master Nov 13, 2025
29 checks passed
@krishnavema krishnavema deleted the ipa-certificate-authority-test branch December 2, 2025 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants