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 secur...@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
