Support custom TLS certs for generating CRSs#253
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentral deployment now aggregates internal and optional custom CA certificates in-process, then uses them in enhanced Central TLS verification with logging, port-forward handling, hostname fallback, and combined verification errors. ChangesCentral TLS handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/deployer/crs.go (1)
108-116: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle non-certificate PEM blocks and parsing errors gracefully.
By manually decoding the PEM file to log certificate details, the code loses the robustness of
x509.CertPool.AppendCertsFromPEM, which silently skips non-certificate blocks and parsing errors. If a user provides a custom CA bundle that includes extraneous blocks (e.g.,PRIVATE KEY,TRUSTED CERTIFICATE, or malformed entries), the current code will fatally fail the deployment.Check the block type and log a warning instead of failing when a block cannot be parsed.
🛡️ Proposed fix
for block, rest := pem.Decode(pemData); block != nil; block, rest = pem.Decode(rest) { + if block.Type != "CERTIFICATE" { + continue + } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { - return nil, fmt.Errorf("parsing CA certificate from %s: %w", d.roxCACertFile, err) + d.logger.Warningf("Skipping invalid certificate in %s: %v", d.roxCACertFile, err) + continue } pool.AddCert(cert) caCertsAdded++ d.logger.Dimf("CA cert #%d: Subject.CN=%q, Issuer.CN=%q, SubjectKeyId=%x", caCertsAdded, cert.Subject.CommonName, cert.Issuer.CommonName, cert.SubjectKeyId) }🤖 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 `@internal/deployer/crs.go` around lines 108 - 116, Update the PEM decoding loop in the CA-loading flow to process only certificate blocks, skipping non-certificate types such as PRIVATE KEY or TRUSTED CERTIFICATE. When x509.ParseCertificate fails for a certificate block, log a warning through d.logger and continue processing remaining blocks instead of returning an error; retain successful pool.AddCert, count, and certificate-detail logging.
🧹 Nitpick comments (1)
internal/deployer/crs.go (1)
248-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicate work and duplicate errors when chain validation fails.
If the initial verification fails due to chain validation (e.g.,
x509.UnknownAuthorityError) rather than a hostname mismatch, retrying the verification with a differentDNSNamewill fail again with the exact same error. This results in duplicate cryptographic work and causeserrors.Join(err, fallbackErr)to output two identical error strings, which can confuse users troubleshooting TLS issues.Consider falling back to
central.stackroxonly if the initial error is ax509.HostnameError.♻️ Proposed refactor
_, err := leaf.Verify(verifyOpts) if err == nil { return nil } + + var hostnameErr x509.HostnameError - if !isACentralCert(leaf) { + if !errors.As(err, &hostnameErr) || !isACentralCert(leaf) { return err }🤖 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 `@internal/deployer/crs.go` around lines 248 - 254, Update the verification fallback logic around leaf.Verify in the certificate validation flow to retry with central.stackrox only when the initial error is an x509.HostnameError. Return other verification errors, including chain-validation failures, immediately without performing the fallback or joining duplicate errors.
🤖 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 `@internal/deployer/deploy_via_operator.go`:
- Around line 596-602: Update the CA PEM assembly around the caCertFile writes
to normalize every block, including internalCA and each caPEMs entry, so it ends
with exactly one newline before the next block is written. Reuse bytes to trim
existing trailing newline characters and append one separator, ensuring
pem.Decode can recognize all concatenated certificates.
---
Outside diff comments:
In `@internal/deployer/crs.go`:
- Around line 108-116: Update the PEM decoding loop in the CA-loading flow to
process only certificate blocks, skipping non-certificate types such as PRIVATE
KEY or TRUSTED CERTIFICATE. When x509.ParseCertificate fails for a certificate
block, log a warning through d.logger and continue processing remaining blocks
instead of returning an error; retain successful pool.AddCert, count, and
certificate-detail logging.
---
Nitpick comments:
In `@internal/deployer/crs.go`:
- Around line 248-254: Update the verification fallback logic around leaf.Verify
in the certificate validation flow to retry with central.stackrox only when the
initial error is an x509.HostnameError. Return other verification errors,
including chain-validation failures, immediately without performing the fallback
or joining duplicate errors.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 4a4c6ae4-23ab-4539-8a98-dd4785d582bd
📒 Files selected for processing (2)
internal/deployer/crs.gointernal/deployer/deploy_via_operator.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai, try again |
|
✅ Action performedReview finished.
|
|
@coderabbitai, I have addressed your feedback. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Co-authored-by: Vlad Bologa <vbologa@redhat.com>
|
@vladbologa Addressed + contributed an e2e test. |
2 participants
Summary by CodeRabbit
127.0.0.1) and Central-issued certificates (with hostname verification fallback).