- developers - lists.mariadb.org

Re: [Maria-developers] 02587f61f40: MDEV-13095 Implement User Account locking
by Sergei Golubchik 21 Jan '19

21 Jan '19

Hi, Robert! On Jan 20, Robert Bindar wrote: > revision-id: 02587f61f40 (mariadb-10.4.1-100-g02587f61f40) > parent(s): 937c90ce2d3 > author: Robert Bindar <robert(a)mariadb.org> > committer: Sergei Golubchik <serg(a)mariadb.com> > 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(a)mariadb.org

2 2

[Maria-developers] Fwd: ce4f8955d8d: MDEV-17082 Application period tables: CREATE
by Nikita Malyavin 17 Jan '19

17 Jan '19

---------- Forwarded message --------- From: Nikita Malyavin <nikitamalyavin(a)gmail.com> Date: пт, 18 янв. 2019 г. в 01:46 Subject: Re: ce4f8955d8d: MDEV-17082 Application period tables: CREATE To: Sergei Golubchik <serg(a)mariadb.org> Hi, Sergei! > > +# SQL17 p.832 > > +# Let IDCN be an implementation-dependent <constraint name> that is not > > +# equivalent to the <constraint name> of any table constraint descriptor > > +# included in S. > > Not enough context. It was quite difficult to find this quote. > > When you're quoting the standard, please, specify the part name and the > section name. And there's no need to specify a page. For example: > > SQL2016, Part 2, 11.27 <add table period definition>, General Rules, 2)a) > Fixed, as well as all the rest cites > > + Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1), > > We have a macro for that, use > > Table_period_info(STRING_WITH_LEN("SYSTEM_TIME")) > ✅ > > + then no promition is done. > > promotion ✅ > > + Table_period_info& get_table_period_info() > > + { > > + return create_info.period_info; > > + } > > really? :) you've introduced a new method get_table_period_info > only to use it two lines below and nowhere else? > Not really, of course. But agree, it's not worth to have such a shortcut. > > + > > + int add_period(Lex_ident name, Lex_ident_sys_st start, Lex_ident_sys_st end) > > + { > > + Table_period_info &info= get_table_period_info(); > > + if (info.is_set()) > > + { > > + my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application"); > > + return 1; > > + } > > + info.set_period(start, end); > > + info.name= name; > > + > > + Virtual_column_info *constr= new Virtual_column_info(); > > + constr->expr= lt_creator.create(thd, create_item_ident_nosp(thd, &start), > > + create_item_ident_nosp(thd, &end)); > > + add_constraint(&null_clex_str, constr, false); > > + return 0; > > + } > > + > > sp_package *get_sp_package() const; > > > > /** > > diff --git a/sql/table.cc b/sql/table.cc > > index ab1a04ccae4..87fa1ba4d61 100644 > > --- a/sql/table.cc > > +++ b/sql/table.cc > > @@ -1228,6 +1228,14 @@ static const Type_handler *old_frm_type_handler(uint pack_flag, > > } > > > > > > +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period, > > + const uchar *data) > > +{ > > + period.start_fieldno= uint2korr(data); > > + period.end_fieldno= uint2korr(data + sizeof(uint16)); > > not "sizeof(uint16)" but 2. I don't think it's guaranteed that > sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes, > so data + 2 is always correct. > Yes, it's correct, and I've fixed it, but it wasn't about coorrectness -- but readability. Now what 4, or what is 8 -- is the question that the reader could сonsider. This numbers look magical. With using sizeof(uint16) the one could at least understand, that this is about several double-byte words read. The another problem is that sizeof(uint16) is already used in system versioning, few lines below, so the resulting code will look patchy here. > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "VIRTUAL or GENERATED"); > > just use "GENERATED", no English in the error message parameters, please. > VIRTUAL/GENERATED? What'd You say? > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "NULL"); > > why is that? it's unrelated and inconsistent with cases like > > create table t1 (a int null, primary key (a)); > > where a column is silently created NOT NULL. > So, you should ignore explicit NULL and create columns NOT NULL. > > If you want to change this, it should be consistent in all cases. > create an MDEV, suggest there that the statement above should be an error. > but I don't think it's worth spending time on now. > Ok, I understand. Fixed. > > + else if (f->flags & VERS_SYSTEM_FIELD) > > + { > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, > > + f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END"); > > No need to have a special case for that. Use the same error as for vcol_info: > > if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD) > { > my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > f->field_name.str, "GENERATED ALWAYS AS"); > Looks not very much user-friendly I think. Let's leave the verbosity as it was > What does this error have to do with MDEV-16976? > SQL standard clearly says (part 2, 11.3 <table definition>, syntax rules, 2)d) > > d) No <column name> in any <column definition> shall be equivalent to PN. > > (here PN is "period name"). So MDEV-16976 or not, a period name should > never match a column name. > I was thinking to implement it through virtual column. Then this check will become redundant, and we'll get duplicate field error > > + eng "Temporary tables with application period not allowed" > > "application time period" > ✅ > > + > > +ER_PERIOD_FIELD_NOT_FOUND > > + eng "Field %`s in PERIOD FOR %`s is not found in table" > > I don't think this needs a new error message, just use ER_BAD_FIELD_ERROR > ✅ > > + > > +ER_PERIOD_TYPES_MISMATCH > > + eng "PERIOD FOR %`s fields types mismatch" > > + > > +ER_PERIOD_MAX_COUNT_EXCEEDED > > better say ER_MORE_THAN_ONE_PERIOD or something... > "max count exceeded" suggests there is some not-trivial > (and possibly variable) max count > > Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc. > "already defined" looks like a good one. > > And I've already commented (in another patch) on using English > words as arguments - it will look like a bug when the error message is > translated > I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods designed in the way to support a possibility of several periods definition, so i like to treat it as "N periods max supported, but now N=1" > > + eng "cannot specify more than one %s-time period" > > + > > +ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD > > + eng "Could not specify name %`s for field. It is already used by period." > > I don't think this needs a new error message, just use ER_DUP_FIELDNAME > ✅ > > + > > +ER_PERIOD_FIELD_WRONG_ATTRIBUTES > > + eng "Period field %`s cannot be %s" > > + > > +ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD > > + eng "Field %`s is specified for more than one period" > > This looks unused ✅ Sincerely Nikita Malyavin

1 0

[Maria-developers] Please review MDEV-18195 ASAN use-after-poison in my_strcasecmp_utf8 / Item::eq upon prepared statement with ORDER BY NAME_CONST
by Alexander Barkov 17 Jan '19

17 Jan '19

Hi Sanja, Please review a patch for MDEV-18195. Thanks.

1 0

Re: [Maria-developers] d08031dfae2: MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK
by Sergei Golubchik 17 Jan '19

17 Jan '19

Hi, Thirunarayanan! On Jan 16, Thirunarayanan Balathandayuthapani wrote: > revision-id: d08031dfae2 (mariadb-10.0.37-45-gd08031dfae2) > parent(s): 12f362c3338 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2019-01-16 15:46:28 +0530 > message: > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK > > Problem: > ======== > Server fails to notify the engine by not setting the ADD_PK_INDEX and > DROP_PK_INDEX When there is a > i) Change in candidate for primary key. > ii) New candidate for primary key. > > Fix: > ==== > Server sets the ADD_PK_INDEX and DROP_PK_INDEX while doing alter for the > above problematic case. Sorry, that's way too complex. I mean the original code with candidate_key_count, is_candidate_key, and all that. And you're making it more complex. If a unique key is promoted to a primary key, this unique key is always be the first key in the table. So what you need to check is * if the old table didn't have a primary key and the new table has it, then a primary key was added. As easy as if (table->s->primary_key >= MAX_KEY && altered_table->s->primary_key != MAX_KEY) * The opposite case, a primary key was removed * if both tables had a primary key, but the key number 0 in the new table was not key number 0 in the old table - a primary key was changed: if (key_info->usable_key_parts) ... And that should cover all possible cases, as far as I can see. May need some tweaking for key_info->usable_key_parts of newly added indexes, I'm not quite sure what number they get. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] (no subject)
by varsha kukreja 12 Jan '19

12 Jan '19

Hey this is Varsha Kukreaj a third year student from SPITMumbai ..I would like to contribute to the open source community .Can someone help me get started?..I have majorly worked on backend on 2 college sponsored projects and working currently on a project by Government Organization. I have fair knowledge in Javascript, NodeJs, Typescript, Spring Boot, Laravel , Docker and apache thrift. It woukd be great if could if someone could help me get started

2 1

Re: [Maria-developers] MDEV 18092
by Anel Husakovic 10 Jan '19

10 Jan '19

Hi Alexander, I was on vacation too, just came back, thank you for teaching me. Indeed it works, here is a patch with quicker solution and the test case *https://github.com/an3l/server/commit/252ca79493e18fa8fd364992f5022af8a57b79d5 <https://github.com/an3l/server/commit/252ca79493e18fa8fd364992f5022af8a57b7…>* I added the PR also, so if something needs to be improved please let me know. In addition I will try later also to add `store_schema_params()` function to handler. Thanks, Anel On Mon, Dec 31, 2018 at 7:02 AM Alexander Barkov <bar(a)mariadb.com> wrote: > Hi Anel, > > > > On 12/28/2018 02:12 PM, Anel Husakovic wrote: > > Hi Alexander, > > my name is Anel, and I'm working for Foundation. > > I would like to take/ https://jira.mariadb.org/browse/MDEV-18092/ if > > you allow me, and would appreciate any hint in order to learn more about > > this codebase :). > > Nice to meet you! Thanks for working on this! > > > I debugged already a bit and have seen that there is parser error, > > where /*parser_state = m_buf = 0x7fff6403ec90 "CREATE DEFINER=\"\" > > PACKAGE BODY \"db1\".\"employee_tools\" BEGIN END"/ > > is raised because the incorrect body of /Sp_handler_package_spec > > /and/or /Sp_handler_package_body /I suppose ? > > (/empty_body_lex_cstring/() for *Sp_handler_package_spec *and > > *Sp_handler_package_body*), > > > > I tried to change the body of the above methods, but either my syntax is > > not good ("/AS BEGIN NULL; END/") or this is incorrect conclusion :). > > > It seems Sp_handler::sp_load_for_information_schema() > creates a wrong CREATE statement that cannot be parsed. > > This should be fixed. sp_load_for_information_schema() is > supposed to build a simplified CREATE statement that can be parsed. > > > But for exactly this problem, it's probably not necessarily. > > > I think store_schema_params() should just skip PACKAGE and PACKAGE BODY > records quickly. > > These stored objects do not have any parameters or return values > (only procedures and functions have). > So no needs to build a CREATE statement and parse it: > this won't give us any data useful for INFORMATION_SCHEMA.PARAMETERS > anyway. > > > > Ideally, store_schema_params() should be turned into virtual > methods in Sp_handler, doing: > - empty job for PACKAGE and PACKAGE BODY > - adding a return data type record for FUNCTION > - adding parameters for FUNCTION and PROCEDURE > > > A quicker solution would be just to change this block: > > if (!sph) > DBUG_RETURN(0); > > > to something like this: > > if (!sph || > sph->type() == TYPE_ENUM_PACKAGE || > sph->type() == TYPE_ENUM_PACKAGE_BODY) > DBUG_RETURN(0); > > > <skip> > > > Thanks in advance and happy holidays, > > Thanks! Same to you! > > > I'll be on holidays until the 10th of January. > Sorry for possible slow replies during these days. > > > > > > Anel >

2 1

Re: [Maria-developers] Expiration of user passwords
by Sergei Golubchik 08 Jan '19

08 Jan '19

Hi, Robert! On Jan 07, Robert Bindar wrote: > Hi! > > With this email I intend to kick off a debate on the design of > password expiration in MariaDB. > Given this feature may impact the end user experience, I would love if > the experienced folks here will step in and help us come up with the > best approach possible. Random thoughts are highly appreciated as > well. > > *-- Some thoughts* > > I have been trying to wrap my head around how password management > should look like in MariaDB, my conclusion so far is that the less > intrusive it is the greater are our chances for the common user to > like it. > > MySQL released the first version of password expiration in 5.6.6 > (~2012, didn't change much since then). From my research, it seems the > common user was caught off-guard by this feature, got annoyed by its > restrictiveness and chose to disable it permanently. I believe we > should avoid this situation as much as possible. > I can see the whole story going, nevertheless, in a different > direction for our business clients. I suspect the password expiration is more useful in satisfying various enterprise security checklists and requirements than in actually improving security. For example, https://cryptosmith.com/password-sanity/exp-harmful/ http://blog.axantum.com/2009/12/password-expiration-is-meaningless.html etc, you can google up as many as you'd like > **-- How MySQL does it* > > This is a short summary of what's in the MySQL docs about the password > management system. I'll have it here so people that are unaware of how > this feature works in MySQL are up to speed. > > MySQL introduced 3 new columns in mysql.user: > > /** *//*password_expired* (Y/N)// > // /** If 'Y', the user's password will be considered expired */// > //** **password_last_changed* (timestamp)// > // /** The datetime when the user changed the password last time */// > //** **password_lifetime* (smallint)// > // /** The number of days until the password expires (it's not clear whether// > // the days are counted since password_last_changed or since when the// > // expiration statement was executed) *// > > Given we support MySQL 5.7, we have to have them as well I believe. > > The /CREATE USER/ and /ALTER USER/ statements were extended as follows:** > > *** /[ALTER|CREATE] USER user@host PASSWORD EXPIRE [option];/ > > - If no option is specified, the password is expired with immediate effect. > - If option is /DEFAULT/, the /*password_lifetime*/ column is set to the > number of days specified in the */default_password_lifetime/* system var. > - If option is /NEVER/, the user password_lifetime column is set to 0, the > password is never expired for this account. > - Option can also be /INTERVAL n DAY/, in which case password_lifetime > is set to /'n'/ (see column definition above). > > *Password expiration behavior* > MySQL controls the effect of an expired password with the > */disconnect_on_expired_password/* system var. > For active sessions, when a password is expired, the server either > disconnects the client or puts it in a sandbox mode. > New client connections can specify an option so the server knows not to > disconnect them, otherwise the new connection is handled according to the > system var above. > In sandbox mode, the client is able to execute only /SET PASSWORD/, > /ALTER USER/ and /SET/ statements for vars. > > *Other additions* > - C library API for specifying disconnect/sandbox option this should be already done, CONC-314. > - mysql client new */--connect-expired-password/* option > * > **-- Things to improve* > > Below are a few points I found so far that I believe should be discussed > before moving on to a prototype implementation: > > *1.* Syntax wise > > Do we want to follow the path of MySQL and extend the /ALTER USER/ and > /CREATE USER/ statements for password expiration support? > *Opinion**|* The /ALTER/CREATE USER/ statements seem to be doing quite a lot > of things already. Introducing a new statement such as > > * * */PASSWORD EXPIRE user@host [option];/ > > may be worth exploring. I think ALTER/CREATE USER is perfectly expected to set or modify various properties of an account. So it's quite fitting to use it here. "PASSWORD EXPIRE" looks strange. First, SQL statements start with a verb. Second "PASSWORD EXPIRE" (or "EXPIRE PASSWORD", if you'd like) means to expire it now, not somewhere in the future. And anyway, I don't see why one would need a new statement to modify just one property of an account, if all other properties are modified with ALTER USER. > *2.* Password Expiration behavior > > Is it actually a good idea to overengineer the feature with > sandbox/disconnect modes? > *Opinion|* Quite a lot of people are complaining on online platforms > that their SQL statements stopped working suddenly or that they can't > connect to the MySQL server anymore. What about we just ask for a new > password when the next login is attempted? We have ways to disconnect > a client connection anyway if needed. A good choice for the default > behavior of this feature might also do the trick. I'd disable password expiration by default, see urls above :) > *3.* Having passwords expire on a specific date > > MySQL introduced the concept of expiring the passwords after /N/ days. > I can see a good use-case in wanting to expire an account's password > on a specific date/timestamp as well: > > /ALTER USER user1@host PASSWORD EXPIRE DATE tstamp;/ > or > /PASSWORD EXPIRE user1@host DATE tstamp;/ > > Example use cases: > * Let's assume company X has a security sanity check on 10th of November > and wants everybody to update their passwords that day. > * Company Y wants the password expiration event to happen before the > working day starts and thus not interrupt their employees during their > productive hours. Now, that is overengineering, in my opinion :) Let's postpone it until users will be requesting it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 82d7e2094c8: MDEV-7598 Lock user after too many password errors
by Sergei Golubchik 08 Jan '19

08 Jan '19

Hi, Vladislav! On Jan 04, Vladislav Vaintroub wrote: > revision-id: 82d7e2094c8 (mariadb-10.4.1-41-g82d7e2094c8) > parent(s): c4ec6bb69ec > author: Vladislav Vaintroub <wlad(a)mariadb.com> > committer: Vladislav Vaintroub <wlad(a)mariadb.com> > timestamp: 2019-01-04 10:42:05 +0100 > message: > > MDEV-7598 Lock user after too many password errors COM_CHANGE_USER has its own blocking, that works differently. See change_user_notembedded test. 1. Does it also increment acl_user->password_errors counter now? 2. Should COM_CHANGE_USER be changed to behave more like connect? even the answer to the second question is "no", please, add tests for change_user to document the behavior from the first question. > diff --git a/mysql-test/main/max_password_errors.test b/mysql-test/main/max_password_errors.test > new file mode 100644 > index 00000000000..fbb9e699b49 > --- /dev/null > +++ b/mysql-test/main/max_password_errors.test > @@ -0,0 +1,46 @@ > +--source include/not_embedded.inc > +set @old_max_password_errors=@@max_password_errors; > +set global max_password_errors=2; > +create user u identified by 'good_pass'; > + > +# Test that user is blocked after 'max_password_errors' bad passwords > +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT > +error ER_ACCESS_DENIED_ERROR; > +connect(con1, localhost, u, bas_pass); > +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT > +error ER_ACCESS_DENIED_ERROR; > +connect (con1, localhost, u, bad_pass); > +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT > +error ER_USER_IS_BLOCKED; > +connect(con1, localhost, u, good_pass); add another bad_pass connect here, please, to show what error one will get (ER_ACCESS_DENIED_ERROR or ER_USER_IS_BLOCKED). > +# Test that FLUSH PRIVILEGES clears the error > +FLUSH PRIVILEGES; > +connect (con1, localhost, u, good_pass); > +disconnect con1; > diff --git a/mysql-test/main/max_password_errors_auth_named_pipe.opt b/mysql-test/main/max_password_errors_auth_named_pipe.opt I'd better put the test into the plugins suite. That's where all similar tests are. > diff --git a/mysql-test/main/max_password_errors_auth_socket.opt b/mysql-test/main/max_password_errors_auth_socket.opt same > diff --git a/mysql-test/main/max_password_errors_auth_socket.test b/mysql-test/main/max_password_errors_auth_socket.test > new file mode 100644 > index 00000000000..4f31927b02d > --- /dev/null > +++ b/mysql-test/main/max_password_errors_auth_socket.test > @@ -0,0 +1,20 @@ > +# Tests that max_password_errors has no effect on login errors with > +# passwordless plugins (Windows version / auth_named_pipe) > + > +--source include/not_embedded.inc > +--source include/have_unix_socket.inc > + > +set @old_max_password_errors=@@max_password_errors; > +create user nosuchuser identified with 'unix_socket'; Technically, you need to test that $USER != nosuchuser. Otherwise someone someday will surely run it from a nosuchuser account :) May be just include have_unix_socket.inc after you created nosuchuser, it should do the trick. > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > index 8faf332a7dd..3255927d4a6 100644 > --- a/mysql-test/main/mysqld--help.result > +++ b/mysql-test/main/mysqld--help.result > @@ -546,6 +546,11 @@ The following specify which files/extra groups are read (specified before remain > The maximum BLOB length to send to server from > mysql_send_long_data API. Deprecated option; use > max_allowed_packet instead. > + --max-password-errors=# > + 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.Value 0 > + disables user blocking 1. Space before "Value" 2. Why 0 disables blocking? max_connect_errors doesn't have a special 0 value. > --max-prepared-stmt-count=# > Maximum number of prepared statements in the server > --max-recursive-iterations[=#] > @@ -1518,6 +1523,7 @@ max-heap-table-size 16777216 > max-join-size 18446744073709551615 > max-length-for-sort-data 1024 > max-long-data-size 16777216 > +max-password-errors 0 > max-prepared-stmt-count 16382 > max-recursive-iterations 18446744073709551615 see? max-recursive-iterations doesn't have a special 0 value, it simply uses maxint. So does max-join-size. > max-relay-log-size 1073741824 > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 85b54009219..95f92688677 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -12323,6 +12324,23 @@ static bool send_plugin_request_packet(MPVIO_EXT *mpvio, > } > > #ifndef NO_EMBEDDED_ACCESS_CHECKS > + > +/** > + Safeguard to avoid blocking the root, when max_password_errors > + limit is reached. > + > + Currently, we allow password errors for superuser on localhost. > + > + @return true, if password errors should be ignored, and user should not be locked. > +*/ > +static bool ignore_max_password_errors(const ACL_USER *acl_user) > +{ > + const char *host= acl_user->host.hostname; > + return (acl_user->access & SUPER_ACL) > + && (!strcasecmp(host, "localhost") || > + !strcmp(host, "127.0.0.1") || > + !strcmp(host, "::1")); > +} I don't like special cases :( May be after implementing `unix_socket OR mysql_native_password` feature, this special case could be removed? > /** > Finds acl entry in user database for authentication purposes. > > @@ -12341,6 +12359,21 @@ static bool find_mpvio_user(MPVIO_EXT *mpvio) > mysql_mutex_lock(&acl_cache->lock); > > ACL_USER *user= find_user_or_anon(sctx->host, sctx->user, sctx->ip); > + > + if (user && max_password_errors && user->password_errors >= max_password_errors) > + { > + if (ignore_max_password_errors(user)) > + user->password_errors= 0; add a comment here, like // skip a second mutex lock in handle_password_errors() > + else > + { > + mysql_mutex_unlock(&acl_cache->lock); > + my_error(ER_USER_IS_BLOCKED, MYF(0)); > + general_log_print(mpvio->auth_info.thd, COM_CONNECT, > + ER_THD(mpvio->auth_info.thd, ER_USER_IS_BLOCKED)); > + DBUG_RETURN(1); > + } > + } > + > if (user) > mpvio->acl_user= user->copy(mpvio->auth_info.thd->mem_root); > > @@ -13305,6 +13370,8 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len) > break; > case CR_AUTH_USER_CREDENTIALS: > errors.m_authentication= 1; > + if (thd->password && max_password_errors) add && !mpvio->make_it_fail here (see how make_it_fail is set). > + handle_password_errors(acl_user->user.str, acl_user->host.hostname, PASSWD_ERROR_INCREMENT); > break; > case CR_ERROR: > default: Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] Help request: Contribute to MariaDB 10.3 in Debian efforts
by Otto Kekäläinen 07 Jan '19

07 Jan '19

Hello! I've spent the last 3-4 weeks preparing MariaDB 10.3 for Debian. I am almost done, but there are a bunch of smaller issues I need help with. I would be glad for anybody who can look into the issues and give solution ideas - or even file a merge request on Debian's Gitlab instance where the Debian development happens: https://wiki.debian.org/Teams/MySQL/patches Here is the overall tracker for MariadB 10.3 and it shows various issues: https://tracker.debian.org/pkg/mariadb-10.3 Bugs filed against MariaDB in Debian: https://bugs.debian.org/cgi-bin/pkgreport.cgi?repeatmerged=no&src=mariadb-1… Build status in Debian: https://buildd.debian.org/status/package.php?p=mariadb-10.3 Build status on Launchpad (the system Ubuntu uses): https://launchpad.net/~mysql-ubuntu/+archive/ubuntu/mariadb-10.3/+builds?bu… Build/CI status on Debian Salsa (the development system): https://salsa.debian.org/mariadb-team/mariadb-10.3/-/jobs Git repository for the packaging: https://salsa.debian.org/mariadb-team/mariadb-10.3 Please pick somethng that is close to your expertise and help out if you can! :) - Otto

4 5

Re: [Maria-developers] Audit plugin filter expressions.
by Ralf Gebhardt 02 Jan '19

02 Jan '19

And this definition would be used if we are not using tables for users and filters? As with tables we would not have JSON objects for user and role. Alexey Botchkov <holyfoot(a)mariadb.com> schrieb am Mi. 2. Jan. 2019 um 09:33: > Hi, Ralf, Sergei! > > Here below you can see the proposed syntax of filter expressions > for the Audit Plugin. > Similarly to what we see in MySQL, firstly the 'filter'-s are > defined then they are assigned to 'user'-s and 'role'-s. > Role's filters prevail over user's. > > > Toplevel structure is an JSON object with 3 optional keys: > "filters" - an array of 'filter' definitions > "users" - filters assigned to users. > 'roles' - filters assigned to roles. > > Looks like this: > > { > "filters": [ > { > "name" : "all", > "def" : {filter_definition} > }, > { > "name" : "connect_events", > "def" : {filter_definition} > } > ], > "users" : > [ > { > "user" : "root@%", > "filter" : "connect_events" > }, > { > "user" : "hf(a)%.ru", > "filter" : "all" > }, > ], > "roles" : > [ > { > "role" : "sales", > "filter" : "all" > } > ] > } > > User names syntax is same as the used in server. > When looking the 'users' for filter we scan the array and use the first > matching > 'user' and use it's filter. (don't look for other possible matches) > 'role'-s support no wildcards, so are simpler to match. > > > filter_definition: - and array of definitions > filter_array_item [,filter_array_item ...] > > filter_array_item: > filter_name | > simple_filter > > simple_filter: > { > "do_log" : true|false, - an optional key. Supposed to be TRUE if not > specified. > "event_type : [CONNECT, QUERY, TABLE], > "details" : > { > "connect_type" : ["CONNECT, DISCONNECT, CHANGE_USER, SET_ROLE], > "query_type" : [DDL, DML, DCL, SELECT, INSERT ...], > "table_type" : [READ, WRITE], > "query_tables" : ["db.table", ...], > "table_tables" : ["db.table", ...] > } > } > > So examining an event we have the array of simple_filters to > check. If the event matches the simple_filter, we act as the 'do_log" > tells - write it to log or stop looking for match. > If no matches were found, we don't log the event. > > The example of the filter definition: > { > filter_name: "log_ddl_dcl", > filter_def: > { > "event_type": "QUERY", > "details": { "query_type": ["DDL", "DCL"] } > } > } > > { > filter_name: "log_queries_except_test_db", > filter_def: > [ > { > "do_log": FALSE, > "event_type": "QUERY", > "details": { "query_tables": "test.*" } > }, > { > "event_type": "QUERY" > ] > ] > } > > > > That's basically it. > What would be your opinion? :) > > Regards > HF > > -- Ralf Gebhardt, Product Manager MariaDB Corporation Ab | http://www.mariadb.com | m: +49 152-564-823-67 MARIADB OPENWORKS February 25-27, 2019 · NYC · Register Now mariadb.com/openworks <https://mariadb.com/conference/>

1 0