🐛 bug: enforce CertClientFile for AutoCertManager TLS#4312
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughApp.Listen centralizes mTLS client-CA handling by calling new applyClientCert after choosing/cloning tls.Config, removes inline CertClientFile logic from the CertFile branch, and adds tests asserting errors for missing/invalid client CA files and that TLS handler is not attached on error. ChangesmTLS Client-Certificate Refactoring
Sequence Diagram(s)sequenceDiagram
participant App
participant tlsConfig
participant applyClientCert
participant FileSystem
App->>tlsConfig: select or clone base tls.Config
App->>applyClientCert: applyClientCert(tlsConfig, CertClientFile)
applyClientCert->>FileSystem: read CertClientFile (PEM)
applyClientCert->>tlsConfig: set ClientCAs and ClientAuth
App->>App: SetTLSHandler (if tlsHandler non-nil)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a regression where CertClientFile was not enforced when using AutoCertManager, potentially disabling mTLS client-certificate verification for that path.
Changes:
- Centralized client CA loading and TLS client-auth configuration in
applyClientCert(...). - Applied
CertClientFileconsistently to any internally-createdtls.Config(includingAutoCertManager). - Added a regression test ensuring
AutoCertManager+ invalid/missingCertClientFilefails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| listen.go | Moves client CA application into a shared helper and applies it for all internal TLS config branches. |
| listen_test.go | Adds a regression test covering AutoCertManager combined with CertClientFile. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the client certificate application logic into a dedicated applyClientCert function, allowing mTLS to be configured alongside both manual TLS settings and AutoCertManager. A new test case was added to verify error handling for missing certificate files. Review feedback suggests ensuring that an error is returned if a client certificate file is provided without any TLS configuration to prevent silent security downgrades. Additionally, it is recommended to verify the success of certificate parsing in applyClientCert and to include filenames in error messages for better observability.
![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
🤖 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 `@listen.go`:
- Around line 270-271: The code currently calls
clientCertPool.AppendCertsFromPEM(clientCACert) without checking its boolean
return; update the logic around clientCertPool, AppendCertsFromPEM and the usage
of ClientCAs/ClientAuth (RequireAndVerifyClientCert) to validate the return
value and fail early: call ok := clientCertPool.AppendCertsFromPEM(clientCACert)
and if !ok return or propagate a clear error (or log and exit) indicating the
client CA PEM was invalid or contained no certificates so ClientCAs would be
empty; follow the same validation pattern used in internal/tlstest/tls.go to
surface configuration problems instead of letting TLS handshakes fail silently.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 160d708a-85c0-4f01-91bd-f5dd9ab72a5d
📒 Files selected for processing (2)
listen.golisten_test.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/38fe3ebd-ac57-4290-8cbc-5dedb7601a60 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4312 +/- ##
==========================================
+ Coverage 91.33% 91.39% +0.05%
==========================================
Files 132 132
Lines 13089 13098 +9
==========================================
+ Hits 11955 11971 +16
+ Misses 716 710 -6
+ Partials 418 417 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot The new tests added will fail on Windows: === FAIL: . Test_Listen_AutoCert_WithClientCertFile/invalid_client_CA_pem (0.00s) === FAIL: . Test_Listen_AutoCert_WithClientCertFile (0.00s) I think we can just check for |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/12b336e7-50ce-4bef-b4fa-38bcaea34649 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Updated in 7170b88 to assert on |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/c36b8ed0-2ea6-43fd-9fc1-8b2e5ff74bf1 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Motivation
CertClientFilehandling into the CertFile/CertKeyFile branch only, which causedAutoCertManager+CertClientFiledeployments to silently disable mTLS client-certificate verification.Description
applyClientCert(tlsConfig *tls.Config, certClientFile string) errorto centralize reading the client CA file and settingClientAuth/ClientCAs.applyClientCert(...)for any internally-createdtls.Config(after the TLS branch switch and beforeTLSConfigFunc) so theAutoCertManagerpath honorsCertClientFile.Test_Listen_AutoCert_WithClientCertFileto verifyAutoCertManagercombined withCertClientFileis consumed and returns an error when the client CA file is invalid.