[PATCH] auth: fix empty password authentication

[PATCH] auth: fix empty password authentication

[Vladimir Davydov]

If a user has an empty password, we should allow to authenticate it
without specifying a password in IPROTO_AUTH request. In particular,
this is required to authenticate as guest from connectors.

However, we only allow to authenticate without a password if the user
has empty hash2 and is guest, which is never the case, because since
commit 076a842011e0 guest has non-empty hash2 calculated from an empty
password.

The above-mentioned commit circumvents the issue in net.box by passing
an empty password to the server if no password is specified. However,
connectors don't usually do that - they simply omit the password field
altogether, which results in authentication failure.

So instead of replacing no-password with empty-password at the client
side (i.e. in net.box) let's allow authenticating without a password
if the user has an empty password.

Fixes commit 076a842011e0 ("Permit empty passwords in net.box").

While we are at it, make sure on_auth triggers are invoked when password
check is skipped - currently they are not, which would otherwise result
in box-tap/auth test hang.

Fixes commit ecb0e698dfb8 ("gh-844: Authentication trigger feature in
iproto").

Note, IPROTO_AUTH must have IPROTO_TUPLE set so netbox_encode_auth is
incorrect - it skips the field if the password isn't specified. Fix it
to encode an empty array instead.

Closes #4327
---
https://github.com/tarantool/tarantool/issues/4327
https://github.com/tarantool/tarantool/commits/dv/gh-4327-fix-empty-password-auth

 src/box/authentication.cc | 28 ++++++++++++++++++++--------
 src/box/lua/net_box.c     |  6 ++++--
 src/box/lua/net_box.lua   |  3 +--
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/box/authentication.cc b/src/box/authentication.cc
index 811974cb..ebc67e96 100644
--- a/src/box/authentication.cc
+++ b/src/box/authentication.cc
@@ -33,8 +33,13 @@
 #include "session.h"
 #include "msgpuck.h"
 #include "error.h"
+#include "third_party/base64.h"
 
-static char zero_hash[SCRAMBLE_SIZE];
+/**
+ * chap-sha1 of empty string, i.e.
+ * base64_encode(sha1(sha1(""), 0)
+ */
+#define CHAP_SHA1_EMPTY_PASSWORD "vhvewKp0tNyweZQ+cFKAlsyphfg="
 
 void
 authenticate(const char *user_name, uint32_t len, const char *salt,
@@ -47,15 +52,21 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 	const char *scramble;
 	struct on_auth_trigger_ctx auth_res = { user->def->name, true };
 	/*
-	 * Allow authenticating back to GUEST user without
-	 * checking a password. This is useful for connection
+	 * Allow authenticating without a password if the user
+	 * has an empty password. This is useful for connection
 	 * pooling.
 	 */
 	part_count = mp_decode_array(&tuple);
-	if (part_count == 0 && user->def->uid == GUEST &&
-	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
-		/* No password is set for GUEST, OK. */
-		goto ok;
+	if (part_count == 0) {
+		char hash2[SCRAMBLE_SIZE];
+		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
+			      hash2, SCRAMBLE_SIZE);
+		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
+			/* Empty password is set, OK. */
+			goto ok;
+		}
+		/* Otherwise treat as password mismatch. */
+		goto err;
 	}
 
 	access_check_session_xc(user);
@@ -85,16 +96,17 @@ authenticate(const char *user_name, uint32_t len, const char *salt,
 	}
 
 	if (scramble_check(scramble, salt, user->def->hash2)) {
+err:
 		auth_res.is_authenticated = false;
 		if (session_run_on_auth_triggers(&auth_res) != 0)
 			diag_raise();
 		tnt_raise(ClientError, ER_PASSWORD_MISMATCH, user->def->name);
 	}
+ok:
 	/* check and run auth triggers on success */
 	if (! rlist_empty(&session_on_auth) &&
 	    session_run_on_auth_triggers(&auth_res) != 0)
 		diag_raise();
-ok:
 	credentials_init(&session->credentials, user->auth_token,
 			 user->def->uid);
 }
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 001af95d..f1c71c2f 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -141,16 +141,18 @@ netbox_encode_auth(lua_State *L)
 		return luaL_error(L, "Invalid salt");
 
 	/* Adapted from xrow_encode_auth() */
-	mpstream_encode_map(&stream, password != NULL ? 2 : 1);
+	mpstream_encode_map(&stream, 2);
 	mpstream_encode_uint(&stream, IPROTO_USER_NAME);
 	mpstream_encode_strn(&stream, user, user_len);
+	mpstream_encode_uint(&stream, IPROTO_TUPLE);
 	if (password != NULL) { /* password can be omitted */
 		char scramble[SCRAMBLE_SIZE];
 		scramble_prepare(scramble, salt, password, password_len);
-		mpstream_encode_uint(&stream, IPROTO_TUPLE);
 		mpstream_encode_array(&stream, 2);
 		mpstream_encode_str(&stream, "chap-sha1");
 		mpstream_encode_strn(&stream, scramble, SCRAMBLE_SIZE);
+	} else {
+		mpstream_encode_array(&stream, 0);
 	}
 
 	netbox_encode_request(&stream, svp);
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 2892bb3d..0d486fa6 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -222,7 +222,6 @@ local function create_transport(host, port, user, password, callback,
     if user == nil and password ~= nil then
         box.error(E_PROC_LUA, 'net.box: user is not defined')
     end
-    if user ~= nil and password == nil then password = '' end
 
     -- Current state machine's state.
     local state            = 'initial'
@@ -712,7 +711,7 @@ local function create_transport(host, port, user, password, callback,
 
     iproto_auth_sm = function(salt)
         set_state('auth')
-        if not user or not password then
+        if not user then
             set_state('fetch_schema')
             return iproto_schema_sm()
         end
-- 
2.11.0

Re: [PATCH] auth: fix empty password authentication

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/09 17:01]:

> -static char zero_hash[SCRAMBLE_SIZE];
> +/**
> + * chap-sha1 of empty string, i.e.
> + * base64_encode(sha1(sha1(""), 0)
> + */
> +#define CHAP_SHA1_EMPTY_PASSWORD "vhvewKp0tNyweZQ+cFKAlsyphfg="

Please use a string constant not a define.

>  	part_count = mp_decode_array(&tuple);
> -	if (part_count == 0 && user->def->uid == GUEST &&
> -	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
> -		/* No password is set for GUEST, OK. */
> -		goto ok;

Please allow both empty password and no password and treat them
the same on server side.

> +	if (part_count == 0) {
> +		char hash2[SCRAMBLE_SIZE];
> +		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
> +			      hash2, SCRAMBLE_SIZE);
> +		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
> +			/* Empty password is set, OK. */
> +			goto ok;
> +		}
> +		/* Otherwise treat as password mismatch. */
> +		goto err;

I don't get this change and it's not in the changeset comment. We
should not allow anyone except guest to have an empty password. If
the password is empty, not set, like in unix, it should be
impossible toauthenticate to this user, only sudo to it.

>  	netbox_encode_request(&stream, svp);
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 2892bb3d..0d486fa6 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -222,7 +222,6 @@ local function create_transport(host, port, user, password, callback,
>      if user == nil and password ~= nil then
>          box.error(E_PROC_LUA, 'net.box: user is not defined')
>      end
> -    if user ~= nil and password == nil then password = '' end

this looks very wrong, net.box should work with all versions of
the server.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH] auth: fix empty password authentication

[Vladimir Davydov]

On Tue, Jul 09, 2019 at 09:13:30PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/09 17:01]:
> 
> > -static char zero_hash[SCRAMBLE_SIZE];
> > +/**
> > + * chap-sha1 of empty string, i.e.
> > + * base64_encode(sha1(sha1(""), 0)
> > + */
> > +#define CHAP_SHA1_EMPTY_PASSWORD "vhvewKp0tNyweZQ+cFKAlsyphfg="
> 
> Please use a string constant not a define.

Okay.

> 
> >  	part_count = mp_decode_array(&tuple);
> > -	if (part_count == 0 && user->def->uid == GUEST &&
> > -	    memcmp(user->def->hash2, zero_hash, SCRAMBLE_SIZE) == 0) {
> > -		/* No password is set for GUEST, OK. */
> > -		goto ok;
> 
> Please allow both empty password and no password and treat them
> the same on server side.

Guest user can't have no password. Not after commit ecb0e698dfb8
("gh-844: Authentication trigger feature in iproto"). So I don't see
any point in handling the "password is not set" case.

> 
> > +	if (part_count == 0) {
> > +		char hash2[SCRAMBLE_SIZE];
> > +		base64_decode(CHAP_SHA1_EMPTY_PASSWORD, SCRAMBLE_BASE64_SIZE,
> > +			      hash2, SCRAMBLE_SIZE);
> > +		if (memcmp(user->def->hash2, hash2, SCRAMBLE_SIZE) == 0) {
> > +			/* Empty password is set, OK. */
> > +			goto ok;
> > +		}
> > +		/* Otherwise treat as password mismatch. */
> > +		goto err;
> 
> I don't get this change and it's not in the changeset comment. We
> should not allow anyone except guest to have an empty password. If
> the password is empty, not set, like in unix, it should be
> impossible toauthenticate to this user, only sudo to it.

Please see commit ecb0e698dfb8 ("gh-844: Authentication trigger feature
in iproto"). AFAIU we do want to authenticate a user without a password
if it has an empty password. If the password is unset, authentication
won't work, which is consistent with Unix guidelines.

> 
> >  	netbox_encode_request(&stream, svp);
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 2892bb3d..0d486fa6 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -222,7 +222,6 @@ local function create_transport(host, port, user, password, callback,
> >      if user == nil and password ~= nil then
> >          box.error(E_PROC_LUA, 'net.box: user is not defined')
> >      end
> > -    if user ~= nil and password == nil then password = '' end
> 
> this looks very wrong, net.box should work with all versions of
> the server.

Do we really want to care about preserving this backward compatibility
case? We are planning to backport this patch to all supported releases.
In other words, I really don't want to maintain the legacy of the
crooked commit I mentioned above. Changing the way net.box works allows
us to get a test case for free.