tests: add LDAP+KRB5 DNS discovery and SASL tests

[madhuriupadhye]

LDAP+Kerberos DNS SRV and ldap_sasl_canonicalize tests
Add and extend LDAP_KRB5 for DNS SRV discovery and GSSAPI canonicalize behavior.

Test	Coverage
test_ldap_krb5__dns_srv_discovery_with_srv_uri	ldap_uri=_srv_, krb5_server=_srv_; LDAP and Kerberos SRV resolved; SSH auth
test_ldap_krb5__kerberos_srv_discovery_with_ldap_uri_set	BZ 700805 — Kerberos SRV when ldap_uri is explicit
test_ldap_krb5__dns_discovery_with_discovery_domain_set	dns_discovery_domain with ldap_uri removed
test_ldap_krb5__ldap_sasl_canonicalize_handles_reverse_dns_mismatch	BZ 732935 — bogus PTR + ldap_sasl_canonicalize with GSSAPI

Requires sssd-test-framework with:

• NetworkUtils.prepare_ldap_krb5_srv_discovery()
• NetworkUtils.setup_sasl_canonicalize_bogus_ptr() (resolves LDAP/KDC IPv4, returns addresses)
• NetworkUtils.dig() / has_srv_record()
• LDAP.enable_gssapi()
• GetentUtils.resolve_ipv4() / ahostsv4

[gemini-code-assist]

Code Review

This pull request introduces new system tests for SSSD's LDAP and Kerberos integration, specifically covering DNS SRV discovery, backup URI failover, and SASL canonicalization behavior with GSSAPI. The review feedback highlights a need to handle IPv6 addresses correctly in LDAP URIs by using square brackets and suggests reordering test logic to ensure the test skips appropriately when the environment cannot reproduce the specific canonicalization issue.

[gemini-code-assist]

The LDAP URI construction does not account for IPv6 addresses. If ldap_ip is an IPv6 address, it must be enclosed in square brackets (e.g., ldaps://[2001:db8::1]) to be a valid URI. Using a raw IPv6 address will cause SSSD or libldap to fail parsing the URI due to the ambiguous colons.

Suggested change

	client.sssd.domain["ldap_uri"] = f"ldaps://{ldap_ip}"
	ldap_uri = f"ldaps://[{ldap_ip}]" if ":" in ldap_ip else f"ldaps://{ldap_ip}"
	client.sssd.domain["ldap_uri"] = ldap_uri

[gemini-code-assist]

The test logic for skipping when the environment cannot reproduce the issue is flawed. The check if not broken is performed after a second SSSD restart and an assertion. If the environment has a working reverse DNS (i.e., broken is False), the test should skip immediately after the first id call. Otherwise, if the second id call fails for unrelated reasons, the test will incorrectly report a failure instead of skipping.

    broken = client.tools.id("puser1") is None
    if not broken:
        pytest.skip(
            "Reverse DNS for LDAP IP matches expected hostname; cannot reproduce BZ 732935 "
            "canonicalize failure in this environment"
        )

    client.sssd.domain["ldap_sasl_canonicalize"] = "false"
    client.sssd.restart(clean=True)
    assert client.tools.id("puser1") is not None, "id should succeed with ldap_sasl_canonicalize=false"

[sssd-bot]

Review done using Claude Code / claude-opus-4-6

Functional Issues

1. requirements.txt points to a personal fork branch (src/tests/system/requirements.txt:8-9): The upstream sssd-test-framework dependency is commented out and replaced with git+https://github.com/madhuriupadhye/sssd-test-framework@krb_misc2. This must not be merged -- it would break every CI consumer that does not have access to or control over this fork. The corresponding sssd-test-framework changes need to be merged upstream first, and this file must be reverted to point to git+https://github.com/SSSD/sssd-test-framework (or a pinned commit/tag on the upstream repo).

2. Overly broad "unknown" assertion is fragile and likely to false-positive (src/tests/system/tests/test_ldap_krb5.py:555-556): The assertion "unknown" not in log_content.lower() searches the entire SSSD domain log (with debug_level 0xFFF0) for the substring "unknown" anywhere, case-insensitively. At high debug levels SSSD legitimately logs lines containing "unknown" in many contexts (e.g. "unknown attribute", "unknown option", provider status messages). This will produce false test failures unrelated to BZ 700805. The check should match a specific error pattern (e.g. the exact error message that BZ 700805 produces) rather than any occurrence of the word "unknown" in potentially thousands of log lines.

3. kdc_ip is unpacked but never used (src/tests/system/tests/test_ldap_krb5.py:626): The return value kdc_ip from setup_sasl_canonicalize_bogus_ptr() is captured in the tuple unpacking ldap_ip, kdc_ip = ... but is never referenced anywhere in the test body. If the test does not need it, use ldap_ip, _ = ... to signal intent. If it should be used (e.g. to verify KDC reachability or to construct a Kerberos URI), that verification is missing.

4. IPv6 address not bracketed in LDAP URI construction (src/tests/system/tests/test_ldap_krb5.py:658): The LDAP URI is built as f"ldap://{ldap_ip}" where ldap_ip comes from setup_sasl_canonicalize_bogus_ptr(). If this returns an IPv6 address, the resulting URI ldap://2001:db8::1 is malformed -- RFC 2732 requires ldap://[2001:db8::1]. The test also sets lookup_family_order = ipv4_first which makes IPv6 unlikely in practice, but the code should still handle it correctly or document/assert that only IPv4 is returned. (This was also flagged by the existing review comment from gemini-code-assist.)

5. Second commit message ("Testing with IPA") is too vague: The commit adding test_ldap_krb5_ipa.py has the one-line message "Testing with IPA" with no body describing what tests are added, what they cover, or what framework dependencies they require. SSSD commit messages should explain the intent; this should be expanded to match the quality of the first commit message.

Nits & Non-functional Issues

1. Missing @pytest.mark.authentication on all four new tests in test_ldap_krb5.py (lines 405, 487, 561, 607): Every existing test in this file carries @pytest.mark.authentication. All four new tests perform SSH authentication or GSSAPI lookups yet omit this marker, which means they will not be selected by marker-based test collection filters (e.g. -m authentication). The same is true for the three tests in test_ldap_krb5_ipa.py (lines 29, 54, 89) -- they test auth-related scenarios but lack the @pytest.mark.authentication marker.

2. _assert_ldap_krb5_srv_records() is duplicated verbatim between test_ldap_krb5.py:25-30 and test_ldap_krb5_ipa.py:19-24. Consider extracting it into a shared conftest or utility module (src/tests/system/tests/conftest.py does not exist yet but would be the natural home).

3. Implicit string concatenation in assertion message (src/tests/system/tests/test_ldap_krb5.py:677-678): The assertion message uses implicit string concatenation across two string literals: "getent passwd puser1 must succeed with ldap_sasl_canonicalize=false " "while bogus PTR remains in place". While syntactically valid, using an f-string or a single string is clearer and avoids the accidental-omission-of-comma pitfall.

4. test_ldap_krb5__dns_srv_discovery_with_srv_uri and the IPA variant share extensive log-assertion boilerplate: The SRV log assertion block (checking for "Trying to resolve SRV record" and "Marking SRV lookup ... as 'resolved'" for both LDAP and Kerberos) is repeated in test_ldap_krb5.py:467-483, test_ldap_krb5.py:549-553, and test_ldap_krb5_ipa.py:130-149. Factoring this into a helper (similar to _assert_ldap_krb5_srv_records) would reduce duplication and make the assertions easier to maintain.

5. No @pytest.mark.ticket on two of the new tests: test_ldap_krb5__dns_srv_discovery_with_srv_uri (line 405) and test_ldap_krb5__dns_discovery_with_discovery_domain_set (line 559) lack a @pytest.mark.ticket decorator. The other new tests correctly reference their BZ numbers via this marker. If these tests trace to a specific requirement or bugzilla, they should have the marker for traceability.

6. IPA test file lacks @pytest.mark.ticket on all tests: None of the three tests in test_ldap_krb5_ipa.py carry @pytest.mark.ticket decorators. If they trace to existing tickets they should be annotated.

Review of Existing Review Comments

1. gemini-code-assist: IPv6 address bracketing in LDAP URI -- Valid concern. As noted in functional issue above, f"ldap://{ldap_ip}" at line 658 does not handle IPv6. The comment's suggestion to check for : and wrap in brackets is reasonable. This has not been addressed yet.

2. gemini-code-assist: Logic reordering for the skip-when-not-broken check -- This comment references test logic involving a broken variable and pytest.skip(). However, looking at the current PR code for test_ldap_krb5__ldap_sasl_canonicalize_handles_reverse_dns_mismatch, there is no broken variable or pytest.skip() call in the test body at all. The test simply runs three phases (baseline, canonicalize=true with IP URI, canonicalize=false) without any skip logic. It appears this comment was based on a previous revision of the code that has since been rewritten. The concern is no longer applicable to the current version.