[tarantool-patches] Re: [PATCH] auth: fix empty password authentication

  • From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • To: Konstantin Osipov <kostja@xxxxxxxxxxxxx>
  • Date: Wed, 10 Jul 2019 11:13:04 +0300

On Tue, Jul 09, 2019 at 09:13:30PM +0300, Konstantin Osipov wrote:

* Vladimir Davydov <vdavydov.dev@xxxxxxxxx> [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.

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2019