Re: 9e5d4dfc49b: MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data
Hi, Nikita, 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.
* Upon idea, it stores auxiliary user data related to login/security/resources * An unprivileged user can access their own data, and that is the main difference with what mysql.global_priv provides
exactly!
* The fields are currently: USER, WRONG_PASSWORD_ATTEMPTS, EXPIRATION_TIME
I wanted to write about splitting USER into USER and HOST, but indeed it's just USER everywhere else in INFORMATION_SCHEMA (e.g. DEFINER and GRANTEE columns). And functions like USER() and CURRENT_USER() don't split either. So, you're right, let's consistently use user@host everywhere.
diff --git a/mysql-test/main/information_schema_stats.result b/mysql-test/main/information_schema_stats.result index 352bcbab823..e38788872fa 100644 --- a/mysql-test/main/information_schema_stats.result +++ b/mysql-test/main/information_schema_stats.result ... +connect(localhost,naughty_user,wrong_passwd,test,16000,/home/nik/mariadb/bld/mysql-test/var/tmp/mysqld.1.sock);
here and below - you forgot to replace the path with MASTER_MYSOCK
diff --git a/mysql-test/main/information_schema_stats.test b/mysql-test/main/information_schema_stats.test index fd5171c3fb4..dbcabe45965 100644 --- a/mysql-test/main/information_schema_stats.test +++ b/mysql-test/main/information_schema_stats.test @@ -47,3 +47,69 @@ select * from information_schema.index_statistics where table_schema='test' and select * from information_schema.table_statistics where table_schema='test' and table_name='just_a_test'; set global userstat=@save_userstat; --enable_ps2_protocol + +--echo # +--echo # MDEV-23729 INFORMATION_SCHEMA Table info. about user locked due to +--echo # max_password_errors +--echo # +--echo # MDEV-32218 message to notify end-user N-days prior the password get +--echo # expired +--echo #
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.
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 @@ -12956,6 +12956,87 @@ int fill_schema_column_privileges(THD *thd, TABLE_LIST *tables, COND *cond) #endif }
+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? And then don't forget to update @@max_password_errors help text to match. Also, you don't have a test case for FLUSH PRIVILEGES.
+ Column("EXPIRATION_TIME", SLonglong(), NULLABLE),
PASSWORD_EXPIRATION_TIME, the user account itself does not expire.
+ CEnd() + }; +}; + +static bool ignore_max_password_errors(const ACL_USER *acl_user); + +static int fill_logon_schema_record(THD *thd, TABLE * table, ACL_USER *user) +{ + ulonglong lifetime= user->password_lifetime < 0 + ? default_password_lifetime + : user->password_lifetime; + + bool ignore_password_errors= ignore_max_password_errors(user);
why? I think it's still useful to show how many wrong password attempts were there for an account even if it doesn't get blocked.
+ bool ignore_expiration_date= lifetime == 0; + + /* Skip user if nothing to show */ + if (ignore_password_errors && ignore_expiration_date) + return 0; + + Grantee_str grantee(user->user.str, safe_str(user->host.hostname)); + table->field[0]->store(grantee, strlen(grantee), system_charset_info); + if (ignore_password_errors) + { + table->field[1]->set_null(); + } + else + { + table->field[1]->set_notnull(); + table->field[1]->store(user->password_errors); + } + if (ignore_expiration_date) + { + table->field[2]->set_null(); + } + else + { + table->field[2]->set_notnull(); + 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.
+ } + + return schema_table_store_record(thd, table); +} + +int fill_logon_schema_table(THD *thd, TABLE_LIST *tables, COND *cond) +{ + int res= 0; +#ifndef NO_EMBEDDED_ACCESS_CHECKS + 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
+ + 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 2. add a test for it
+ + res= fill_logon_schema_record(thd, table, cur_user); + mysql_mutex_unlock(&acl_cache->lock); + return res; + } + + mysql_mutex_lock(&acl_cache->lock); + for (size_t i= 0; res == 0 && i < acl_users.elements; i++) + { + ACL_USER *user= dynamic_element(&acl_users, i, ACL_USER*); + res= fill_logon_schema_record(thd, table, user); + } + mysql_mutex_unlock(&acl_cache->lock); +#endif + return res; +} +
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei! Thanks for the thorough walkthrough. There are some things that bring me up to an extended discussion, see below. On Sun, 10 Mar 2024 at 17:03, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
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. On the other hand, this will complicate achieving the atomicity on such data access. In my model, a user would have to make a join of two tables. And in current implementation, it would be done in two separate runs, i.e. SELECT * from i_s.USERS joint i_s.LOGON ... would be done as follows: lock(&acl_cache->lock) fetch data for USERS table unlock(&acl_cache->lock) lock(&acl_cache->lock) fetch data for LOGON table unlock(&acl_cache->lock) *(or in the opposite table order)* So that would leave a chance to result in the inconsistent data per row. But: 1. I think it's not a problem for the data with these semantics (i.e. login data doesn't have to be synchronized with the user info table) 2. It can be fixed with some `lock_tables` callback in the information schema api. This one also has a price of holding a lock for the statement duration. 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?
* Upon idea, it stores auxiliary user data related to login/security/resources * An unprivileged user can access their own data, and that is the main difference with what mysql.global_priv provides
exactly!
And also the race condition similar to the one mentioned above.
* The fields are currently: USER, WRONG_PASSWORD_ATTEMPTS, EXPIRATION_TIME
I wanted to write about splitting USER into USER and HOST, but indeed it's just USER everywhere else in INFORMATION_SCHEMA (e.g. DEFINER and GRANTEE columns). And functions like USER() and CURRENT_USER() don't split either. So, you're right, let's consistently use user@host everywhere.
And for some (incorrect) reason the field in USER_STATISTICS is called userhost, rather than user.
diff --git a/mysql-test/main/information_schema_stats.result b/mysql-test/main/information_schema_stats.result index 352bcbab823..e38788872fa 100644 --- a/mysql-test/main/information_schema_stats.result +++ b/mysql-test/main/information_schema_stats.result ...
+connect(localhost,naughty_user,wrong_passwd,test,16000,/home/nik/mariadb/bld/mysql-test/var/tmp/mysqld.1.sock);
here and below - you forgot to replace the path with MASTER_MYSOCK
Will check, thanks.
diff --git a/mysql-test/main/information_schema_stats.test b/mysql-test/main/information_schema_stats.test index fd5171c3fb4..dbcabe45965 100644 --- a/mysql-test/main/information_schema_stats.test +++ b/mysql-test/main/information_schema_stats.test @@ -47,3 +47,69 @@ select * from information_schema.index_statistics where table_schema='test' and select * from information_schema.table_statistics where table_schema='test' and table_name='just_a_test'; set global userstat=@save_userstat; --enable_ps2_protocol + +--echo # +--echo # MDEV-23729 INFORMATION_SCHEMA Table info. about user locked due to +--echo # max_password_errors +--echo # +--echo # MDEV-32218 message to notify end-user N-days prior the password get +--echo # expired +--echo #
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 ase a use-case note.
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 @@ -12956,6 +12956,87 @@ int fill_schema_column_privileges(THD *thd, TABLE_LIST *tables, COND *cond) #endif }
+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"? And then don't forget to update @@max_password_errors help text to
match.
about what? 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.
+ Column("EXPIRATION_TIME", SLonglong(), NULLABLE),
PASSWORD_EXPIRATION_TIME, the user account itself does not expire.
👍
+static int fill_logon_schema_record(THD *thd, TABLE * table, ACL_USER *user)
+{ + ulonglong lifetime= user->password_lifetime < 0 + ? default_password_lifetime + : user->password_lifetime; + + bool ignore_password_errors= ignore_user);
why? I think it's still useful to show how many wrong password attempts were there for an account even if it doesn't get blocked.
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?
+ 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??)
+ 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.
+
+ 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. 2. add a test for it If what I said above is true, it doesn't seem possible. -- Yours truly, Nikita Malyavin
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
participants (2)
-
Nikita Malyavin -
Sergei Golubchik