From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 10 Jul 2019 11:13:04 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH] auth: fix empty password authentication
Message-ID: <20190710081304.xs7akojupdinlgsl@esperanza>
References: <d477dafd5d73c82bba3df0444aba58827aecd2bd.1562680546.git.vdavydov.dev@gmail.com>
 <20190709181330.GB12956@atlas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190709181330.GB12956@atlas>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

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.