Hi, Nikita, On Mar 11, Nikita Malyavin wrote:
On Sun, 10 Mar 2024 at 17:03, Sergei Golubchik <serg@mariadb.org> wrote:
On Mar 10, Nikita Malyavin wrote:
revision-id: 9e5d4dfc49b (mariadb-11.4.1-10-g9e5d4dfc49b) parent(s): 929c2e06aae author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2024-02-29 17:24:27 +0100 message:
MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data
* A new table INFORMATION_SCHEMA.LOGON is introduced.
There were many cases where we lack an INFORMATION_SCHEMA with all user accounts. Forcing users to select from mysql.user is a shame.
Let's use the chance and create a table USERS. It doesn't need more columns now than what you've created, but we'll likely create more in the future.
For me, USERS sounds like an information about a user itself, Like maybe username, email, real name, home address. Maybe I am picturing a wrong similarity with the typical web world. Yet, even in a closer example like Linux, the data is stored in a *faillog* PAM "database" (i don't know the details on its structure). As we are just providing a view to this data, we may want to separate it by different tables with different semantics.
Well, in MariaDB a "user" is something created with "CREATE USER", so no email or real name, but properties that can be set with CREATE USER or modified with ALTER USER.
Anyway, accessing the rest of supposedly auth-related data, like max_connections, would first require some redesign of we will contend LOCK_user_conn.
==== So given all this, what do you think, still? One table USERS for everything, or a few different tables?
I think most of per-user information can go into one table. But authentication, for example, cannot, a user can have many auth plugins, so it'll require a separate table.
I don't see how you test "info about user locked due to max_password_errors". This is the reason for implementing this new table, it has to be tested. At least add
select * from information_schema.logon where wrong_password_attemps >= @max_password_errors;
and show it it's empty at first and then not empty.
Why to include it in the test? It's a documentation issue. Can be added in the jira ticket as a use-case note.
Because it was the main feature you needed to implement? :) I think there must be a test proving that whatever you've implemented actually does provide the functionality that was the goal of this task in the first place.
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 14450a5a610..4d199bf0e61 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc
+namespace Show +{ + ST_FIELD_INFO users_fields_info[] = + { + Column("USER", Userhost(), NOT_NULL), + Column("WRONG_PASSWORD_ATTEMPTS", SLonglong(), NULLABLE),
@@max_password_errors is documented as "If there is more than this number of failed connect attempts due to invalid password, user will be blocked from further connections until FLUSH_PRIVILEGES"
Let's use call incorrect password consistently everywhere. It could be "bad password", "wrong password", "incorrect password", "invalid password", but it should be the same everywhere.
I personally think that "invalid" implies some kind if validity check, e.g. not shorter than 10 characters, not equal to the username, etc and a password that doesn't satisfy these validity rules is "invalid".
Trying to login with an incorrect password is a different kind of error, a password can be valid but wrong.
May be, ask Ian and/or Daniel about it?
Maybe given that we already have max_password_errors, we'd better just name it "password_errors"?
Sounds good to me. But still, please, ask documentation writers too.
And then don't forget to update @@max_password_errors help text to match.
about what?
I meant, if you at the end agree to use "wrong" then the column should be WRONG_PASSWORD_ATTEMPTS and @@max_password_errors help text should say "... due to wrong password". That is, whatever word you and documentation writers will choose, it should be used everywhere consistently. Of course, if you'll use simply "password_errors" for a column, then there is no adjective at all and my request becomes meaningless.
Also, you don't have a test case for FLUSH PRIVILEGES.
Why? There's already main.max_password_errors to test how it works. We only provide the view for the value behind that logic.
May be to show that you indeed "only provide the view for the value"?
Probably, you are right... but then we'll have to explain why some accounts don't get blocked with max_password_errors reached... What will happen if:
1. Log in with user@localhost 2. Give it CONNECTION_ADMIN_ACL privilege. 3. make some wrong password login attempts, > @@max_password_errors 4. revoke CONNECTION_ADMIN_ACL ?
I guess we will see the user blocked. Shouldn't we add ignore_password_errors field then?
in that case the user *is* blocked, isn't it? one can no longer login as that user.
+ table->field[2]->store(user->password_last_changed
+ + user->password_lifetime * 3600 * 24, true);
I think it'd be more generally useful to show password_last_changed and expiration period separately.
I find a number of days value inconvenient and hard to transform. Perhaps, a datetime field would be even more convenient, though. What I would want to know, as a user, is *when* am I going to be expired. This becomes even more messy when you'll get 0 days, but still will be able to log in (til when exactly??)
Yes, I thought password_last_changed would be a datetime column and expiration period in days, so one can do SELECT password_last_changed + INTERVAL expiration_period DAYS FROM But perhaps it'd be more convenient to return expiration datetime directly, I'm not sure about it.
+ bool see_whole_table= check_global_access(thd, PROCESS_ACL, true) == 0;
I don't think PROCESS_ACL is a very logical choice here. And there's nothing better as far as I can see.
May be let's just do
if (check_access(thd, SELECT_ACL, "mysql", NULL, NULL, 1, 1))
? There are many checks like that in e.g. sql_parse.cc
I borrowed this check from user_stats.cc. I don't know which one is better. The PROCESS_ACL check seems more lightweight to me.
Sure, it's more lightweight. But 1) it's wrong. I don't really have a good explanation of why PROCESS should allow a user to see other users 2) SELECT on "mysql" is needed anyway. Anyone who has it can see all users, so she should be able to do it in I_S too. See, e.g. fill_schema_proc - it a user has SELECT on "mysql.proc", she can see all routines in all schemas. So, because 1 is dubious and 2 has to be done anyway, I suggested to do only 2 now and get back to 1 later. May be with a new dedicated privilege. May be with an existing one.
+ TABLE *table= tables->table; + + if (!see_whole_table) + { + mysql_mutex_lock(&acl_cache->lock); + ACL_USER *cur_user= find_user_exact(thd->security_ctx->priv_host, + thd->security_ctx->priv_user);
1. cur_user can be NULL if someone dropped it while there was an active connection for this user
We still have at least one connection. If we are here, it's still in the array.
you have a connection, but not necessarily an entry in the acl_users array.
2. add a test for it
If what I said above is true, it doesn't seem possible.
Just try it, it'll only take few minutes. Like CREATE USER foo@localhost; connect foo,localhost,foo; connection default; DROP USER foo@localhost; connection foo; # and here you are, select from your table Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org