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