Reimplement certificate checks in Python

[jakduch]

Summary

• replace the shell certificate validator with a maintainable Python implementation
• emit one consistent status line for every validation while preserving existing exit codes
• verify that custom certificates cover the server FQDN and configured aliases, including wildcard coverage
• validate the requested proxy hostname when generating a custom auth bundle

Validation

• 44 unit tests passed
• ruff passed for tests, src, development scripts, and inventories
• full src Ansible lint passed
• Python bytecode compilation and git diff checks passed

Fixes #714

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 60d71436-e5ff-4b12-b11d-862cf8003a31

📥 Commits

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

📒 Files selected for processing (6)

• docs/user/certificates.md
• src/playbooks/auth-bundle/auth-bundle.yaml
• src/roles/certificate_checks/defaults/main.yml
• src/roles/certificate_checks/files/foreman-certificate-check
• src/roles/certificate_checks/tasks/main.yml
• tests/unit/foreman_ceritificate_check_test.py

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

---

📝 Walkthrough

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 reflect 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 the listed certificate checks. Tests cover expected-name validation and updated diagnostics. The documentation describes the Python command and its FQDN and alias checks.

Priority: ⬇️ Low

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

Change: Feature

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 check results and return failure codes

Loading

Merge Risk: ⚪ Minimal · up to 38304

This change replaces the shell certificate validator with a Python command. The role now checks that custom certificates cover the server FQDN and configured aliases, and the auth bundle is checked against the requested hostname. Alias wiring and exit-code aggregation match the previous behavior, and no outstanding defects were found. The change appears ready to merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 38304

The reviewed deployment path adds hostname checks while retaining failure-based rejection of invalid certificates. No security regression was established, but the checker is a security-sensitive deployment gate.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed validation gate affects certificate acceptance on managed hosts and custom-server auth-bundle generation, rather than establishing an attacker-reachable service entrypoint.

Trust Boundaries and Controls

• observed — Certificate and hostname inputs cross into the local validation process through command arguments. The role supplies expected names, and an uncovered name produces failure code 11.

Resilience and Maintainability Implications

• observed — The checker rejects missing required files and unsupported certificate encoding before subsequent validation, rather than treating those states as successful checks.

Hardening Proposals

• proposed — For any independently operated CLI path that relies on a certificate for a particular hostname, supply that hostname explicitly; the CLI otherwise leaves expected-name checking optional.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (5 skipped: 5 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: replacing the shell certificate validator with a Python implementation.
Description check	✅ Passed	The description directly explains the Python reimplementation, validation behavior, expected certificate names, proxy hostname validation, and reported test results.
Linked Issues check	✅ Passed	Issue #714 requires a Python replacement for the shell validator, consistent validation output, and certificate name coverage. The PR replaces the script with Python, emits one [OK] or [FAIL] resu…
Out of Scope Changes check	✅ Passed	The changed documentation, role defaults, task arguments, auth-bundle argument, Python validator, and unit tests all support Issue #714 objectives or their required integration. No unrelated change is…

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (5 skipped: 5 unsupported.)

• Fix all pre-merge checks with AI

---

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.