maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi,

Please review commits for

  MDEV-12321 "authentication plugin: SET PASSWORD support"

They're now in the bb-10.4-serg branch, the last seven commits:

=========================
commit 62e8340f513
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Wed Oct 17 12:48:13 2018 +0200

    MDEV-12321 authentication plugin: SET PASSWORD support
    
    Support SET PASSWORD for authentication plugins.
    
    Authentication plugin API is extended with two optional methods:
    * hash_password() is used to compute a password hash (or digest)
      from the plain-text password. This digest will be stored in mysql.user
      table
    * preprocess_hash() is used to convert this digest into some memory
      representation that can be later used to authenticate a user.
      Build-in plugins convert the hash from hexadecimal or base64 to binary,
      to avoid doing it on every authentication attempt.
    
    Note a change in behavior: when loading privileges (on startup or on
    FLUSH PRIVILEGES) an account with an unknown plugin was loaded with a
    warning (e.g. "Plugin 'foo' is not loaded"). But such an account could
    not be used for authentication until the plugin is installed. Now an
    account like that will not be loaded at all (with a warning, still).
    Indeed, without plugin's preprocess_hash() method the server cannot know
    how to load an account. Thus, if a new authentication plugin is
    installed run-time, one might need FLUSH PRIVILEGES to activate all
    existing accounts that were using this new plugin.

commit 8266ca26f27
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Mon Oct 15 01:39:03 2018 +0200

    misc cleanups
    
    * remove dead code (from .yy)
    * remove redundant commands from the test
    * extract common code into a reusable function
      (get_auth_plugin, push_new_user)
    * rename update_user_table->update_user_table_password
    * simplify acl_update_role
    * don't strdup a string that's already in a memroot
      (in ACL_ROLE::ACL_ROLE(ACL_USER*))
    * create parent_grantee and role_grants dynamic arrays with size 0.
      to avoid any memory allocations when roles aren't used.

commit fd0bcb5e791
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Sun Oct 14 13:52:52 2018 +0200

    Use mysql.user.authentication_string for password
    
    Don't distinguish between a "password hash" and "authentication string"
    anymore. Now both are stored in mysql.user.authentication_string, both
    are handled identically internally. A "password hash" is just how some
    particular plugins interpret authentication string.
    
    Set mysql.user.plugin even if there is no password. The server will use
    mysql_native_password plugin in these cases, let's make it expicit.
    
    Remove LEX_USER::pwhash.

commit 15f30100e0d
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Sat Oct 13 18:32:05 2018 +0200

    cleanup: sql_acl.cc remove fix_plugin_ptr()
    
    it was doing two my_strcasecmp() unconditionally, to optimize away one
    conditional my_strcasecmp() later.

commit df9d95d2a85
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Sat Oct 13 11:30:39 2018 +0200

    cleanup: sql_acl.cc remove username=NULL
    
    Some parts of sql_acl.cc historically assumed that empty username
    is represented by username=NULL, other parts used username="" for that.
    And most of the code wasn't sure and checked both
    (like in `if (!user || !user[0])`).
    
    Change it to use an empty string everywhere.

commit 8b3cbc1e469
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Fri Oct 12 18:48:15 2018 +0200

    cleanup: sql_acl.cc password->LEX_CSTRING

commit 2fc7a20da6a
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Fri Oct 12 19:24:28 2018 +0200

    cleanup: safe_lexcstrdup_root()
=========================

I've split somewhat independent changes into separate commits, but, of
course, if you'd like to review one big patch you can `git diff` them
all together.

Now, the syntax problem.

Old MySQL syntax (for the last ~20 years) was

(1)  GRANT ... TO user IDENTIFIED BY 'plain-text password'
(2)  GRANT ... TO user IDENTIFIED BY PASSWORD 'password hash'
(3)  SET PASSWORD = 'password hash'
(4)  SET PASSWORD = PASSWORD('plain-text password')
(5)  SET PASSWORD = OLD_PASSWORD('plain-text password')

here, syntax (1) and (4) were forcing mysql_native_password
authentication, (5) was forcing mysql_old_password, and (2) and (3) were
auto-detecting, based on the hash length.

Later MariaDB and MySQL added support for pluggable authentication with
the syntax

(6)  GRANT ... TO user IDENTIFIED VIA plugin AS 'password hash'

MySQL 5.7 added support for specifying plain-text password for plugins
using the syntax

(7) GRANT ... TO user IDENTIFIED WITH plugin BY 'plain-text password'

I don't quite like it because there's no logical reason why "BY" means a
plain-text password, while "AS" means a hash. Also we support "USING"
instead of "AS", which also means a hash. One can easily get lost in all
these USING/AS/BY and what special semantics each of them has.

The syntax I've implemented is based on SET PASSWORD:

(8) GRANT ... TO user IDENTIFIED VIA plugin AS PASSWORD('plain-text password')

This is quite intuitive and pretends that there's a sql function
PASSWORD() which returns a hash and it's used as an expression where a
hash is anyway expected. Same works in SET PASSWORD too, obviously.
A PASSWORD() function actually exists and returns the password hash.

The problem here is that PASSWORD() function becomes quite magical. It
returns a different password, depending on what plugin the user is
using. One can still do SELECT PASSWORD("foo"), OLD_PASSWORD("foo"),
but they'll return values for mysql_native_password and
mysql_old_password as before. In the context of SET PASSWORD or GRANT
(or CREATE/ALTER USER) it becomes context dependent, it's a bit
difficult to swallow.

Another approach would be not to pretend it's a function. Say

(9) GRANT ... TO user IDENTIFIED VIA plugin AS PASSWORD 'plain-text password'
    SET PASSWORD = PASSWORD 'plain-text password'

but, unfortunately, it is exactly backwards from the historical behaviour
of (1) and (2).

All in all I'm leaning towards (8), but I'm not quite happy with it :(
One way to solve it could be to extend PASSWORD() function to allow a
second argument, plugin name, like in

  SELECT PASSWORD("foo", "ed25519")

Yet another way could be to remove SQL-level functions PASSWORD() and
OLD_PASSWORD(). That would be my favorite, they always were nothing but
trouble. But I wouldn't risk doing it now :)

Regards,
Sergei