🔥 feat: Add support for configuring TLS Min Version #3248
Conversation
This commit will resolve gofiber#3239 For more info: gofiber#3239
dozheiny
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes introduce a new field, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)listen_test.go (1)
Enhance test coverage and improve code organization. While the test covers essential scenarios, consider the following improvements:
// go test -run Test_Listen_TLSMinVersion
func Test_Listen_TLSMinVersion(t *testing.T) {
testPreforkMaster = true
app := New()
// Invalid TLSMinVersion
require.Panics(t, func() {
- _ = app.Listen(":443", ListenConfig{TLSMinVersion: tls.VersionTLS10}) //nolint:errcheck // ignore error
+ _ = app.Listen(":0", ListenConfig{TLSMinVersion: tls.VersionTLS10}) //nolint:errcheck // ignore error
})
require.Panics(t, func() {
- _ = app.Listen(":443", ListenConfig{TLSMinVersion: tls.VersionTLS11}) //nolint:errcheck // ignore error
+ _ = app.Listen(":0", ListenConfig{TLSMinVersion: tls.VersionTLS11}) //nolint:errcheck // ignore error
})
// Prefork
require.Panics(t, func() {
- _ = app.Listen(":443", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS10}) //nolint:errcheck // ignore error
+ _ = app.Listen(":0", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS10}) //nolint:errcheck // ignore error
})
require.Panics(t, func() {
- _ = app.Listen(":443", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS11}) //nolint:errcheck // ignore error
+ _ = app.Listen(":0", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS11}) //nolint:errcheck // ignore error
})
+ // Helper function for shutdown
+ shutdownAfterDelay := func(app *App) {
+ go func() {
+ time.Sleep(1000 * time.Millisecond)
+ assert.NoError(t, app.Shutdown())
+ }()
+ }
+
+ // Test TLS 1.2 (default version)
+ shutdownAfterDelay(app)
+ require.NoError(t, app.Listen(":0", ListenConfig{
+ TLSMinVersion: tls.VersionTLS12,
+ CertFile: "./.github/testdata/ssl.pem",
+ CertKeyFile: "./.github/testdata/ssl.key",
+ }))
+
// Valid TLSMinVersion
- go func() {
- time.Sleep(1000 * time.Millisecond)
- assert.NoError(t, app.Shutdown())
- }()
+ shutdownAfterDelay(app)
require.NoError(t, app.Listen(":0", ListenConfig{TLSMinVersion: tls.VersionTLS13}))
// Valid TLSMinVersion with Prefork
- go func() {
- time.Sleep(1000 * time.Millisecond)
- assert.NoError(t, app.Shutdown())
- }()
- require.NoError(t, app.Listen(":99999", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS13}))
+ shutdownAfterDelay(app)
+ require.NoError(t, app.Listen(":0", ListenConfig{DisableStartupMessage: true, EnablePrefork: true, TLSMinVersion: tls.VersionTLS13}))
+
+ // Verify TLS version during handshake
+ ln := fasthttputil.NewInmemoryListener()
+ go func() {
+ app.Listener(ln, ListenConfig{
+ TLSMinVersion: tls.VersionTLS13,
+ CertFile: "./.github/testdata/ssl.pem",
+ CertKeyFile: "./.github/testdata/ssl.key",
+ })
+ }()
+
+ conn, err := tls.Dial("tcp", ln.Addr().String(), &tls.Config{
+ InsecureSkipVerify: true,
+ })
+ require.NoError(t, err)
+ require.Equal(t, uint16(tls.VersionTLS13), conn.ConnectionState().Version)
+ conn.Close()
}
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3248 +/- ##
=======================================
Coverage 82.69% 82.70%
=======================================
Files 115 115
Lines 11383 11389 +6
=======================================
+ Hits 9413 9419 +6
Misses 1562 1562
Partials 408 408
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
listen.go (1)
136-136: Consider using TLS 1.3 as the default minimum version.While TLS 1.2 is secure, TLS 1.3 (introduced in 2018) offers improved security and performance. Consider using
tls.VersionTLS13as the default, while allowing users to downgrade to TLS 1.2 if needed for compatibility.-TLSMinVersion: tls.VersionTLS12, +TLSMinVersion: tls.VersionTLS13,- cfg.TLSMinVersion = tls.VersionTLS12 + cfg.TLSMinVersion = tls.VersionTLS13Also applies to: 156-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
listen.go(5 hunks)
🔇 Additional comments (2)
listen.go (2)
53-57: LGTM! Field addition is well-documented and properly typed.
The new TLSMinVersion field is correctly implemented with appropriate type and documentation.
182-182: Verify TLSConfigFunc behavior with MinVersion.
The implementation correctly sets MinVersion in both TLS configurations. However, be aware that TLSConfigFunc can override this setting. Consider documenting this behavior.
Also applies to: 206-206
✅ Verification successful
Let me gather more information about the actual implementation of TLSConfigFunc usage in the codebase.
Let me try a different approach to find the TLSConfigFunc usage.
Let me check the full context around where TLSConfigFunc is called to understand the order of operations.
TLSConfigFunc behavior with MinVersion is correctly implemented
The code shows that MinVersion is set before TLSConfigFunc is called, and TLSConfigFunc is explicitly designed to allow customization of the TLS configuration. This is the expected behavior as:
- MinVersion is set first in the TLS configuration
- TLSConfigFunc is called afterward, allowing intentional overrides if needed
- The behavior is documented in the config struct with clear comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any TLSConfigFunc usage that might override MinVersion
rg -l 'TLSConfigFunc.*tls\.Config' | while read -r file; do
echo "=== $file ==="
rg -A 5 'TLSConfigFunc.*tls\.Config' "$file"
done
Length of output: 1510
Script:
#!/bin/bash
# Search for the actual implementation where TLSConfigFunc is called
ast-grep --pattern 'if $_.TLSConfigFunc != nil {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Search for TLSConfigFunc usage in the implementation
rg -A 5 'TLSConfigFunc.*\(' --type go
Length of output: 994
Script:
#!/bin/bash
# Get more context around the TLSConfigFunc usage
rg -B 10 -A 10 'cfg\.TLSConfigFunc\(tlsConfig\)' --type go
Length of output: 781
gaby
changed the title
|
@dozheiny Can you also update the documentation under the To fix the struct issue, the |
|
Should I change the default value of the TLS version to 1.3? |
V1.2 is fine as default. Not everything supports 1.3, and 1.2 is still active |
|
@dozheiny Although we shouldn't allow the usage of TLS1.0 and TLS1.1 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/whats_new.md (1)
133-139: Enhance TLS minimum version documentation.While the documentation accurately describes the new feature, consider adding:
- List of available TLS version constants (e.g.,
tls.VersionTLS10,tls.VersionTLS11,tls.VersionTLS12,tls.VersionTLS13).- Security implications of choosing different TLS versions.
- Information about the default value behavior.
- More comprehensive configuration examples.
Consider expanding the documentation with this example:
### Configurable TLS Minimum Version We have added support for configuring the TLS minimum version. This field allows you to set the TLS minimum version for TLSAutoCert and the server listener. +Available TLS version constants: +- `tls.VersionTLS10` (not recommended for security reasons) +- `tls.VersionTLS11` (not recommended for security reasons) +- `tls.VersionTLS12` (recommended minimum) +- `tls.VersionTLS13` (most secure) + +The default value is `tls.VersionTLS12` if not specified. + +Example usage: ```go app.Listen(":444", fiber.ListenConfig{TLSMinVersion: tls.VersionTLS12})
+For enhanced security, you can enforce TLS 1.3:
+```go
+app.Listen(":444", fiber.ListenConfig{
- TLSMinVersion: tls.VersionTLS13,
- // Additional TLS configuration...
+})
+```</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6256c853a2d6ea2226470a77d0d6ac4fdb6a26d1 and b5ae998fce2cdcb0975c3e5c7b408b9e6343d126. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/whats_new.md` (1 hunks) * `listen.go` (5 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * listen.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@gaby So I think creating a type for TLS versions is the best way. |
|
@dozheiny You can compare using: if cfg.TLSMinVersion == 0 {
cfg.TLSMinVersion = tls.VersionTLS12
}
if cfg.TLSMinVersion =! tls.VersionTLS12 || cfg.TLSMinVersion =! tls.VersionTLS13 {
panic("Supported TLS versions: 1.2, 1.3")
}Right? That way we dont have a custom type |
|
@gaby But we break the application with panic. |
That's fine cause its an invalid configuration. If you search for "panic" in the code base you will see we use it often for bad configs |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
listen.go (2)
111-115: LGTM! Consider enhancing the documentation.The new TLSMinVersion field is well-integrated with appropriate type and documentation.
Consider adding the actual TLS version constants in the documentation for clarity:
// TLSMinVersion allows to set tls minimum version. // -// Default: VersionTLS12 +// Default: tls.VersionTLS12 (0x0303) +// Supported: tls.VersionTLS12 (0x0303), tls.VersionTLS13 (0x0304) // WARNING: TLS1.0 and TLS1.1 versions are not supported.
157-163: LGTM! Consider improving the error message.The initialization and validation of TLSMinVersion are well-implemented. The panic on invalid versions aligns with the framework's approach to configuration errors.
Consider making the panic message more informative:
- panic("Supported TLS versions: 1.2, 1.3") + panic(fmt.Sprintf("fiber: unsupported TLS version: %#x. Supported versions are tls.VersionTLS12 (0x0303) and tls.VersionTLS13 (0x0304)", cfg.TLSMinVersion))
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
listen.go(5 hunks)listen_test.go(1 hunks)
🔇 Additional comments (4)
listen.go (3)
111-115: LGTM! Well-documented field addition.
The new TLSMinVersion field is properly typed and documented, with clear warnings about unsupported versions.
157-163: LGTM! Robust validation of TLS version.
The validation ensures only secure TLS versions (1.2 and 1.3) are allowed, with appropriate error handling via panic, which is consistent with the codebase's approach to configuration errors.
186-187: LGTM! Consistent TLS version configuration.
The TLSMinVersion is correctly applied in both TLS configuration scenarios.
Also applies to: 210-211
listen_test.go (1)
236-248: LGTM! Comprehensive validation tests.
The test cases thoroughly verify that using unsupported TLS versions (1.0 and 1.1) triggers panics in both regular and prefork modes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
listen.go (1)
157-163: Consider using constants for supported TLS versions.The validation logic is correct, but consider defining constants for supported TLS versions to improve code maintainability.
+const ( + MinTLSVersion = tls.VersionTLS12 + MaxTLSVersion = tls.VersionTLS13 +) if cfg.TLSMinVersion == 0 { - cfg.TLSMinVersion = tls.VersionTLS12 + cfg.TLSMinVersion = MinTLSVersion } -if cfg.TLSMinVersion != tls.VersionTLS12 && cfg.TLSMinVersion != tls.VersionTLS13 { +if cfg.TLSMinVersion < MinTLSVersion || cfg.TLSMinVersion > MaxTLSVersion { panic("unsupported TLS version, please use tls.VersionTLS12 or tls.VersionTLS13") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/api/fiber.md(1 hunks)listen.go(5 hunks)
🔇 Additional comments (2)
listen.go (1)
111-115: LGTM! Well-documented field addition.
The new TLSMinVersion field is properly documented with clear default value and warning about unsupported versions.
docs/api/fiber.md (1)
119-120: LGTM! Clear and accurate documentation.
The documentation for both AutoCertManager and TLSMinVersion fields is clear, accurate, and consistent with the implementation.
|
@gaby Thanks for fixing lint issues and adding tests |
This pull request introduces a new configuration option to set the minimum TLS version in the
listen.gofile. The most important changes include adding theTLSMinVersionfield to theListenConfigstruct and updating the default configuration and relevant methods to use this new field.New TLS Configuration Option:
listen.go: AddedTLSMinVersionfield to theListenConfigstruct to allow setting the minimum TLS version.listen.go: Set the default value ofTLSMinVersiontotls.VersionTLS12in thelistenConfigDefaultfunction.listen.go: Updated thelistenConfigDefaultfunction to ensureTLSMinVersionis set totls.VersionTLS12if not provided.Code Updates to Use New Configuration:
listen.go: Modified theListenmethod to use theTLSMinVersionfrom theListenConfigstruct for thetls.Configobject.This commit will resolve #3239