[Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Vladislav Shpilevoy]

Replication's applier encoded an auth request with exactly the
same parameters as extracted by the URI parser. I.e. when no
password was specified, the parser returned it as NULL, and it was
not encoded. The relay, received such an auth request, complained
that IPROTO_TUPLE field is not specified (this is password).

Such an error confuses - a user didn't do anything illegal, he
just used URI like 'login@host:port', without a password after the
login.

The patch makes the applier use an empty string as a default
password.

An alternative was to force a user always set a password even if
it is an empty string, like that: 'login:@host:port'. And if a
password was not found in an auth request, then reject it with a
password mismatch error. But in that case a URI of kind
'login@host:port' becomes useless - it can never pass. In
addition, netbox already uses an empty string as a default
password. So the only way to make it consistent, and don't break
anything - repeat netbox logic for replication URIs.

Closes #4605
---
Issue: https://github.com/tarantool/tarantool/issues/4605
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4605-empty-password-replication

 src/box/applier.cc                            |  4 +-
 .../replication/gh-4605-empty-password.result | 52 +++++++++++++++++++
 .../gh-4605-empty-password.test.lua           | 17 ++++++
 test/replication/suite.cfg                    |  1 +
 4 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 test/replication/gh-4605-empty-password.result
 create mode 100644 test/replication/gh-4605-empty-password.test.lua

diff --git a/src/box/applier.cc b/src/box/applier.cc
index a04d13564..9467718d7 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -373,7 +373,9 @@ applier_connect(struct applier *applier)
 	/* Authenticate */
 	applier_set_state(applier, APPLIER_AUTH);
 	xrow_encode_auth_xc(&row, greeting.salt, greeting.salt_len, uri->login,
-			    uri->login_len, uri->password, uri->password_len);
+			    uri->login_len,
+			    uri->password != NULL ? uri->password : "",
+			    uri->password_len);
 	coio_write_xrow(coio, &row);
 	coio_read_xrow(coio, ibuf, &row);
 	applier->last_row_time = ev_monotonic_now(loop());
diff --git a/test/replication/gh-4605-empty-password.result b/test/replication/gh-4605-empty-password.result
new file mode 100644
index 000000000..ec33c4914
--- /dev/null
+++ b/test/replication/gh-4605-empty-password.result
@@ -0,0 +1,52 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+box.schema.user.create('test_user', {password = ''})
+ | ---
+ | ...
+box.schema.user.grant('test_user', 'replication')
+ | ---
+ | ...
+
+test_run:cmd("create server replica_auth with rpl_master=default, script='replication/replica_auth.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server replica_auth with wait=True, wait_load=True, args='test_user 0.1'")
+ | ---
+ | - true
+ | ...
+
+test_run:switch('replica_auth')
+ | ---
+ | - true
+ | ...
+i = box.info
+ | ---
+ | ...
+i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i
+ | ---
+ | - true
+ | ...
+
+test_run:switch('default')
+ | ---
+ | - true
+ | ...
+test_run:cmd("stop server replica_auth")
+ | ---
+ | - true
+ | ...
+test_run:cmd("cleanup server replica_auth")
+ | ---
+ | - true
+ | ...
+test_run:cmd("delete server replica_auth")
+ | ---
+ | - true
+ | ...
+
+box.schema.user.drop('test_user')
+ | ---
+ | ...
diff --git a/test/replication/gh-4605-empty-password.test.lua b/test/replication/gh-4605-empty-password.test.lua
new file mode 100644
index 000000000..0e178e15a
--- /dev/null
+++ b/test/replication/gh-4605-empty-password.test.lua
@@ -0,0 +1,17 @@
+test_run = require('test_run').new()
+box.schema.user.create('test_user', {password = ''})
+box.schema.user.grant('test_user', 'replication')
+
+test_run:cmd("create server replica_auth with rpl_master=default, script='replication/replica_auth.lua'")
+test_run:cmd("start server replica_auth with wait=True, wait_load=True, args='test_user 0.1'")
+
+test_run:switch('replica_auth')
+i = box.info
+i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i
+
+test_run:switch('default')
+test_run:cmd("stop server replica_auth")
+test_run:cmd("cleanup server replica_auth")
+test_run:cmd("delete server replica_auth")
+
+box.schema.user.drop('test_user')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index eb25077d8..dcf52f247 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -11,6 +11,7 @@
     "on_schema_init.test.lua": {},
     "long_row_timeout.test.lua": {},
     "join_without_snap.test.lua": {},
+    "gh-4605-empty-password.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
         "vinyl": {"engine": "vinyl"}
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/04 18:21]:
> Replication's applier encoded an auth request with exactly the
> same parameters as extracted by the URI parser. I.e. when no
> password was specified, the parser returned it as NULL, and it was
> not encoded. The relay, received such an auth request, complained
> that IPROTO_TUPLE field is not specified (this is password).
> 
> Such an error confuses - a user didn't do anything illegal, he
> just used URI like 'login@host:port', without a password after the
> login.
> 
> The patch makes the applier use an empty string as a default
> password.
> 
> An alternative was to force a user always set a password even if
> it is an empty string, like that: 'login:@host:port'. And if a
> password was not found in an auth request, then reject it with a
> password mismatch error. But in that case a URI of kind
> 'login@host:port' becomes useless - it can never pass. In
> addition, netbox already uses an empty string as a default
> password. So the only way to make it consistent, and don't break
> anything - repeat netbox logic for replication URIs.

LGTM.

Obviously this is a crutch, but let's see if it is a useful one.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Vladislav Shpilevoy]

On 04/11/2019 18:40, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/04 18:21]:
>> Replication's applier encoded an auth request with exactly the
>> same parameters as extracted by the URI parser. I.e. when no
>> password was specified, the parser returned it as NULL, and it was
>> not encoded. The relay, received such an auth request, complained
>> that IPROTO_TUPLE field is not specified (this is password).
>>
>> Such an error confuses - a user didn't do anything illegal, he
>> just used URI like 'login@host:port', without a password after the
>> login.
>>
>> The patch makes the applier use an empty string as a default
>> password.
>>
>> An alternative was to force a user always set a password even if
>> it is an empty string, like that: 'login:@host:port'. And if a
>> password was not found in an auth request, then reject it with a
>> password mismatch error. But in that case a URI of kind
>> 'login@host:port' becomes useless - it can never pass. In
>> addition, netbox already uses an empty string as a default
>> password. So the only way to make it consistent, and don't break
>> anything - repeat netbox logic for replication URIs.
> 
> LGTM.
> 
> Obviously this is a crutch, but let's see if it is a useful one.
> 
> 

I agree, I am on your side that we should not set an implicit
empty string password by default. But what is more important,
our API should be consistent. Netbox already sets default empty
string password. And we can't break it. So the only solution -
do the same for replication.

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Vladislav Shpilevoy]

Sorry, found a bug in the test.

> diff --git a/test/replication/gh-4605-empty-password.result b/test/replication/gh-4605-empty-password.result
> new file mode 100644
> index 000000000..ec33c4914
> --- /dev/null
> +++ b/test/replication/gh-4605-empty-password.result
> @@ -0,0 +1,52 @@
> +test_run:switch('replica_auth')
> + | ---
> + | - true
> + | ...
> +i = box.info
> + | ---
> + | ...
> +i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i

Other instance's ID is 'i.id % 2 + 1'.

I force pushed the fixed test.

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Vladislav Shpilevoy]

Also I forgot a description for the test.
Here it is: (force pushed to the branch)

========================================================================

diff --git a/test/replication/gh-4605-empty-password.result b/test/replication/gh-4605-empty-password.result
index 4cb2c37c1..defdfcfcd 100644
--- a/test/replication/gh-4605-empty-password.result
+++ b/test/replication/gh-4605-empty-password.result
@@ -2,6 +2,16 @@
 test_run = require('test_run').new()
  | ---
  | ...
+
+--
+-- gh-4605: replication and netbox both use URI as a remote
+-- resource identifier. If URI does not contain a password, netbox
+-- assumes it is an empty string - ''. But replication's applier
+-- wasn't assuming the same, and just didn't send a password at
+-- all, when it was not specified in the URI. It led to a strange
+-- error message and inconsistent behaviour. The test checks, that
+-- replication now also uses an empty string password by default.
+
 box.schema.user.create('test_user', {password = ''})
  | ---
  | ...
diff --git a/test/replication/gh-4605-empty-password.test.lua b/test/replication/gh-4605-empty-password.test.lua
index 5472b66cd..f42a55f81 100644
--- a/test/replication/gh-4605-empty-password.test.lua
+++ b/test/replication/gh-4605-empty-password.test.lua
@@ -1,4 +1,14 @@
 test_run = require('test_run').new()
+
+--
+-- gh-4605: replication and netbox both use URI as a remote
+-- resource identifier. If URI does not contain a password, netbox
+-- assumes it is an empty string - ''. But replication's applier
+-- wasn't assuming the same, and just didn't send a password at
+-- all, when it was not specified in the URI. It led to a strange
+-- error message and inconsistent behaviour. The test checks, that
+-- replication now also uses an empty string password by default.
+
 box.schema.user.create('test_user', {password = ''})
 box.schema.user.grant('test_user', 'replication')
 
========================================================================

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:47]:
> Also I forgot a description for the test.
> Here it is: (force pushed to the branch)

LGTM (I did not pay close attention to this, mostly focusing on
making sure there are no contradictions with design/other
features, as usual).


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 1/1] replication: use empty password by default

[Kirill Yukhin]

Hello,

On 04 ноя 18:10, Vladislav Shpilevoy wrote:
> Replication's applier encoded an auth request with exactly the
> same parameters as extracted by the URI parser. I.e. when no
> password was specified, the parser returned it as NULL, and it was
> not encoded. The relay, received such an auth request, complained
> that IPROTO_TUPLE field is not specified (this is password).
> 
> Such an error confuses - a user didn't do anything illegal, he
> just used URI like 'login@host:port', without a password after the
> login.
> 
> The patch makes the applier use an empty string as a default
> password.
> 
> An alternative was to force a user always set a password even if
> it is an empty string, like that: 'login:@host:port'. And if a
> password was not found in an auth request, then reject it with a
> password mismatch error. But in that case a URI of kind
> 'login@host:port' becomes useless - it can never pass. In
> addition, netbox already uses an empty string as a default
> password. So the only way to make it consistent, and don't break
> anything - repeat netbox logic for replication URIs.
> 
> Closes #4605
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4605
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4605-empty-password-replication

I've checked your patch into 1.10, 2.2 and master.

--
Regards, Kirill Yukhin