[autobackport: sssd-2-10] Tests:Added IPA Certificate Authority Tests #8187
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new system test for smart card authentication with a nested su command. The changes look good overall, but I have a couple of suggestions to improve the robustness and security of the new test. Specifically, I'm pointing out a hardcoded PIN and the use of time.sleep(), which can lead to flaky tests.
| client.sssd.pam["pam_cert_auth"] = "True" | ||
| client.sssd.start() | ||
| client.svc.restart("virt_cacard.service") | ||
| time.sleep(1) |
There was a problem hiding this comment.
Using a fixed time.sleep(1) can lead to flaky tests. The test might fail if the service takes longer than 1 second to restart, or it might wait unnecessarily if the service restarts faster. It's more robust to poll for the service status in a loop with a timeout to ensure it's active before proceeding.
| time.sleep(1) | |
| for _ in range(10): | |
| result = client.host.conn.run("systemctl is-active virt_cacard.service") | |
| if result.rc == 0: | |
| break | |
| time.sleep(1) | |
| else: | |
| pytest.fail("virt_cacard.service did not become active in time.") |
| client.svc.restart("virt_cacard.service") | ||
| time.sleep(1) | ||
|
|
||
| result = client.host.conn.run("su - ipacertuser1 -c 'su - ipacertuser1 -c whoami'", input="123456") |
There was a problem hiding this comment.
Hardcoding the smartcard PIN '123456' directly in the test logic is a security risk and a bad practice, even in tests. It's better to define it as a constant at the module level (e.g., SMARTCARD_PIN = "123456") and use the constant here. This makes it easier to manage and change if needed, and improves code readability.
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
Approving as is to match the original.
Reviewed-by: Dan Lavu <dlavu@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com> (cherry picked from commit 7fb3721) Reviewed-by: Scott Poore <spoore@redhat.com>
|
The pull request was accepted by @alexey-tikhonov with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
sssd-bot
force-pushed
the
SSSD-sssd-backport-pr8159-to-sssd-2-10
branch
from
2347852 to
7837345
Compare
6 participants
This is an automatic backport of PR#8159 Tests:Added IPA Certificate Authority Tests to branch sssd-2-10, created by @krishnavema.
Please make sure this backport is correct.
Note
The commits were cherry-picked without conflicts.
You can push changes to this pull request
Original commits
Backported commits
Original Pull Request Body
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.