Skip to content

Add AD certificate authentication test - #8453

Open
krishnavema wants to merge 1 commit into
SSSD:masterfrom
krishnavema:test_ad_certificate_authority
Open

krishnavema wants to merge 1 commit into
SSSD:masterfrom
krishnavema:test_ad_certificate_authority

Conversation

@krishnavema

Copy link
Copy Markdown
Contributor

No description provided.

@krishnavema
krishnavema requested a review from spoore1 February 16, 2026 16:11

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

Copy link
Copy Markdown

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 system test for AD certificate authentication using a virtual smartcard. The changes include updating a test dependency and adding the new test case with a helper function for certificate enrollment. The overall approach is sound, but I've identified one area for improvement to increase test reliability by replacing a fixed sleep with a more robust polling mechanism.

Comment thread src/tests/system/tests/test_ad.py Outdated
client.sssd.restart()

client.svc.restart("virt_cacard.service")
time.sleep(5)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using a fixed time.sleep(5) can lead to flaky tests. If the virt_cacard.service takes longer to start under load, the test might fail. Conversely, if the service starts quickly, the test is unnecessarily delayed. A more robust approach is to poll for the service's status until it becomes active.

Suggested change
time.sleep(5)
for _ in range(10):
result = client.host.conn.run("systemctl is-active --quiet virt_cacard.service", raise_on_error=False)
if result.rc == 0:
break
time.sleep(1)
else:
pytest.fail("virt_cacard.service did not become active in time.")

Comment thread src/tests/system/tests/test_ad.py Fixed
Comment thread src/tests/system/tests/test_ad.py Outdated
Comment thread src/tests/system/tests/test_ad.py Outdated
Comment thread src/tests/system/tests/test_ad.py
@krishnavema
krishnavema force-pushed the test_ad_certificate_authority branch from f9ad7bf to 2888874 Compare October 9, 2026 10:37
@krishnavema

Copy link
Copy Markdown
Contributor Author

Comment thread src/tests/system/tests/test_ad.py
@krishnavema
krishnavema force-pushed the test_ad_certificate_authority branch from 2888874 to 40ef7aa Compare October 12, 2026 13:37
@sumit-bose

Copy link
Copy Markdown
Contributor

Hi,

please rebase to get Pavel's "test: fix missing cmocka prerequisite includes in test_nfsidmap.c" included to make the CI builds pass.

bye,
Sumit

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants