feat: Implement support for passing an issuer argument to the provider. - #346
feat: Implement support for passing an issuer argument to the provider.#346Nihlus wants to merge 4 commits into
Conversation
This enables the usage of FreeIPA sub-CAs, which use the "IPA" CA in certmonger but have a separate issuer.
📝 WalkthroughWalkthroughChangesIssuer option propagation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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
🤖 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 `@module_utils/certificate_lsr/providers/certmonger.py`:
- Around line 331-334: Update cert_needs_update() to compare the requested
issuer parameter with the existing Certmonger template-issuer metadata,
alongside the existing CA and certificate/CSR checks. Return that the
certificate needs updating when the issuers differ, so request_certificate()
reapplies the -X issuer option while preserving current behavior when they match
or no issuer is requested.
In `@tasks/main.yml`:
- Line 126: Update the certificate_requests schema in defaults/main.yml to
document issuer as an optional per-request field, matching its consumption in
the tasks/main.yml certificate request flow. Keep the existing
certificate_requests: [] default unchanged.
- Line 126: Add regression coverage for the certificate_requests task path in
tasks/main.yml, including one entry with a defined issuer and another without
issuer. Assert that certificate_request receives the issuer when provided and
omits the parameter when absent, covering the item.issuer default(omit)
behavior.
🪄 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: 5d538fbf-c1ce-4d02-a6a9-5a54c0f957ad
📒 Files selected for processing (3)
library/certificate_request.pymodule_utils/certificate_lsr/providers/certmonger.pytasks/main.yml
| run_before: "{{ item.run_before | default(omit) }}" | ||
| run_after: "{{ item.run_after | default(omit) }}" | ||
| ca: "{{ item.ca | default(omit) }}" | ||
| issuer: "{{ item.issuer | default(omit) }}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document issuer in defaults/main.yml.
This task now consumes item.issuer, so the certificate_requests item schema must list issuer as an optional key. Keep certificate_requests: [] unchanged.
As per path instructions, optional per-request fields forwarded by tasks/main.yml must be documented in defaults/main.yml.
🤖 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 `@tasks/main.yml` at line 126, Update the certificate_requests schema in
defaults/main.yml to document issuer as an optional per-request field, matching
its consumption in the tasks/main.yml certificate request flow. Keep the
existing certificate_requests: [] default unchanged.
Source: Path instructions
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "issuer|certificate_request|certmonger" tests tasks/main.yml library/certificate_request.py module_utils/certificate_lsr/providers/certmonger.pyRepository: linux-system-roles/certificate
Length of output: 11465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidates =="
fd -a 'tests_.*\.yml|assert_certificate_parameters.yml|tasks/main.yml|defaults/main.yml|library/certificate_request.py' . | sed 's#^\./##'
echo "== issuer occurrences in repo =="
rg -n '^\s*issuer:|issuer=|issuer|Certificate Requests|certificate_requests:' -S .
echo "== tasks/main.yml 110-132 =="
sed -n '110,132p' tasks/main.yml
echo "== library module param/examples around issuer =="
sed -n '70,90p' library/certificate_request.py
sed -n '370,385p' library/certificate_request.py
echo "== first issuer occurrences in tests =="
python3 - <<'PY'
import pathlib, re
target_re = re.compile(r'\bissuer\s*:')
for p in sorted(pathlib.Path('tests').glob('*.yml')):
text = p.read_text()
if target_re.search(text):
for i,line in enumerate(text.splitlines(),1):
if target_re.search(line):
print(f"{p}:{i}:{line}")
PYRepository: linux-system-roles/certificate
Length of output: 7463
Add regression coverage for issuer forwarding.
tasks/main.yml now forwards item.issuer to certificate_request, but the test suite has no certificate_requests entries with issuer:. Add tests for both a defined issuer and omitted issuer to validate the added task path.
🤖 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 `@tasks/main.yml` at line 126, Add regression coverage for the
certificate_requests task path in tasks/main.yml, including one entry with a
defined issuer and another without issuer. Assert that certificate_request
receives the issuer when provided and omits the parameter when absent, covering
the item.issuer default(omit) behavior.
Source: Path instructions
|
We will need a test for this - I think changing tests_subject.yml and tests/tasks/assert_certificate_parameters.yml to add and check the issuer field would be sufficient. |
|
Added test and updated README.md. I wasn't able to test that the CA actually issues a certificate with the correct issuer since the local self-signed CA just ignores the option, but I have validated that it works on my FreeIPA deployment. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/tests_issuer.yml (1)
2-40: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd reconciliation, failure, and cleanup coverage.
This test only verifies initial successful issuance. It does not exercise an existing certificate whose issuer changes, a failure/mismatch scenario, or cleanup in an
alwaysblock taggedtests::cleanup; stale certmonger state can therefore make reruns non-idempotent.As per path instructions, tests must cover success and failure, use cleanup in an
alwayssection, tag cleanup tasks withtests::cleanup, and remain rerunnable.🤖 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_issuer.yml` around lines 2 - 40, Add reconciliation coverage to the certificate test by rerunning issuance against the existing certificate after changing its issuer, and add an explicit failure/mismatch assertion. Structure cleanup in an always block, tag every cleanup task with tests::cleanup, and remove certificate and certmonger state so repeated runs remain idempotent. Preserve the existing successful issuance and verification through the current certificate_requests and certificates flows.Source: Path instructions
🧹 Nitpick comments (1)
module_utils/certificate_lsr/providers/certmonger.py (1)
218-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize the multiline condition with Black.
Please reformat this parenthesized condition and run
tox -e black,flake8before committing.As per path instructions, Python files must follow PEP 8, be formatted with Black, and pass
tox -e black,flake8.🤖 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 `@module_utils/certificate_lsr/providers/certmonger.py` around lines 218 - 219, Reformat the multiline condition in the certificate update logic using Black’s standard layout, preserving its existing boolean behavior. Then run tox -e black,flake8 and resolve any formatting or lint failures before committing.Source: Path instructions
🤖 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 `@module_utils/certificate_lsr/providers/certmonger.py`:
- Around line 210-219: Update the idempotency comparison in the certmonger
parameter flow around _get_certmonger_issuer_from_params and
_get_certmonger_issuer_for_existing_cert so an unset issuer does not differ from
persisted template-issuer metadata. When issuer is omitted, ignore the existing
issuer during the needs-update decision; retain normal comparison when issuer is
explicitly configured.
In `@README.md`:
- Line 68: Add a certificate_requests usage example in README.md that includes
the issuer field, preferably demonstrating a FreeIPA sub-CA configuration. Keep
the existing issuer field documentation and ensure the example clearly shows how
users provide this new variable.
In `@tests/tasks/assert_certificate_parameters.yml`:
- Around line 171-187: Fix the issuer-template assertion by correcting the
invalid `cert['issuer_template')` reference to use matching brackets, and add
the same `cert.issuer_template is defined` condition to the “Verify certificate
issuer template” task so it only runs when the field exists.
In `@tests/tests_issuer.yml`:
- Around line 17-20: Replace the direct include_role invocation in the
certificate setup task with the approved centrally managed role wrapper,
preserving the set_vars.yml behavior. Ensure tests/tests_issuer.yml contains no
direct include_role, import_role, or roles invocation.
---
Outside diff comments:
In `@tests/tests_issuer.yml`:
- Around line 2-40: Add reconciliation coverage to the certificate test by
rerunning issuance against the existing certificate after changing its issuer,
and add an explicit failure/mismatch assertion. Structure cleanup in an always
block, tag every cleanup task with tests::cleanup, and remove certificate and
certmonger state so repeated runs remain idempotent. Preserve the existing
successful issuance and verification through the current certificate_requests
and certificates flows.
---
Nitpick comments:
In `@module_utils/certificate_lsr/providers/certmonger.py`:
- Around line 218-219: Reformat the multiline condition in the certificate
update logic using Black’s standard layout, preserving its existing boolean
behavior. Then run tox -e black,flake8 and resolve any formatting or lint
failures before committing.
🪄 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: 01fbd6f6-6d28-4226-82a4-68780a14aecf
📒 Files selected for processing (4)
README.mdmodule_utils/certificate_lsr/providers/certmonger.pytests/tasks/assert_certificate_parameters.ymltests/tests_issuer.yml
| issuer_from_params = self._get_certmonger_issuer_from_params() | ||
| issuer_from_existing_cert = self._get_certmonger_issuer_for_existing_cert() | ||
| self.module.debug( | ||
| "issuer_from_params == issuer_from_existing_cert: {0}".format( | ||
| issuer_from_params == issuer_from_existing_cert | ||
| ) | ||
| ) | ||
|
|
||
| if (needs_update or ca_from_params != ca_from_existing_cert or | ||
| issuer_from_params != issuer_from_existing_cert): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${CERT_FILE:?}"
: "${CA:?}"
getcert list -f "$CERT_FILE" | grep 'issuer template' || true
getcert resubmit -f "$CERT_FILE" -c "$CA" -w
getcert list -f "$CERT_FILE" | grep 'issuer template' || trueRepository: linux-system-roles/certificate
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked file:"
git ls-files | rg '^module_utils/certificate_lsr/providers/certmonger\.py$' || true
echo
echo "Relevant excerpts around issuer handling:"
sed -n '180,235p' module_utils/certificate_lsr/providers/certmonger.py | cat -n -v
echo
sed -n '320,365p' module_utils/certificate_lsr/providers/certmonger.py | cat -n -v
echo
echo "Search issuer/resubmit/issue references:"
rg -n "issuer|template-issuer|resubmit|-X|get_certmgr|certmgr" module_utils/certificate_lsr/providers/certmonger.pyRepository: linux-system-roles/certificate
Length of output: 5514
🌐 Web query:
certmonger getcert resubmit issuer template-issuer -X omitted behavior
💡 Result:
When using the getcert resubmit command in certmonger, the -X (or --issuer) option is used to explicitly request a certificate from a specific issuer [1][2]. If the --issuer option is omitted, certmonger does not attempt to use a template-issuer by default; instead, it relies on the configuration already associated with the existing tracking request [3][4]. In standard operation, getcert resubmit targets an existing certificate tracking entry [3][5]. If the -i (nickname) option is provided, certmonger uses the settings stored for that specific request [3][5]. If the -i option is omitted, it attempts to match the tracking entry based on the provided key and certificate storage locations (e.g., -d, -n, or -f) [3][4]. When you resubmit, the operation uses the CA, profile/template, and issuer configurations that were established during the initial getcert request or via subsequent modifications, unless explicitly overridden by command-line arguments like -c (CA), -T (profile), or -X (issuer) [1][4]. If you do not specify an issuer, the existing, previously configured issuer for that tracking request remains in effect [3][5].
Citations:
- 1: https://manpages.debian.org/testing/certmonger/getcert-resubmit.1.en.html
- 2: https://www.mankier.com/1/getcert-resubmit
- 3: https://manpages.debian.org/stretch/certmonger/getcert-resubmit.1.en.html
- 4: https://hexmos.com/freedevtools/man-pages/user-commands/user-and-authentication/getcert-rekey/
- 5: https://linux.die.net/man/1/getcert-resubmit
🌐 Web query:
Red Hat certmonger -X --issuer optional resubmit CA profile stored settings documentation
💡 Result:
In Certmonger, there is no single "issuer optional" setting for resubmission; instead, certificate tracking entries store specific CA configurations that define how the certificate is requested and renewed [1][2]. When you run getcert resubmit, Certmonger uses the settings already associated with that specific tracking request [3][4]. If you need to change the profile (template) or issuer settings during resubmission, you can use flags in the getcert resubmit command to override or specify new parameters for that operation [3][4]. Key points regarding settings and resubmission: 1. Tracking Persistence: Certmonger stores the details of every tracking request (including CA, profile, and issuer information) in the filesystem, typically under /var/lib/certmonger/requests/ [5]. When you initiate a getcert resubmit -i <nickname>, Certmonger refers to these stored settings by default [3][4]. 2. Overriding CA Profiles: You can change the certificate profile used for a resubmission by using the -T (template/profile) option [3][6]. For example: getcert resubmit -i <nickname> -T <new_profile> [3] 3. Overriding CA/Issuer: If you need to submit to a different CA or specify a different issuer context, you can use the -c option to specify a different CA configuration [3][6]. Note that the "issuer" itself is generally determined by the CA that receives the request, rather than being a manual configuration option you set directly in the client [1]. 4. Advanced Parameter Passing: When using Dogtag-based CAs, you can pass additional parameters to the server during submission using the -O flag (via dogtag-submit) [7]. This allows for overriding server-supplied defaults or providing specific values required by the enrollment profile [8][7]. For complex reconfigurations, it is often recommended to verify the current tracking status with getcert list or getcert list-cas to ensure the nickname and CA configuration match your expectations before resubmitting [1][9].
Citations:
- 1: https://www.freeipa.org/page/Certmonger
- 2: https://docs.pagure.org/certmonger/
- 3: https://manpages.debian.org/stretch/certmonger/getcert-resubmit.1.en.html
- 4: https://manpages.ubuntu.com/manpages/xenial/man1/getcert-resubmit.1.html
- 5: https://manpages.debian.org/trixie/certmonger/certmonger.8.en.html
- 6: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/10/html/managing_certificates_in_idm/obtaining-an-idm-certificate-for-a-service-using-certmonger-assembly
- 7: https://www.mankier.com/8/certmonger-dogtag-submit
- 8: https://www.mankier.com/8/certmonger-dogtag-ipa-renew-agent-submit
- 9: https://manpages.opensuse.org/Tumbleweed/certmonger/getcert.1.en.html
Ignore unset issuer for certmonger idempotency.
issuer is optional and the resubmit command only adds -X when it is set, while template-issuer persists in certmonger metadata. Omitting -X leaves the stored issuer/profile in effect, so comparing None to a stored issuer makes the role resubmit on every run. Handle unset issuer explicitly, e.g. by ignoring existing template-issuer for idempotency or providing a reset path.
🤖 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 `@module_utils/certificate_lsr/providers/certmonger.py` around lines 210 - 219,
Update the idempotency comparison in the certmonger parameter flow around
_get_certmonger_issuer_from_params and _get_certmonger_issuer_for_existing_cert
so an unset issuer does not differ from persisted template-issuer metadata. When
issuer is omitted, ignore the existing issuer during the needs-update decision;
retain normal comparison when issuer is explicitly configured.
| |----------------------|---------------------------------------------------------------------------------------------------|:-----------:|:--------:|-------------------------| | ||
| | name | Name of the certificate. A full path can be used to choose the directory where files will be stored.| str | yes | - | | ||
| | ca | CA that will issue the certificate. See [CAs and Providers](#cas-and-providers). | str | yes | - | | ||
| | issuer | The name of the issuer at the CA that will issue the certificate. The interpretation of the issuer may vary depending on each provider, and may be ignored if the CA only has one or a default issuer. | str | no | - | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an issuer usage example.
The new field is documented, but README.md should also show a certificate_requests example using issuer, preferably for a FreeIPA sub-CA scenario.
As per path instructions, README.md must document all new user-facing variables and include usage examples for new functionality.
🤖 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 `@README.md` at line 68, Add a certificate_requests usage example in README.md
that includes the issuer field, preferably demonstrating a FreeIPA sub-CA
configuration. Keep the existing issuer field documentation and ensure the
example clearly shows how users provide this new variable.
Source: Path instructions
| - name: Retrieve issuer template | ||
| shell: >- | ||
| set -euo pipefail; | ||
| getcert list -f {{ cert['path'] }} | | ||
| grep 'issuer template' | | ||
| sed 's/^\s\+issuer template: //g' | ||
| register: result | ||
| changed_when: false | ||
| when: cert.issuer_template is defined | ||
|
|
||
| - name: Verify certificate issuer template | ||
| assert: | ||
| that: | ||
| - cert['issuer_template') | trim == result.stdout | trim | ||
| fail_msg: >- | ||
| {{ cert['issuer_template'] | trim }} != | ||
| {{ result.stdout | trim }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the issuer-template assertion and gate it consistently.
Line 184 has invalid Jinja syntax () instead of ]). Also, when: cert.issuer_template is defined only guards retrieval; the assertion still runs for certificates without that field and accesses a missing key.
Proposed fix
- name: Verify certificate issuer template
assert:
that:
- - cert['issuer_template') | trim == result.stdout | trim
+ - cert['issuer_template'] | trim == result.stdout | trim
fail_msg: >-
{{ cert['issuer_template'] | trim }} !=
{{ result.stdout | trim }}
+ when: cert.issuer_template is defined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Retrieve issuer template | |
| shell: >- | |
| set -euo pipefail; | |
| getcert list -f {{ cert['path'] }} | | |
| grep 'issuer template' | | |
| sed 's/^\s\+issuer template: //g' | |
| register: result | |
| changed_when: false | |
| when: cert.issuer_template is defined | |
| - name: Verify certificate issuer template | |
| assert: | |
| that: | |
| - cert['issuer_template') | trim == result.stdout | trim | |
| fail_msg: >- | |
| {{ cert['issuer_template'] | trim }} != | |
| {{ result.stdout | trim }} | |
| - name: Retrieve issuer template | |
| shell: >- | |
| set -euo pipefail; | |
| getcert list -f {{ cert['path'] }} | | |
| grep 'issuer template' | | |
| sed 's/^\s\+issuer template: //g' | |
| register: result | |
| changed_when: false | |
| when: cert.issuer_template is defined | |
| - name: Verify certificate issuer template | |
| assert: | |
| that: | |
| - cert['issuer_template'] | trim == result.stdout | trim | |
| fail_msg: >- | |
| {{ cert['issuer_template'] | trim }} != | |
| {{ result.stdout | trim }} | |
| when: cert.issuer_template is defined |
🤖 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/tasks/assert_certificate_parameters.yml` around lines 171 - 187, Fix
the issuer-template assertion by correcting the invalid
`cert['issuer_template')` reference to use matching brackets, and add the same
`cert.issuer_template is defined` condition to the “Verify certificate issuer
template” task so it only runs when the field exists.
|
[citest] |
| if needs_update or ca_from_params != ca_from_existing_cert: | ||
|
|
||
| issuer_from_params = self._get_certmonger_issuer_from_params() | ||
| issuer_from_existing_cert = self._get_certmonger_issuer_for_existing_cert() |
There was a problem hiding this comment.
| issuer_from_existing_cert = self._get_certmonger_issuer_for_existing_cert() | |
| issuer_from_existing_cert = self._get_certmonger_issuer_for_existing_cert() | |
| issuer_needs_update = ( | |
| issuer_from_params is not None | |
| and issuer_from_params != issuer_from_existing_cert | |
| ) |
| ) | ||
|
|
||
| if (needs_update or ca_from_params != ca_from_existing_cert or | ||
| issuer_from_params != issuer_from_existing_cert): |
There was a problem hiding this comment.
| issuer_from_params != issuer_from_existing_cert): | |
| issuer_needs_update): |
| - name: Verify certificate issuer template | ||
| assert: | ||
| that: | ||
| - cert['issuer_template') | trim == result.stdout | trim |
There was a problem hiding this comment.
| - cert['issuer_template') | trim == result.stdout | trim | |
| - cert['issuer_template'] | trim == result.stdout | trim |
| - cert['issuer_template') | trim == result.stdout | trim | ||
| fail_msg: >- | ||
| {{ cert['issuer_template'] | trim }} != | ||
| {{ result.stdout | trim }} |
There was a problem hiding this comment.
| {{ result.stdout | trim }} | |
| {{ result.stdout | trim }} | |
| when: cert.issuer_template is defined |
|
I added some suggestions which should fix the errors. You'll also need to format the python code with python |
spetrosi
changed the title
3 participants
Enhancement:
Implement support for passing an issuer argument to the provider.
Reason:
This enables the usage of FreeIPA sub-CAs, which use the "IPA" CA in certmonger but have a separate issuer.
Result:
Users can issue certificates signed by IPAs sub-CAs.
Issue Tracker Tickets (Jira or BZ if any):
Summary by CodeRabbit
New Features
issuersupport for certificate requests, including per-certificate configuration.issuerduring certificate issuance when supported.Bug Fixes
issuermismatches and triggers updates when necessary.Documentation & Tests
issuerparameter in the certificate requests variables table.