-
Notifications
You must be signed in to change notification settings - Fork 468
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
Support TLS for replication #1630
Conversation
I have asked @sheyt0 to test, but please don't block this PR with our reply. We will anyway test this, but not sure if it will be during this week or next. |
Thanks! It is OK to test it in next week since we do not require this PR to be merged ASAP. There is still work to be done before we can merge this PR. |
@PragmaTwice сould you leave a little doc on how to use TLS for replication? With this config
and on slave
I get such errors
|
@sheyt0 Sure. Here are my test steps:
(all of them are in PEM format)
./kvrocks --dir datadir1 --port 6666 --tls-port 6676 --log-dir stdout --tls-cert-file cert/server.crt --tls-key-file cert/server.key --tls-ca-cert-file cert/ca.crt
./kvrocks --dir datadir2 --port 6667 --tls-port 6677 --log-dir stdout --tls-cert-file cert/server.crt --tls-key-file cert/server.key --tls-ca-cert-file cert/ca.crt --tls-replication yes --slaveof "127.0.0.1 6676"
➜ build git:(tls-replica) ✗ redis-cli -p 6666
127.0.0.1:6666> get a
(nil)
127.0.0.1:6666> set a 1
OK
127.0.0.1:6666> set b 2
OK
127.0.0.1:6666> set c 3
OK
127.0.0.1:6666> exit
➜ build git:(tls-replica) ✗ redis-cli -p 6667
127.0.0.1:6667> get a
"1"
127.0.0.1:6667> get b
"2"
127.0.0.1:6667> get c
"3"
127.0.0.1:6667> Server logs:
Replica logs:
|
Do I understand correctly that one set of certs should be used (same certs for master and slave)? |
Yeah, certs for master and slave must be the same. |
In the current implementation, this is not suitable for us. We need mTLS, not just TLS. |
Although I previously do not plan to support it in this PR, the feature is not hard to implement. I will try to support it later. |
@sheyt0 Sorry, I think I mis-understand the problem. The certs of master and slave can be different. I have tried and it works well. But you need to be noticed that the server connection (listen to kvrocks clients) and the client connection (connect to the master) of the slave must use the same cert, which currently is not able to split into different certs. So internally it includes four part of SSL context:
The two certs in master cannot be different (and also in slave), but the server-side cert in master and client-side cert in slave can be different. |
@sheyt0 And, I have found why you cannot start TLS connection between master and slave. Currently slave must have a tls-port to have TLS connection to master, this is a BUG since it is unnecessary. I will fix it soon. |
It is fixed. Now I think the slave can disable the tls-port (if you want). |
Now it works |
You need to build kvrocks with cmake option Please refer to https://github.com/apache/kvrocks#build for details. |
tls-replication params I shout config on master node or slave node? |
You need to configure the Also, you can open a discussion or question in slack channel rather than reply in this PR thread. |
It closes #1501.
Currently we do not support splitting cert between server and client.
This PR does not plan to support TLS for cluster mode, like slot migration.
@gofort It will be nice if you guys still want this feature and would like to try to do some tests on the patch.