feat: update codis fe / proxy with tls; topom updated #2383

kolinfluence · 2024-02-01T18:44:02Z

codis fe / proxy tls + topom update

sprappcom · 2024-02-02T19:29:54Z

@AlexStocks please look into it.

codis/config/dashboard.toml

codis/pkg/proxy/.proxy.go.swo

codis/admin/.codis-fe-admin.sh.swo

machinly · 2024-03-09T16:00:42Z

codis/cert.pem

It's better not to provide the public key file. Instead, you can add a generation command in the readme.

@machinly the public key file is definitely a dummy one.
you put "filesystem" as default, surely a dummy version is fine right? ... maybe you can modify with error message generated when there's no cert / key files present after you merge this and delete them. will that do fine?

machinly · 2024-03-09T16:02:08Z

codis/config/proxy.toml

Do not modify the default configuration

can you only pull what is needed?

machinly · 2024-03-09T16:02:58Z

codis/key.pem

It's better not to provide the private key file. Instead, you can add a generation command in the readme.

machinly · 2024-03-09T16:03:54Z

codis/pkg/proxy/config.go

-jodis_name = ""
-jodis_addr = ""
+jodis_name = "etcd"
+jodis_addr = "127.0.0.1:2379"


Do not modify the default parameter values

can you only pull what is needed?

machinly · 2024-03-09T16:06:19Z

codis/pkg/topom/config.go

@@ -25,8 +25,10 @@ const DefaultConfig = `
 # Set Coordinator, only accept "zookeeper" & "etcd" & "filesystem".
 # for zookeeper/etcd, coorinator_auth accept "user:password" 
 # Quick Start
-coordinator_name = "filesystem"


Do not modify the default parameter values

can you only pull what is needed?

codis/pkg/models/store.go

luky116 · 2024-03-10T14:39:07Z

codis/config/proxy.toml

-# slow command list e.g. hgetall, mset
-slow_cmd_list = ""
+# quick command list
+quick_cmd_list = "get,set"


Do not modify the default configuration

luky116 · 2024-03-10T14:48:38Z

codis/admin/codis-fe-admin.sh

 CODIS_FE_ADDR="0.0.0.0:9090"
+CODIS_FE_TLS=1
+CODIS_FE_TLS_KEY="/usr/local/src/pika/codis/key.pem"


Comment this code

Can you help add the rest?

kolinfluence · 2024-03-11T00:11:24Z

noted. will revert.

kolinfluence · 2024-03-14T13:32:46Z

@machinly pls check again, i've done all. sorry for the issues.
for code generation, do provide what examples you think is good. for the cert is just dummy test like your "filesystem" thing.

luky116 · 2024-04-21T15:37:45Z

codis-fe cannot access, have you tried in you local env?

codis/pkg/topom/config.go

luky116 · 2024-04-21T15:50:13Z

proxy cannot connect?

luky116 · 2024-04-21T16:05:39Z

Judging from your code, you want to add the TLS protocol to the redis protocol port of the proxy layer. If so, can dashboard and fe not add TLS? Because they interact through the http protocol, the proxy's redis protocol port is only responsible for the user's redis client connection.

kolinfluence · 2024-04-21T18:33:42Z

@luky116 sorry was busy playing with lru benchmark tests update here so a bit lagged behind on pika (but it's on my radar daily).
https://github.com/cloudxaas/gocache/tree/main/lrux/bytes
need fast lru. (promoting it coz may be able to be used in codis too and would hope for improvement to make it faster, kind of applicable to cache)

back to your question...

the dashboard and fe is added for remote users to the dashboard and fe. it's optional configuration parameter. if you set tls = true, then it will do the tls feature, else wont affect performance at runtime.
it'll be useful for wider adoption considering a lot of dev can remotely access their dashboard / fe etc knowing it's more secure. basically it wont affect the runtime performance.
dashboard and fe why not add tls to interact with https protocol too?
ok i see your point. so maybe we can also do a tlsfe = 1, tlsdashboard = 1, tlspika = 1 etc kind of parameter too. we can allow http/https to run concurrently without affecting performance and the fe/dashboard choosing which to connect to.

good?

kolinfluence · 2024-04-21T18:37:06Z

proxy cannot connect?

you need to use redis-cli with secure tls to connect put
redis-cli --tls
as option parameter

pls remember it will only use tls for proxy if it's set to true. so accordingly, we will need to put tls-something for everything else etc where we want tls to have. so maybe tls-pika = true in future when pika has tls? but proxy shld definitely have tls.

AlexStocks · 2024-06-05T07:28:38Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2024-06-05T07:33:45Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves multiple files with significant changes including the introduction of TLS in the network layer, modifications to configuration handling, and updates to the proxy and topom modules. The complexity of the changes and the security implications of correctly implementing TLS require careful review to ensure reliability and security.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The error handling in the TLS setup could be improved by providing more specific error messages or handling specific types of errors differently.
⚡ Possible issues	Performance Concern: The introduction of TLS might impact the performance due to encryption overhead. It would be beneficial to benchmark the performance before and after these changes to ensure that the impact is within acceptable limits.
🔒 Security concerns	No

Code feedback:

relevant file	codis/cmd/fe/main.go
suggestion	Consider setting more fields in the `tls.Config` to ensure better security, such as `MinVersion` to enforce the use of TLS 1.2 or higher. This helps prevent the use of older, less secure versions of TLS. [important]
relevant line	Certificates: []tls.Certificate{cert},

relevant file	codis/pkg/proxy/proxy.go
suggestion	Add error logging before returning the error when the TLS certificate fails to load. This will help in debugging issues related to TLS configuration by providing a clear log message about what went wrong. [important]
relevant line	return errors.Trace(err)

relevant file	codis/pkg/models/store.go
suggestion	Implement error handling for the case when `Decode` returns `nil` but no error. This could potentially lead to a situation where the function proceeds without a valid token, which might not be the intended behavior. [medium]
relevant line	if t.Token == token {

relevant file	codis/pkg/topom/topom.go
suggestion	Add validation for the input byte slice `b` in the `Decode` function to check if it's nil or empty before attempting to decode it. This prevents unnecessary processing and potential errors when dealing with empty data. [medium]
relevant line	if err := jsonDecode(s, b); err != nil {

AlexStocks · 2024-06-05T13:33:55Z

@CodiumAI-Agent /improve

CodiumAI-Agent · 2024-06-05T13:35:23Z

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Remove the private key from the codebase to enhance security It is highly insecure to store private keys within the codebase, especially in plaintext format. This can lead to security vulnerabilities as unauthorized users could potentially access sensitive information. Consider using environment variables or a secure vault solution to manage private keys. codis/key.pem [1-28] ------BEGIN PRIVATE KEY----- -MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQClOgprziC/+nQe -... ------END PRIVATE KEY----- +# Private key securely stored outside of the codebase Suggestion importance[1-10]: 10 Why: Storing private keys in the codebase is a significant security risk. Using environment variables or secure vault solutions is a best practice to prevent unauthorized access to sensitive information.	10
Possible bug	Add checks for TLS certificate and key file paths before loading them Consider adding a check for the `--tls-cert` and `--tls-key` flags before attempting to load the TLS key pair. This ensures that the necessary parameters are provided, avoiding runtime panics due to missing arguments. codis/cmd/fe/main.go [205] -cert, err := tls.LoadX509KeyPair(utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key")) +certFile, keyFile := utils.ArgumentMust(d, "--tls-cert"), utils.ArgumentMust(d, "--tls-key") +if certFile == "" \|\| keyFile == "" { + log.Panic("TLS certificate or key file path must be provided") +} +cert, err := tls.LoadX509KeyPair(certFile, keyFile) Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime panic by ensuring that the necessary TLS certificate and key file paths are provided before attempting to load them. This is a crucial check that enhances the robustness of the code.	9
Possible bug	Ensure safe token comparison by checking for nil after decoding Add error handling for the `Decode` function to ensure that the token comparison only occurs if the decoding is successful. This prevents potential nil pointer dereferences. codis/pkg/models/store.go [133-138] -t, err := Decode(b); +t, err := Decode(b) if err != nil { return err } -if t.Token == token { +if t != nil && t.Token == token { return s.Release() } Suggestion importance[1-10]: 8 Why: This suggestion prevents potential nil pointer dereferences by ensuring that the token comparison only occurs if the decoding is successful. This is an important fix to avoid runtime errors.	8
Enhancement	Improve error handling for IP replacement to manage failures more effectively Add error handling for `utils.ReplaceUnspecifiedIP` to handle potential failures gracefully instead of only logging the error. This ensures that the function's failure is properly managed. codis/pkg/proxy/proxy.go [145-147] x, err := utils.ReplaceUnspecifiedIP(proto, l.Addr().String(), config.HostProxy) if err != nil { + log.Errorf("Failed to replace unspecified IP: %s", err) return err } Suggestion importance[1-10]: 7 Why: The suggestion improves error handling by logging the error and returning it, which helps in debugging and ensures that failures are managed properly. However, it is a minor enhancement rather than a critical fix.	7
Enhancement	Wrap errors in the `Decode` function to enhance error information Add error wrapping in the `Decode` function to provide more context about the error, which can be useful for debugging. codis/pkg/topom/topom.go [24-25] if err := jsonDecode(s, b); err != nil { - return nil,err + return nil, fmt.Errorf("json decode error: %w", err) } Suggestion importance[1-10]: 6 Why: The suggestion enhances error information by wrapping errors with additional context. This is useful for debugging but is a minor improvement.	6

sprappcom added 7 commits February 1, 2024 14:41

update codis proxy to accept tls connection

9658b06

tls for proxy works, tls for fe in progress

6ec04e3

codis fe tls done

10b22fc

codis tls fe minor update

083b737

codis fe minor update

4517f87

@chenmin1992 remove my own topom

342d893

optimize code

8ad0b61

github-actions bot added 🧹 Updates This will not be worked on Invalid PR Title labels Feb 1, 2024

kolinfluence changed the title ~~codis fe / proxy tls + topom update~~ feat: update codis fe / proxy with tls; topom updated Feb 2, 2024

github-actions bot removed the Invalid PR Title label Feb 2, 2024

baixin01 requested a review from chejinge February 4, 2024 02:41

AlexStocks added the 3.5.4 label Feb 6, 2024

luky116 reviewed Feb 9, 2024

View reviewed changes

codis/config/dashboard.toml Outdated Show resolved Hide resolved

machinly requested changes Mar 9, 2024

View reviewed changes

luky116 reviewed Mar 10, 2024

View reviewed changes

Delete codis/pkg/proxy/.proxy.go.swo

e233d10

github-actions bot added the ✏️ Feature New feature or request label Mar 14, 2024

kolinfluence added 9 commits March 14, 2024 09:07

Delete codis/admin/.codis-fe-admin.sh.swo

e99e587

Update store.go

c631795

Update config.go

7b765b7

Update Makefile

6eb8e3a

Update config.go

f2cfcee

Update proxy.toml

b8bda63

Update proxy.toml

6a8a404

Update codis-fe-admin.sh

10038c3

Update dashboard.toml

7f78d00

Update topom.go

8d00a6f

AlexStocks added the 4.0.0 label Mar 20, 2024

Mixficsol removed the 3.5.4 label Mar 20, 2024

luky116 reviewed Apr 21, 2024

View reviewed changes

codis/pkg/topom/config.go Show resolved Hide resolved

OpenAtomFoundation deleted a comment from Issues-translate-bot Apr 21, 2024

OpenAtomFoundation deleted a comment from CodiumAI-Agent Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update codis fe / proxy with tls; topom updated #2383

feat: update codis fe / proxy with tls; topom updated #2383

kolinfluence commented Feb 1, 2024

sprappcom commented Feb 2, 2024

machinly Mar 9, 2024

kolinfluence Apr 23, 2024

machinly Mar 9, 2024

kolinfluence Mar 14, 2024

machinly Mar 9, 2024

machinly Mar 9, 2024

kolinfluence Mar 14, 2024

machinly Mar 9, 2024

kolinfluence Mar 14, 2024

luky116 Mar 10, 2024

luky116 Mar 10, 2024

kolinfluence Mar 14, 2024

kolinfluence commented Mar 11, 2024

kolinfluence commented Mar 14, 2024

luky116 commented Apr 21, 2024

luky116 commented Apr 21, 2024

luky116 commented Apr 21, 2024

kolinfluence commented Apr 21, 2024 •

edited

kolinfluence commented Apr 21, 2024 •

edited

AlexStocks commented Jun 5, 2024

CodiumAI-Agent commented Jun 5, 2024

AlexStocks commented Jun 5, 2024

CodiumAI-Agent commented Jun 5, 2024

feat: update codis fe / proxy with tls; topom updated #2383

Are you sure you want to change the base?

feat: update codis fe / proxy with tls; topom updated #2383

Conversation

kolinfluence commented Feb 1, 2024

sprappcom commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolinfluence commented Mar 11, 2024

kolinfluence commented Mar 14, 2024

luky116 commented Apr 21, 2024

luky116 commented Apr 21, 2024

luky116 commented Apr 21, 2024

kolinfluence commented Apr 21, 2024 • edited

kolinfluence commented Apr 21, 2024 • edited

AlexStocks commented Jun 5, 2024

CodiumAI-Agent commented Jun 5, 2024

PR Review 🔍

AlexStocks commented Jun 5, 2024

CodiumAI-Agent commented Jun 5, 2024

PR Code Suggestions ✨

kolinfluence commented Apr 21, 2024 •

edited

kolinfluence commented Apr 21, 2024 •

edited