feature: Redis server TLS support #104
Conversation
Signed-off-by: Martin René Sørensen <[email protected]>
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RedisDriver
participant TLSConfig
Client->>RedisDriver: Initialize with TLSConfig
RedisDriver->>TLSConfig: Read RootCa
TLSConfig-->>RedisDriver: Return TLS settings
RedisDriver->>Client: Establish secure connection
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
kv/config.go (2)
33-36: Add field documentation and consider path validationWhile the struct is well-structured, consider these improvements:
- Add GoDoc comments describing the struct and its fields, including:
- Valid values for MinVersion
- Expected format for CaFile
- Consider validating the CaFile path early
Add documentation like this:
+// TLSConfig contains TLS/SSL configuration for Redis connections type TLSConfig struct { + // MinVersion specifies the minimum TLS version. Valid values: "1.0", "1.1", "1.2", "1.3" (default) MinVersion string `mapstructure:"min_version"` + // CaFile specifies the path to a PEM file containing CA certificates CaFile string `mapstructure:"ca_file"` }
45-56: Consider enhancing version handling robustnessWhile the implementation is functionally correct, consider these improvements for better maintainability and clarity:
+// TLS version string constants +const ( + TLSVersion10 = "1.0" + TLSVersion11 = "1.1" + TLSVersion12 = "1.2" + TLSVersion13 = "1.3" +) func (t *TLSConfig) TLSVersion() uint16 { + // Return default if MinVersion is empty + if t.MinVersion == "" { + return tls.VersionTLS13 + } switch t.MinVersion { - case "1.0": + case TLSVersion10: return tls.VersionTLS10 - case "1.1": + case TLSVersion11: return tls.VersionTLS11 - case "1.2": + case TLSVersion12: return tls.VersionTLS12 default: return tls.VersionTLS13 } }kv/driver.go (1)
107-110: Assign RootCAs to tlsConfig when using TLSCurrently,
tlsConfig.RootCAsis set only ifCaFileorMinVersionis specified. However, to ensure the default system root CAs are used even whenCaFileis not provided, consider assigningrootCAsunconditionally when TLS is in use.Modify the condition to check for TLS usage instead:
-if d.cfg.TLSConfig.CaFile != "" || d.cfg.TLSConfig.MinVersion != "" { +if d.cfg.TLSConfig != nil { tlsConfig.RootCAs = rootCAs redisOptions.TLSConfig = tlsConfig }Alternatively, always assign
RootCAswhen TLS is configured:tlsConfig.RootCAs = rootCAs redisOptions.TLSConfig = tlsConfigThis ensures that the system's root certificates are available for server certificate verification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kv/config.go(3 hunks)kv/driver.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint
kv/driver.go
[high] 82-82: G402: TLS MinVersion too low.
(gosec)
🔇 Additional comments (3)
kv/config.go (3)
4-4: LGTM: Required import for TLS support
The crypto/tls import is necessary for TLS version constants and configuration.
30-30: LGTM: Clean addition of TLS configuration field
The TLSConfig field is properly integrated into the existing Config struct with consistent mapstructure tag formatting.
Line range hint 39-43: Verify if InitDefaults needs TLS initialization
The InitDefaults method doesn't initialize TLS fields. This is likely correct as TLS should be opt-in, but please verify this is the intended behavior for Azure Redis Cache scenarios.
✅ Verification successful
Let me gather more context about the TLS-related fields and their usage.
TLS initialization in InitDefaults is correctly implemented as opt-in
The current implementation is correct. The TLS configuration is intentionally not initialized with defaults because:
- TLS fields (MinVersion and CaFile) are optional and only used when explicitly set:
- MinVersion defaults to TLS 1.3 when provided but empty
- CaFile is only required for custom CA certificates
- The driver.go implementation properly handles both TLS and non-TLS scenarios, only applying TLS configuration when either field is set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any default TLS settings in the codebase
rg -l "TLS" . | xargs rg "InitDefaults|default"
Length of output: 333
Script:
#!/bin/bash
# Check TLS-related fields and their usage in the config
ast-grep --pattern 'type Config struct {
$$$
}'
# Check for any TLS configuration patterns in tests or examples
rg -A 5 "TLS" .
Length of output: 5421
|
Hey @sicet7 👋 Just a few notes:
|
rustatian
changed the title
Suggested by: #104 (comment) Signed-off-by: Martin René Sørensen <[email protected]>
Suggested by: #104 (review) Signed-off-by: Martin René Sørensen <[email protected]>
Signed-off-by: Martin René Sørensen <[email protected]>
Suggested by: #104 (comment) Signed-off-by: Martin René Sørensen <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
kv/config.go (1)
29-29: Add documentation for the TLS configuration fieldPlease add a comment explaining the purpose and usage of the TLS configuration.
+// TLSConfig contains TLS/SSL configuration for Redis connections TLSConfig TLSConfig `mapstructure:"tls"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kv/config.go(1 hunks)kv/driver.go(3 hunks)
🔇 Additional comments (3)
kv/config.go (1)
29-33: Verify TLS configuration alignment with RoadRunner standards
As mentioned in the PR comments, the TLS configuration should be unified across RoadRunner. Let's verify the existing TLS configuration patterns.
✅ Verification successful
Let me gather more information about TLS configurations across the codebase to ensure a comprehensive verification.
Let me try one more search to find any TLS-related configurations in other parts of the codebase, particularly focusing on the root and config directories.
Based on the gathered information, I can now provide the final response:
TLS configuration is appropriately implemented
The current TLS configuration implementation in the KV plugin follows secure practices:
- Uses TLS 1.2 as minimum version
- Properly handles CA certificates
- Maintains a simple, focused configuration structure that matches the requirements
The implementation is sufficient for the KV plugin's specific needs, and there's no evidence of conflicting TLS configuration patterns in the codebase that would require unification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other TLS configurations in the codebase
echo "Searching for TLS configuration patterns..."
rg -A 10 'type.*TLS.*Config.*struct' --type go
echo "Searching for TLS version handling..."
rg -A 5 'MinVersion.*tls\.' --type go
Length of output: 892
Script:
#!/bin/bash
# Search for TLS-related configurations in other parts of the codebase
echo "Searching for TLS-related imports and usages..."
rg 'tls\.' --type go -B 2 -A 2
echo "Searching for TLS configuration struct fields..."
rg 'tls.*mapstructure' --type go -B 2 -A 2
echo "Searching for CA file configurations..."
rg 'ca_file.*mapstructure' --type go -B 2 -A 2
Length of output: 607
Script:
#!/bin/bash
# Search for files that might contain TLS configurations
echo "Searching for potential config files..."
fd -e go -e yaml -e json . | grep -i "config\|tls"
echo "Searching for TLS-related code in config files..."
rg -l "tls" --type go $(fd -e go . | grep -i "config")
echo "Checking imports in the driver file..."
rg '^import \($' -A 10 kv/driver.go
Length of output: 638
kv/driver.go (2)
Line range hint 59-80: LGTM: Redis configuration options are well-structured
The Redis configuration options are comprehensive and include all necessary connection parameters.
82-91: LGTM: Secure TLS configuration with proper error handling
The implementation:
- Sets a secure default of TLS 1.2
- Properly handles system certificate pool errors
- Provides clear error messages
kv/config.go
Outdated
| @@ -26,6 +26,11 @@ type Config struct { | |||
| IdleTimeout time.Duration `mapstructure:"idle_timeout"` | |||
| IdleCheckFreq time.Duration `mapstructure:"idle_check_freq"` | |||
| ReadOnly bool `mapstructure:"read_only"` | |||
| TLSConfig TLSConfig `mapstructure:"tls"` | |||
There was a problem hiding this comment.
Should be a pointer: *TLSConfig to distinguish between empty and non set values.
kv/config.go
Outdated
| } | ||
|
|
||
| type TLSConfig struct { | ||
| CaFile string `mapstructure:"ca_file"` |
There was a problem hiding this comment.
Also, please, follow the standard RR configuration naming for the TLS options, example: https://github.com/roadrunner-server/grpc/blob/master/tests/configs/.rr-grpc-rq-tls-rootca.yaml#L20
kv/driver.go
Outdated
| }) | ||
| } | ||
|
|
||
| if d.cfg.TLSConfig.CaFile != "" { |
kv/driver.go
Outdated
| rootCAs, sysCertErr := x509.SystemCertPool() | ||
| if sysCertErr != nil { | ||
| rootCAs = x509.NewCertPool() | ||
| d.log.Warn("unable to load system certificate pool, using empty pool", zap.Error(sysCertErr)) | ||
| } | ||
|
|
||
| if _, crtExistErr := os.Stat(d.cfg.TLSConfig.CaFile); crtExistErr != nil { | ||
| return nil, errors.E(op, crtExistErr) | ||
| } | ||
|
|
||
| bytes, crtReadErr := os.ReadFile(d.cfg.TLSConfig.CaFile) | ||
| if crtReadErr != nil { | ||
| return nil, errors.E(op, crtReadErr) | ||
| } | ||
|
|
||
| if !rootCAs.AppendCertsFromPEM(bytes) { | ||
| return nil, errors.E(op, errors.Errorf("failed to parse certificates from PEM file '%s'. Please ensure the file contains valid PEM-encoded certificates", d.cfg.TLSConfig.CaFile)) | ||
| } | ||
|
|
||
| tlsConfig.RootCAs = rootCAs | ||
| redisOptions.TLSConfig = tlsConfig | ||
| } |
There was a problem hiding this comment.
It is better to move TLS configuration to the separate package
There was a problem hiding this comment.
sorry i am new to go, when you say new package do you mean in another repo or simply in another directory?? 😅
There was a problem hiding this comment.
Sure, np, I'll help you 😃
Just a new file called tlsconfig.go or similar :)
|
@rustatian Some thing like this? |
|
Yeah 👍 |
Suggestions implemented: #104 (comment) #104 (comment) #104 (comment) #104 (comment) Signed-off-by: Martin René Sørensen <[email protected]>
I don't mind at all, go right ahead :-) |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
kv/tlsconfig.go (3)
3-9: Consider organizing importsWhile functionally correct, consider organizing imports into groups: standard library, external packages, and internal packages.
import ( "crypto/tls" "crypto/x509" "os" "github.com/roadrunner-server/errors" "go.uber.org/zap" )🧰 Tools
🪛 golangci-lint
5-5: File is not
goimports-ed(goimports)
28-30: Improve error handling specificityThe error handling could be more specific to help users identify configuration issues.
- if _, crtExistErr := os.Stat(c.RootCa); crtExistErr != nil { - return nil, crtExistErr + if _, crtExistErr := os.Stat(c.CAFile); crtExistErr != nil { + return nil, errors.Errorf("CA file not accessible: %v", crtExistErr) } - bytes, crtReadErr := os.ReadFile(c.RootCa) + bytes, crtReadErr := os.ReadFile(c.CAFile) if crtReadErr != nil { - return nil, crtReadErr + return nil, errors.Errorf("failed to read CA file: %v", crtReadErr) }Also applies to: 32-35
37-39: Add logging for certificate parsingConsider adding debug logging to help troubleshoot certificate issues.
if !rootCAs.AppendCertsFromPEM(bytes) { + log.Debug("failed to parse certificates", zap.String("file", c.CAFile)) return nil, errors.Errorf("failed to parse certificates from PEM file '%s'. Please ensure the file contains valid PEM-encoded certificates", c.RootCa) } + log.Debug("successfully loaded certificates", zap.String("file", c.CAFile))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
kv/config.go(1 hunks)kv/driver.go(2 hunks)kv/tlsconfig.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kv/config.go
- kv/driver.go
🧰 Additional context used
🪛 golangci-lint
kv/tlsconfig.go
5-5: File is not goimports-ed
(goimports)
|
A few updates: d90dce6 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
kv/tlsconfig.go (1)
1-131: Consider architectural improvementsTo enhance the robustness and maintainability of the TLS implementation:
Add comprehensive test coverage:
- Unit tests for TLS configuration parsing
- Integration tests with Redis TLS connections
- Test cases for various certificate scenarios
Implement proper configuration validation:
- Validate certificate file paths
- Validate TLS version values
- Add configuration examples in documentation
Security considerations:
- Document security implications of TLS settings
- Add logging for TLS-related events
- Consider adding certificate expiration monitoring
Would you like assistance in implementing any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)kv/driver.go(2 hunks)kv/tlsconfig.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kv/driver.go
🔇 Additional comments (2)
go.mod (1)
15-15: LGTM! Verify TLS version compatibility.
The addition of golang.org/x/sys as a direct dependency is appropriate for implementing TLS support. This version should support all required TLS functionality.
Since there was a recommendation in the PR comments to set minimum TLS version to 1.2 (as 1.0 and 1.1 are deprecated by Microsoft), please verify that this version of the sys package fully supports TLS 1.2 and above.
kv/tlsconfig.go (1)
117-131: Verify requirement for client certificate support
The client certificate functionality wasn't mentioned in the PR objectives. While the implementation is correct, please clarify:
- Is client certificate support a requirement for Azure Cache for Redis?
- If not required, consider removing this functionality to reduce attack surface
- If required, ensure proper security review of certificate reloading mechanism
| type TLSConfig struct { | ||
| Cert string `mapstructure:"cert"` | ||
| Key string `mapstructure:"key"` | ||
| RootCa string `mapstructure:"root_ca"` | ||
| } |
There was a problem hiding this comment.
Align struct with PR objectives and add documentation
The TLSConfig struct needs to be updated to match the PR objectives and documentation requirements:
- Field names should match configuration keys
- Missing
min_versionfield - Missing documentation for struct and fields
- Unexplained addition of
certandkeyfields
+// TLSConfig contains TLS/SSL configuration for Redis connections
type TLSConfig struct {
- Cert string `mapstructure:"cert"`
- Key string `mapstructure:"key"`
- RootCa string `mapstructure:"root_ca"`
+ // MinVersion specifies minimum TLS version (1.0, 1.1, 1.2, 1.3)
+ MinVersion string `mapstructure:"min_version"`
+ // CAFile is the path to PEM file containing CA certificates
+ CAFile string `mapstructure:"ca_file"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type TLSConfig struct { | |
| Cert string `mapstructure:"cert"` | |
| Key string `mapstructure:"key"` | |
| RootCa string `mapstructure:"root_ca"` | |
| } | |
| // TLSConfig contains TLS/SSL configuration for Redis connections | |
| type TLSConfig struct { | |
| // MinVersion specifies minimum TLS version (1.0, 1.1, 1.2, 1.3) | |
| MinVersion string `mapstructure:"min_version"` | |
| // CAFile is the path to PEM file containing CA certificates | |
| CAFile string `mapstructure:"ca_file"` | |
| } |
| func tlsConfig(conf *TLSConfig) (*tls.Config, error) { | ||
| if conf == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| tlsConfig := defaultTLSConfig(conf) | ||
| if conf.RootCa != "" { | ||
| // error is always nil here | ||
| certPool, err := x509.SystemCertPool() | ||
| if err != nil { | ||
| // error is always nil here | ||
| return nil, err | ||
| } | ||
|
|
||
| if certPool == nil { | ||
| certPool = x509.NewCertPool() | ||
| } | ||
|
|
||
| // we already checked this file in the config.go | ||
| rca, err := os.ReadFile(conf.RootCa) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if ok := certPool.AppendCertsFromPEM(rca); !ok { | ||
| return nil, err | ||
| } | ||
|
|
||
| tlsConfig.RootCAs = certPool | ||
| } | ||
|
|
||
| if _, crtExistErr := os.Stat(conf.RootCa); crtExistErr != nil { | ||
| return nil, crtExistErr | ||
| } | ||
|
|
||
| return tlsConfig, nil | ||
| } |
There was a problem hiding this comment.
Fix critical issues in TLS configuration logic
Several critical issues need to be addressed:
- Certificate existence check is performed after reading the file
- Undefined error variable used in return statement
- Missing TLS version configuration support
- Unnecessary error comments
func tlsConfig(conf *TLSConfig) (*tls.Config, error) {
if conf == nil {
return nil, nil
}
+ // Check certificate existence first
+ if conf.CAFile != "" {
+ if _, err := os.Stat(conf.CAFile); err != nil {
+ return nil, fmt.Errorf("ca_file error: %w", err)
+ }
+ }
+
tlsConfig := defaultTLSConfig(conf)
- if conf.RootCa != "" {
- // error is always nil here
+
+ // Configure TLS version
+ if conf.MinVersion != "" {
+ version, err := parseTLSVersion(conf.MinVersion)
+ if err != nil {
+ return nil, err
+ }
+ tlsConfig.MinVersion = version
+ }
+
+ if conf.CAFile != "" {
certPool, err := x509.SystemCertPool()
if err != nil {
- // error is always nil here
return nil, err
}
@@ -31,12 +31,11 @@
certPool = x509.NewCertPool()
}
- // we already checked this file in the config.go
- rca, err := os.ReadFile(conf.RootCa)
+ rca, err := os.ReadFile(conf.CAFile)
if err != nil {
return nil, err
}
if ok := certPool.AppendCertsFromPEM(rca); !ok {
- return nil, err
+ return nil, fmt.Errorf("failed to append CA certificate")
}
tlsConfig.RootCAs = certPool
}
- if _, crtExistErr := os.Stat(conf.RootCa); crtExistErr != nil {
- return nil, crtExistErr
- }
-
return tlsConfig, nil
}Committable suggestion skipped: line range outside the PR's diff.
|
Have you tried testing this PR on your |
|
ahh... |
|
Ah, you may try to temporarily comment |
|
I made it work with these changes: |
Signed-off-by: Valery Piashchynski <[email protected]> Signed-off-by: Martin René Sørensen <[email protected]>
|
Yeah, this is a correct change. Because |
…ded. Signed-off-by: Martin René Sørensen <[email protected]>
|
I have cherry picked it in now, i tried to force push with a new cherry pick for your commit to see if i could get it to not complain about the commit being "Unverified" 😅 |
|
Not a problem 😃 |
|
Will do, is there any perticular branch that i should make the merge towards on the |
|
Sorry for the late response, yeah, master is ok, thank you 👍 |
|
Thank you @sicet7 👍 |
Reason for This PR
We are (at my employer ASG-Digital) using Azure's product
Azure Cache for Redisand we were having some issues connecting to their default configuration which disables non-tls connections and uses their own CA to sign their certificates.I implemented the required changes to be able to connect to Azure Cache for Redis default configuration without having to re-enable non-tls connections in the Azure control panel.
Description of Changes
I added a
tlsconfiguration section under the redis configuration to allow for configuration of themin_versionandca_fileoptions.The
min_versionoption configures the minimum TLS version that can be used for connecting to the remote redis server. It accepts the following values:1.0,1.1,1.2and1.3, it defaults to1.3if only theca_fileoption is provided.The
ca_fileis a path either global or relative, to a file containing one or more PEM certificates, that should be used to verify the server certificate.These changes are made so that existing configurations should still work without any changes needed.
The TLS configuration is only applied to the connection configuration if one or more of the configuration options
min_versionorca_fileis provided.Snipped from the RoadRunner configuration i used for testing
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).I was not able to find any documentation in need of updating in this repo.
CHANGELOG.md.I was not able to find the mentioned file in this repo.
Summary by CodeRabbit
New Features
TLSConfigstruct created to manage TLS settings, including certificate paths.NewRedisDriverfunction to utilize TLS settings for secure connections.Bug Fixes