Skip to content

Add per-auth-source cacert support to LDAP tests - #22217

Draft
adamruzicka wants to merge 4 commits into
SatelliteQE:masterfrom
adamruzicka:ldap-auth-source-cacert
Draft

Add per-auth-source cacert support to LDAP tests#22217
adamruzicka wants to merge 4 commits into
SatelliteQE:masterfrom
adamruzicka:ldap-auth-source-cacert

Conversation

@adamruzicka

@adamruzicka adamruzicka commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem Statement

theforeman/foreman#11107 adds a cacert attribute to ldap auth servers which obsoletes the old way of handling ca certs (by including in the system trust store).

Solution

Switch to per-source CA cert handling.

Related Issues

https://redhat.atlassian.net/browse/SAT-45933
https://redhat.atlassian.net/browse/SAT-46986

Requires:

Summary by Sourcery

Switch LDAP authentication tests to use per-auth-source CA certificates instead of system trust store configuration.

New Features:

  • Add test coverage for LDAPS authentication sources failing connection when no cacert is provided.
  • Add helper to retrieve LDAP CA certificates as PEM for use in tests and CLI workflows.
  • Add CLI test ensuring LDAP auth sources created with cacert via hammer persist and expose the cacert field.

Enhancements:

  • Update destructive LDAP authentication tests to configure LDAPS using the new per-source cacert attribute rather than OS-level certificate installation.

Tests:

  • Remove legacy certificate setup helpers from UI and destructive LDAP tests and replace them with per-auth-source cacert usage and new LDAPS negative/positive scenarios.

@adamruzicka adamruzicka added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Jul 22, 2026
@sourcery-ai

sourcery-ai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Refactors LDAP tests to stop installing CA certs into the system trust store and instead use per-auth-source CA certificate configuration, adding helpers to fetch CA certs as PEM and new test coverage for the cacert behavior in both UI and CLI flows.

Sequence diagram for fetching LDAP CA cert PEM per auth source

sequenceDiagram
    participant Test as LdapTest
    participant Sat as SatelliteHost
    participant IPA as IpaServer
    participant AD as AdServer

    Test->>Sat: get_ldap_cacert_pem(sat, "ipa", hostname)
    activate Sat
    Sat->>IPA: execute(curl -s http://hostname/ipa/config/ca.crt)
    IPA-->>Sat: PEM CA cert
    Sat-->>Test: PEM CA cert
    deactivate Sat

    Test->>Sat: get_ldap_cacert_pem(sat, "ad", hostname)
    activate Sat
    Sat->>Sat: execute(yum install cifs-utils)
    Sat->>AD: execute(mount -t cifs //hostname/c$ /mnt)
    Sat->>Sat: execute(cat /mnt/Users/Administrator/.../satqe-QE-SAT6-AD-CA.cer)
    alt [certificate not in PEM]
        Sat->>Sat: execute(openssl x509 -inform DER -in cert_path)
    end
    Sat->>Sat: execute(umount /mnt)
    Sat-->>Test: PEM CA cert
    deactivate Sat
Loading

File-Level Changes

Change Details Files
Remove legacy OS trust-store based CA certificate setup from LDAP tests and rely on per-auth-source cacert configuration instead.
  • Delete helper functions that downloaded/installed IPA/AD CA certificates into the system trust store in UI and destructive LDAP test modules
  • Stop using CERT_PATH constant and related filesystem manipulation and httpd restarts in tests
tests/foreman/ui/test_ldap_authentication.py
tests/foreman/destructive/test_ldap_authentication.py
Add reusable helper to fetch LDAP CA certificates as PEM without modifying the OS trust store.
  • Introduce get_ldap_cacert_pem in satellite_auth component fixture module which fetches IPA CA via HTTP and AD CA via CIFS mount, converting to PEM if needed
  • Ensure AD mount is cleaned up with umount in a finally block and validate command success via assertions
pytest_fixtures/component/satellite_auth.py
Update destructive LDAP UI tests to supply per-auth-source cacert and cover LDAPS creation path.
  • Retrieve CA cert via get_ldap_cacert_pem in destructive test_positive_create_with_https and pass it to the UI as ldap_server.cacert while keeping LDAPS login validation intact
  • Simplify test docstring wording from HTTPS to LDAPS and remove server-type-specific certificate installation logic
tests/foreman/destructive/test_ldap_authentication.py
Extend CLI LDAP auth source tests to validate creating IPA auth sources with cacert-file.
  • Use get_ldap_cacert_pem to obtain IPA CA cert, write it to a temporary file on the Satellite, and create an LDAPS LDAP auth source via hammer with --cacert-file
  • Assert the created auth source is persisted and that the returned CLI info includes the cacert field; clean up the temporary CA file
tests/foreman/cli/test_ldapauthsource.py
Add UI regression coverage ensuring LDAPS test connection fails when no cacert is configured.
  • Introduce test_negative_ldaps_without_cacert that enables LDAPS on an existing IPA auth source without providing a cacert and expects test_connection to fail with an AssertionError
tests/foreman/ui/test_ldap_authentication.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch 2 times, most recently from 21847d9 to 0fec79b Compare July 22, 2026 12:54
module_target_sat.cli.LDAPAuthSource.info({'name': new_name})

@pytest.mark.usefixtures("ldap_tear_down")
def test_positive_create_with_cacert_ipa(self, default_ipa_host, module_target_sat):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note for reviewers:
I can't say I'm exactly happy about this test, but I'm afraid we don't really have another way of covering this in an end-to-end fashion.

:parametrized: yes
"""
ldap_data, auth_source = ldap_auth_source
with session, pytest.raises(AssertionError):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note for reviewers:
I'm not exactly happy about this. Due to https://projects.theforeman.org/issues/39552 , the webui only emits a toast notification saying "Error". Pretty hard to get more details out of that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, this is failing, will need to look at it more.

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 0fec79b to 87a2e40 Compare July 22, 2026 13:21
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16201
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py --external-logging
Test Result : = 3 failed, 93 passed, 2 deselected, 779 warnings, 10 errors in 53943.46s (14:59:03) =

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Jul 23, 2026
@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 87a2e40 to 5da549c Compare July 23, 2026 07:36

@adamruzicka adamruzicka left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The run uncovered two things I did wrong in the tests, fixed those. rhsso tests are (most likely) failing due to expired certs.

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch 3 times, most recently from a56f38a to caa17a5 Compare July 23, 2026 13:31
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
  --verbose
  tests/foreman/cli/test_ldapauthsource.py
  tests/foreman/destructive/test_ldap_authentication.py
  tests/foreman/ui/test_ldap_authentication.py
  -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16220
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : ==== 3 failed, 1 passed, 104 deselected, 87 warnings in 5223.13s (1:27:03) =====

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from caa17a5 to 19b44d3 Compare July 23, 2026 16:26
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
  --verbose
  tests/foreman/cli/test_ldapauthsource.py
  tests/foreman/destructive/test_ldap_authentication.py
  tests/foreman/ui/test_ldap_authentication.py
  -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16228
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : = 3 failed, 1 passed, 104 deselected, 41 warnings, 1 error in 2892.93s (0:48:12) =

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch 2 times, most recently from 72003ec to d56a4d9 Compare July 23, 2026 17:51
Replace OS trust store certificate installation with the new
per-auth-source cacert attribute from Foreman PR SatelliteQE#11107. The
set_certificate_in_satellite() helpers are removed in favor of
get_ldap_cacert_pem() which fetches the CA cert as a PEM string
for direct use in auth source creation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from d56a4d9 to 3380ff2 Compare July 23, 2026 17:55
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
  --verbose
  tests/foreman/cli/test_ldapauthsource.py
  tests/foreman/destructive/test_ldap_authentication.py
  tests/foreman/ui/test_ldap_authentication.py
  -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/cli/test_ldapauthsource.py
    tests/foreman/destructive/test_ldap_authentication.py
    tests/foreman/ui/test_ldap_authentication.py
    -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16239
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : ==== 2 failed, 2 passed, 104 deselected, 24 warnings in 6881.62s (1:54:41) =====

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 1619b45 to eecf486 Compare July 24, 2026 17:19
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\]
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16242
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\] --external-logging
Test Result : ================== 1 failed, 9 warnings in 2776.14s (0:46:16) ==================

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 70a5fca to 92a50b7 Compare July 24, 2026 18:32
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\]
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16244
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\] --external-logging
Test Result : ================== 1 failed, 9 warnings in 2869.46s (0:47:49) ==================

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 92a50b7 to 705339a Compare July 24, 2026 19:42
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/cli/test_ldapauthsource.py
    tests/foreman/destructive/test_ldap_authentication.py
    tests/foreman/ui/test_ldap_authentication.py
    -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16251
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : ==== 1 failed, 3 passed, 104 deselected, 25 warnings in 4682.83s (1:18:02) =====

@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\]
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16260
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/destructive/test_ldap_authentication.py::test_positive_create_with_https\[AD_2019\] --external-logging
Test Result : ================== 1 failed, 9 warnings in 2867.65s (0:47:47) ==================

@adamruzicka
adamruzicka force-pushed the ldap-auth-source-cacert branch from 705339a to 60f75d9 Compare July 27, 2026 14:53
Use the Satellite host cert as a wrong cacert on the first attempt
instead of temporarily removing RH IT Root CAs from the trust store,
making the test non-destructive.

This, apart from making the test faster, also tests certificate pinning.
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/cli/test_ldapauthsource.py
    tests/foreman/destructive/test_ldap_authentication.py
    tests/foreman/ui/test_ldap_authentication.py
    -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

1 similar comment
@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/cli/test_ldapauthsource.py
    tests/foreman/destructive/test_ldap_authentication.py
    tests/foreman/ui/test_ldap_authentication.py
    -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16273
Build Status: UNSTABLE
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : ==== 1 failed, 3 passed, 104 deselected, 72 warnings in 3249.72s (0:54:09) =====

Comment thread tests/foreman/destructive/test_ldap_authentication.py
Comment thread tests/foreman/destructive/test_ldap_authentication.py
Comment thread tests/foreman/ui/test_ldap_authentication.py Outdated
}
)
# No error details in toast https://projects.theforeman.org/issues/39552
assert error.match('Danger alert: Error')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

>>> with pytest.raises(AssertionError) as error:
...     assert False, "This is a string"
... error.match("This is a")
... 
True

So this doesn't really test whether some details follow, or it's just "Error".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My two comments really mean "this will pass whether there are details or not, as long as there at least is 'Error'" - but that doesn't seem like it really tests anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The issue on foreman's side got removed in the meantime, allowing me to match more closely

@adamruzicka

Copy link
Copy Markdown
Contributor Author
trigger: test-robottelo
pytest: >-
    --verbose
    tests/foreman/cli/test_ldapauthsource.py
    tests/foreman/destructive/test_ldap_authentication.py
    tests/foreman/ui/test_ldap_authentication.py
    -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection"
airgun: 2486
nailgun: 1430
theforeman:
    foreman: 11107
    hammer-cli-foreman: 658

@Satellite-QE

Copy link
Copy Markdown
Collaborator

PRT Result

Build Number: 16287
Build Status: SUCCESS
PRT Comment: pytest --verbose tests/foreman/cli/test_ldapauthsource.py tests/foreman/destructive/test_ldap_authentication.py tests/foreman/ui/test_ldap_authentication.py -k "test_positive_create_with_cacert_ipa or test_positive_create_with_https or test_ldaps_cacert_test_connection" --external-logging
Test Result : ========= 4 passed, 104 deselected, 71 warnings in 3216.96s (0:53:36) ==========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Jul 29, 2026
backup_dir = '/tmp/ca-backup'
module_target_sat.execute(f'mkdir -p {backup_dir}')
module_target_sat.execute(f'mv {anchors}/*-IT-Root-CA.pem {backup_dir}/')
module_target_sat.execute('update-ca-trust extract')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Technically, extract isn't necessary. Just FYI, this is ok.

@lhellebr lhellebr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I like this PR, it's great coverage improvement and even improves the structure a bit. Thanks, Adam!

Optionally, you can add some test cases with incorrect and invalid (as in, not a validly formatted) cert.

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

Labels

No-CherryPick PR doesnt need CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants