feat: certificate trust#341
Open
Klaas- wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new ChangesCertificate Trust Store Feature
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/tests_trust.yml (2)
1-156: 📐 Maintainability & Code Quality | 🔵 TrivialAdd failure scenario tests for
certificate_trust.The path instruction for
tests/tests_*.ymlstates: "Tests should verify both success and failure scenarios." Currently only success paths (install fromsrc, install fromcontent, remove) are tested. Consider adding tests for:
- Missing required
namefield- Providing more than one of
content/src/urlwhenstate: presenturl-based provisioning (if supported)As per path instructions: "Tests should verify both success and failure scenarios".
[medium_effort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tests_trust.yml` around lines 1 - 156, The trust role tests only cover successful install and removal flows; add negative coverage in tests/tests_trust.yml for certificate_trust validation. Extend the existing trust scenarios around certificate_trust and linux-system-roles.certificate to assert failures when name is missing and when more than one of content/src/url is provided with state: present, and add a url-based case if the role supports it. Use the current include_role/task structure and existing verify/assert patterns so the new failure cases are checked alongside the existing success tests.Source: Path instructions
94-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify inline-content trust anchor is actually trusted and has correct ownership.
Play 2 only asserts file existence and mode (
0644), but doesn't verify ownership (root:root) or that the certificate is actually trusted viaopenssl verify— both of which are checked in Play 1 (lines 55–65). Add the same assertions for consistency.💚 Suggested additions
- name: Assert that the trust anchor was installed assert: that: - __trust_anchor_content.stat.exists - __trust_anchor_content.stat.mode == '0644' + - __trust_anchor_content.stat.pw_name == 'root' + - __trust_anchor_content.stat.gr_name == 'root' + + - name: Check that the inline test CA certificate is now trusted + command: openssl verify {{ __trust_test_ca_cert }} + register: __trust_verify_content + changed_when: false + + - name: Assert that the inline test CA certificate is now trusted + assert: + that: "__trust_verify_content.stdout is search(': OK')"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tests_trust.yml` around lines 94 - 103, Play 2 only checks that the inline trust anchor file exists and has mode 0644, but it is missing the ownership and trust verification assertions already used in Play 1. Update the trust-anchor validation around __trust_anchor_content in tests/tests_trust.yml to also assert root:root ownership from the stat result and add an openssl verify check to confirm lsr-trust-test-content.crt is actually trusted, keeping the assertions consistent with the existing trust-anchor checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasks/certificate_trust.yml`:
- Around line 10-40: The trust-certificate install flow in certificate_trust.yml
must validate each item before running the install tasks so exactly one source
is provided when state is present. Add a precheck task for certificate_trust
entries that enforces one and only one of content, src, or url is defined, and
fail fast with a clear message if multiple or none are set. Keep the existing
copy and get_url tasks in place, but make them rely on the validation instead of
silently overwriting or skipping items.
In `@tests/tests_trust.yml`:
- Around line 105-156: The trust tests are split across multiple plays and the
cleanup runs only at the end, so failures can leave `/tmp` artifacts behind.
Refactor the trust test flow in tests/tests_trust.yml so the test tasks run
inside a single block with an always section, and move the cleanup for
__trust_test_ca_cert and __trust_test_ca_key into that always section. Make sure
the cleanup task(s) are tagged with tests::cleanup, and keep the existing
trust-anchor removal/assertion logic inside the block so it still references the
same certificate_trust and __trust_* symbols.
- Around line 80-86: The trust test currently calls
linux-system-roles.certificate directly with include_role, but it should use the
wrapper task instead. Update tests/tests_trust.yml to invoke the role through
tasks/run_role_with_clear_facts.yml, and make sure any CA content needed after
facts are cleared is read at evaluation time rather than from
__trust_test_ca_slurp. Refer to the trust test task using certificate_trust and
the wrapper task path when making the change.
---
Nitpick comments:
In `@tests/tests_trust.yml`:
- Around line 1-156: The trust role tests only cover successful install and
removal flows; add negative coverage in tests/tests_trust.yml for
certificate_trust validation. Extend the existing trust scenarios around
certificate_trust and linux-system-roles.certificate to assert failures when
name is missing and when more than one of content/src/url is provided with
state: present, and add a url-based case if the role supports it. Use the
current include_role/task structure and existing verify/assert patterns so the
new failure cases are checked alongside the existing success tests.
- Around line 94-103: Play 2 only checks that the inline trust anchor file
exists and has mode 0644, but it is missing the ownership and trust verification
assertions already used in Play 1. Update the trust-anchor validation around
__trust_anchor_content in tests/tests_trust.yml to also assert root:root
ownership from the stat result and add an openssl verify check to confirm
lsr-trust-test-content.crt is actually trusted, keeping the assertions
consistent with the existing trust-anchor checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c16ed6a9-adfc-4d18-9801-c37434e37f7f
📒 Files selected for processing (10)
.ostree/packages-runtime.txtREADME.mddefaults/main.ymltasks/certificate_trust.ymltasks/main.ymltests/setup-snapshot.ymltests/tests_trust.ymlvars/Debian.ymlvars/Suse.ymlvars/main.yml
richm
reviewed
Jul 9, 2026
richm
reviewed
Jul 9, 2026
Author
|
I can squash it if that is wanted. |
richm
reviewed
Jul 10, 2026
| tasks: | ||
| - name: Run certificate_trust tests | ||
| block: | ||
| - name: Ensure openssl is installed |
Collaborator
There was a problem hiding this comment.
Suggested change
| - name: Ensure openssl is installed | |
| - name: Get var __certificate_is_ostree and other role vars | |
| include_role: | |
| name: linux-system-roles.certificate | |
| tasks_from: set_vars.yml | |
| public: true | |
| - name: Ensure openssl is installed |
Yes, this is somewhat painful due to ostree
richm
reviewed
Jul 10, 2026
| - name: Ensure openssl is installed | ||
| package: | ||
| name: openssl | ||
| state: present |
Collaborator
There was a problem hiding this comment.
Suggested change
| state: present | |
| state: present | |
| use: "{{ (__certificate_is_ostree | d(false)) | | |
| ternary('ansible.posix.rhel_rpm_ostree', omit) }}" |
richm
reviewed
Jul 10, 2026
richm
reviewed
Jul 10, 2026
| when: | ||
| - item.state | d('present') == 'present' | ||
| - item.content is defined or item.src is defined | ||
| register: __certificate_trust_copied |
Collaborator
There was a problem hiding this comment.
Suggested change
| register: __certificate_trust_copied | |
| register: __certificate_trust_copied | |
| no_log: "{{ certificate_secure_logging }}" |
I believe content may contain a key - we don't want that to go to the log by default
richm
reviewed
Jul 10, 2026
| when: | ||
| - item.state | d('present') == 'present' | ||
| - item.url is defined | ||
| register: __certificate_trust_downloaded |
Collaborator
There was a problem hiding this comment.
Suggested change
| register: __certificate_trust_downloaded | |
| register: __certificate_trust_downloaded | |
| no_log: "{{ certificate_secure_logging }}" |
Same here - not sure if this could in any way print any of the key material to the log
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhancement:
Fixes #339
Adds a variable to add trusted CA certificates to the system.
Summary by CodeRabbit
certificate_trustsupport to manage system trust-store anchors, including adding/removing certificates from inline content, local files, or URLs.certificate_trustvariable reference, per-platform trust-store paths/commands, and new add/remove examples.