Add password authentication to Redis ports #2952

pschafhalter · 2018-09-25T05:54:48Z

What do these changes do?

Adds password authentication to Redis ports via ray.init(redis_password="my_password")
Throws exception if redis_password is set, but Raylet is not running
Adds a test to verify that Redis ports are secure

Related issue number

AmplabJenkins · 2018-09-25T07:25:22Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8341/
Test PASSed.

AmplabJenkins · 2018-09-25T08:08:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8343/
Test FAILed.

python/ray/test/test_ray_init.py

robertnishihara · 2018-09-26T06:23:07Z

python/ray/services.py

@@ -1315,6 +1365,7 @@ def start_ray_processes(address_info=None,
                        num_redis_shards=1,
                        redis_max_clients=None,
                        redis_protected_mode=False,
+                        redis_password=None,


We probably want to remove all instances of the redis_protected_mode/protected_mode argument, right? And also essentially remove everything from #2697, right (e.g., the code for making the temp file and stuff)?

Yes, I was planning to do that in a follow-up PR so I don't change too much at once. If you think it's more appropriate, I'll clean up redis_protected_mode in this PR.

A follow up PR is fine.

robertnishihara · 2018-09-26T06:29:12Z

src/ray/gcs/redis_context.cc

@@ -135,7 +135,30 @@ RedisContext::~RedisContext() {
  }
 }

-Status RedisContext::Connect(const std::string &address, int port, bool sharding) {
+static std::string default_password = "";


This appears in multiple places. Consider moving it to https://github.com/ray-project/ray/blob/master/src/ray/constants.h.

AmplabJenkins · 2018-09-26T09:58:21Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8361/
Test PASSed.

AmplabJenkins · 2018-10-03T10:21:12Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8525/
Test PASSed.

AmplabJenkins · 2018-10-11T03:21:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8593/
Test PASSed.

AmplabJenkins · 2018-10-13T00:51:10Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8606/
Test PASSed.

AmplabJenkins · 2018-10-16T04:28:44Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8670/
Test PASSed.

robertnishihara · 2018-10-16T04:37:35Z

doc/source/security.rst

+-----------------------------
+
+Ray instances should run on a secure network without public facing ports.
+The most common threat for Ray instances is unautherized access to Redis,


typo "unauthorized"

robertnishihara · 2018-10-16T04:40:56Z

python/ray/test/test_ray_init.py

+    password = "some_password"
+    exception = None
+
+    # Fix for #3045


Include the full URL

Actually, I don't think we need to make a global variable, in generally we should be doing less in the fixture and more in the actual test. Why not move everything to the actual tests and just leave something like

ray/test/runtest.py

Lines 205 to 209 in 6240ccb

@pytest.fixture

def shutdown_only():

yield None

# The code after the yield will run as teardown code.

ray.shutdown()

in the fixture?

robertnishihara · 2018-10-16T04:41:59Z

python/ray/test/test_ray_init.py

+
+@pytest.fixture
+def start_ray_with_password():
+    ray.shutdown()


Why are we calling ray.shutdown at the start of the fixture? We don't do this in any of our other tests.

Instead, our fixtures usually do something like

ray.init() yield None ray.shutdown()

robertnishihara · 2018-10-16T04:48:16Z

python/ray/test/test_ray_init.py

+    try:
+        info = ray.init(redis_password=password)
+    except Exception as e:
+        info = ray.init(redis_password=None)


What is the purpose of this codepath?

I intended to test that an exception is thrown when passing a redis_password without using raylet, although I can do this instead:

ray/test/runtest.py

Lines 2165 to 2167 in 6240ccb

@pytest.mark.skipif(

os.environ.get("RAY_USE_NEW_GCS") == "on",

reason="New GCS API doesn't have a Python API yet.")

Yeah, I'd just skip the test entirely in the non-raylet case. If you want to include a test for the non-raylet case then I'd just make it a separate test.

robertnishihara · 2018-10-16T04:48:48Z

python/ray/test/test_ray_init.py

+
+
+@pytest.fixture
+def use_credis():


You can also just use

ray/test/runtest.py

Lines 2165 to 2167 in 6240ccb

@pytest.mark.skipif(

os.environ.get("RAY_USE_NEW_GCS") == "on",

reason="New GCS API doesn't have a Python API yet.")

robertnishihara · 2018-10-16T04:49:52Z

python/ray/test/test_ray_init.py

+    def test_raylet_only(self, start_ray_with_password, use_credis):
+        password, info, exception, use_raylet = start_ray_with_password
+        if use_raylet and not use_credis:
+            assert exception is None


It's natural to just run the test and if it raises an exception, then pytest will report the error. Instead, we're catching the exception and then asserting that the exception is not None, which is very roundabout.

robertnishihara · 2018-10-16T04:50:27Z

python/ray/test/test_ray_init.py

+
+
+class TestRedisPassword(object):
+    def test_raylet_only(self, start_ray_with_password, use_credis):


These tests can probably all be combined into one.

robertnishihara · 2018-10-16T04:51:38Z

src/ray/gcs/redis_context.cc

@@ -135,7 +135,30 @@ RedisContext::~RedisContext() {
  }
 }

-Status RedisContext::Connect(const std::string &address, int port, bool sharding) {
+Status authenticate_redis(redisContext *context, const std::string &password) {


These functions don't follow the naming convention I think. @pcmoritz can comment, but I think it should be AuthenticateRedis.

robertnishihara · 2018-10-16T06:44:44Z

python/ray/test/test_ray_init.py

+        # Check that we can run a task
+        task_id = f.remote()
+        ready, running = ray.wait([task_id], timeout=5000)
+        assert len(ready) > 0, "Expected task to complete"


Kind of a nit, but task_id should be object_id.

Also, instead of doing wait followed by an assert, maybe just do ray.get(object_id).

Do you mind if I keep the ray.wait? While testing, I ran into a case where ray.get wouldn't fail, but block indefinitely (I think this was when I discovered the issues with the new GCS).

AmplabJenkins · 2018-10-16T07:42:27Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8672/
Test PASSed.

AmplabJenkins · 2018-10-16T08:15:52Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8673/
Test PASSed.

AmplabJenkins · 2018-10-16T09:39:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8676/
Test FAILed.

robertnishihara · 2018-10-16T16:23:19Z

@pschafhalter looks good to me except there are linting errors. Could you fix those?

AmplabJenkins · 2018-10-16T18:05:25Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8680/
Test FAILed.

AmplabJenkins · 2018-10-16T18:44:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/8681/
Test PASSed.

Follow-up to #2925 and #2952. Removes the Redis protected mode implementation from Ray which was replaced by Redis port authentication.

igorkost-ibm · 2018-10-24T10:41:29Z

File python/ray/worker.py [line 2084]: redis_password wasn't passed to services.record_log_files_in_redis(...) function.

pschafhalter · 2018-10-24T17:41:30Z

@igoriokas thanks! I'll fix this in a follow-up PR.

robertnishihara reviewed Sep 26, 2018

View reviewed changes

pschafhalter changed the title ~~Add password authentication to Redis ports~~ [WIP] Add password authentication to Redis ports Oct 3, 2018

This was referenced Oct 4, 2018

[Ray] Secure Redis ports modin-project/modin#108

Closed

Support authenticating Redis ports ray-project/credis#11

Open

pschafhalter added 16 commits October 10, 2018 12:21

Implement Redis authentication

5cb5af8

Throw exception for legacy Ray

e6c669e

Add test

772cff0

Formatting

fde3c16

Fix bugs in CLI

e1fe5dd

Fix bugs in Raylet

053ae44

Move default password to constants.h

ce24f42

Use pytest.fixture

8cac24b

Fix bug

386d69b

Authenticate using formatted strings

901e8b4

Add missing passwords

ef62552

Add test

9291512

Improve authentication of async contexts

2b41641

Disable Redis authentication for credis

0e6609e

Update test for credis

1be3105

Fix rebase artifacts

7200454

pschafhalter force-pushed the redis-password branch from 30ba031 to 7200454 Compare October 11, 2018 01:53

pschafhalter added 2 commits October 12, 2018 16:16

Fix formatting

bc514fd

Add workaround for issue ray-project#3045

15edd1d

pschafhalter changed the title ~~[WIP] Add password authentication to Redis ports~~ Add password authentication to Redis ports Oct 12, 2018

robertnishihara reviewed Oct 16, 2018

View reviewed changes

Address comments

d13edca

robertnishihara reviewed Oct 16, 2018

View reviewed changes

Address comments

6944814

Adress comments

8e090fe

pschafhalter added 2 commits October 16, 2018 10:08

Use ray.get

00c045f

Fix lint

f3d68e2

pcmoritz approved these changes Oct 17, 2018

View reviewed changes

pcmoritz merged commit a41bbc1 into ray-project:master Oct 17, 2018

pschafhalter deleted the redis-password branch October 17, 2018 07:21

pschafhalter mentioned this pull request Oct 17, 2018

Remove Redis protected mode #3073

Merged

robertnishihara pushed a commit that referenced this pull request Oct 18, 2018

Remove Redis protected mode (#3073)

b82fd15

Follow-up to #2925 and #2952. Removes the Redis protected mode implementation from Ray which was replaced by Redis port authentication.

robertnishihara mentioned this pull request Oct 19, 2018

Redis protected mode does not secure Redis ports #2925

Closed

pschafhalter mentioned this pull request Oct 24, 2018

Fix password authentication in worker #3124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add password authentication to Redis ports #2952

Add password authentication to Redis ports #2952

pschafhalter commented Sep 25, 2018

AmplabJenkins commented Sep 25, 2018

AmplabJenkins commented Sep 25, 2018

robertnishihara Sep 26, 2018

pschafhalter Sep 26, 2018

robertnishihara Oct 16, 2018

robertnishihara Sep 26, 2018

AmplabJenkins commented Sep 26, 2018

AmplabJenkins commented Oct 3, 2018

AmplabJenkins commented Oct 11, 2018

AmplabJenkins commented Oct 13, 2018

AmplabJenkins commented Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

pschafhalter Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

robertnishihara Oct 16, 2018

pschafhalter Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

robertnishihara commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

igorkost-ibm commented Oct 24, 2018

pschafhalter commented Oct 24, 2018

	@pytest.fixture
	def shutdown_only():
	yield None
	# The code after the yield will run as teardown code.
	ray.shutdown()

	@pytest.mark.skipif(
	os.environ.get("RAY_USE_NEW_GCS") == "on",
	reason="New GCS API doesn't have a Python API yet.")



		class TestRedisPassword(object):
		def test_raylet_only(self, start_ray_with_password, use_credis):

Add password authentication to Redis ports #2952

Add password authentication to Redis ports #2952

Conversation

pschafhalter commented Sep 25, 2018

What do these changes do?

Related issue number

AmplabJenkins commented Sep 25, 2018

AmplabJenkins commented Sep 25, 2018

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

AmplabJenkins commented Sep 26, 2018

AmplabJenkins commented Oct 3, 2018

AmplabJenkins commented Oct 11, 2018

AmplabJenkins commented Oct 13, 2018

AmplabJenkins commented Oct 16, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

robertnishihara commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

AmplabJenkins commented Oct 16, 2018

igorkost-ibm commented Oct 24, 2018

pschafhalter commented Oct 24, 2018