-
Notifications
You must be signed in to change notification settings - Fork 270
feat(redis): add REDIS_SSL_CERT_REQS and TLS config support #471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds TLS configuration support for Redis connections, which is a great feature. The implementation is mostly well done.
I've found a couple of issues:
- There's a significant issue in
internal/types/app/config.gowhere theREDIS_SSL_CERT_REQSconfiguration is handled in a misleading and unsafe way.CERT_OPTIONALis treated asCERT_REQUIRED, and invalid values are silently ignored. - In
internal/utils/cache/redis.go, an unrelated change toScanMapincorrectly ignores a potential error, which should be propagated.
I've provided specific comments and suggestions to address these points. Please take a look.
| switch strings.ToUpper(strings.TrimSpace(c.RedisSSLCertReqs)) { | ||
| case "CERT_NONE": | ||
| tlsConf.InsecureSkipVerify = true | ||
| case "CERT_OPTIONAL": | ||
| tlsConf.InsecureSkipVerify = false | ||
| case "CERT_REQUIRED", "": | ||
| tlsConf.InsecureSkipVerify = false | ||
| default: | ||
| tlsConf.InsecureSkipVerify = false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handling of REDIS_SSL_CERT_REQS has a couple of issues:
CERT_OPTIONALis implemented identically toCERT_REQUIRED(InsecureSkipVerify = false). This is misleading, asCERT_OPTIONALtypically implies that certificate verification is attempted but not enforced. Go'scrypto/tlspackage does not have a direct equivalent for this behavior. To avoid confusion, it would be better to explicitly disallow this value.- The
defaultcase silently treats any invalid value asCERT_REQUIRED. This can hide misconfigurations. It would be safer to return an error for unknown values.
I suggest refactoring this switch statement to be stricter and clearer.
switch strings.ToUpper(strings.TrimSpace(c.RedisSSLCertReqs)) {
case "CERT_NONE":
tlsConf.InsecureSkipVerify = true
case "CERT_REQUIRED", "":
tlsConf.InsecureSkipVerify = false
default:
return nil, fmt.Errorf("invalid value for REDIS_SSL_CERT_REQS: %q. Supported values are 'CERT_NONE', 'CERT_REQUIRED'", c.RedisSSLCertReqs)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Oak10, I'd like to follow the suggestions, Let's me know your thoughts
| _ = ScanMapAsync[V](key, match, func(m map[string]V) error { | ||
| for k, v := range m { | ||
| result[k] = v | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| }, context...) | ||
|
|
||
| return result, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change correctly passes the context... to ScanMapAsync, which was a bug. However, it also explicitly ignores the error returned by ScanMapAsync by assigning it to _.
The ScanMapAsync function can return errors from the underlying Redis HSCAN command. By ignoring this error, ScanMap might return a partially populated map or an empty map without signaling that an error occurred.
The function signature of ScanMap is (map[string]V, error), which implies that errors should be propagated. The error should be handled and returned to the caller.
if err := ScanMapAsync[V](key, match, func(m map[string]V) error {
for k, v := range m {
result[k] = v
}
return nil
}, context...); err != nil {
return nil, err
}
return result, nil
Description
Added REDIS_SSL_CERT_REQS option to the Redis connection to allow configuring TLS certificate verification (e.g. CERT_NONE, CERT_REQUIRED).
Fixes #450
Type of Change
Essential Checklist
Testing
Bug Fix (if applicable)
Fixes #123orCloses #123)Additional Information
Please provide any additional context that would help reviewers understand the changes.