[PATCH] auth: fix empty password authentication
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Jul 10 11:13:04 MSK 2019
On Tue, Jul 09, 2019 at 09:13:30PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at 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.
More information about the Tarantool-patches
mailing list