🔥 feat: add PEM-based TLS support for ListenConfig#4021
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds in-memory PEM-based TLS support to ListenConfig via new CertPEM, CertKeyPEM, and CertClientPEM fields; enforces mutual exclusivity with file-based TLS fields; extends TLS initialization and client-CA handling; and updates docs and tests for PEM usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Fiber Listen
participant TLS as tls.Config / TLSHandler
participant CertSource as Cert Source (PEM/File/AutoCertManager)
Client->>Server: Connect (TLS handshake)
Server->>TLS: Use tls.Config / call GetCertificate
TLS->>CertSource: Request certificate (PEM/File/Auto/CertManager)
CertSource-->>TLS: Return certificate (from CertPEM/KeyPEM, CertFile/KeyFile, or AutoCertManager)
TLS-->>Server: Complete handshake (apply ClientAuth if configured)
Server-->>Client: Accept connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
==========================================
- Coverage 91.22% 91.20% -0.03%
==========================================
Files 119 119
Lines 11000 11057 +57
==========================================
+ Hits 10035 10084 +49
- Misses 610 617 +7
- Partials 355 356 +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:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding support for PEM-based TLS configuration, allowing TLS setup from in-memory data. The implementation is robust, with thorough validation to handle various configuration scenarios and prevent ambiguity between PEM and file-based inputs. The documentation updates are clear, and the new unit tests provide good coverage for both happy paths and validation logic. I've included a few suggestions to simplify some of the validation logic and refactor a duplicated code block for better maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding support for PEM-based TLS configuration, which enhances flexibility for secret management. The implementation is robust, featuring comprehensive validation logic and extensive test coverage that addresses numerous edge cases. The documentation has also been updated clearly to reflect these new capabilities.
My review focuses on improving the structure and readability of the new validation logic in listen.go. I've identified a couple of opportunities to refactor for better maintainability by reducing code duplication and simplifying complex conditional logic. My suggestions aim to make the code cleaner and more straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
listen_test.go (1)
532-573: Addt.Parallel()at the start of each test function to follow guidelines.Test data files are verified to exist at
.github/testdata/. However, bothTest_Listen_MutualTLSandTest_Listen_MutualTLS_Preforkare missingt.Parallel()calls at the beginning. Per coding guidelines for**/*_test.gofiles, each test must invoket.Parallel()at the start to maximize concurrency.
♻️ Duplicate comments (2)
listen_test.go (1)
25-45: Unused variable declaration on line 33.The
var err errordeclaration on line 33 is unnecessary. The:=on lines 35 and 38 will handle the assignment correctly sincecertPEMandkeyPEMare new variables that require the short declaration form.🔧 Suggested fix
func readTLSPEM(t *testing.T, certPath, keyPath string) tlsPEM { t.Helper() - var err error - certPEM, err := os.ReadFile(filepath.Clean(certPath)) require.NoError(t, err) keyPEM, err := os.ReadFile(filepath.Clean(keyPath)) require.NoError(t, err)listen.go (1)
207-229: Unreachable code block at lines 227-229.After the validation at lines 199-205, either both
hasCertPEMandhasCertKeyPEMare true (or both false), and similarly forhasCertFile/hasCertKeyFile. This means:
(hasCertPEM || hasCertKeyPEM)≡hasServerPEM(hasCertFile || hasCertKeyFile)≡hasServerFileThe condition on line 227 becomes:
hasServerPEM && hasServerFile && (!hasServerPEM || !hasServerFile), which is always false when bothhasServerPEMandhasServerFileare true.🔧 Suggested fix: remove unreachable code
if hasServerPEM && hasServerFile { certFileBytes, err := os.ReadFile(filepath.Clean(cfg.CertFile)) if err != nil { return fmt.Errorf("tls: cannot read certFile=%q to compare with CertPEM: %w", cfg.CertFile, err) } if !bytes.Equal(certFileBytes, cfg.CertPEM) { return fmt.Errorf("tls: CertPEM does not match certFile=%q", cfg.CertFile) } certKeyFileBytes, err := os.ReadFile(filepath.Clean(cfg.CertKeyFile)) if err != nil { return fmt.Errorf("tls: cannot read keyFile=%q to compare with CertKeyPEM: %w", cfg.CertKeyFile, err) } if !bytes.Equal(certKeyFileBytes, cfg.CertKeyPEM) { return fmt.Errorf("tls: CertKeyPEM does not match keyFile=%q", cfg.CertKeyFile) } } - - if (hasCertPEM || hasCertKeyPEM) && (hasCertFile || hasCertKeyFile) && (!hasServerPEM || !hasServerFile) { - return errors.New("tls: provide either CertPEM/CertKeyPEM or CertFile/CertKeyFile, not a mix") - }
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring TLS/mTLS from in-memory PEM data, allowing applications to use certificates from environment variables or secrets managers without writing files to disk.
Changes:
- Added three new fields to
ListenConfig:CertPEM,CertKeyPEM, andCertClientPEMfor PEM-encoded certificate data - Implemented comprehensive validation logic to ensure cert/key pairs are complete and to detect/verify when both PEM and file-based inputs are provided
- Updated TLS configuration logic to support PEM-based certificates with clear precedence (PEM takes priority over files when both are present and match)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| listen.go | Added new PEM fields to ListenConfig, implemented validation logic for PEM/file combinations, refactored TLS configuration to support PEM data with proper precedence |
| listen_test.go | Added helper functions (tlsPEM struct, readTLSPEM) at top of file, comprehensive tests for PEM-based TLS flows and validation error paths, updated mTLS tests to use client.pem |
| docs/api/fiber.md | Documented new CertPEM, CertKeyPEM, and CertClientPEM fields in config table, added example for TLS with PEM data |
| docs/whats_new.md | Added example showing PEM-based TLS configuration using environment variables |
| .github/testdata/client.pem | Added new client certificate file for mTLS testing (replaces ca-chain.cert.pem references) |
Motivation
Description
CertPEM,CertKeyPEM, andCertClientPEMfields toListenConfigand documented them indocs/api/fiber.mdanddocs/whats_new.md.App.Listento require matching cert/key pairs, forbid ambiguous mixes of PEM and file inputs, compare file contents with supplied PEM when both are provided, and return clear errors on mismatch.tls.X509KeyPair(cfg.CertPEM, cfg.CertKeyPEM)and configured client CA pools from PEM usingx509.NewCertPool()+AppendCertsFromPEMto enable mTLS from in-memory data, and attached aTLSHandlerto the TLS config.Test_Listen_TLS_PEM,Test_Listen_TLS_PEM_Validation) and moved thetlsPEMhelper andreadTLSPEMto the top oflisten_test.gofor reuse.Related #4019