[core] Support encrypted redis connection. #28628

fishbone · 2022-09-19T23:58:37Z

Why are these changes needed?

This PR support encrypted redis connection. If the address starts with rediss:https://, it'll start the redis in SSL mode.

To support better ssl, redis is also upgraded. It's not supposed to impact ray because it's the user's responsibility to provide a redis.

Right now all flags are provided by the os envs. It's not ideal and we should design a better format when we refactor the db layer.

Besides, right now gRPC is using OS envs for the encrypted connection in ray, just follow this pattern for now.

Related issue number

Closes #27371

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

fishbone · 2022-09-21T03:59:08Z

hmmm, it seems the building of redis has some issues. I'm looking into this.

fishbone · 2022-09-21T05:05:26Z

sentinel.c:34:10: fatal error: openssl/ssl.h: No such file or directory
   34 | #include "openssl/ssl.h"
      |          ^~~~~~~~~~~~~~~

I need to change the bazel rule to build redis.

scv119 · 2022-09-21T15:17:54Z

src/ray/common/ray_config_def.h

@@ -655,6 +655,16 @@ RAY_CONFIG(std::string, TLS_SERVER_CERT, "")
 RAY_CONFIG(std::string, TLS_SERVER_KEY, "")
 RAY_CONFIG(std::string, TLS_CA_CERT, "")

+/// Location of Redis TLS credentials
+/// https://github.com/redis/hiredis/blob/c78d0926bf169670d15cfc1214e4f5d21673396b/README.md#hiredis-openssl-wrappers
+RAY_CONFIG(bool, REDIS_ENABLE_SSL, false)


hmm why upper case?

scv119

also, is it possible to add a test to show it works e2e?

scv119 · 2022-09-21T15:18:53Z

src/ray/gcs/redis_context.cc

+ }
+}
+
+


should we call redisFreeSSLContext here in the destructor?

simon-mo

I'm a little bit confused by this PR. What's the workflow for:

use SSL with the redis bundled/started by Ray (I assume this is supported by the services.py:_start_redis_instance change. Do users need to supply all the CERTs parameters?
Which env var does the user use to connect to hosted redis (e.g. aws memorystore https://docs.aws.amazon.com/memorydb/latest/devguide/getting-startedclusters.connecttonode.html)

simon-mo · 2022-09-21T00:02:33Z

bazel/ray_deps_setup.bzl

+ # patches = [
+ #  "@com_github_ray_project_ray//thirdparty/patches:redis-quiet.patch",
+ # ],


do we still need this patch?

didn't mean to approve, this PR needs documentation change as well

fishbone · 2022-09-21T21:11:20Z

@scv119 ah,, I forget to include the unit test. I actually have one but forgot to add.

fishbone · 2022-09-21T21:16:18Z

@simon-mo the services change is only for testing. Ray will not start redis for the user anymore. Ideally, the Redis change should be moved from the service.py to test.py.
So:

use SSL with the redis bundled/started by Ray (I assume this is supported by the services.py:_start_redis_instance change. Do users need to supply all the CERTs parameters?

It's the user's responsibility to bring a redis. How to setup the cert it depends on the Redis doc.

Which env var does the user use to connect to hosted redis (e.g. aws memorystore https://docs.aws.amazon.com/memorydb/latest/devguide/getting-startedclusters.connecttonode.html)

I'm not quite sure about AWS's use case. I think here it's more related to the Redis client itself. Everything is based on Redis protocol.

fishbone · 2022-09-21T21:21:49Z

didn't mean to approve, this PR needs documentation change as well

@simon-mo right now, all the GCS Fault Tolerance features are only to support ray serve and the only place where ray serves can be HA will be in KubeRay Ray Service Operator. The users can't simply use these features by themselves.

I think we should just add the flags into the kuberay's doc. So it shouldn't be in this one. I'll add it once it's merged.

Signed-off-by: Yi Cheng <[email protected]>

ijrsvt · 2022-09-27T18:39:19Z

bazel/ray_deps_setup.bzl

@@ -98,8 +98,8 @@ def ray_deps_setup():
 auto_http_archive(
 name = "com_github_antirez_redis",
 build_file = "@com_github_ray_project_ray//bazel:BUILD.redis",
- url = "https://github.com/redis/redis/archive/6.0.10.tar.gz",
- sha256 = "900cb82227bac58242c9b7668e7113cd952253b256fe04bbdab1b78979cf255a",
+ url = "https://github.com/redis/redis/archive/refs/tags/7.0.4.tar.gz",


Can we switch to 7.0.5 for Security Fixes?

simon-mo

can we consider a better API like RAY_REDIS_ADDRESS=rediss:https://...?

fishbone · 2022-09-27T22:15:05Z

@simon-mo updated to rediss:https://

…anch Signed-off-by: Yi Cheng <[email protected]>

Signed-off-by: Yi Cheng <[email protected]>

fishbone · 2022-10-03T05:28:21Z

test failure not related.

This reverts commit 7167f9c.

…ct#28628)"" This reverts commit 5d7be34.

Reverts #28628 Looks like this fails the build in the master. I verified it works after reverting this PR ``` ERROR: /Users/sangbincho/work/ray/BUILD.bazel:2764:18 Executing genrule //:cp_redis failed: (Exit 64): bash failed: error executing command (cd /private/var/tmp/_bazel_sangbincho/f7d6f25d281df169000c577629db7adf/sandbox/darwin-sandbox/1985/execroot/com_github_ray_project_ray && \ exec env - \ PATH=/Users/sangbincho/google-cloud-sdk/bin:/Users/sangbincho/anaconda3/envs/core/bin:/Users/sangbincho/anaconda3/condabin:/Users/sangbincho/.nvm/versions/node/v14.19.2/bin:/Library/Frameworks/Python.framework/Versions/3.8/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Users/sangbincho/.cargo/bin:/Users/sangbincho/bin \ /bin/bash -c 'source external/bazel_tools/tools/genrule/genrule-setup.sh; mkdir tmp-redis-bin cp bazel-out/darwin-opt/bin/external/com_github_antirez_redis/copy_redis/redis bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/bin/redis-cli bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/bin/redis-server bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/include ./tmp-redis-bin/ -rf cp tmp-redis-bin/redis-server bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-server chmod +x bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-server cp tmp-redis-bin/redis-cli bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-cli chmod +x bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-cli rm -rf tmp-redis-bin ') Execution platform: @local_config_platform//:host ```

This PR supports the encrypted redis connection. If the address starts with rediss:https://, it'll start the redis in SSL mode. To support better ssl, redis is also upgraded. It's not supposed to impact ray because it's the user's responsibility to provide a redis. Right now all flags are provided by the os envs. It's not ideal and we should design a better format when we refactor the db layer. Besides, right now gRPC is using OS envs for the encrypted connection in ray, just follow this pattern for now. Signed-off-by: Weichen Xu <[email protected]>

Reverts ray-project#28628 Looks like this fails the build in the master. I verified it works after reverting this PR ``` ERROR: /Users/sangbincho/work/ray/BUILD.bazel:2764:18 Executing genrule //:cp_redis failed: (Exit 64): bash failed: error executing command (cd /private/var/tmp/_bazel_sangbincho/f7d6f25d281df169000c577629db7adf/sandbox/darwin-sandbox/1985/execroot/com_github_ray_project_ray && \ exec env - \ PATH=/Users/sangbincho/google-cloud-sdk/bin:/Users/sangbincho/anaconda3/envs/core/bin:/Users/sangbincho/anaconda3/condabin:/Users/sangbincho/.nvm/versions/node/v14.19.2/bin:/Library/Frameworks/Python.framework/Versions/3.8/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Users/sangbincho/.cargo/bin:/Users/sangbincho/bin \ /bin/bash -c 'source external/bazel_tools/tools/genrule/genrule-setup.sh; mkdir tmp-redis-bin cp bazel-out/darwin-opt/bin/external/com_github_antirez_redis/copy_redis/redis bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/bin/redis-cli bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/bin/redis-server bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis/include ./tmp-redis-bin/ -rf cp tmp-redis-bin/redis-server bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-server chmod +x bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-server cp tmp-redis-bin/redis-cli bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-cli chmod +x bazel-out/darwin-opt/bin/external/com_github_antirez_redis/redis-cli rm -rf tmp-redis-bin ') Execution platform: @local_config_platform//:host ``` Signed-off-by: Weichen Xu <[email protected]>

fishbone marked this pull request as ready for review September 20, 2022 23:59

fishbone assigned scv119 and simon-mo Sep 20, 2022

fishbone force-pushed the rediss-support branch from e625292 to 977abdd Compare September 21, 2022 00:01

fishbone assigned rkooo567 Sep 21, 2022

fishbone force-pushed the rediss-support branch 2 times, most recently from 9fbbec2 to 1afb5a9 Compare September 21, 2022 00:21

scv119 reviewed Sep 21, 2022

View reviewed changes

src/ray/gcs/redis_context.cc

}

}

Copy link

Contributor

scv119 Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call redisFreeSSLContext here in the destructor?

scv119 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Sep 21, 2022

simon-mo previously approved these changes Sep 21, 2022

View reviewed changes

simon-mo self-requested a review September 21, 2022 20:08

fishbone force-pushed the rediss-support branch from 952f959 to 7adaf08 Compare September 26, 2022 21:52

reset

7d578a7

Signed-off-by: Yi Cheng <[email protected]>

fishbone force-pushed the rediss-support branch from f5b383b to 7d578a7 Compare September 26, 2022 23:24

stash

37250da

Signed-off-by: Yi Cheng <[email protected]>

ijrsvt reviewed Sep 27, 2022

View reviewed changes

ijrsvt mentioned this pull request Sep 27, 2022

[Redis] Upgrade to 6.2.7 for Vuln Fixes #28813

Closed

7 tasks

simon-mo approved these changes Sep 27, 2022

View reviewed changes

fishbone added 2 commits October 1, 2022 21:16

Merge remote-tracking branch 'upstream/master' into rediss-support-br…

3ccd837

…anch Signed-off-by: Yi Cheng <[email protected]>

make openssl into bazel

ec7d467

Signed-off-by: Yi Cheng <[email protected]>

fishbone force-pushed the rediss-support branch from b01a624 to ec7d467 Compare October 2, 2022 00:04

fix comment

fe3839e

Signed-off-by: Yi Cheng <[email protected]>

fishbone added 4 commits October 2, 2022 01:30

[rebuild_base] rebuild base

f4c3c55

Signed-off-by: Yi Cheng <[email protected]>

fix

30e782a

Signed-off-by: Yi Cheng <[email protected]>

fix lint

be816ef

Signed-off-by: Yi Cheng <[email protected]>

fix win build

03a8443

Signed-off-by: Yi Cheng <[email protected]>

fishbone merged commit 7167f9c into ray-project:master Oct 3, 2022

fishbone deleted the rediss-support branch October 3, 2022 05:28

rkooo567 added a commit that referenced this pull request Oct 3, 2022

Revert "[core] Support encrypted redis connection. (#28628)"

e71d0d6

This reverts commit 7167f9c.

rkooo567 mentioned this pull request Oct 3, 2022

Revert "[core] Support encrypted redis connection." #28991

Merged

rkooo567 added a commit to rkooo567/ray that referenced this pull request Oct 3, 2022

Revert "[core] Support encrypted redis connection. (ray-project#28628)"

5d7be34

This reverts commit 7167f9c.

rkooo567 added a commit to rkooo567/ray that referenced this pull request Oct 3, 2022

Revert "Revert "[core] Support encrypted redis connection. (ray-proje…

b3f6587

…ct#28628)"" This reverts commit 5d7be34.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Support encrypted redis connection. #28628

[core] Support encrypted redis connection. #28628

fishbone commented Sep 19, 2022 •

edited

Loading

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

scv119 Sep 21, 2022

scv119 left a comment

scv119 Sep 21, 2022

simon-mo left a comment

simon-mo Sep 21, 2022

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

ijrsvt Sep 27, 2022

simon-mo left a comment

fishbone commented Sep 27, 2022

fishbone commented Oct 3, 2022

[core] Support encrypted redis connection. #28628

[core] Support encrypted redis connection. #28628

Conversation

fishbone commented Sep 19, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

scv119 Sep 21, 2022

Choose a reason for hiding this comment

scv119 left a comment

Choose a reason for hiding this comment

scv119 Sep 21, 2022

Choose a reason for hiding this comment

simon-mo left a comment

Choose a reason for hiding this comment

simon-mo Sep 21, 2022

Choose a reason for hiding this comment

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

fishbone commented Sep 21, 2022

ijrsvt Sep 27, 2022

Choose a reason for hiding this comment

simon-mo left a comment

Choose a reason for hiding this comment

fishbone commented Sep 27, 2022

fishbone commented Oct 3, 2022

fishbone commented Sep 19, 2022 •

edited

Loading