maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #11669
Re: 297ac6aa5b0: MDEV-7597 Expiration of user passwords
Hi, Robert!
On Feb 13, Robert Bindar wrote:
> revision-id: 297ac6aa5b0 (mariadb-10.4.1-101-g297ac6aa5b0)
> parent(s): ce02e7c9acc
> author: Robert Bindar <robert@xxxxxxxxxxx>
> committer: Robert Bindar <robert@xxxxxxxxxxx>
> timestamp: 2019-02-07 17:47:13 +0200
> message:
>
> MDEV-7597 Expiration of user passwords
>
> This patch adds support for expiring user passwords.
> The following statements are extended:
> CREATE USER user@localhost PASSWORD EXPIRE [option]
> ALTER USER user@localhost PASSWORD EXPIRE [option]
> If no option is specified, the password is expired with immediate
> effect. If option is DEFAULT, global policy applies according to
> the default_password_lifetime system var (if 0, password never
> expires, if N, password expires every N days). If option is NEVER,
> the password never expires and if option is INTERVAL N DAY, the
> password expires every N days.
> The feature also supports the disconnect_on_expired_password system
> var and the --connect-expired-password client option.
>
> diff --git a/libmariadb b/libmariadb
> index 34f8887af03..70f2964dc4d 160000
> --- a/libmariadb
> +++ b/libmariadb
> @@ -1 +1 @@
> -Subproject commit 34f8887af03d022416dd6593de91d0706e57f46b
> +Subproject commit 70f2964dc4de116f4b50732cfec7cb566e082b4c
This shouldn't be in your commit
> diff --git a/mysql-test/main/grant5.result b/mysql-test/main/grant5.result
> index 3d0f331e58a..3bb81266b4d 100644
> --- a/mysql-test/main/grant5.result
> +++ b/mysql-test/main/grant5.result
> @@ -67,21 +67,21 @@ show create user u1@h;
> ERROR 28000: Can't find any matching row in the user table
> show create user u2@h;
> CREATE USER for u2@h
> -CREATE USER 'u2'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' ACCOUNT UNLOCK
> +CREATE USER 'u2'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
Just like with account locking, I wouldn't print default behavior clauses here
For example, we don't print
REQUIRE NONE WITH MAX_QUERIES_PER_HOUR 0 MAX_UPDATES_PER_HOUR 0 MAX_CONNECTIONS_PER_HOUR 0 MAX_USER_CONNECTIONS 0
> show create user u3@h;
> CREATE USER for u3@h
> -CREATE USER 'u3'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' ACCOUNT UNLOCK
> +CREATE USER 'u3'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
> show create user u4@h;
> CREATE USER for u4@h
> -CREATE USER 'u4'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' ACCOUNT UNLOCK
> +CREATE USER 'u4'@'h' IDENTIFIED BY PASSWORD '*975B2CD4FF9AE554FE8AD33168FBFC326D2021DD' PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
> show create user u5@h;
> ERROR 28000: Can't find any matching row in the user table
> show create user u6@h;
> CREATE USER for u6@h
> -CREATE USER 'u6'@'h' IDENTIFIED BY PASSWORD '78a302dd267f6044' ACCOUNT UNLOCK
> +CREATE USER 'u6'@'h' IDENTIFIED BY PASSWORD '78a302dd267f6044' PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
> show create user u7@h;
> CREATE USER for u7@h
> -CREATE USER 'u7'@'h' IDENTIFIED BY PASSWORD '78a302dd267f6044' ACCOUNT UNLOCK
> +CREATE USER 'u7'@'h' IDENTIFIED BY PASSWORD '78a302dd267f6044' PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
> show create user u8@h;
> ERROR 28000: Can't find any matching row in the user table
> grant select on *.* to u1@h;
> diff --git a/mysql-test/main/password_expiration_dbug.test b/mysql-test/main/password_expiration_dbug.test
> new file mode 100644
> index 00000000000..99cbf5a6efb
> --- /dev/null
> +++ b/mysql-test/main/password_expiration_dbug.test
> @@ -0,0 +1,57 @@
> +#
> +# Test password expiration INTERVAL and default_password_lifetime options
> +#
> +
> +--source include/have_debug.inc
> +--source include/not_embedded.inc
> +
> +set @old_dbug=@@global.debug_dbug;
> +set global debug_dbug="+d,password_expiration_interval_sec";
> +
> +--echo #
> +--echo # PASSWORD EXPIRE DEFAULT should use the default_password_lifetime
> +--echo # system var to set the number of days till expiration
> +--echo #
> +set global disconnect_on_expired_password=ON;
> +set global default_password_lifetime=1;
> +create user user1@localhost password expire default;
> +--sleep 2
I don't like that. all sleep-based tests have proven to be very unreliable
in buildbot :(
I don't have any good ideas, you cannot change the global server
time from the test. May be, mess with a timezone?
Like, set the expiration for a few minutes (like you do)
and change server's timezone to move the time forward?
Might work. Depends on what time you use for expiration - local or UTC.
> +--replace_result $MASTER_MYPORT MYSQL_PORT $MASTER_MYSOCK MYSQL_SOCK
> +--error ER_MUST_CHANGE_PASSWORD_LOGIN
> +connect(con1,localhost,user1);
> +drop user user1@localhost;
> +
> +--echo #
> +--echo # PASSWORD EXPIRE INTERVAL should expire a client's password after
> +--echo # X seconds and not before
> +--echo #
> +set global disconnect_on_expired_password=ON;
> +create user user1@localhost password expire interval 2 day;
> +--replace_result $MASTER_MYPORT MYSQL_PORT $MASTER_MYSOCK MYSQL_SOCK
> +connect(con1,localhost,user1);
> +disconnect con1;
> +connection default;
> +--sleep 3
> +--replace_result $MASTER_MYPORT MYSQL_PORT $MASTER_MYSOCK MYSQL_SOCK
> +--error ER_MUST_CHANGE_PASSWORD_LOGIN
> +connect(con1,localhost,user1);
> +drop user user1@localhost;
> +
> +--echo #
> +--echo # PASSWORD EXPIRE NEVER should override the other policies and never
> +--echo # expire a client's password
> +--echo #
> +set global disconnect_on_expired_password=ON;
> +create user user1@localhost password expire interval 2 day;
> +alter user user1@localhost password expire never;
> +--sleep 3
> +--replace_result $MASTER_MYPORT MYSQL_PORT $MASTER_MYSOCK MYSQL_SOCK
> +connect(con1,localhost,user1);
> +disconnect con1;
> +connection default;
> +drop user user1@localhost;
> +
> +set global debug_dbug=@old_dbug;
> +set global disconnect_on_expired_password=default;
> +set global default_password_lifetime=default;
> +
> diff --git a/mysql-test/main/system_mysql_db_fix40123.result b/mysql-test/main/system_mysql_db_fix40123.result
> index 3903dbdad94..a96a33ce54f 100644
> --- a/mysql-test/main/system_mysql_db_fix40123.result
> +++ b/mysql-test/main/system_mysql_db_fix40123.result
> @@ -108,6 +108,8 @@ user CREATE TABLE `user` (
> `plugin` char(64) CHARACTER SET latin1 NOT NULL DEFAULT '',
> `authentication_string` text COLLATE utf8_bin NOT NULL,
> `password_expired` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
> + `password_last_changed` timestamp NOT NULL DEFAULT current_timestamp(),
> + `password_lifetime` smallint(5) unsigned NOT NULL DEFAULT 0,
I've fixed it, so when you rebase they should go away, make sure to
rerun these tests.
> `account_locked` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
> `is_role` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
> `default_role` char(80) COLLATE utf8_bin NOT NULL DEFAULT '',
> diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql
> index 04a9bd9785a..e8af55993f5 100644
> --- a/scripts/mysql_system_tables_fix.sql
> +++ b/scripts/mysql_system_tables_fix.sql
> @@ -805,6 +807,9 @@ 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,
Why do you need an explicit password_expired value?
I'd just use password_last_changed=0 for that. It'll work
automatically (changed too back in the past so it makes it expired)
without an explicit check for password_expired==1
> + 'password_last_changed', UNIX_TIMESTAMP(password_last_changed),
> + 'password_lifetime', ifnull(password_lifetime, -1),
> 'account_locked', 'Y'=account_locked,
> 'default_role', default_role,
> 'is_role', 'Y'=is_role)) as Priv
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 4536abd2907..e0db27165c4 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -6940,8 +6940,9 @@ ER_NOT_VALID_PASSWORD
> eng "Your password does not satisfy the current policy requirements"
>
> ER_MUST_CHANGE_PASSWORD
> - eng "You must SET PASSWORD before executing this statement"
> + eng "You must change your password using either ALTER USER or SET PASSWORD before executing this statement"
> bgn "Трябва първо да си смените паролата със SET PASSWORD за да можете да изпълните тази команда"
> + rum "Trebuie sa iti schimbi parola folosind ALTER USER sau SET PASSWORD inainte de a executa aceasta comanda"
Let's keep the old SET PASSWORD message. It seems that ALTER USER
is more appropriate for DBA, for account administration, not
for mere users.
>
> ER_FK_NO_INDEX_CHILD
> eng "Failed to add the foreign key constaint. Missing index for constraint '%s' in the foreign table '%s'"
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index 983686d51c7..f0a8ffb1a60 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -634,7 +638,7 @@ static ACL_ROLE *find_acl_role(const char *user);
> static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_CSTRING *u, const LEX_CSTRING *h, const LEX_CSTRING *r);
> static ACL_USER_BASE *find_acl_user_base(const char *user, const char *host);
> static bool update_user_table_password(THD *, const User_table&,
> - const ACL_USER &);
> + ACL_USER &);
use only const references or pointers.
So, this one should become a pointer.
> static bool acl_load(THD *thd, const Grant_tables& grant_tables);
> static inline void get_grantor(THD *thd, char* grantor);
> static bool add_role_user_mapping(const char *uname, const char *hname, const char *rname);
> @@ -2705,6 +2806,7 @@ bool acl_getroot(Security_context *sctx, const char *user, const char *host,
> sctx->master_access= 0;
> sctx->db_access= 0;
> *sctx->priv_user= *sctx->priv_host= *sctx->priv_role= 0;
> + sctx->set_password_expired(false);
I'd rather call init() here. Future-proof.
>
> if (host[0]) // User, not Role
> {
> @@ -3457,6 +3565,17 @@ bool change_password(THD *thd, LEX_USER *user)
> goto end;
> }
>
> + if (sctx->password_expired() &&
> + (strcmp(acl_user->user.str, sctx->priv_user) ||
> + my_strcasecmp(system_charset_info,
> + acl_user->host.hostname,
> + sctx->priv_host)))
> + {
> + mysql_mutex_unlock(&acl_cache->lock);
> + my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
> + goto end;
> + }
> +
technically, check_alter_user() should've done that already,
I'm not sure you need to repeat the check here.
> if (update_user_table_password(thd, tables.user_table(), *acl_user))
> {
> mysql_mutex_unlock(&acl_cache->lock); /* purecov: deadcode */
> @@ -3930,6 +4049,12 @@ static bool update_user_table_password(THD *thd, const User_table& user_table,
> user_table.set_auth(user.plugin.str, user.plugin.length,
> user.auth_string.str, user.auth_string.length);
>
> + /* Update the persistent password expired state of user */
> + user_table.set_password_expired(false);
> + thd->set_time();
Hmm, why? This is done at the start of every statement, why do you repeat it
here?
> + Timeval now= thd->query_start_timeval();
Just simply m_time_t now= thd->query_start();
Don't use query_start_timeval() or query_start_sec_part()
unless you _really_ need microsecond precision. They have a side-effect
of server remembering that you asked for microseconds, so they'll
be logged in a binary log.
I've now removed THD::query_start_timeval(), it's too error-prone
> + user_table.set_password_last_changed(now.tv_sec);
> +
> if (unlikely(error= table->file->ha_update_row(table->record[1],
> table->record[0])) &&
> error != HA_ERR_RECORD_IS_THE_SAME)
> @@ -3937,6 +4062,19 @@ static bool update_user_table_password(THD *thd, const User_table& user_table,
> table->file->print_error(error,MYF(0)); /* purecov: deadcode */
> DBUG_RETURN(1);
> }
> +
> + /* Update the acl password expired state of user */
> + user.password_last_changed= now.tv_sec;
> + user.password_expired= false;
> +
> + /* If user is the connected user, reset the password expired field on sctx
> + and allow the user to exit sandbox mode */
> + if (!strcmp(user.user.str, thd->security_ctx->priv_user) &&
> + !my_strcasecmp(system_charset_info,
> + user.host.hostname,
> + thd->security_ctx->priv_host))
this should be a method in ACL_USER or in Security_context - it's used
too often.
> + thd->security_ctx->set_password_expired(false);
> +
> DBUG_RETURN(0);
> }
>
> @@ -4153,6 +4291,68 @@ static int replace_user_table(THD *thd, const User_table &user_table,
> user_table.set_account_locked(lock_value);
> new_acl_user.account_locked= lock_value;
> }
> +
> + thd->set_time();
> + Timeval now= thd->query_start_timeval();
same as above
> + if (!old_row_exists)
> + {
> + user_table.set_password_last_changed(now.tv_sec);
> + new_acl_user.password_last_changed= now.tv_sec;
> + new_acl_user.use_default_pass_lifetime= true;
> + }
> +
> + /* Unexpire the user password */
> + if (combo->is_changing_password)
Is it needed? Couldn't you just check if the password was specified?
> + {
> + user_table.set_password_expired(false);
> + user_table.set_password_last_changed(now.tv_sec);
> + new_acl_user.password_last_changed= now.tv_sec;
> + new_acl_user.password_expired= false;
> +
> + /* If combo is the connected user, reset the password expired field on sctx
> + and allow the user to exit sandbox mode */
> + if (!strcmp(combo->user.str, thd->security_ctx->priv_user) &&
> + !my_strcasecmp(system_charset_info,
> + combo->host.str,
> + thd->security_ctx->priv_host))
> + thd->security_ctx->set_password_expired(false);
I tend to think (again) that ALTER USER is for DBA's, and the user
should use SET PASSWORD. So, let's not allow ALTER USER in the sandbox
mode and, consequently, you shouldn't need the check above either.
For example, ALTER USER can specify MAX_QUERIES_PER_HOUR.
Surely, it's for DBA to specify the limit, it doesn't make any sense
to allow every user to overwrite his own limits.
And even for authentication, think of PAM. A user can change his own
password, but not /etc/pam.d/login for example. Similarly here
a user should be able to change his password, but not the authentication
plugin.
> + }
> +
> + switch (lex->account_options.password_expire) {
> + case PASSWORD_EXPIRE_UNSPECIFIED:
> + break;
> + case PASSWORD_EXPIRE_NOW:
> + user_table.set_password_expired(true);
> + new_acl_user.password_expired= true;
> + new_acl_user.use_default_pass_lifetime= false;
why do you change use_default_pass_lifetime?
> + break;
> + case PASSWORD_EXPIRE_NEVER:
> + user_table.set_password_lifetime(0);
> + new_acl_user.password_lifetime= 0;
> + new_acl_user.use_default_pass_lifetime= false;
> + break;
> + case PASSWORD_EXPIRE_DEFAULT:
> + user_table.set_password_lifetime_null();
> + new_acl_user.use_default_pass_lifetime= true;
> + new_acl_user.password_lifetime= -1;
why two variables to say the same thing?
> + break;
> + case PASSWORD_EXPIRE_INTERVAL:
> + longlong interval= lex->account_options.num_expiration_days;
> + Timestamp exp_tstamp(new_acl_user.password_last_changed, 0);
> + int warn= 0;
> + exp_tstamp.add(3600 * 24 * interval, &warn);
I'm not sure why you're doing it. If the user sets too many days here -
fine, why would that be a problem?
> + if (warn & MYSQL_TIME_WARN_OUT_OF_RANGE)
> + {
> + char num[MAX_BIGINT_WIDTH + 1];
> + my_snprintf(num, sizeof(num), "%lu", interval);
> + my_error(ER_WRONG_VALUE, MYF(0), "DAY", num);
> + goto end;
> + }
> + user_table.set_password_lifetime(interval);
> + new_acl_user.password_lifetime= interval;
> + new_acl_user.use_default_pass_lifetime= false;
> + break;
> + }
> }
>
> if (old_row_exists)
> @@ -10548,6 +10764,39 @@ int mysql_alter_user(THD* thd, List<LEX_USER> &users_list)
>
> LEX_USER *tmp_lex_user;
> List_iterator<LEX_USER> users_list_iterator(users_list);
> + bool password_expired= thd->security_ctx->password_expired();
> + bool own_password_changed= false;
> +
> + /* Precheck the users list to see if the password of the connected
> + user is being changed */
> + if (password_expired)
> + { // TODO: check bug with identified by '' not blanking the password
> + Security_context *sctx= thd->security_ctx;
> + while ((tmp_lex_user= users_list_iterator++))
> + {
> + LEX_USER* lex_user= get_current_user(thd, tmp_lex_user, false);
> +
> + bool is_self= !strcmp(lex_user->user.str, sctx->priv_user) &&
> + !my_strcasecmp(system_charset_info,
> + lex_user->host.str,
> + sctx->priv_host);
> + if (lex_user->is_changing_password && is_self)
> + {
> + own_password_changed= true;
> + break;
> + }
> + }
> +
> + if (!own_password_changed)
> + {
> + mysql_mutex_unlock(&acl_cache->lock);
> + mysql_rwlock_unlock(&LOCK_grant);
> + my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
already commented earlier. Just do ER_MUST_CHANGE_PASSWORD always
if in a sandbox mode
> + DBUG_RETURN(1);
> + }
> + users_list_iterator.init(users_list);
> + }
> +
> while ((tmp_lex_user= users_list_iterator++))
> {
> LEX_USER* lex_user= get_current_user(thd, tmp_lex_user, false);
> @@ -13294,6 +13543,44 @@ static void handle_password_errors(const char *user, const char *hostname, PASSW
> #endif
> }
>
> +bool check_password_lifetime(THD *thd, const ACL_USER *acl_user)
> +{
> + /* the password should never expire */
> + if (!acl_user->password_lifetime)
> + return false;
> +
> + longlong interval= acl_user->password_lifetime;
> + if (acl_user->use_default_pass_lifetime)
> + {
> + mysql_mutex_lock(&LOCK_global_system_variables);
> + interval= default_password_lifetime;
> + mysql_mutex_unlock(&LOCK_global_system_variables);
I'd skip the mutex lock here. The worst that can happen, if someone would be
logging in when default_password_lifetime is just being modified - that this
someone might be classified incorrectly, allowed in with a expired password
or not allowed with a non-expired. Which is a small price to pay to avoid
every single user (because most users never change default settings) taking
yet another mutex lock on every single login.
> +
> + /* default global policy applies, and that is password never expires */
> + if (!interval)
> + return false;
> + }
> +
> + thd->set_time();
> + Timeval tval_now= thd->query_start_timeval();
and again
> + Timestamp tstamp_now(tval_now);
> + Timestamp expiration_tstamp(acl_user->password_last_changed, 0);
> + int interval_sec= 3600 * 24 * interval;
> + int warn= 0;
> +
> + /* this helps test set a testable password lifetime in seconds not days */
> + DBUG_EXECUTE_IF("password_expiration_interval_sec", { interval_sec= interval; });
> +
> + expiration_tstamp.add(interval_sec, &warn);
> +
> + DBUG_ASSERT(!(warn & MYSQL_TIME_WARN_OUT_OF_RANGE));
> +
> + if (tstamp_now.cmp(expiration_tstamp) >= 0) {
> + return true;
> + }
why not to write simply
if ((thd->query_start() - acl_user->password_last_changed)/24/3600 > interval)
this keeps all the code in one (or two) lines, instead of spreading it over
two Timestamp::Timestamp constructors, Timestamp::add() and Timestamp::cmp()
methods. And it doesn't have a problem with large interval values, as a bonus.
> +
> + return false;
> +}
>
> /**
> Perform the handshake, authorize the client and update thd sctx variables.
> @@ -13512,6 +13799,25 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len)
> DBUG_RETURN(1);
> }
>
> + bool disconnect_password_expired= true;
> + bool client_can_handle_exp_pass= thd->client_capabilities &
> + CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS;
> + bool password_lifetime_due= check_password_lifetime(thd, acl_user);
> + mysql_mutex_lock(&LOCK_global_system_variables);
> + disconnect_password_expired= disconnect_on_expired_password;
> + mysql_mutex_unlock(&LOCK_global_system_variables);
same as above
> +
> + if (!client_can_handle_exp_pass && disconnect_password_expired &&
> + (acl_user->password_expired || password_lifetime_due))
by now you've locked the mutex twice, did some calculations to see
if the password expired... and only then you've checked
acl_user->password_expired.
> + {
> + status_var_increment(denied_connections);
> + my_error(ER_MUST_CHANGE_PASSWORD_LOGIN, MYF(0));
> + DBUG_RETURN(1);
> + }
> +
> + sctx->set_password_expired(acl_user->password_expired ||
> + password_lifetime_due);
> +
> /*
> Don't allow the user to connect if he has done too many queries.
> As we are testing max_user_connections == 0 here, it means that we
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 69fabee708c..57b043fb8e4 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1340,6 +1342,11 @@ class Security_context {
> restore_security_context(THD *thd, Security_context *backup);
> #endif
> bool user_matches(Security_context *);
> +
> + bool password_expired()
> + { return pass_expired; }
> + void set_password_expired(bool val)
> + { pass_expired= val; }
Better remove them, this class doesn't have other
trivial setters/getters either.
> };
>
>
> diff --git a/sql/sql_const.h b/sql/sql_const.h
> index 82f3b9c21f2..3d888c25a9f 100644
> --- a/sql/sql_const.h
> +++ b/sql/sql_const.h
> @@ -275,6 +275,7 @@
> #define DELAYED_QUEUE_SIZE 1000
> #define DELAYED_WAIT_TIMEOUT (5*60) /**< Wait for delayed insert */
> #define MAX_CONNECT_ERRORS 100 ///< errors before disabling host
> +#define DEFAULT_PASSWORD_LIFETIME 0 ///< days before passwords expire
no need to create a name for that
>
> #define LONG_TIMEOUT ((ulong) 3600L*24L*365L)
>
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 65b52b5b5da..35e9e869b49 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -1687,6 +1687,17 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
> thd->get_stmt_da()->set_skip_flush();
> }
>
> + if (unlikely(thd->security_ctx->password_expired() &&
> + command != COM_QUERY &&
> + command != COM_STMT_CLOSE &&
> + command != COM_STMT_SEND_LONG_DATA &&
> + command != COM_PING &&
> + command != COM_QUIT))
Why do you allow COM_STMT_CLOSE and COM_STMT_SEND_LONG_DATA?
> + {
> + my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
> + goto dispatch_end;
> + }
> +
> switch (command) {
> case COM_INIT_DB:
> {
> @@ -3249,6 +3260,14 @@ mysql_execute_command(THD *thd)
> #endif
> DBUG_ENTER("mysql_execute_command");
>
> + if (thd->security_ctx->password_expired() &&
> + lex->sql_command != SQLCOM_SET_OPTION &&
> + lex->sql_command != SQLCOM_ALTER_USER)
SQLCOM_SET_OPTION would be enough
> + {
> + my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
> + DBUG_RETURN(1);
> + }
> +
> DBUG_ASSERT(thd->transaction.stmt.is_empty() || thd->in_sub_stmt);
> /*
> Each statement or replication event which might produce deadlock
> diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> index cb822fc2e98..e621bd6519e 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -4188,14 +4188,11 @@ Prepared_statement::execute_loop(String *expanded_query,
> if (set_parameters(expanded_query, packet, packet_end))
> return TRUE;
>
> -#ifdef NOT_YET_FROM_MYSQL_5_6
> - if (unlikely(thd->security_ctx->password_expired &&
> - !lex->is_change_password))
> + if (unlikely(thd->security_ctx->password_expired()))
Really? How can one end up here, if you don't allow COM_STMT_PREPARE
and COM_STMT_EXECUTE and SQLCOM_PREPARE/SQLCOM_EXECUTE ?
> {
> my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
> return true;
> }
> -#endif
>
> reexecute:
> // Make sure that reprepare() did not create any new Items.
> @@ -4361,15 +4358,12 @@ Prepared_statement::execute_bulk_loop(String *expanded_query,
> }
> read_types= FALSE;
>
> -#ifdef NOT_YET_FROM_MYSQL_5_6
> - if (unlikely(thd->security_ctx->password_expired &&
> - !lex->is_change_password))
> + if (unlikely(thd->security_ctx->password_expired()))
Same
> {
> my_error(ER_MUST_CHANGE_PASSWORD, MYF(0));
> thd->set_bulk_execution(0);
> return true;
> }
> -#endif
>
> // iterations changed by set_bulk_parameters
> while ((iterations || start_param) && !error && !thd->is_error())
> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> index 96a73a85267..e06272e4564 100644
> --- a/sql/sql_type.cc
> +++ b/sql/sql_type.cc
> @@ -618,20 +618,35 @@ Sec9 & Sec9::round(uint dec)
> return *this;
> }
>
> +void Timestamp::set_out_of_range(uint sec, uint usec, int *warn)
> +{
> + *warn|= MYSQL_TIME_WARN_OUT_OF_RANGE;
> + tv_sec= sec;
> + tv_usec= usec;
> +}
>
> void Timestamp::round_or_set_max(uint dec, int *warn)
> {
> DBUG_ASSERT(dec <= TIME_SECOND_PART_DIGITS);
> if (add_nanoseconds_usec(msec_round_add[dec]) &&
> tv_sec++ >= TIMESTAMP_MAX_VALUE)
> - {
> - tv_sec= TIMESTAMP_MAX_VALUE;
> - tv_usec= TIME_MAX_SECOND_PART;
> - *warn|= MYSQL_TIME_WARN_OUT_OF_RANGE;
> - }
> + set_out_of_range(TIMESTAMP_MAX_VALUE, TIME_MAX_SECOND_PART, warn);
> +
> my_timeval_trunc(this, dec);
> }
>
> +Timestamp& Timestamp::add(int sec, int *warn)
> +{
> + longlong res= tv_sec + sec;
> + if (res >= TIMESTAMP_MAX_VALUE)
> + set_out_of_range(TIMESTAMP_MAX_VALUE, TIME_MAX_SECOND_PART, warn);
> + else if (res < 0)
> + set_out_of_range(0, 0, warn);
> + else
> + tv_sec= (my_time_t) res;
> +
> + return *this;
> +}
you don't need Timestamp class at all, nor any changes here.
>
> bool Temporal::add_nanoseconds_with_round(THD *thd, int *warn,
> date_conv_mode_t mode,
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 8f42b18c176..919aae6858f 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -8030,6 +8032,34 @@ opt_account_option:
> {
> Lex->account_options.account_locked= ACCOUNTLOCK_UNLOCKED;
> }
> + | PASSWORD_SYM EXPIRE_SYM opt_password_expire
> + { }
> + ;
> +opt_password_expire:
> + /* Nothing */
> + {
> + Lex->account_options.password_expire= PASSWORD_EXPIRE_NOW;
> + }
> + | NEVER_SYM
> + {
> + Lex->account_options.password_expire= PASSWORD_EXPIRE_NEVER;
> + }
> + | DEFAULT
> + {
> + Lex->account_options.password_expire= PASSWORD_EXPIRE_DEFAULT;
> + }
> + | INTERVAL_SYM real_ulong_num DAY_SYM
use NUM instead of real_ulong_num it'll make a number within UINT_MAX32
range which is what you need here anyway,
no reason to limit it to UINT_MAX16.
> + {
> + if (unlikely($2 == 0 || $2 > UINT_MAX16))
> + {
> + char num[MAX_BIGINT_WIDTH + 1];
> + my_snprintf(num, sizeof(num), "%lu", $2);
> + my_yyabort_error((ER_WRONG_VALUE, MYF(0), "DAY", num));
> + }
> +
> + Lex->account_options.password_expire= PASSWORD_EXPIRE_INTERVAL;
> + Lex->account_options.num_expiration_days= $2;
> + }
> ;
>
> ev_alter_on_schedule_completion:
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> index 37b3d65e20a..ff521c10d9b 100644
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -1507,6 +1507,25 @@ static Sys_var_ulong Sys_max_connections(
> DEFAULT(MAX_CONNECTIONS_DEFAULT), BLOCK_SIZE(1), NO_MUTEX_GUARD,
> NOT_IN_BINLOG, ON_CHECK(0), ON_UPDATE(fix_max_connections));
>
> +static Sys_var_ulong Sys_default_password_lifetime(
> + "default_password_lifetime",
> + "This defines the global password expiration policy. Defaults to 0, "
> + "meaning automatic password expiration is disabled. If the value is a "
> + "positive integer N, the passwords must be changed every N days. This "
> + "behavior can be overriden using the password expiration options in "
> + "ALTER USER.",
Don't say "Defaults to 0", Just "0 means password expiration is disabled".
> + GLOBAL_VAR(default_password_lifetime), CMD_LINE(REQUIRED_ARG),
> + VALID_RANGE(0, UINT_MAX), DEFAULT(DEFAULT_PASSWORD_LIFETIME),
> + BLOCK_SIZE(1));
> +
> +static Sys_var_mybool Sys_disconnect_on_expired_password(
> + "disconnect_on_expired_password",
> + "This variable controls how the server handles clients with expired "
> + "passwords. If enabled, the server disconnects the client, otherwise "
> + "the server puts the client in sandbox mode.",
Not exactly. "How the server handles clients _that don't know about the
sandbox mode_"
> + GLOBAL_VAR(disconnect_on_expired_password), CMD_LINE(OPT_ARG),
> + DEFAULT(FALSE));
> +
> static Sys_var_ulong Sys_max_connect_errors(
> "max_connect_errors",
> "If there is more than this number of interrupted connections from "
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx
