maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

Hi, Serg

On Tue, Sep 7, 2021 at 7:22 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Oleksandr!
>
> you forgot to check in the result file
>
> On Sep 07, Oleksandr Byelkin wrote:
> > revision-id: ca4ef7185da (mariadb-10.6.1-79-gca4ef7185da)
> > parent(s): 271afbc88e4
> > author: Oleksandr Byelkin
> > committer: Oleksandr Byelkin
> > timestamp: 2021-09-07 14:35:05 +0200
> > message:
> >
> > MDEV-9245 password "reuse prevention" validation plugin
> >
> > diff --git a/plugin/reuse_password_check/CMakeLists.txt
> b/plugin/reuse_password_check/CMakeLists.txt
> > --- /dev/null
> > +++ b/plugin/reuse_password_check/CMakeLists.txt
> > @@ -0,0 +1,4 @@
> > +
> > +ADD_DEFINITIONS(-DMYSQL_SERVER)
>
> as I wrote to HF, this should not be needed.
>

So far it is needed, otherwise I got this:
plugin/password_reuse_check/password_reuse_check.c: In function ‘validate’:
plugin/password_reuse_check/password_reuse_check.c:170:7: error: implicit
declaration of function ‘mysql_real_connect_local’; did you mean
‘mysql_real_connect_cont’? [-Werror=implicit-function-declaration]
  170 |   if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL)
      |       ^~~~~~~~~~~~~~~~~~~~~~~~
      |       mysql_real_connect_cont
plugin/password_reuse_check/password_reuse_check.c:170:60: error:
comparison between pointer and integer [-Werror]
  170 |   if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL)
      |                                                            ^~
cc1: all warnings being treated as errors
[1145/1618] Building CXX object
plugin...ocket.dir/handlersocket/database.cpp.o
ninja: build stopped: subcommand failed.


>
> > +
> > +MYSQL_ADD_PLUGIN(reuse_password_check reuse_password_check.c
> MODULE_ONLY)
>
> Unless you have a reason why this must be "MODULE_ONLY", please remove
> MODULE_ONLY and try to compile and test with
> -DPLUGIN_REUSE_PASSWORD_CHECK=STATIC
>
> OK

> also, I'd suggest "password_reuse_check" not "reuse_password_check".
>
OK

>
> > diff --git a/plugin/reuse_password_check/reuse_password_check.c
> b/plugin/reuse_password_check/reuse_password_check.c
> > --- /dev/null
> > +++ b/plugin/reuse_password_check/reuse_password_check.c
> > @@ -0,0 +1,236 @@
> > +/* Copyright (c) 2021, Oleksandr Byelkin and MariaDB
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; version 2 of the License.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, write to the Free Software
> > +   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1335  USA */
> > +
> > +#include <my_config.h>
> > +#include <my_global.h>
> > +#include <my_base.h>
> > +#include <mysql/plugin_password_validation.h>
> > +#include <mysql.h>
>
> only plugin_password_validation.h should be needed here.
>

OK, I removed unlikely() and added only needed defines with comment for
what it is needed


>
> > +
> > +
> > +#include <mysqld_error.h>
>
> this might be needed all right
>
> > +#include <mysql/service_sha2.h>
>
> but this isn't needed either
>
> OK

> > +
> > +#define HISTORY_DB_NAME "reuse_password_check_history"
> > +
> > +#define SQL_BUFF_LEN 2048
> > +
> > +// 0 - unlimited, otherwise number of days to check
> > +static unsigned interval= 0;
> > +
> > +// helping string for bin_to_hex512
> > +static char digits[]= "0123456789ABCDEF";
> > +
> > +
> > +/**
> > +  Convert string of 512 bits (64 bytes) to hex representation
> > +
> > +  @param to              pointer to the result puffer
> > +                         (should be at least 64*2 bytes)
> > +  @param str             pointer to 512 bits (64 bytes string)
> > +*/
> > +
> > +static void bin_to_hex512(char *to, const unsigned char *str)
> > +{
> > +  const unsigned char *str_end= str + (512/8);
> > +  for (; str != str_end; ++str)
> > +  {
> > +    *to++= digits[((uchar) *str) >> 4];
> > +    *to++= digits[((uchar) *str) & 0x0F];
> > +  }
> > +}
> > +
> > +
> > +/**
> > +  Send SQL error as ER_UNKNOWN_ERROR for information
> > +
> > +  @param mysql           Connection handler
> > +*/
> > +
> > +static void report_sql_error(MYSQL *mysql)
> > +{
> > +  my_printf_error(ER_UNKNOWN_ERROR, "[%d] %s", ME_WARNING,
> > +                  mysql_errno(mysql), mysql_error(mysql));
>
> may be include plugin name here?
>
done

>
> > +}
> > +
> > +
> > +/**
> > +  Create the history of passwords table for this plugin.
> > +
> > +  @param mysql           Connection handler
> > +
> > +  @retval 1 - Error
> > +  @retval 0 - OK
> > +*/
> > +
> > +static int create_table(MYSQL *mysql)
> > +{
> > +  if (mysql_real_query(mysql,
> > +        // 512/8 = 64
> > +        STRING_WITH_LEN("CREATE TABLE mysql." HISTORY_DB_NAME
> > +                        " ( hash binary(64),"
> > +                        " time timestamp,"
>
> make it "default current_timestamp", just to be explicit.
> even though it's implicitly default current_timestamp already.
>
done

>
> > +                        " primary key (hash), index tm (time) )"
> > +                        " ENGINE=Aria")))
> > +  {
> > +    report_sql_error(mysql);
> > +    return 1;
> > +  }
> > +  return 0;
> > +}
> > +
> > +
> > +/**
> > +  Run this query and create table if needed
> > +
> > +  @param mysql           Connection handler
> > +  @param query           The query to run
> > +  @param len             length of the query text
> > +
> > +  @retval 1 - Error
> > +  @retval 0 - OK
> > +*/
> > +
> > +static int run_query_with_table_creation(MYSQL *mysql, const char
> *query,
> > +                                         size_t len)
> > +{
> > +  if (unlikely(mysql_real_query(mysql, query, len)))
> > +  {
> > +    unsigned int rc= mysql_errno(mysql);
> > +    if (rc != ER_NO_SUCH_TABLE)
> > +    {
> > +      // suppress this error in case of try to add the same password
> twice
> > +      if (rc != ER_DUP_ENTRY)
> > +        report_sql_error(mysql);
> > +      return 1;
> > +    }
> > +    if (create_table(mysql))
> > +      return 1;
> > +    if (unlikely(mysql_real_query(mysql, query, len)))
> > +    {
> > +      report_sql_error(mysql);
> > +      return 1;
> > +    }
> > +  }
> > +  return 0;
> > +}
> > +
> > +
> > +/**
> > +  Password validator
> > +
> > +  @param username        User name (part of whole login name)
> > +  @param password        Password to validate
> > +  @param hostname        Host name (part of whole login name)
> > +
> > +  @retval 1 - Password is not OK or an error happened
> > +  @retval 0 - Password is OK
> > +*/
> > +
> > +static int validate(const MYSQL_CONST_LEX_STRING *username,
> > +                    const MYSQL_CONST_LEX_STRING *password,
> > +                    const MYSQL_CONST_LEX_STRING *hostname)
> > +{
> > +  MYSQL *mysql= NULL;
> > +  size_t key_len= username->length + password->length +
> hostname->length;
> > +  size_t buff_len= (key_len > SQL_BUFF_LEN ? key_len : SQL_BUFF_LEN);
> > +  size_t len;
> > +  char *buff= malloc(buff_len);
> > +  unsigned char hash[512/8];
> > +  char escaped_hash[512/8*2 + 1];
> > +  if (!buff)
> > +    return 1;
> > +
> > +  mysql= mysql_init(NULL);
> > +  if (!mysql)
> > +  {
> > +    free(buff);
> > +    return 1;
> > +  }
> > +
> > +  memcpy(buff, hostname->str, hostname->length);
> > +  memcpy(buff + hostname->length, username->str, username->length);
> > +  memcpy(buff + hostname->length + username->length, password->str,
> > +          password->length);
> > +  buff[key_len]= 0;
> > +  bzero(hash, sizeof(hash));
> > +  my_sha512(hash, buff, key_len);
> > +  if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL)
> > +    goto sql_error;
> > +
> > +  if (interval)
> > +  {
> > +    // trim the table
> > +    len= snprintf(buff, buff_len,
> > +                  "DELETE FROM mysql." HISTORY_DB_NAME
> > +                  " WHERE time < DATE_SUB(NOW(), interval %d day)",
> > +                  interval);
> > +    if (unlikely(run_query_with_table_creation(mysql, buff, len)))
> > +      goto sql_error;
> > +  }
> > +
> > +  bin_to_hex512(escaped_hash, hash);
> > +  escaped_hash[512/8*2]= '\0';
> > +  len= snprintf(buff, buff_len,
> > +                "INSERT INTO mysql." HISTORY_DB_NAME "(hash) "
> > +                "values (x'%s')",
> > +                escaped_hash);
> > +  if (unlikely(run_query_with_table_creation(mysql, buff, len)))
> > +    goto sql_error;
> > +
> > +  free(buff);
> > +  mysql_close(mysql);
> > +  return 0; // OK
> > +
> > +sql_error:
> > +  free(buff);
> > +  if (mysql)
> > +    mysql_close(mysql);
> > +  return 1; // Error
> > +}
> > +
> > +static MYSQL_SYSVAR_UINT(interval, interval, PLUGIN_VAR_RQCMDARG,
> > +  "How old (in days) passwords to check (0 means unlimited)", NULL,
> NULL,
>
> better "Password history retention period in days (0 means unlimited)"
>
OK

>
> >
> > +  0, 0, 365*100, 1);
> > +
> > +
> > +static struct st_mysql_sys_var* sysvars[]= {
> > +  MYSQL_SYSVAR(interval),
> > +  NULL
> > +};
> > +
> > +static struct st_mariadb_password_validation info=
> > +{
> > +  MariaDB_PASSWORD_VALIDATION_INTERFACE_VERSION,
> > +  validate
> > +};
> > +
> > +maria_declare_plugin(simple_password_check)
>
> ^^^ must be the same name as in CMakeLists.txt,
> otherwise static linking won't work.
>
fixed

>
> > +{
> > +  MariaDB_PASSWORD_VALIDATION_PLUGIN,
> > +  &info,
> > +  "reuse_password_check",
> > +  "Oleksandr Byelkin",
> > +  "Reusage of password check",
>
> Better "Password reuse check"
> or, as it's a description, "Prevent password reuse"
>
done

>
> > +  PLUGIN_LICENSE_GPL,
> > +  NULL,
> > +  NULL,
> > +  0x0100,
> > +  NULL,
> > +  sysvars,
> > +  "1.0",
> > +  MariaDB_PLUGIN_MATURITY_ALPHA
> > +}
> > +maria_declare_plugin_end;
> >
> > diff --git a/mysql-test/suite/plugins/t/reuse_password_check.test
> b/mysql-test/suite/plugins/t/reuse_password_check.test
> > --- /dev/null
> > +++ b/mysql-test/suite/plugins/t/reuse_password_check.test
> > @@ -0,0 +1,58 @@
> > +--source include/not_embedded.inc
> > +
> > +if (!$REUSE_PASSWORD_CHECK_SO) {
> > +  skip No REUSE_PASSWORD_CHECK plugin;
> > +}
> > +
> > +install soname "reuse_password_check";
> > +
> > +set @save_reuse_password_check_interval=
> @@reuse_password_check_interval;
> > +
> > +set global reuse_password_check_interval= 0;
> > +
> > +--echo # Default value (sould be unlimited i.e. 0)
> > +SHOW GLOBAL VARIABLES like "reuse_password_check%";
> > +
> > +--echo # insert user
> > +grant select on *.* to user_name@localhost identified by 'test_pwd';
>
> what about sql mode that doesn't allow to create users with grant?
> I don't see where you've disabled that.
>

good question; I have no idea why it works, now I am trying to figure it
out.


> > +
> > +#--error ER_NOT_VALID_PASSWORD
> > +#grant select on *.* to user_name@localhost identified by 'test_pwd';
> > +#show warnings;
>
> why is that commented out?
>
fixed

>
> > +
> > +--error ER_CANNOT_USER
> > +alter user user_name@localhost identified by 'test_pwd';
> > +show warnings;
> > +
> > +# Plugin does not work for it
> > +#--error ER_NOT_VALID_PASSWORD
> > +#SET PASSWORD FOR user_name@localhost = PASSWORD('test_pwd');
> > +
> > +--echo # check exparation
> > +
> > +set global reuse_password_check_interval= 10;
> > +
> > +--error ER_CANNOT_USER
> > +alter user user_name@localhost identified by 'test_pwd';
> > +show warnings;
> > +select hex(hash) from mysql.reuse_password_check_history;
> > +
> > +--echo # emulate old password
> > +update mysql.reuse_password_check_history set time= date_sub(now(),
> interval
> > +11 day);
> > +
> > +alter user user_name@localhost identified by 'test_pwd';
> > +show warnings;
> > +
> > +drop user user_name@localhost;
> > +
> > +show create table mysql.reuse_password_check_history;
> > +select count(*) from mysql.reuse_password_check_history;
> > +
> > +drop table mysql.reuse_password_check_history;
> > +
> > +set @save_reuse_password_check_interval=
> @@reuse_password_check_interval;
> > +
> > +set global reuse_password_check_interval=
> @save_reuse_password_check_interval;
> > +
> > +uninstall plugin reuse_password_check;
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>