froxlor / Froxlor

The server administration software for your needs - The official Froxlor development Git repository
http://www.froxlor.org
GNU General Public License v2.0
1.62k stars 455 forks source link

DbManagerMySQL->grantPrivilegesTo() seems to be somewhat bogus #1225

Closed realrellek closed 7 months ago

realrellek commented 7 months ago

Describe the bug I was updating an old server in anticipation of a new one and so I was wiping everything that looked like "syscp" and changed it to froxlor. At some point, I wanted to rename the database - which is not a thing, so I created a new one and imported a dump.

Now I wanted to give my (renamed) froxlor user access and didn't know how, so I looked at how Froxlor would do it when creating a new customer database.

And I found this function which I believe gets ultimately called: https://github.com/Froxlor/Froxlor/blob/854c930696ad1706a10caacc591d06c87936e133/lib/Froxlor/Database/Manager/DbManagerMySQL.php#L81-L138

And I think it does not really do what you'd expect.

So first of all, the $pwd_plugin is hardcoded with the deprecated 'mysql_native_plugin' (see #1214). Anyway. It's weird what then happens.

If the password is supposed to be encrypted and we're on MariaDB (Version < 5.7.0 or > 10), then use whatever is the database default because it is not specified. Otherwise (MySQL 8 for example), use the pwd_plugin, i.e. the deprecated mysql_native_plugin.

And if the password is not supposed to be encrypted, then do what MariaDB does and just use the server default.

You know, I'm not sure what $p_encrypted is used for. I would imagine it is from back in the day when MySQL allowed (or defaulted?) plaintext passwords in its User table. But nowadays, hashed passwords are the default and I don't think you can even store passwords in plaintext in MySQL.

So long story short, I feel like $p_encrypted has done its duty and should be ignored from now on, with just relying on the server defaults.

System information

To Reproduce Yes.

Expected behavior It should just create the user or change the password. Don't specify the password plugin, especially since mysql_native_plugin is deprecated

Logfiles Nope.

Additional context Nothing to see here. :)

d00p commented 7 months ago

So first of all, the $pwd_plugin is hardcoded with the deprecated 'mysql_native_plugin' (see https://github.com/Froxlor/Froxlor/issues/1214).

Which, if you open it, is already fixed

You know, I'm not sure what $p_encrypted is used for. I would imagine it is from back in the day when MySQL allowed (or defaulted?) plaintext passwords in its User table. But nowadays, hashed passwords are the default and I don't think you can even store passwords in plaintext in MySQL.

It's not for plaintext passwords, quiet the opposite. It is used (internally) when the mysql-access-hosts value changes and we need to add a user for a new host - as we only have the encrypted password, we set this directly.

See difference in Query:

IDENTIFIED BY PASSWORD 'hashedpassword'
---
IDENTIFIED BY 'plaintextpassword'

Or from the syntax at: https://mariadb.com/kb/en/create-user/

[...]
authentication_option:
  IDENTIFIED BY 'password' 
  | IDENTIFIED BY PASSWORD 'password_hash'
  | IDENTIFIED {VIA|WITH} authentication_rule [OR authentication_rule  ...]
[...]

So long story short, no change needed, it does exactly what it's supposed to do.

realrellek commented 7 months ago

Which, if you open it, is already fixed

Yes I know, #1214 is already fixed and closed.

... So long story short, no change needed, it does exactly what it's supposed to do.

Alright, thank you for the explanation.

Nevermind then and sorry for the interruption 😳