Skip to content

Add AD certificate authentication test#8453

Draft
krishnavema wants to merge 1 commit intoSSSD:masterfrom
krishnavema:test_ad_certificate_authority
Draft

Add AD certificate authentication test#8453
krishnavema wants to merge 1 commit intoSSSD:masterfrom
krishnavema:test_ad_certificate_authority

Conversation

@krishnavema
Copy link
Contributor

No description provided.

@krishnavema krishnavema requested a review from spoore1 February 16, 2026 16:11
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 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.

client.sssd.restart()

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

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.")

username = "adcertuser1"
try:
ad.user(username).delete()
except Exception:

Check notice

Code scanning / CodeQL

Empty except Note test

'except' clause does nothing but pass and there is no explanatory comment.
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.

1 participant