feat(replication): allow non-tls connections between replica and master #1419 #1490

kostasrim · 2023-06-28T16:08:47Z

Addresses #1419 and form the basis for #1418.

Allow non-tls connections between replica and master on admin port
Add test
Add tls certificate generator

.github/workflows/regression-tests.yml

tests/dragonfly/tls_replication_test.py

src/facade/dragonfly_connection.cc

royjacobson · 2023-07-02T14:06:43Z

General comment about this: Whether we want TLS is a question of what network interface we listen on and its security. If the network doesn't need TLS then we don't need TLS at all, if the network needs TLS then we should always have TLS. From a security perspective I really don't like that it is specific to replication.

kostasrim · 2023-07-02T17:28:25Z

@royjacobson

General comment about this: Whether we want TLS is a question of what network interface we listen on and its security. If the network doesn't need TLS then we don't need TLS at all, if the network needs TLS then we should always have TLS. From a security perspective I really don't like that it is specific to replication.

Why do you think it's a security issue if it's specific for replication? If we are on a private network then why would the replica need to communicate over tls with master? (I think this was the original idea).

I am not an expert in security, so plz add your concerns here and we can chat a little bit tomorrow on tomorrow's standup.

royjacobson · 2023-07-02T21:26:26Z

@royjacobson

General comment about this: Whether we want TLS is a question of what network interface we listen on and its security. If the network doesn't need TLS then we don't need TLS at all, if the network needs TLS then we should always have TLS. From a security perspective I really don't like that it is specific to replication.

Why do you think it's a security issue if it's specific for replication? If we are on a private network then why would the replica need to communicate over tls with master? (I think this was the original idea).

I am not an expert in security, so plz add your concerns here and we can chat a little bit tomorrow on tomorrow's standup.

My problem is that this is an unclean security abstraction: If the network is private then all connections should be without TLS. Making this relaxation specific for replication creates a difference in the network security level that is based on application logic and not on the network logic.

src/facade/dragonfly_connection.cc

.github/workflows/regression-tests.yml

adiholden · 2023-07-05T14:16:37Z

src/facade/dragonfly_connection.cc

  }
-  FiberSocketBase* peer = tls_sock ? (FiberSocketBase*)tls_sock.get() : socket_.get();


why was this removed?

Because I simplified it a little bit, it gets initialized at the top :)

Maybe I am missing something but in the old flow tls_sock is assigned to peer if tls_sock not null. I dont that you update peer with tls_sock in the new flow

how is it working?
I would expect the test you wrote to fail.

Indeed I messed this up on refactoring and HUGE thumps up for catching this. It works, because we only test the admin port and not the TLS connection itself -- we have 0 tests for that (although my other PR addresses this and it would have been caught there).

src/facade/dragonfly_connection.h

tests/dragonfly/tls_replication_test.py

adiholden · 2023-07-05T14:35:34Z

tests/dragonfly/tls_replication_test.py

+        raise "Non tls connection connected on master with tls. This should NOT happen"
+    except redis.ConnectionError:
+        pass
+


lets add a connection via admin port and run DBSIZE command, see that it passes

this can also be a test in connection_test.py as checking that running command via admin port does not require tls is not related to replication

I ended up having it here. It's not related to replication but it doesn't require to create a new test case for that. It's small detail ;)

hmm, I kind of agree with Adi here. The feature here is being able to connect to the admin port without using the tls. I think the slave part is redundant machinery to cover this test.

Maybe I missed something but @adiholden asked to promt admin port directly (on previous commits that was not the case because of how I initially implemented this) -- which I addressed.

Now for (# 2. Try to connect on master without admin port. This should fail.), I think it's good to be here because it asserts that the changes/flags introduced that allowed non-tls connections on admin port did not also allow non-tls connections on other ports (it could be the case that I messed up the if conditions here: https://github.com/dragonflydb/dragonfly/pull/1490/files#diff-b88cbca1b2c9337ab2a37262f9d516b117f4996da25e2ae173e0e129f2b82b20R310) -- this guards as against that potential mistake and that's why I put it here.

The comment here is that the changes in dfly code are not specific for replication, we enable connecting from admin port without tls. Therefor we would like to test this specifically and the replication test is another test to check that replication work with this, we can add this test but f.e if replication with no tls breaks this does not mean that connecting via admin port with no tls fails.
f.e if the PR was about adding admin port to dfly I would test it in connection test, it is importatnt to validate that replication as well works with admin port there fore I will add another test for replication with admin port
I believe that we need

adiholden · 2023-07-05T14:42:50Z

tests/dragonfly/tls_replication_test.py

+    c_replica = aioredis.Redis(port=replica.port)
+    res = await c_replica.execute_command("REPLICAOF localhost " + str(ADMIN_PORT))
+    assert b"OK" == res
+    time.sleep(10)


instead of sleep
use
await check_all_replicas_finished([c_replica], c_master)
The clients here should be connected to admin port

please remove the sleep

I would have taken an oath that I removed it, I really don't know how it got left -- removed

adiholden · 2023-07-05T14:45:21Z

tests/dragonfly/tls_replication_test.py

+@dfly_args({"dbfilename": "test-tls-replication-{{timestamp}}"})
+async def test_replication_all(df_local_factory, df_seeder_factory, t_master, t_replica):
+    # 1. Spin up dragonfly without tls, debug populate and then shut it down
+    master = df_local_factory.create(port=BASE_PORT, proactor_threads=t_master)


I think that this first step is redundant, as you can run the dubug populate command from admin port when starting master with the tls args

adiholden · 2023-07-05T14:58:39Z

tests/dragonfly/tls_replication_test.py

+    #   b. Allow non-tls connection from replica to master
+    master = df_local_factory.create(admin_port=ADMIN_PORT, admin_nopass=True, **with_tls_args, port=BASE_PORT, proactor_threads=t_master)
+    master.start()
+    # we need to sleep, because currently tls connections on master fail


we need to sleep because lts connection fail? I dont understand this.. where do you create connection? how is sleeping helps here?

that was left from the rpevious PR. I will remove it

adiholden · 2023-07-05T15:01:13Z

tests/dragonfly/tls_replication_test.py

+    replica = df_local_factory.create(port=BASE_PORT + 1, proactor_threads=t_replica,)
+    replica.start()
+    c_replica = aioredis.Redis(port=replica.port)
+    res = await c_replica.execute_command("REPLICAOF localhost " + str(ADMIN_PORT))


"REPLICAOF localhost {master.admin_port}"

adiholden · 2023-07-05T15:02:48Z

tests/dragonfly/tls_replication_test.py

+        pass
+
+    # 4. Spin up a replica and initiate a REPLICAOF
+    replica = df_local_factory.create(port=BASE_PORT + 1, proactor_threads=t_replica,)


I suggest creating replica with tls args as well, you can run the commands on replica using the admin port

romange · 2023-07-05T18:16:32Z

tests/dragonfly/tls_replication_test.py

+                    "no_tls_on_admin_port": "true"}
+    return with_tls_args
+
+def gen_tls_cert(dfly_path):


consider making it a fixture (in conftest.py) similarly to other resources being consumed by tests.

romange · 2023-07-05T18:17:37Z

src/facade/dragonfly_connection.h

@@ -19,6 +19,7 @@
 #include "core/fibers.h"
 #include "facade/facade_types.h"
 #include "facade/resp_expr.h"
+#include "util/tls/tls_socket.h"


why did you need it here?

Oh it's FiberSocketBase *, I completely missed that, I thought it's used explicitly. removed.

adiholden · 2023-07-06T04:58:29Z

tests/dragonfly/tls_replication_test.py

+    db_size = await c_replica.execute_command("DBSIZE")
+    assert 100 == db_size
+
+    master.stop()


I believe you dont need to call stop at the end of the test as this is done in df_local_factory

royjacobson · 2023-07-06T07:44:41Z

tests/dragonfly/connection_test.py

@@ -2,6 +2,7 @@
 import pytest
 import asyncio
 from redis import asyncio as aioredis
+from redis.exceptions import ConnectionError


ConnectionError is a Python builtin so it's probably best not to override it

royjacobson · 2023-07-06T07:45:10Z

tests/dragonfly/connection_test.py

+@pytest.mark.asyncio
+@dfly_args({"admin_nopass" : True})
+async def test_reject_non_tls_connections_on_tls_master(gen_tls_cert, tls_server_key_file_name, tls_server_cert_file_name, df_local_factory, df_seeder_factory):
+    gen_tls_cert


Suggested change

gen_tls_cert

?

royjacobson · 2023-07-06T07:48:11Z

src/facade/dragonfly_connection.cc

-      LOG(WARNING) << "Error handshaking " << aresult.error().message();
-      return;
+    const bool no_tls_on_admin_port = absl::GetFlag(FLAGS_no_tls_on_admin_port);
+    if (!no_tls_on_admin_port || (no_tls_on_admin_port && !IsAdmin())) {


I find this a bit more readable -

Suggested change

if (!no_tls_on_admin_port || (no_tls_on_admin_port && !IsAdmin())) {

if (!(IsAdmin() && no_tls_on_admin_port)) {

royjacobson · 2023-07-06T07:50:59Z

tests/dragonfly/__init__.py

@@ -159,6 +159,13 @@ def stop_all(self):
    def __str__(self):
        return f"Factory({self.args})"

+    @property
+    def dfly_path(self):


Maybe use os.path.basedir here? hard-coding the test binary name is a bit annoying if people try to bisect regressions etc

adiholden · 2023-07-06T08:03:54Z

tests/dragonfly/connection_test.py

+        await c_master.execute_command("DBSIZE")
+        raise "Non tls connection connected on master with tls. This should NOT happen"
+    except redis.exceptions.ConnectionError:
+         pass


please connect via admin port also and check it success

adiholden · 2023-07-06T08:08:59Z

tests/dragonfly/tls_replication_test.py

+@pytest.mark.parametrize("t_master, t_replica", replication_cases)
+@dfly_args({"dbfilename": "test-tls-replication-{{timestamp}}", "admin_nopass" : True})
+async def test_no_tls_on_admin_port(gen_tls_cert, tls_server_key_file_name, tls_server_cert_file_name, df_local_factory, df_seeder_factory, t_master, t_replica):
+    gen_tls_cert


remove gen_tls_cert

adiholden · 2023-07-06T08:09:57Z

tests/dragonfly/tls_replication_test.py

+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("t_master, t_replica", replication_cases)
+@dfly_args({"dbfilename": "test-tls-replication-{{timestamp}}", "admin_nopass" : True})


I believe you can drop the dbfilename param
why is the admin_nopass needed?

to make our life easier so I don't have to setup a password

why password is required if requirepass flag is not set?

because from my understanding you still need to provide an auth token according to #1181

I quote:

However for auth protected setups we still need to provide an auth token for clients that connect via the admin port.

adiholden · 2023-07-06T08:17:20Z

tests/dragonfly/tls_replication_test.py

+ADMIN_PORT = 1211
+
+"""
+Test replication with tls and non-tls options


Actually this is just another replication test.. you can just move it to replicaiton_test.py

adiholden · 2023-07-06T08:30:04Z

tests/dragonfly/conftest.py

+def tls_server_cert_file_name():
+    return "df-cert.pem"
+
+@pytest.fixture(scope="function")


I think its better to use scope="session" so this function will run only once when we are running regression tests

adiholden · 2023-07-06T09:10:53Z

tests/dragonfly/conftest.py

+    # Use CA's private key to sign dragonfly's CSR and get back the signed certificate
+    tls_server_cert = dfly_path + tls_server_cert_file_name
+    step3 = fr'openssl x509 -req -in {tls_server_req} -days 1 -CA {ca_cert} -CAkey {ca_key} -CAcreateserial -out {tls_server_cert}'
+    subprocess.run(step3, shell=True)


gen_tls_cert can return the path of key and certificate file names and instead of defining tls_server_cert_file_name and tls_server_key_file_name set the filenames to constant var

kostasrim force-pushed the fix_replication_with_tls branch 2 times, most recently from 78fb3ca to 70bde27 Compare June 29, 2023 16:46

kostasrim added 2 commits June 30, 2023 18:20

feat(replication): allow non-tls connections between replica and master

7f07249

test: add pytest for tls replication and update CI

af2d393

kostasrim force-pushed the fix_replication_with_tls branch from 70bde27 to af2d393 Compare June 30, 2023 15:20

kostasrim changed the title ~~feat(replication): allow non-tls connections between replica and master (DO NOT REVIEW WIP)~~ feat(replication): allow non-tls connections between replica and master #1419 Jun 30, 2023

kostasrim marked this pull request as ready for review June 30, 2023 15:26

fix: add starts_with lambda, since it's a c++20 function

8cbe4e3

kostasrim commented Jun 30, 2023

View reviewed changes

.github/workflows/regression-tests.yml Outdated Show resolved Hide resolved

kostasrim commented Jun 30, 2023

View reviewed changes

tests/dragonfly/tls_replication_test.py Outdated Show resolved Hide resolved

kostasrim self-assigned this Jun 30, 2023

chakaz reviewed Jul 1, 2023

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

royjacobson reviewed Jul 2, 2023

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

romange reviewed Jul 4, 2023

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

fix: address comments and forward to admin_port for non_tls connections

eb45bb4

kostasrim requested a review from romange July 5, 2023 13:50

kostasrim added 2 commits July 5, 2023 16:51

refactor: restore CheckForHttpProto

d7e3295

fix: restore helio

2ccf1fe

kostasrim requested a review from royjacobson July 5, 2023 13:59

adiholden reviewed Jul 5, 2023

View reviewed changes

.github/workflows/regression-tests.yml Outdated Show resolved Hide resolved

adiholden reviewed Jul 5, 2023

View reviewed changes

src/facade/dragonfly_connection.h Outdated Show resolved Hide resolved

adiholden reviewed Jul 5, 2023

View reviewed changes

tests/dragonfly/tls_replication_test.py Outdated Show resolved Hide resolved

adiholden reviewed Jul 5, 2023

View reviewed changes

fix: address gh comments

7645bdc

adiholden reviewed Jul 5, 2023

View reviewed changes

kostasrim requested a review from adiholden July 5, 2023 14:47

adiholden reviewed Jul 5, 2023

View reviewed changes

kostasrim added 3 commits July 5, 2023 18:10

fix: address gh comments

2e7868d

fix: address gh comments

deb6e69

Merge branch 'main' into fix_replication_with_tls

872e69c

kostasrim enabled auto-merge (squash) July 5, 2023 15:38

kostasrim requested a review from adiholden July 5, 2023 15:39

romange reviewed Jul 5, 2023

View reviewed changes

kostasrim added 2 commits July 5, 2023 23:44

fix: add removed socket init and remove unused header

ae65b0a

refactor: move gen_tls_cert to conftest.py

ce1d404

adiholden reviewed Jul 6, 2023

View reviewed changes

refactor: move connection related test to connection_test.py

4e9ed9b

kostasrim requested a review from adiholden July 6, 2023 07:32

royjacobson reviewed Jul 6, 2023

View reviewed changes

adiholden reviewed Jul 6, 2023

View reviewed changes

fix: refactor and address gh comments

dcf35b6

kostasrim requested review from royjacobson and adiholden July 6, 2023 09:30

refactor: remove unused flags and rename

5b32759

adiholden approved these changes Jul 6, 2023

View reviewed changes

kostasrim merged commit 15481b8 into main Jul 6, 2023

romange deleted the fix_replication_with_tls branch July 6, 2023 13:12

		}
		FiberSocketBase* peer = tls_sock ? (FiberSocketBase*)tls_sock.get() : socket_.get();

	if (!no_tls_on_admin_port \|\| (no_tls_on_admin_port && !IsAdmin())) {
	if (!(IsAdmin() && no_tls_on_admin_port)) {

feat(replication): allow non-tls connections between replica and master #1419 #1490

feat(replication): allow non-tls connections between replica and master #1419 #1490

Uh oh!

Conversation

kostasrim commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

royjacobson commented Jul 2, 2023

Uh oh!

kostasrim commented Jul 2, 2023

Uh oh!

royjacobson commented Jul 2, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adiholden Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kostasrim commented Jun 28, 2023 •

edited

Loading

adiholden Jul 5, 2023 •

edited

Loading

romange Jul 5, 2023 •

edited

Loading