[Maria-developers] MDEV-12321 authentication plugin: SET PASSWORD support
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@mariadb.org> 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@mariadb.org> 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@mariadb.org> 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@mariadb.org> 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@mariadb.org> 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@mariadb.org> Date: Fri Oct 12 18:48:15 2018 +0200 cleanup: sql_acl.cc password->LEX_CSTRING commit 2fc7a20da6a Author: Sergei Golubchik <serg@mariadb.org> 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
Hi Sergei! Here are my review comments, inline. Read them from oldest to newest commit, that's how I wrote them. :) On 17/10/2018 23:40, Sergei Golubchik wrote:
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@mariadb.org> 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. First a note about the behavior change:
This potentially causes a difficulty in debugging failed user authentication. Before, if say unix socket auth plugin was not installed when trying to connect, you'd get ER_PLUGIN_IS_NOT_LOADED, now you get ER_ACCESS_DENIED_ERROR. This provided a hint as to what went wrong. Is there a way for an admin to track down the issue now, without resorting to blind "FLUSH PRIVILEGES;"? (I've never been in that position so I don't know if this is really an issue or not) Also, warnings now on flush privileges are hard to associate with a particular user, if one has a lot of them. Maybe we add a "for user xxx" to the warnings? Can one update privileges on a user which was not loaded? I saw that dropping works, but granting something leads to a weird side-effect of updating the plugin & authentication_string fields for the user: #### Before grant. (from grant5.test) select user, host, plugin, authentication_string from mysql.user where user = 'u1'; user host plugin authentication_string u1 h mysql_native_password bad #### After grant grant SELECT on *.* to u1@h; select user, host, plugin, authentication_string from mysql.user where user = 'u1'; user host plugin authentication_string u1 h mysql_native_password If before grant, the user would have had something other than mysql_native_password, that one would have been changed too. It's a weird corner case, but do you think this is the correct way to handle it? I'd rather not mess with changing the plugin and authentication_string in this case. (I did see the code about the historical-hack and guess_auth_plugin) I also did check this in 10.2 (just because it was already compiled, but nothing significant changed in 10.3), that grant in this case would not drop the user's password. Here's an example: CREATE USER foo@localhost identified by 'pwd'; select user, host, password, plugin, authentication_string from mysql.user where user='foo'; #This grant won't drop the password. grant select on *.* to foo@localhost; select user, host, password, plugin, authentication_string from mysql.user where user='foo'; update mysql.user set authentication_string = 'bad' where user='foo'; flush privileges; select user, host, password, plugin, authentication_string from mysql.user where user='foo'; # This grant will drop the password. grant select on *.* to foo@localhost; select user, host, password, plugin, authentication_string from mysql.user where user='foo'; connect con1, localhost, foo,,; # foo can select everything, with no password select user, host from mysql.user; I suggest we don't overwrite authentication string in this case within the grant statement in this case. Why the change in mysql-test/suite/rpl/include/rpl_mixed_dml.inc? It feels a bit related to the paragraph above. Now about the code itself: hash_password function call could lead to buffer overflow, then all sorts of potential security hacks if the plugin doesn't play nice and writes more than MAX_SCRAMBLE_LENGTH to the buffer. Not sure if there's a way to guard against this or if we want to in the first place. It is the admin's job to load sane password plugins right? I know this would imply a bit more work, but shouldn't we test the extra API methods with a mock-up plugin too? Would preprocess_hash call failure actually exit in sane way in the caller function? Currently we don't report any specific error or warning message. (same for hash_password) That's all I could find. There are a few bits of code style changes, but I'm going to merge a big one with reverse privileges. Will end up fixing them there anyway. Vicențiu
commit 8266ca26f27 Author: Sergei Golubchik <serg@mariadb.org> 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@mariadb.org> 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. Looks good. Lots of test updates. First patch that seemed to have
Looks good, except that you don't seem to be simplifying acl_update_role, but acl_update user. update_user_table_password needs more indentation now for the 2 extra lines of arguments on the function's definition. potential to introduce bugs. I looked to see if anything was missing, but couldn't find anything.
commit 15f30100e0d Author: Sergei Golubchik <serg@mariadb.org> 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. Looks good. Interesting find. I knew the reason for fix_plugin_ptr, but I assumed it's used more often than it actually is.
commit df9d95d2a85 Author: Sergei Golubchik <serg@mariadb.org> 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.
Looks good. In the future we should do this for other bits too (such as host, plugin, etc.) I'd rather look at getting rid of get_field calls and converting the resulting string to int or float (such as how we do for user_resource part of ACL_USER). We're doing a bit of that with reverse privileges, so no need to do it in this change set.
commit 8b3cbc1e469 Author: Sergei Golubchik <serg@mariadb.org> Date: Fri Oct 12 18:48:15 2018 +0200
cleanup: sql_acl.cc password->LEX_CSTRING Looks good. commit 2fc7a20da6a Author: Sergei Golubchik <serg@mariadb.org> Date: Fri Oct 12 19:24:28 2018 +0200
cleanup: safe_lexcstrdup_root()
Looks good.
=========================
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
Hi, Vicențiu! On Oct 21, Vicențiu Ciorbaru wrote:
Hi Sergei!
Here are my review comments, inline. Read them from oldest to newest commit, that's how I wrote them. :)
commit df9d95d2a85 Author: Sergei Golubchik <serg@mariadb.org> 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.
In the future we should do this for other bits too (such as host, plugin, etc.) I'd rather look at getting rid of get_field calls and converting the resulting string to int or float (such as how we do for user_resource part of ACL_USER). We're doing a bit of that with reverse privileges, so no need to do it in this change set.
I tried to, but it was too time-consuming so I gave up. All other changes were related to the MDEV I was doing, but this host cleanup was not, so I didn't want to spend too much time on it
commit fd0bcb5e791 Author: Sergei Golubchik <serg@mariadb.org> 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. Looks good. Lots of test updates. First patch that seemed to have potential to introduce bugs. I looked to see if anything was missing, but couldn't find anything.
Right, I use "cleanup:" in the commit subject when it's an internal change that does not change any user visible behavior. This one doesn't have the magic word... :)
commit 62e8340f513 Author: Sergei Golubchik <serg@mariadb.org> 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. First a note about the behavior change:
This potentially causes a difficulty in debugging failed user authentication. Before, if say unix socket auth plugin was not installed when trying to connect, you'd get ER_PLUGIN_IS_NOT_LOADED, now you get ER_ACCESS_DENIED_ERROR. This provided a hint as to what went wrong. Is there a way for an admin to track down the issue now, without resorting to blind "FLUSH PRIVILEGES;"? (I've never been in that position so I don't know if this is really an issue or not)
I saw two possible solutions: 1. load users w/o a plugin anyway, mark ACL_USER in memory as "incomplete". Now if such a user is used in authentication (somebody tries to log in as it) and a plugin is found this time - complete initialization and remove the "incomplete" flag. In fact, such lazy initialization can be done for all ACL_USER, plugins or not. 2. on any INSTALL authentication_plugin, perform an automatic FLUSH PRIVILEGES. First one is the most faithful emulation of the old behavior, second is simpler but might have strange side effects. And, really, I don't think it's a sufficiently common or important use case to justify a workaround. Normally plugins aren't loaded/unloaded all the time, they're installed once and then used. And if a new plugin is loaded for the first time, there cannot be any accounts that use it. So, this must be a pretty uncommon case. One can just do FLUSH PRIVILEGES, if needed. In rpm/deb packages, in particular, we tend to use plugin-load-add in my.cnf files, not run-time INSTALL PLUGIN statement.
Also, warnings now on flush privileges are hard to associate with a particular user, if one has a lot of them. Maybe we add a "for user xxx" to the warnings?
Hmm, I don't know. I've noticed this issue, but I'm not sure of the security implications. For FLUSH PRIVILEGES one needs RELOAD privilege, for seeing all users one needs SELECT on mysql.user. I could use a different warning, depending on the privileges, but it'd be very weird...
Can one update privileges on a user which was not loaded? I saw that dropping works, but granting something leads to a weird side-effect of updating the plugin & authentication_string fields for the user:
#### Before grant. (from grant5.test)
select user, host, plugin, authentication_string from mysql.user where user = 'u1'; user host plugin authentication_string u1 h mysql_native_password bad
#### After grant
grant SELECT on *.* to u1@h; select user, host, plugin, authentication_string from mysql.user where user = 'u1'; user host plugin authentication_string u1 h mysql_native_password
If before grant, the user would have had something other than mysql_native_password, that one would have been changed too. It's a weird corner case, but do you think this is the correct way to handle it? I'd rather not mess with changing the plugin and authentication_string in this case. (I did see the code about the historical-hack and guess_auth_plugin)
I also did check this in 10.2 (just because it was already compiled, but nothing significant changed in 10.3), that grant in this case would not drop the user's password.
Here's an example: CREATE USER foo@localhost identified by 'pwd';
select user, host, password, plugin, authentication_string from mysql.user where user='foo'; #This grant won't drop the password. grant select on *.* to foo@localhost; select user, host, password, plugin, authentication_string from mysql.user where user='foo';
update mysql.user set authentication_string = 'bad' where user='foo'; flush privileges;
select user, host, password, plugin, authentication_string from mysql.user where user='foo'; # This grant will drop the password. grant select on *.* to foo@localhost; select user, host, password, plugin, authentication_string from mysql.user where user='foo';
connect con1, localhost, foo,,; # foo can select everything, with no password select user, host from mysql.user;
I suggest we don't overwrite authentication string in this case within the grant statement in this case.
Sure. In the spirit of what was said above, I've fixed the bug by throwing an error, similar to what we do with corrupted tables - DROP works, but nothing else does.
Why the change in mysql-test/suite/rpl/include/rpl_mixed_dml.inc? It feels a bit related to the paragraph above.
because mysql_install_db still uses INSERT, the plugin column is initially empty, but any GRANT changes it to "mysql_native_password". This fails post-test checks. Normally it's not an issue, because tests create new users, not grant anything to root@localhost. But a couple of tests do, and I added `UPDATE mysql.user SET plugin=''` to one of them and removed a `GRANT EVENT TO root@localhost` from the other (it was completely unnecessary, as root has all privileges anyway).
Now about the code itself:
hash_password function call could lead to buffer overflow, then all sorts of potential security hacks if the plugin doesn't play nice and writes more than MAX_SCRAMBLE_LENGTH to the buffer. Not sure if there's a way to guard against this or if we want to in the first place. It is the admin's job to load sane password plugins right?
1. Yes. 2. plugin is executed in the server address space, if a plugin wants to do something bad, we cannot stop it 3. not MAX_SCRAMBLE_LENGTH, hash_password() gets the buffer length in the last argument, it doesn't need to rely on any defines that aren't part of the API.
I know this would imply a bit more work, but shouldn't we test the extra API methods with a mock-up plugin too?
dunno, they're tested with mysql_native_password, mysql_old_password, and ed25519.
Would preprocess_hash call failure actually exit in sane way in the caller function? Currently we don't report any specific error or warning message. (same for hash_password)
Yes, preprocess_hash errors are tested (because it fails if the hash is of incorrect length and this happens in tests). I don't see how hash_password can fail. The error is still returned, if it would, but it'd be up to a plugin to produce a meaningful error message. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei! About the syntax problem, inline: On 17/10/2018 23:40, Sergei Golubchik wrote:
Hi,
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.
I can also never remember what is the difference between BY/AS/USING.
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 :)
I think (8) is ok and intuitively that's what I tried to do when writing test cases for your patch. I personally would add the extra parameters to PASSWORD. I don't think we should remove PASSWORD(<plain-text>) function, but we should clearly mark it as not-recommended. One might need to figure out what hash a plugin generates for a certain plain-text string and I don't see a way one would do that currently without this extension. Vicențiu
participants (2)
-
Sergei Golubchik -
Vicențiu Ciorbaru