Conversation
WalkthroughAdds ChangesCertificate Validation Utilities and CLI Integration
Sequence Diagram(s)sequenceDiagram
participant bootstrap as bootstrap.sh step_validate
participant cli as enclave tools CLI
participant check_chains as check_certificate_chains
participant cert_utils as cert_utils
participant openssl as openssl subprocess
bootstrap->>cli: check-certificate-chains --config certs_vars
cli->>check_chains: check_certificate_chains_main(config)
check_chains->>check_chains: read+parse YAML, extract sslCACertificate + chain fields
loop each chain field (API, Ingress)
check_chains->>cert_utils: pem_blocks(chain_pem)
cert_utils-->>check_chains: list of PEM blocks
check_chains->>cert_utils: is_self_signed(last_cert)
cert_utils->>openssl: x509 issuer/subject
openssl-->>cert_utils: issuer= / subject= output
cert_utils-->>check_chains: True / False
opt sslCACertificate present
check_chains->>cert_utils: openssl_verify(ca_pem, last_cert)
cert_utils->>openssl: verify -no-CAfile -no-CApath
openssl-->>cert_utils: return code
cert_utils-->>check_chains: True / False
end
check_chains-->>check_chains: collect issue strings
end
alt issues found
check_chains-->>cli: raise CertificateValidationError
cli-->>bootstrap: non-zero exit + error message
else all pass
check_chains-->>cli: success
cli-->>bootstrap: "Certificate chain check passed." (stdout)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="rh-ecosystem-edge/enclave"
PR_NUM=535
echo "=== PR Details ==="
gh pr view "$PR_NUM" --repo "$REPO" --json title,body,baseRefName,headRefName,files,state
echo ""
echo "=== PR Diff ==="
gh pr diff "$PR_NUM" --repo "$REPO"Length of output: 30263 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== bootstrap.sh head (first 30 lines) ==="
head -50 bootstrap.sh
echo ""
echo "=== pipefail or errexit settings in bootstrap.sh ==="
grep -n "pipefail\|set -e\|set -o" bootstrap.sh || echo "(none found)"
echo ""
echo "=== certs_vars definition in bootstrap.sh ==="
grep -n "certs_vars" bootstrap.sh || echo "(none found)"
echo ""
echo "=== Full step_validate function ==="
grep -n -A 20 "step_validate" bootstrap.sh || echo "(none found)"
echo ""
echo "=== openssl availability check / dependency handling ==="
grep -rn "openssl" src/enclave/tools/ || echo "(none)"
echo ""
echo "=== _CHAIN_FIELDS in validate_certificates.py (full context) ==="
grep -n "_CHAIN_FIELDS\|sslCA\|sslAPI\|sslIngress" src/enclave/tools/validate_certificates.py
echo ""
echo "=== ca_pem flow in quay_registry_ca.py ==="
grep -n "ca_pem\|is_self_signed" src/enclave/tools/quay_registry_ca.pyLength of output: 6998
Review: Complete Certificate Chain with Root CA for External RegistriesOverall risk: MEDIUM — the approach is sound and the 🔴 Medium —
|
Contributor
Author
|
✨ Claude Code: Thank you for the review. Here is how each point was addressed: 🔴 Medium — 🔴 Medium — 🔴 Medium — 🟡 Low — 🟡 Low — ℹ️ Nit — Duplicate timeout constant — Deliberate design. @coderabbitai approve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@operators/cincinnati-operator/tasks.yaml`:
- Around line 89-131: The three debug tasks that print UpdateService CR status,
pod status, and pod logs are exposing sensitive information such as tokens,
endpoints, and customer data in CI/CD artifacts. Remove or conditionally gate
the debug tasks named "Show UpdateService CR status", "Show UpdateService pods
status", and "Show UpdateService pod logs" that output the full stdout content
from the respective command registrations (r_updateservice_cr_status.stdout,
r_updateservice_pods_status.stdout, r_updateservice_pod_logs.stdout). Instead,
either eliminate these debug statements entirely or restructure them to only log
high-level status information (success/failure) without exposing raw YAML or log
content.
- Around line 65-70: The until condition for the r_update_service_deployment
task is missing a critical check to ensure the Deployment controller has
observed the latest spec changes. Add a verification that
status.observedGeneration equals metadata.generation to the until condition
alongside the existing replica count checks. This ensures that the condition
only succeeds when the controller has processed the latest spec (indicated by
observedGeneration matching the current metadata.generation), preventing false
positives from stale status that existed before the spec update.
- Around line 2-5: The task "Set UpdateService replicas" hardcodes
updateservice_replicas to 1 using set_fact, which prevents inventory and group
variables from configuring environment-specific replica counts. Remove the
hardcoded set_fact entirely and instead define updateservice_replicas in
appropriate group_vars or inventory variable files for each environment, or
alternatively add a conditional to the set_fact task so it only sets the value
if the variable is not already defined from higher-precedence sources.
In `@playbooks/tasks/trust_quay_registry_ca_for_image_config.yaml`:
- Around line 14-18: The argv variable in the
trust_quay_registry_ca_for_image_config task uses a folded scalar (>-) which
causes the Jinja2 list template to render as a string representation instead of
a native list object. The ansible.builtin.command module requires argv to be a
native list. Replace the folded scalar (>-) syntax with explicit YAML list
format (using dashes for each list item) to ensure argv is properly rendered as
a native list object, similar to the pattern used elsewhere in the playbook.
In `@src/enclave/tools/cert_utils.py`:
- Around line 32-35: In the exception handler for subprocess.TimeoutExpired and
OSError (around the logger.warning call), replace the `return False` statement
with `raise RuntimeError("OpenSSL unavailable: %s" % exc)` or similar. This
makes the subprocess/environment failure explicit to callers in
validate_certificates.py and quay_registry_ca.py so they can distinguish between
actual certificate properties and detection failures, rather than conflating a
tooling error with an actual "not self-signed" result.
In `@src/enclave/tools/quay_registry_ca.py`:
- Around line 119-125: The function currently logs a warning when
_openssl_verify fails but still appends the unverified ca_pem to the trust
bundle and returns it, allowing invalid certificates to flow downstream. Replace
the warning log with a RuntimeError that should be raised immediately when
_openssl_verify returns False, ensuring the function fails fast instead of
returning an unverified certificate chain. Remove the return statement that
combines ca_certs and ca_pem, as it should only execute when verification
succeeds. Update any existing tests that expect the function to return
unverified certificates to instead expect a RuntimeError to be raised.
In `@src/enclave/tools/validate_certificates.py`:
- Around line 25-39: The certificate validation code is raising generic
RuntimeError and TypeError exceptions at multiple locations (the OSError
handler, the yaml.YAMLError handler, and the type check), which weakens the
failure contract. Create a custom exception class specifically for certificate
validation failures (such as CertificateValidationError), then replace all
instances of raising RuntimeError and TypeError throughout the file with this
custom exception while preserving the existing descriptive error messages that
reference config_path and error details. This applies to all error handlers in
the validation flow including the file read exception, YAML parse exception, and
type validation checks.
- Around line 54-66: The is_self_signed() function at line 54 returns False in
both cases where the certificate is genuinely not self-signed and when OpenSSL
is unavailable or times out, making it impossible to distinguish between a true
validation failure and a tooling failure. Refactor is_self_signed() to return a
distinct failure state (such as raising an exception or returning a three-state
value) instead of just a boolean, then update the condition block that checks
is_self_signed(certs[-1]) to handle the failure case explicitly by failing
closed (adding an issue or logging an error) when the tooling check cannot be
completed rather than proceeding with insufficient information.
- Around line 41-57: The condition on line 55 that checks `if ca_pem:` only
validates that sslCACertificate is non-empty, allowing malformed or invalid PEM
data to be treated as a valid trust anchor. This causes incomplete-chain
validation to be skipped, deferring the error to later TLS handshakes. Before
accepting ca_pem as a remediation signal, validate its structure using the
pem_blocks function (the same function already used to parse chain_pem earlier
in the loop). Modify the condition to check both that ca_pem is non-empty AND
that it contains valid PEM blocks, ensuring only properly formatted certificate
data suppresses the incomplete-chain error.
In `@src/tests/test_quay_registry_ca.py`:
- Around line 164-170: The test
`test_chain_trust_anchor_pem_appends_ca_pem_when_chain_incomplete` is not
mocking the `_openssl_verify` function, which causes it to execute a real
OpenSSL subprocess when `_chain_trust_anchor_pem` is called with the `ca_pem`
parameter. Add a mocker.patch call for `_openssl_verify` in this test (following
the same pattern used in other tests in this file) to keep the unit test
hermetic and prevent environment dependencies that can cause CI flakiness.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4c85e6a1-7e70-4994-9bf7-96b5498f52a7
📒 Files selected for processing (12)
bootstrap.shoperators/cincinnati-operator/tasks.yamlplaybooks/tasks/trust_quay_registry_ca_for_image_config.yamlsrc/enclave/tools/cert_utils.pysrc/enclave/tools/cli.pysrc/enclave/tools/quay_registry_ca.pysrc/enclave/tools/validate_certificates.pysrc/tests/test_cert_utils.pysrc/tests/test_quay_registry_ca.pysrc/tests/test_tools_cli.pysrc/tests/test_validate_certificates.pyvalidations.sh
|
No residual medium or high-risk items. Proceeding with approval. 🐇 ✨ ✅ Action performedComments resolved and changes approved. |
rporres
force-pushed
the
531-again
branch
2 times, most recently
from
deac5f8 to
5991aed
Compare
github-actions
Bot
added
deployment
rporres
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/CONFIGURATION_REFERENCE.md`:
- Around line 1092-1111: The markdown in the certificate-chain validation
section violates MD031 because the fenced examples are not preceded by blank
lines. Update the documentation around the existing example blocks in
CONFIGURATION_REFERENCE.md so there is a blank line before each fenced block,
including the yaml example under the “Example” label and the bash command under
“To validate manually”; keep the surrounding headings and text unchanged.
In `@src/enclave/tools/cert_utils.py`:
- Around line 63-87: The self-signed check in is_self_signed() is currently
based only on issuer/subject DN equality, which can incorrectly mark a crafted
certificate as complete. Keep the existing DN comparison as a fast precheck, but
add a real self-signature verification step in the same function before
returning True, using a cryptographic check such as an openssl verify flow with
-check_ss_sig. Update the logic so chain_trust_anchor_pem() only treats a cert
as self-signed when is_self_signed() confirms both the DN match and the
signature validity.
In `@src/enclave/tools/cli.py`:
- Around line 47-61: The certificates.yaml handling in cli.py can crash with an
AttributeError when yaml.safe_load returns a non-mapping value, because the code
calls raw.get in the certificates_config block without verifying the parsed
object type. Update the certificates_config parsing logic to validate that raw
is a dict before accessing sslCACertificate, and if it is not, raise a
ClickException with a clear message; mirror the isinstance(raw, dict) safeguard
already used in check_certificate_chains to keep the CLI error path user-facing
and consistent.
In `@src/tests/test_tools_cli.py`:
- Around line 75-120: The resolve-quay-registry-ca coverage is missing a
regression test for a non-mapping certificates.yaml input, which is the crash
case in cli.py. After adding the isinstance(raw, dict) guard in
resolve_quay_registry_ca, add a test alongside the existing CliRunner cases that
writes a YAML sequence/scalar file, invokes the resolve-quay-registry-ca
command, and asserts a non-zero exit plus a clear error message mentioning the
expected YAML mapping.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: be5ffd87-eed4-499c-9f4e-b773231f1520
📒 Files selected for processing (14)
bootstrap.shconfig/certificates.example.yamldocs/CONFIGURATION_REFERENCE.mddocs/DEPLOYMENT_GUIDE.mdplaybooks/tasks/trust_quay_registry_ca_for_image_config.yamlsrc/enclave/tools/cert_utils.pysrc/enclave/tools/check_certificate_chains.pysrc/enclave/tools/cli.pysrc/enclave/tools/quay_registry_ca.pysrc/tests/test_cert_utils.pysrc/tests/test_check_certificate_chains.pysrc/tests/test_quay_registry_ca.pysrc/tests/test_tools_cli.pyvalidations.sh
The previous TLS chain completion for resolve-quay-registry-ca relied on scanning the LZ system trust store: O(n) openssl subprocess calls per CA in the bundle, silent failure for custom/private CAs absent from the store, and a result dependent on trust store state that may be stale in long-running disconnected deployments. Replace the system store lookup with an explicit CA from sslCACertificate, passed via --certificates-config (reads from certificates.yaml) or --ca-pem (inline PEM) on resolve-quay-registry-ca; the options are mutually exclusive. The Ansible task that invokes the command is updated accordingly. Add check-certificate-chains (enclave tools subcommand) to validate chain completeness and CA consistency before installation: - chain ends with a non-self-signed certificate and sslCACertificate is absent → error - only a leaf certificate present and sslCACertificate is absent → error - chain ends with a self-signed root but sslCACertificate does not verify it → error (CA hierarchy mismatch) Certificate utilities (openssl_verify, is_self_signed, pem_blocks) are consolidated in cert_utils.py for reuse across tools. validations.sh gains checkCACert() for PEM format and expiry checks. Documentation and example config updated to clarify when sslCACertificate is required and how chain validation works. Signed-off-by: Rafa Porres Molina <rporresm@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>
rporres
changed the title
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
1 participant
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.
The previous TLS chain completion for resolve-quay-registry-ca relied on
scanning the LZ system trust store: O(n) openssl subprocess calls per CA
in the bundle, silent failure for custom/private CAs absent from the store,
and a result dependent on trust store state that may be stale in
long-running disconnected deployments.
Replace the system store lookup with an explicit CA from sslCACertificate,
passed via --certificates-config (reads from certificates.yaml) or --ca-pem
(inline PEM) on resolve-quay-registry-ca; the options are mutually
exclusive. The Ansible task that invokes the command is updated accordingly.
Add check-certificate-chains (enclave tools subcommand) to validate chain
completeness and CA consistency before installation:
absent → error
it → error (CA hierarchy mismatch)
Certificate utilities (openssl_verify, is_self_signed, pem_blocks) are
consolidated in cert_utils.py for reuse across tools. validations.sh gains
checkCACert() for PEM format and expiry checks. Documentation and example
config updated to clarify when sslCACertificate is required and how chain
validation works.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation