tests: port LDAP+Kerberos tests to pytest by madhuriupadhye · Pull Request #8544 · SSSD/sssd

madhuriupadhye · 2026-03-23T11:07:03Z

Port three downstream krb_misc_bugzilla bash tests to upstream pytest
BZ 773660: Clock skew errors logged to syslog
BZ 869150: ldap_child handles missing keytab without segfault
BZ 805281: Correct principal selected from multi-realm keytab

gemini-code-assist

Code Review

This pull request ports three bash tests for LDAP+Kerberos to pytest. The new tests are well-structured and cover important scenarios like clock skew handling, missing keytabs, and multi-realm principal selection. My review focuses on improving the robustness of the clock skew test by removing a hardcoded external dependency, which will make the test more reliable.

gemini-code-assist · 2026-03-23T11:08:48Z

src/tests/system/tests/test_ldap_krb.py

+    client.host.conn.run(
+        "systemctl stop chronyd 2>/dev/null; chronyd -q 'server clock.redhat.com iburst' 2>/dev/null; systemctl start chronyd 2>/dev/null || true",
+        raise_on_error=False,
+    )


The command to sync NTP is overly complex, suppresses errors, and relies on a hardcoded external server (clock.redhat.com). This can make the test flaky and hard to debug. A more robust approach is to use chronyc to trigger an immediate clock step with the configured NTP servers.

# Sync chrony to NTP result = client.host.conn.run("chronyc -a makestep", raise_on_error=False) assert "200 OK" in result.stdout or result.rc == 0, f"chrony makestep failed: {result.stderr}!"

src/tests/system/tests/test_ldap_krb.py

madhuriupadhye · 2026-03-23T15:31:02Z

/gemini review

gemini-code-assist

Code Review

This pull request ports three LDAP+Kerberos tests from bash to pytest, which is a great improvement for maintainability. The new tests are well-structured and cover important bug-fix scenarios.

My review has identified a few issues that should be addressed:

A critical issue in requirements.txt where a dependency points to a personal fork. This must be changed to an official repository before merging.
The tests have some reliability concerns. One test depends on a hardcoded external NTP server, which can cause flakiness.
Two tests lack robust resource cleanup (for keytab files). If a test fails, it could leave the system in a modified state and affect subsequent test runs. I've suggested using try...finally blocks to ensure cleanup always happens.

Addressing these points will improve the reliability and maintainability of the new tests.

gemini-code-assist · 2026-03-23T15:36:42Z

src/tests/system/requirements.txt

 git+https://github.com/next-actions/pytest-tier
 git+https://github.com/next-actions/pytest-output
-git+https://github.com/SSSD/sssd-test-framework
+git+https://github.com/madhuriupadhye/sssd-test-framework@krb_misc


The sssd-test-framework dependency points to a personal fork (madhuriupadhye/sssd-test-framework@krb_misc). This introduces a significant risk as personal forks may not be stable, maintained, or accessible in the long term. Before merging, this should be changed to point to the official repository, preferably a specific tag or commit hash from the main project to ensure reproducible builds.

gemini-code-assist · 2026-03-23T15:36:42Z

src/tests/system/tests/test_ldap_krb.py

+    client.host.conn.run(
+        "systemctl stop chronyd 2>/dev/null;"
+        " chronyd -q 'server clock.redhat.com iburst' 2>/dev/null;"
+        " systemctl start chronyd 2>/dev/null || true",
+        raise_on_error=False,
+    )


The test hardcodes an external NTP server clock.redhat.com. This creates a dependency on an external service and network connectivity, which can lead to flaky tests. It's recommended to use an NTP server provided by the test infrastructure or a more generic pool address if one must be used. If possible, abstract this into a configurable variable.

src/tests/system/tests/test_ldap_krb.py

thalman · 2026-03-27T14:01:14Z

src/tests/system/tests/test_ldap_krb.py

+    :setup:
+        1. Add user puser1 to LDAP and KDC
+        2. Configure SSSD with LDAP-KRB5, ldap_sasl_mech=GSSAPI
+        3. Move /etc/krb5.keytab to /etc/krb5.keytab.moved


Sorry I do not get this test. What I see is

create keytab file (on kdc)

download it

upload it to he other host

rename it so it does not matches config and therefore is not used

Why we bother with this file if it is not used at all?

Also you say here it is /etc/krb5.keytab but then you use variable keytab_path set to /tmp/ldap_test.keytab.

The test creates a keytab at /tmp/ldap_test.keytab, configures SSSD to use it via ldap_krb5_keytab, then moves it away. The docstring said /etc/krb5.keytab which was wrong — updated the docstring to match keytab_path. Also add a comment explaining: we create a valid keytab first so SSSD config is correct, then remove it to test the crash path.

thalman · 2026-03-27T14:04:21Z

src/tests/system/requirements.txt

 git+https://github.com/next-actions/pytest-tier
 git+https://github.com/next-actions/pytest-output
-git+https://github.com/SSSD/sssd-test-framework
+git+https://github.com/madhuriupadhye/sssd-test-framework@krb_misc


This must be reverted before merging IMO.

thalman · 2026-03-27T14:07:41Z

src/tests/system/tests/test_ldap_krb.py

+        pytest.skip(f"ktutil failed to create mixed keytab: {result.stderr or result.stdout}")
+
+    # Backup and install mixed keytab
+    client.host.conn.run(


I do not see any restore for this backup. Why not simply remove it with utils.fs.rm() - it should take care for restore if that file exists.

Used client.fs.rm("/etc/krb5.keytab") instead of manual backup/restore — the framework handles auto-restore.

thalman · 2026-03-27T14:12:46Z

src/tests/system/tests/test_ldap_krb.py

+    host_principal = f"host/{client.host.hostname}"
+    kdc.principal(host_principal).add(password=None)
+    kdc.kadmin(f"ktadd -k {keytab_path} {host_principal}")
+    result = client.host.conn.run(


Did you consider using fs.download() / fs.upload()? That would be much cleaner with regard of getting the environment into original state when the test finishes.

Replace the raw scp commands with framework's client.fs.download() / kdc.fs.upload() for keytab transfer between hosts.

thalman · 2026-03-27T14:13:32Z

src/tests/system/tests/test_ldap_krb.py

+    host_principal = f"host/{client.host.hostname}"
+    kdc.principal(host_principal).add(password=None)
+    kdc.kadmin(f"ktadd -k {valid_keytab} {host_principal}")
+    result = client.host.conn.run(


download()/upload()?

thalman

LGTM, thanks

We just have to wait for requirements.txt not to point to personal repo

- Port three downstream krb_misc_bugzilla bash tests to upstream pytest - BZ 773660: Clock skew errors logged to syslog - BZ 869150: ldap_child handles missing keytab without segfault - BZ 805281: Correct principal selected from multi-realm keytab Signed-off-by: Madhuri Upadhye <mupadhye@redhat.com>

danlavu · 2026-04-01T17:44:39Z

src/tests/system/tests/test_ldap_krb.py

@@ -0,0 +1,283 @@
+"""


I think this file, should match the provider name, so test_ldap_krb5.py

danlavu · 2026-04-01T17:48:49Z

src/tests/system/tests/test_ldap_krb.py

+"""
+SSSD krb misc bugzilla tests.
+
+Port of sssd-qe client/krb_provider/krb_misc_bugzilla tests.


Not a port, it's a rewrite. I don't think we need to reference where these tests came from.

SSSD LDAP/KRB5 Tests. Tests where the 'id_provider` is set to 'ldap' and the `auth_provider` is set to 'krb5'.

danlavu · 2026-04-01T17:52:35Z

src/tests/system/tests/test_ldap_krb.py

+    # Configure SSSD with LDAP+KRB5, chpass_provider=krb5
+    client.sssd.common.krb5_auth(kdc)
+    client.sssd.domain["chpass_provider"] = "krb5"
+    client.sssd.domain["enumerate"] = "true"


I don't think enumerate is required. Can you check? If it's not, let's remove it so we minimize the extra configuration parameters.

danlavu · 2026-04-01T18:03:08Z

src/tests/system/tests/test_ldap_krb.py

+        "forwardable = yes\nkdc_timesync = 0",
+    )
+    client.fs.write("/etc/krb5.conf", krb5_conf)
+    client.host.conn.run("restorecon -v /etc/krb5.conf", raise_on_error=False)


You shouldn't need to restore the context.

danlavu · 2026-04-01T18:06:54Z

src/tests/system/tests/test_ldap_krb.py

+    """
+    :title: Kerberos errors (clock skew) are logged to syslog
+    :setup:
+        1. Ensure chronyc is available


I would simplify/combine the docstring steps to something like this.

:title: Clock skew errors are logged to syslog. This rest requires 'chronyc'; the test will skip if missing. :setup: 1. Create user and kerberos principal 2. Configure SSSD 3. Disable time sync and start SSSD :step: 1. Login as user 2. Change the system time to be one day ahead 3. Login again 4. Check the logs :expectedresults: 1. Login successful 2. System time has changed 3. Login fails 4. Logs contain a time skew message

Please do something similar with the next tests as well. Remember, docstrings should summarize and explain the scope of the test; the details are in the code.

danlavu · 2026-04-01T18:09:08Z

src/tests/system/tests/test_ldap_krb.py

+        pytest.skip(f"chronyd not running or chrony manual mode failed: {result.stdout}")
+
+    try:
+        # Move system time forward (+1 day) to cause clock skew


Not part of this PR, but we should see how many tests require the time to be incorrect. I think quite a few, so this would be a nice addition to the framework.

danlavu · 2026-04-01T18:10:35Z

src/tests/system/tests/test_ldap_krb.py

+        auth_ok = client.auth.ssh.password("a1m2user", "Secret123")
+        assert not auth_ok, "Auth should fail due to clock skew!"
+    finally:
+        # Always restore time even if asserts fail


I find this is the only inline comment that improves comprehension of the test; I'd remove the others.

gemini-code-assist bot reviewed Mar 23, 2026

View reviewed changes

github-advanced-security bot found potential problems Mar 23, 2026

View reviewed changes

src/tests/system/tests/test_ldap_krb.py Fixed Show fixed Hide fixed

madhuriupadhye force-pushed the ldap_krb1 branch from 5e56975 to 83e1b5d Compare March 23, 2026 11:31

gemini-code-assist bot reviewed Mar 23, 2026

View reviewed changes

madhuriupadhye force-pushed the ldap_krb1 branch 4 times, most recently from 45f0377 to 812552e Compare March 24, 2026 11:10

madhuriupadhye assigned danlavu Mar 26, 2026

alexey-tikhonov requested review from danlavu and thalman March 26, 2026 13:44

alexey-tikhonov assigned thalman Mar 26, 2026

thalman requested changes Mar 27, 2026

View reviewed changes

madhuriupadhye force-pushed the ldap_krb1 branch 2 times, most recently from 68ae3d4 to dc51504 Compare March 30, 2026 14:05

madhuriupadhye requested a review from thalman March 31, 2026 10:19

thalman reviewed Apr 1, 2026

View reviewed changes

madhuriupadhye force-pushed the ldap_krb1 branch from dc51504 to 59a0f4e Compare April 1, 2026 15:35

danlavu requested changes Apr 1, 2026

View reviewed changes

Conversation

madhuriupadhye commented Mar 23, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madhuriupadhye commented Mar 23, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thalman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants