maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Robert!

On Jan 20, Robert Bindar wrote:
> revision-id: 02587f61f40 (mariadb-10.4.1-100-g02587f61f40)
> parent(s): 937c90ce2d3
> author: Robert Bindar <robert@xxxxxxxxxxx>
> committer: Sergei Golubchik <serg@xxxxxxxxxxx>
> timestamp: 2019-01-20 00:33:44 +0100
> message:
> 
> MDEV-13095 Implement User Account locking
> 
> Add server support for user account locking.
> This patch adds two new statements for denying a user's subsequent login
> attempts:
>     LOCK USER[S]
>         user_name [, user2_name ] ...
>     UNLOCK USER[S]
>         user_name [, user2_name ] ...

This doesn't seem to be correct anymore :)

> The SHOW GRANTS command was updated to support locking state
> information.
> 
> diff --git a/mysql-test/include/switch_to_mysql_user.inc b/mysql-test/include/switch_to_mysql_user.inc
> index f5801db6114..eff1d98c2df 100644
> --- a/mysql-test/include/switch_to_mysql_user.inc
> +++ b/mysql-test/include/switch_to_mysql_user.inc
> @@ -45,6 +45,9 @@ CREATE TABLE mysql.user (
>    plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL,
>    authentication_string TEXT NOT NULL,
>    password_expired ENUM('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> +  password_last_changed datetime NULL,
> +  password_lifetime int(20) unsigned NULL,
> +  account_locked enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,

No-no. mysql.user structure was frozen in time the moment I pushed the
commit with the new structure. It doesn't change anymore.

To test 5.7 support, just add ALTER TABLE in one specific test file.
See main/system_mysql_db_507.test

>    is_role enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
>    default_role char(80) binary DEFAULT '' NOT NULL,
>    max_statement_time decimal(12,6) DEFAULT 0 NOT NULL,
> diff --git a/mysql-test/main/lock_user.result b/mysql-test/main/lock_user.result
> --- /dev/null
> +++ b/mysql-test/main/lock_user.result
> @@ -0,0 +1,132 @@
> +connect con2,localhost,user2;
> +disconnect con2;
> +connection default;
> +alter user user1@localhost account unlock;
> +#
> +# A user shouldn't be able to lock itself out

"lock himself out"

And why not? A user can drop himself or change the password to some
garbage. It's the same as locking or worse.

> +#
> +create user newuser@localhost;
> +grant CREATE USER on *.* to newuser@localhost;
> +connect con1,localhost,newuser;
> +connection con1;
> +alter user newuser@localhost account lock;
> +ERROR HY000: Operation ALTER USER failed for 'newuser'@'localhost'
> +disconnect con1;
> +connection default;
> +drop user newuser@localhost;
...
> +alter user user1@localhost account lock;
> +show grants for user1@localhost;
> +Grants for user1@localhost
> +GRANT USAGE ON *.* TO 'user1'@'localhost'
> +ALTER USER 'user1'@'localhost' ACCOUNT LOCK

No, I don't think so. It's MySQL compatibility feature, make it MySQL
compatible. And it's just not very logical to put ALTER USER in SHOW GRANTS.

So, SHOW GRANTS should not show ACCOUNT LOCK, but SHOW CREATE USER
should.

> diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql
> index f788f5d67d5..01d686ac345 100644
> --- a/scripts/mysql_system_tables.sql
> +++ b/scripts/mysql_system_tables.sql
> @@ -79,7 +79,10 @@ CREATE DEFINER=root@localhost SQL SECURITY DEFINER VIEW IF NOT EXISTS user AS SE
>    CAST(IFNULL(JSON_VALUE(Priv, '$.max_user_connections'), 0) AS SIGNED) AS max_user_connections,
>    IFNULL(JSON_VALUE(Priv, '$.plugin'), '') AS plugin,
>    IFNULL(JSON_VALUE(Priv, '$.authentication_string'), '') AS authentication_string,
> -  'N' AS password_expired,
> +  ELT(IFNULL(JSON_VALUE(Priv, '$.password_expired'), 0) + 1, 'N', 'Y') AS password_expired,
> +  CAST(JSON_UNQUOTE(JSON_EXTRACT(Priv, '$.password_last_changed')) AS DATETIME) AS password_last_changed,
> +  CAST(JSON_VALUE(Priv, '$.password_lifetime') AS UNSIGNED) AS password_lifetime,
> +  ELT(IFNULL(JSON_VALUE(Priv, '$.account_locked'), 0) + 1, 'N', 'Y') AS account_locked,

No-no. mysql.user is frozen, it doesn't change anymore.
It stays always as it was in 10.3.
This was the whole point of moving to JSON, to never ever add new
columns to mysql.user table.

>    ELT(IFNULL(JSON_VALUE(Priv, '$.is_role'), 0) + 1, 'N', 'Y') AS is_role,
>    IFNULL(JSON_VALUE(Priv, '$.default_role'), '') AS default_role,
>    CAST(IFNULL(JSON_VALUE(Priv, '$.max_statement_time'), 0.0) AS DECIMAL(12,6)) AS max_statement_time
> diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql
> index 82ec2faa12d..5442e5fee73 100644
> --- a/scripts/mysql_system_tables_fix.sql
> +++ b/scripts/mysql_system_tables_fix.sql
> @@ -643,12 +643,16 @@ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL,
>  ALTER TABLE user MODIFY plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL,
>                   MODIFY authentication_string TEXT NOT NULL;
>  ALTER TABLE user ADD password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL;
> +ALTER TABLE user ADD password_last_changed datetime NULL after password_expired;
> +ALTER TABLE user ADD password_lifetime int(20) unsigned NULL after password_last_changed;
> +ALTER TABLE user ADD account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL after password_lifetime;

But that's okay, because otherwise CREATE ... SELECT below won't work.

>  ALTER TABLE user ADD is_role enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL;
>  ALTER TABLE user ADD default_role char(80) binary DEFAULT '' NOT NULL;
>  ALTER TABLE user ADD max_statement_time decimal(12,6) DEFAULT 0 NOT NULL;
>  -- Somewhere above, we ran ALTER TABLE user .... CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin.
>  --  we want password_expired column to have collation utf8_general_ci.
>  ALTER TABLE user MODIFY password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL;
> +ALTER TABLE user MODIFY account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL;

I don't think it's needed, we're dropping the table anyway.

>  -- Checking for any duplicate hostname and username combination are exists.
> @@ -804,6 +808,10 @@ IF 'BASE TABLE' = (select table_type from information_schema.tables where table_
>                      'max_statement_time', max_statement_time,
>                      'plugin', if(plugin>'',plugin,if(length(password)=16,'mysql_old_password','mysql_native_password')),
>                      'authentication_string', if(plugin>'',authentication_string,password),
> +                    'password_expired', 'Y'=password_expired,
> +                    'password_last_changed', password_last_changed,
> +                    'password_lifetime', password_lifetime,

It would be better to put password expiration changes in the
password expiration commit.

> +                    'account_locked', 'Y'=account_locked,
>                      'default_role', default_role,
>                      'is_role', 'Y'=is_role)) as Priv
>    FROM user;
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index fe6fc9148bd..e4fc96b61cd 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -798,12 +806,25 @@ class Grant_table_base
>        else if (start_priv_columns)
>            break;
>      }
> +
> +    end_security_columns= end_priv_columns;
> +    for (; end_security_columns < num_fields(); end_security_columns++)
> +    {
> +      Field *field= m_table->field[end_security_columns];
> +      const char *colname= field->field_name.str;
> +      if (field->real_type() == MYSQL_TYPE_ENUM &&
> +         !strcmp(colname, "is_role"))
> +        break;
> +    }

Are you trying to make it super-generic, like with privileges, for an
arbitrary number of security columns? Don't bother, 5.7 was the last
MySQL version with privilege tables, they won't be adding more "security
columns" to it.

You could simply add accessors like

  longlong get_password_lifetime() const
  {
    Field *f= get_field(end_priv_columns + 11, MYSQL_TYPE_LONG);
    return f ? f->val_int() : 0;
  }

without changing anything else in User_table_tabular.

>    }
>  
>    /* The index at which privilege columns start. */
>    uint start_priv_columns;
>    /* The index after the last privilege column */
>    uint end_priv_columns;
> +  /* The index after the last security column, i.e. index of is_role. This index
> +     is equal to num_fields if the is_role column does not exist */
> +  uint end_security_columns;
>  
>    TABLE *m_table;
>  };
> @@ -8758,7 +8838,6 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_CSTRING *u,
>    return (ROLE_GRANT_PAIR *)
>      my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length);
>  }
> -
>  static bool show_role_grants(THD *thd, const char *username,

restore the empty line please

>                               const char *hostname, ACL_USER_BASE *acl_entry,
>                               char *buff, size_t buffsize)
> @@ -10555,6 +10634,52 @@ int mysql_alter_user(THD* thd, List<LEX_USER> &users_list)
>    DBUG_RETURN(result);
>  }
>  
> +/*
> +  Mark an user account as locked or unlocked.
> +
> +  SYNOPSIS

Doxygen comments. This style that you've copied is from (iirc) before 2005:)

> +    lock_user_account()
> +    thd                         The current thread.
> +    user_table                  A TL_WRITE opened User_table
> +    lex_user                    The user to lock/unlock.
> +    acl_user                    The acl user to be updated
> +
> +  RETURN
> +    > 0         Error. Error message already sent.
> +    0           OK.
> +*/
> +static int lock_user_account(THD* thd, const User_table &user_table,
> +                             LEX_USER *lex_user, ACL_USER *acl_user)
> +{
> +  bool update_account_locking= thd->lex->account_options.update_account_locking;
> +  bool account_locked_value= thd->lex->account_options.account_locked_value;
> +
> +  DBUG_ENTER("lock_user_account");
> +
> +  if (!update_account_locking)
> +    DBUG_RETURN(0);
> +
> +  mysql_mutex_assert_owner(&acl_cache->lock);
> +
> +  /*
> +     Do not allow the current user to lock itself out.
> +  */
> +  ACL_USER *current_acl_user= find_user_wild(thd->security_ctx->priv_host,
> +                                             thd->security_ctx->priv_user);
> +  if (!current_acl_user ||
> +      (!strcmp(lex_user->user.str, current_acl_user->user.str) &&
> +       !my_strcasecmp(system_charset_info, lex_user->host.str,
> +                      current_acl_user->host.hostname)))
> +  {
> +    DBUG_RETURN(1);
> +  }
> +
> +  acl_user->account_locked= account_locked_value;
> +
> +  user_table.set_account_locked(account_locked_value);
> +
> +  DBUG_RETURN(0);
> +}
>  
>  static bool
>  mysql_revoke_sp_privs(THD *thd, Grant_tables *tables, const Sp_handler *sph,
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 8f62dca4aec..6faa5966d29 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -1148,6 +1148,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize);
>    Non-reserved keywords
>  */
>  
> +%token  <kwd>  ACCOUNT_SYM

comment /* MYSQL */

>  %token  <kwd>  ACTION                        /* SQL-2003-N */
>  %token  <kwd>  ADMIN_SYM                     /* SQL-2003-N */
>  %token  <kwd>  ADDDATE_SYM                   /* MYSQL-FUNC */
> +opt_account_option:
> +          ACCOUNT_SYM LOCK_SYM
> +          {
> +            Lex->account_options.update_account_locking= true;
> +            Lex->account_options.account_locked_value= true;
> +          }
> +        | ACCOUNT_SYM UNLOCK_SYM
> +          {
> +            Lex->account_options.update_account_locking= true;
> +            Lex->account_options.account_locked_value= false;

Why wouldn't you make it a 3-state enum { NOT_SPECIFIED, LOCKED, UNLOCKED } ?
looks more natural here.

> +          }
> +        ;
> +
>  ev_alter_on_schedule_completion:
>            /* empty */ { $$= 0;}

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx