[jakduch]

Why are you introducing these changes? (Problem description, related links)

Fixes #714. Replace the shell certificate validator with a Python implementation that preserves its exit-code contract and makes its checks easier to maintain.

What are the changes introduced in this pull request?

• Emit consistent status lines for certificate validation.
• Check the CA bundle, key, certificate validity and server-name coverage, including wildcards.
• Check the requested proxy hostname when creating a custom auth bundle.
• Update the certificate documentation and regression coverage.

This is independent of other open PRs. Rebased the single commit onto current master. The previous CI run failed while installing Vagrant from an unavailable PPA, before deployment testing.

How to test this pull request

Run python -m pytest --confcutdir=tests/unit tests/unit/foreman_ceritificate_check_test.py -q, then run the deployment matrix with default and custom certificates. Exercise valid certificates and invalid keys, chains and hostnames.

Local verification after rebase: all 12 tests in that certificate suite passed using the existing OpenSSL 3 installation. Ruff passed across the repository's configured Python paths, targeted Ansible lint passed, and git diff --check passed. Full deployment testing remains for CI. No local VM or container stack was started.

Checklist

• Tests added/updated (if applicable)
• Documentation updated (if applicable)

AI assistance: Codex. Project signature: GPT_5.6 Sol.

[coderabbitai]

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d930a02f-112b-4615-9151-8fa612ce248d

📥 Commits

Reviewing files that changed from the base of the PR and between 3830444 and f12c97e.

📒 Files selected for processing (1)

• docs/user/certificates.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The certificate checker is replaced with a Python CLI. The role passes the server FQDN and configured aliases as expected names. Tests and documentation cover the expected-name checks and updated output.

Changes: Certificate validation

Layer / File(s)	Summary
Configure expected certificate names src/roles/certificate_checks/defaults/main.yml, src/playbooks/auth-bundle/auth-bundle.yaml, src/roles/certificate_checks/tasks/main.yml	The role combines the host FQDN and configured aliases, removes duplicates, and passes the names to the checker for custom server certificates.
Implement and validate the Python checker src/roles/certificate_checks/files/foreman-certificate-check, tests/unit/foreman_ceritificate_check_test.py, docs/user/certificates.md	The Python CLI replaces the shell implementation and performs certificate checks. Tests and documentation cover expected-name validation and updated diagnostics.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: ehelms

Sequence Diagram(s)

sequenceDiagram
  participant AuthBundlePlaybook
  participant CertificateChecksRole
  participant ForemanCertificateCheck
  AuthBundlePlaybook->>CertificateChecksRole: Set expected names for custom server
  CertificateChecksRole->>ForemanCertificateCheck: Pass certificate, key, CA bundle, and expected names
  ForemanCertificateCheck->>ForemanCertificateCheck: Run certificate validation checks
  ForemanCertificateCheck-->>CertificateChecksRole: Print results and return failure codes

Loading

Merge Risk: Minimal · up to f12c9

The documentation accurately describes the configured FQDN and alias coverage; no actionable merge risk remains in the reviewed change.

Architecture risk: Low · up to f12c9

The change affects 3 systems.

Changed systems: src, docs, tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — src (service) was modified; 4 changed files map to changed impact.
• observed — docs (service) was modified; 1 changed file maps to changed impact.
• observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in src/playbooks/auth-bundle/auth-bundle.yaml: The certificate_checks role now checks the custom server certificate against the current hostname via certificate_checks_expected_names.
• observed — Modified behavior in src/roles/certificate_checks/defaults/main.yml: Adds certificate_checks_expected_names, combining the host FQDN with server_aliases (or an empty list when undefined) and retaining unique values.
• observed — Modified behavior in src/roles/certificate_checks/files/foreman-certificate-check: The Bash implementation and its shell argument handling, checks, and exit flow were replaced by a Python command-line validator. It accepts required certificate, key, and CA-bundle paths plus optional comma-separated expected names; combines check failure codes, and stops early for missing files, unsupported certificate encoding, or an uninspectable certificate. It checks certificate and CA-bundle expiration, CA status, key passphrase and certificate match, CA verification, bundle size and trust rules, DNS names and expected-name coverage, key usage, and CA signing algorithms.
• observed — Modified behavior in src/roles/certificate_checks/tasks/main.yml: The installation task label changes from “Install foreman-certificate-check” to “Install certificate checker.”

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Issue #714 asks for a shell-free validator, consistent output, and certificate-name checks. The Python validator emits [OK] or [FAIL] for checks and combines failure codes with bitwise OR. The rol…
Out of Scope Changes check		The documentation, role defaults and task updates, auth-bundle integration, validator, and tests support Issue #714 and its certificate-name validation. No unrelated change is demonstrated.
Title check		The title clearly and concisely identifies the main change: replacing the certificate checks implementation with Python.
Description check		The description directly explains the Python reimplementation, validation changes, testing, and documentation updates.

Full details: Linked Issues check

Explanation

Issue #714 asks for a shell-free validator, consistent output, and certificate-name checks. The Python validator emits [OK] or [FAIL] for checks and combines failure codes with bitwise OR. The role supplies the FQDN and aliases as expected names; the validator checks exact and wildcard DNS coverage. Unit tests cover wildcard coverage and missing names. The auth-bundle task also passes the requested hostname for custom certificates.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jakduch]

I investigated the two failed jobs in this run:

• The CentOS Stream 10 fapolicyd proxy job denied trusted /usr/bin/bash executions after fapolicyd evicted an active subject from its cache. The same failure occurs on the current master branch, so I split the shared fix into Increase fapolicyd subject cache forklift#1990 (Fixes Increase fapolicyd subject cache for CI workloads forklift#1989).
• The CentOS Stream 9 FIPS proxy job hit the kernel OOM killer during backup/lifecycle testing and then lost SSH access. The equivalent job is green on other runs and the failure occurred outside the certificate-check code.

All certificate-check-specific lint and deployment jobs are green. I will rerun the complete workflow after the current run finishes rather than retrying only selected jobs.

[jakduch]

The workflow has now completed. The failures are the two unrelated runner/environment failures described above; all other matrix jobs, including the upgrade job, passed.

I tried to rerun the complete workflow (not individual jobs), but GitHub rejected it because fork contributors do not have repository admin permission. Could a maintainer please rerun the full failed workflow run 36087064533?

[jakduch]

The failing deployment jobs are caused by the shared development-memory issue, not by the certificate-check changes: the SOS journal records candlepin.service being killed by the OOM killer. #865 limits the Candlepin heap for development tuning and has passed the complete CI matrix.

[jakduch]

The failing deployment jobs hit the Candlepin heap/OOM problem fixed on current master by #865. I rebased this branch onto that fix; CI is running again.

[jakduch]

The only remaining exact-head failure is the CentOS Stream 10 fapolicyd content-proxy job. The proxy ran out of resources while pulling images; this shared matrix problem is fixed by PR #884, where the equivalent job is green. The certificate-check code is not involved. I will rebase after PR #884 lands.

[ehelms]

Did you consider a design whereby the checks are handled by Ansible directly, and instead of a stand-alone script there is a foremanctl command when needing to run it stand-alone?

If not, could you compare the two? I'm not sure which will be best, but I'd like use to do the due diligence.

[jakduch]

I compared the two approaches now.

Doing this directly in Ansible is attractive for the basic checks: stat and community.crypto.x509_certificate_info can cover file presence and certificate metadata. The remaining checks still need either custom Python or several openssl commands though: strict chain verification against the supplied bundle and server purpose, certificate/key public-key comparison, PEM vs DER diagnosis, trusted-certificate entries, bundle-size limits, and the existing aggregated result.

Putting those into the role would spread one validator over a fairly long list of registered tasks with failed_when: false, then a separate foremanctl playbook would need to reproduce the same input and output contract. It would also make an ad-hoc check depend on Ansible inventory and execution.

The current split keeps Ansible as the orchestrator and one Python implementation as the validator. It runs on the managed host where the certificate paths exist, has deterministic unit tests, reports all failures in one invocation, and can be reused by a future foremanctl certificate-check command without duplicating the checks. The cost is installing one small helper and retaining the OpenSSL CLI dependency, which the role already installs.

Based on that comparison I would keep the Python helper as the single implementation and add a thin foremanctl command around it separately if we want a supported standalone UX. If you prefer that command to be part of this PR, I can add it here.

[jakduch]

Rebased onto current master in c2bc59a, keeping one commit and the same feature patch. The previous deployment jobs stopped while installing Vagrant from the unavailable PPA. All 12 targeted certificate tests passed locally using OpenSSL 3, along with targeted Ansible lint and Ruff. No pending PR is required for this retry. Fresh CI has been started.