Tests:Added IPA Certificate Authority Tests by krishnavema · Pull Request #8159 · SSSD/sssd

krishnavema · 2025-10-27T17:51:20Z

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.

gemini-code-assist

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.

src/tests/system/tests/test_ipa.py

danlavu

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

src/tests/system/tests/test_ipa.py

danlavu

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

danlavu

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.

thalman

LGTM, thanks Vema

src/tests/system/tests/test_ipa.py

sumit-bose

Hi,

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

bye,
Sumit

sssd-bot · 2025-11-06T11:21:35Z

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

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

sumit-bose · 2025-11-07T10:09:23Z

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 · 2025-11-10T20:53:15Z

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

spoore1

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

src/tests/system/tests/test_ipa.py

Reviewed-by: Dan Lavu <dlavu@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>

spoore1

LGTM

krishnavema changed the title ~~tests:Added IPA Certificate Authority Tests~~ Tests:Added IPA Certificate Authority Tests Oct 27, 2025

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

src/tests/system/tests/test_ipa.py Outdated Show resolved Hide resolved

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

danlavu requested changes Oct 30, 2025

View reviewed changes

src/tests/system/tests/test_ipa.py Outdated Show resolved Hide resolved

src/tests/system/tests/test_ipa.py Outdated Show resolved Hide resolved

src/tests/system/tests/test_ipa.py Show resolved Hide resolved

src/tests/system/tests/test_ipa.py Outdated Show resolved Hide resolved

krishnavema force-pushed the ipa-certificate-authority-test branch from f260683 to d8ebff0 Compare October 31, 2025 10:22

krishnavema requested a review from danlavu October 31, 2025 16:16

danlavu reviewed Nov 1, 2025

View reviewed changes

danlavu self-requested a review November 1, 2025 02:11

danlavu self-assigned this Nov 1, 2025

danlavu approved these changes Nov 1, 2025

View reviewed changes

danlavu added the Waiting for review label Nov 1, 2025

krishnavema force-pushed the ipa-certificate-authority-test branch from d8ebff0 to 2a38ec0 Compare November 1, 2025 05:05

krishnavema requested a review from sumit-bose November 3, 2025 13:33

krishnavema assigned sumit-bose Nov 3, 2025

krishnavema added the Tests label Nov 4, 2025

thalman approved these changes Nov 4, 2025

View reviewed changes

sumit-bose reviewed Nov 4, 2025

View reviewed changes

src/tests/system/tests/test_ipa.py Outdated Show resolved Hide resolved

krishnavema force-pushed the ipa-certificate-authority-test branch 2 times, most recently from bcf380a to 8714ac2 Compare November 4, 2025 14:04

pbrezina force-pushed the master branch 3 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27

krishnavema requested a review from sumit-bose November 4, 2025 14:45

krishnavema force-pushed the ipa-certificate-authority-test branch from 8714ac2 to bbedf8b Compare November 4, 2025 16:40

sumit-bose approved these changes Nov 5, 2025

View reviewed changes

krishnavema added the Accepted label Nov 5, 2025

sssd-bot force-pushed the ipa-certificate-authority-test branch from bbedf8b to e564d17 Compare November 5, 2025 14:34

krishnavema removed the Accepted label Nov 5, 2025

SSSD deleted a comment from sssd-bot Nov 5, 2025

sumit-bose removed the Waiting for review label Nov 6, 2025

sumit-bose added the Accepted label Nov 6, 2025

sssd-bot force-pushed the ipa-certificate-authority-test branch from e564d17 to d4e858d Compare November 6, 2025 11:21

spoore1 added backport-to-sssd-2-9 backport-to-sssd-2-10 backport-to-sssd-2-11 labels Nov 6, 2025

sumit-bose mentioned this pull request Nov 7, 2025

passwordless-GDM: smartcard and passkey ikerexxe/sssd#12

Merged

krishnavema force-pushed the ipa-certificate-authority-test branch from d4e858d to aacaefd Compare November 7, 2025 11:53

spoore1 reviewed Nov 11, 2025

View reviewed changes

src/tests/system/tests/test_ipa.py Show resolved Hide resolved

tests:Added IPA Certificate Authority Tests

7fb3721

Reviewed-by: Dan Lavu <dlavu@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>

krishnavema force-pushed the ipa-certificate-authority-test branch from aacaefd to 7fb3721 Compare November 12, 2025 16:22

spoore1 approved these changes Nov 12, 2025

View reviewed changes

krishnavema merged commit 0d2b75f into master Nov 13, 2025
29 checks passed

This was referenced Nov 13, 2025

[autobackport: sssd-2-9] Tests:Added IPA Certificate Authority Tests #8186

Closed

[autobackport: sssd-2-10] Tests:Added IPA Certificate Authority Tests #8187

Merged

[autobackport: sssd-2-11] Tests:Added IPA Certificate Authority Tests #8188

Merged

krishnavema deleted the ipa-certificate-authority-test branch December 2, 2025 01:26

Conversation

krishnavema commented Oct 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

thalman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sumit-bose left a comment

Choose a reason for hiding this comment

Uh oh!

sssd-bot commented Nov 6, 2025

Uh oh!

sumit-bose commented Nov 7, 2025

Uh oh!

danlavu commented Nov 10, 2025

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants