You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/08/28 15:40:28 UTC

[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #797: Add TLS support for kvrocks connections

PragmaTwice opened a new pull request, #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797

   We bring TLS support for kvrocks, refer to [redis TLS](https://redis.io/docs/manual/security/encryption/).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r961308941


##########
.github/workflows/kvrocks.yaml:
##########
@@ -126,14 +130,15 @@ jobs:
       - name: Setup macOS
         if: ${{ startsWith(matrix.os, 'macos') }}
         run: |
-          brew install cmake gcc autoconf automake libtool
+          brew install cmake gcc autoconf automake libtool openssl
           echo "NPROC=$(sysctl -n hw.ncpu)" >> $GITHUB_ENV
+          echo "CMAKE_EXTRA_DEFS=-DOPENSSL_ROOT_DIR=/usr/local/opt/openssl" >> $GITHUB_ENV

Review Comment:
   Can also use `brew link --force openssl`



##########
README.md:
##########
@@ -69,14 +69,14 @@ Kvrocks has the following key features:
 ```shell
 # CentOS / RedHat
 sudo yum install -y epel-release
-sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3
+sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3 which openssl-devel
 
 # Ubuntu / Debian
 sudo apt update
-sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3
+sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
 
 # macOS
-brew install autoconf automake libtool cmake
+brew install autoconf automake libtool cmake openssl

Review Comment:
   ```suggestion
   brew install autoconf automake libtool cmake openssl
   # Please force linking the openssl if still can't find it after installing openssl
   brew link --force openssl
   ```



##########
README.md:
##########
@@ -69,14 +69,14 @@ Kvrocks has the following key features:
 ```shell
 # CentOS / RedHat
 sudo yum install -y epel-release
-sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3
+sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3 which openssl-devel
 
 # Ubuntu / Debian
 sudo apt update
-sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3
+sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
 
 # macOS
-brew install autoconf automake libtool cmake
+brew install autoconf automake libtool cmake openssl

Review Comment:
   ```suggestion
   brew install autoconf automake libtool cmake openssl
   
   # Please force linking the openssl if still can't find after installing openssl
   brew link --force openssl
   ```



##########
src/worker.cc:
##########
@@ -105,9 +115,38 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd,
   }
   event_base *base = evconnlistener_get_base(listener);
   auto evThreadSafeFlags = BEV_OPT_THREADSAFE | BEV_OPT_DEFER_CALLBACKS | BEV_OPT_UNLOCK_CALLBACKS;
-  bufferevent *bev = bufferevent_socket_new(base,
-                                            fd,
-                                            evThreadSafeFlags);
+
+  bufferevent *bev;
+#ifdef ENABLE_OPENSSL
+  if (local_port == worker->svr_->GetConfig()->tls_port) {
+    auto ssl = SSL_new(worker->svr_->ssl_ctx_.get());
+    if (!ssl) {
+      LOG(ERROR) << "Failed to construct SSL structure for new connection: " << SSLErrors{};
+      evutil_closesocket(fd);
+      return;
+    }
+    bev = bufferevent_openssl_socket_new(base, fd, ssl, BUFFEREVENT_SSL_ACCEPTING, evThreadSafeFlags);
+  } else {
+    bev = bufferevent_socket_new(base, fd, evThreadSafeFlags);
+  }
+#else
+  bev = bufferevent_socket_new(base, fd, evThreadSafeFlags);
+#endif
+  if (!bev) {
+    auto socket_err = evutil_socket_error_to_string(EVUTIL_SOCKET_ERROR());
+#ifdef ENABLE_OPENSSL
+    LOG(ERROR) << "Failed to construct socket for new connection: " << socket_err << ", SSL error: " << SSLErrors{};
+#else
+    LOG(ERROR) << "Failed to construct socket for new connection: " << socket_err;
+#endif
+    evutil_closesocket(fd);

Review Comment:
   ```suggestion
       SSL_free(ssl);
       evutil_closesocket(fd);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1236564564

   does replication support TLS?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r963357325


##########
src/config.cc:
##########
@@ -95,8 +95,8 @@ Config::Config() {
   };
   FieldWrapper fields[] = {
       {"daemonize", true, new YesNoField(&daemonize, false)},
-      {"bind", true, new StringField(&binds_, "127.0.0.1")},

Review Comment:
   It seems a bug in github, I do not remove this line.
   
   https://github.com/apache/incubator-kvrocks/pull/797/files#diff-76602be0b8dbf09757c540e3b4c474b477cac3bdf38ca318e6284cb34020cd2eR97-R105



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1232922778

   > @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.
   
   Thanks! That is weird. Is the openssl system built-in, or installed from brew?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1233100678

   > > > @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.
   > > 
   > > 
   > > Thanks! That is weird. Is the openssl system built-in, or installed from brew?
   > 
   > Installed from HomeBrew
   
   
   > > > @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.
   > > 
   > > 
   > > Thanks! That is weird. Is the openssl system built-in, or installed from brew?
   > 
   > Installed from HomeBrew
   
   Thanks. I am now trying to configure more available ciphers and protocols to workaround it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1233028750

   > > @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.
   > 
   > Thanks! That is weird. Is the openssl system built-in, or installed from brew?
   
   Installed from HomeBrew
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1236571511

   > does replication support TLS?
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1242912644

   Thanks all. Merging. Feel free to open issues or discussions if you have any problem or suggestion after merge.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1242873323

   > @PragmaTwice I think we can merge this PR if no further discussion and other folks also can review and call a new discussion if they come back. cc @ShooterIT @caipengbo
   
   Great, I will merge it today.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962104026


##########
README.md:
##########
@@ -69,14 +69,14 @@ Kvrocks has the following key features:
 ```shell
 # CentOS / RedHat
 sudo yum install -y epel-release
-sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3
+sudo yum install -y git gcc gcc-c++ make cmake autoconf automake libtool libstdc++-static python3 which openssl-devel
 
 # Ubuntu / Debian
 sudo apt update
-sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3
+sudo apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
 
 # macOS
-brew install autoconf automake libtool cmake
+brew install autoconf automake libtool cmake openssl

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r963357325


##########
src/config.cc:
##########
@@ -95,8 +95,8 @@ Config::Config() {
   };
   FieldWrapper fields[] = {
       {"daemonize", true, new YesNoField(&daemonize, false)},
-      {"bind", true, new StringField(&binds_, "127.0.0.1")},

Review Comment:
   It seems a bug in github, I do not remove this line. Just solved some conflict.
   
   https://github.com/apache/incubator-kvrocks/pull/797/files#diff-76602be0b8dbf09757c540e3b4c474b477cac3bdf38ca318e6284cb34020cd2eR97-R105



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1232865902

   @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1232763613

   Hi all, this PR is now ready for review!
   
   There is an error in TCL TLS test only in macOS, and I think I test a lot to ensure it works well in linux(centos or archlinux, openssl 1.0 or 1.1).
   I will try to figure out the error in mac, and I will be very appreciated if you help me, since I have no Mac device.
   
   cc @git-hulk @ShooterIT @tisonkun @caipengbo 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962515815


##########
.github/workflows/kvrocks.yaml:
##########
@@ -65,10 +65,10 @@ jobs:
       matrix:
         include:
           - name: Darwin Clang
-            os: macos-latest

Review Comment:
   Okay, thanks



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r963348936


##########
src/config.cc:
##########
@@ -95,8 +95,8 @@ Config::Config() {
   };
   FieldWrapper fields[] = {
       {"daemonize", true, new YesNoField(&daemonize, false)},
-      {"bind", true, new StringField(&binds_, "127.0.0.1")},

Review Comment:
   We should not remove this line?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1234376300

   @PragmaTwice I didn't find out the reason but I manually test the tls on macOS and it works normally. Another piece of information is Redis also didn't enable tls test cases on macOS, can see the [redis daily ci](https://github.com/redis/redis/blob/unstable/.github/workflows/daily.yml). I guess maybe there's something wrong with `tcl-tls` since I failed to run tls test cases on my mac.
   
   ```
   ❯ ./runtest --single unit/tls --single unit/info
   Cleanup: may take some time... OK
   Starting test server at port 21079
   [ready]: 77784
   Testing unit/tls
   [ready]: 77785
   Testing unit/info
   [ready]: 77786
   [ready]: 77783
   [ready]: 77787
   [ready]: 77788
   [ready]: 77790
   [ready]: 77789
   [ready]: 77791
   [ready]: 77792
   [ready]: 77793
   [ready]: 77795
   [ready]: 77798
   [ready]: 77796
   [ready]: 77794
   [ready]: 77797
   --- LOG CONTENT ---
   --- LOG CONTENT ---
   [err]: Can't start the Redis server
   CONFIGURATION:always-show-logo yes
   notify-keyspace-events KEA
   daemonize no
   ```
   
   So I think we can disable the tls test cases on mac as well. @ShooterIT @PragmaTwice can correct me if I was wrong.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1233101607

   > > > > @PragmaTwice Yes, I took a look at openssl ciphers on my side and it only supported TLSv1.2 and SSLv3, but not sure what's the minica cert default cipher. Will take time to dive more recently.
   > > > 
   > > > 
   > > > Thanks! That is weird. Is the openssl system built-in, or installed from brew?
   > > 
   > > 
   > > Installed from HomeBrew
   > 
   > Thanks. I am now trying to configure more available ciphers and protocols to workaround it.
   
   hmmmm... I tried it on my M1 machine, it works normally.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962485058


##########
.github/workflows/kvrocks.yaml:
##########
@@ -65,10 +65,10 @@ jobs:
       matrix:
         include:
           - name: Darwin Clang
-            os: macos-latest

Review Comment:
   @PragmaTwice Why do we switch macOS to 11



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962184271


##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,6 +173,18 @@ jobs:
         run: |
           export ${{ matrix.runtime_env_vars }}
           ./x.py test tcl build --cli-path $HOME/local/bin/redis-cli --dont-clean
+          if [[ -z "${{ matrix.without_openssl }}" ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
+            git clone https://github.com/jsha/minica
+            cd minica && go build && cd ..
+            ./minica/minica --domains localhost
+            mkdir -p tests/tcl/tests/tls
+            cp localhost/cert.pem tests/tcl/tests/tls/server.crt
+            cp localhost/key.pem tests/tcl/tests/tls/server.key
+            cp localhost/cert.pem tests/tcl/tests/tls/client.crt
+            cp localhost/key.pem tests/tcl/tests/tls/client.key
+            cp minica.pem tests/tcl/tests/tls/ca.crt
+            ./x.py test tcl build --tls --single unit/tls --single unit/command --dont-clean

Review Comment:
   ```suggestion
               ./x.py test tcl build --tls --single unit/tls --dont-clean
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1232786155

   > > Hi all, this PR is now ready for review!
   > > There is an error in TCL TLS test only in macOS, and I think I test a lot to ensure it works well in linux(centos or archlinux, openssl 1.0 or 1.1, tls v1 v1.1 v1.2 or v1.3). I will try to figure out the error in mac, and I will be very appreciated if you help me, since I have no Mac device.
   > > cc @git-hulk @ShooterIT @tisonkun @caipengbo
   > 
   > Thanks @PragmaTwice, I will try to locate macOS issue, not sure whether it caused by macOS ssl configured to forbid the older version like 1.2
   
   So thanks! The version of openssl in mac is probably 3.0, and you can check here to see the error:
   
   https://github.com/apache/incubator-kvrocks/runs/8110693005?check_suite_focus=true
   
   To reproduce the error, you can just follow `kvrocks.yml`:
   ```
               git clone https://github.com/jsha/minica
               cd minica && go build && cd ..
               ./minica/minica --domains localhost
               mkdir -p tests/tcl/tests/tls
               cp localhost/cert.pem tests/tcl/tests/tls/server.crt
               cp localhost/key.pem tests/tcl/tests/tls/server.key
               cp localhost/cert.pem tests/tcl/tests/tls/client.crt
               cp localhost/key.pem tests/tcl/tests/tls/client.key
               cp minica.pem tests/tcl/tests/tls/ca.crt
               ./x.py test tcl build --tls --single unit/tls --single unit/command --dont-clean
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1234412538

   @git-hulk So thanks for your help! 
   
   As discussed earlier, maybe tcl-tls is linked against the builtin openssl in mac, thus it behaves weirdly. I will temporarily disable the TCL TLS tests on MacOS so this PR can move on. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962499234


##########
.github/workflows/kvrocks.yaml:
##########
@@ -65,10 +65,10 @@ jobs:
       matrix:
         include:
           - name: Darwin Clang
-            os: macos-latest

Review Comment:
   macos-11 is equal to macos-latest, referring to https://github.com/actions/runner-images#available-images
   
   I used to try macos-12, but I reverted the change later.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r962104053


##########
.github/workflows/kvrocks.yaml:
##########
@@ -126,14 +130,15 @@ jobs:
       - name: Setup macOS
         if: ${{ startsWith(matrix.os, 'macos') }}
         run: |
-          brew install cmake gcc autoconf automake libtool
+          brew install cmake gcc autoconf automake libtool openssl
           echo "NPROC=$(sysctl -n hw.ncpu)" >> $GITHUB_ENV
+          echo "CMAKE_EXTRA_DEFS=-DOPENSSL_ROOT_DIR=/usr/local/opt/openssl" >> $GITHUB_ENV

Review Comment:
   done



##########
src/worker.cc:
##########
@@ -105,9 +115,38 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd,
   }
   event_base *base = evconnlistener_get_base(listener);
   auto evThreadSafeFlags = BEV_OPT_THREADSAFE | BEV_OPT_DEFER_CALLBACKS | BEV_OPT_UNLOCK_CALLBACKS;
-  bufferevent *bev = bufferevent_socket_new(base,
-                                            fd,
-                                            evThreadSafeFlags);
+
+  bufferevent *bev;
+#ifdef ENABLE_OPENSSL
+  if (local_port == worker->svr_->GetConfig()->tls_port) {
+    auto ssl = SSL_new(worker->svr_->ssl_ctx_.get());
+    if (!ssl) {
+      LOG(ERROR) << "Failed to construct SSL structure for new connection: " << SSLErrors{};
+      evutil_closesocket(fd);
+      return;
+    }
+    bev = bufferevent_openssl_socket_new(base, fd, ssl, BUFFEREVENT_SSL_ACCEPTING, evThreadSafeFlags);
+  } else {
+    bev = bufferevent_socket_new(base, fd, evThreadSafeFlags);
+  }
+#else
+  bev = bufferevent_socket_new(base, fd, evThreadSafeFlags);
+#endif
+  if (!bev) {
+    auto socket_err = evutil_socket_error_to_string(EVUTIL_SOCKET_ERROR());
+#ifdef ENABLE_OPENSSL
+    LOG(ERROR) << "Failed to construct socket for new connection: " << socket_err << ", SSL error: " << SSLErrors{};
+#else
+    LOG(ERROR) << "Failed to construct socket for new connection: " << socket_err;
+#endif
+    evutil_closesocket(fd);

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1242768373

   @PragmaTwice I think we can merge this PR if no further discussion and other folks also can review and call a new discussion if they come back. cc @ShooterIT @caipengbo 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice merged pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1232771980

   > Hi all, this PR is now ready for review!
   > 
   > There is an error in TCL TLS test only in macOS, and I think I test a lot to ensure it works well in linux(centos or archlinux, openssl 1.0 or 1.1, tls v1 v1.1 v1.2 or v1.3). I will try to figure out the error in mac, and I will be very appreciated if you help me, since I have no Mac device.
   > 
   > cc @git-hulk @ShooterIT @tisonkun @caipengbo
   
   Thanks @PragmaTwice, I will try to locate macOS issue, not sure whether it caused by macOS ssl configured to forbid the older version like 1.2


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1237633171

   Redis also separate the replication TLS, so I think it's a good idea to only implement server TLS in this PR. Even we don't implement the replication TLS unless someone required it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#issuecomment-1237764562

   @ShooterIT @caipengbo Can take a look when you're free. This PR involved many files and it'll be conflicts frequently if it's pending too long.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #797: Add TLS support for kvrocks connections

Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #797:
URL: https://github.com/apache/incubator-kvrocks/pull/797#discussion_r963361723


##########
src/config.cc:
##########
@@ -95,8 +95,8 @@ Config::Config() {
   };
   FieldWrapper fields[] = {
       {"daemonize", true, new YesNoField(&daemonize, false)},
-      {"bind", true, new StringField(&binds_, "127.0.0.1")},

Review Comment:
   @PragmaTwice Yes, that's weird. It only can be seen in last commit, let ignore this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org