maria-developers team mailing list archive

[<wlad@xxxxxxxxxxx>]

> -----Original Message-----
> From: Sergei Golubchik <serg@xxxxxxxxxxx>
> Sent: Friday, 4 January 2019 15:06
> To: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> Cc: maria-developers@xxxxxxxxxxxxxxxxxxx
> Subject: Re: 82d7e2094c8: MDEV-7598 Lock user after too many password
> errors
>
> Hi, Vladislav!

Hi Sergei,
Thanks for looking!
 
> On Jan 04, Vladislav Vaintroub wrote:
> > revision-id: 82d7e2094c8 (mariadb-10.4.1-41-g82d7e2094c8)
> > parent(s): c4ec6bb69ec
> > author: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> > committer: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> > 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?
Yes, it does, inside acl_authenticate()

> 2. Should COM_CHANGE_USER be changed to behave more like connect?

I would not change existing behaviour of COM_CHANGE_USER . I think it is more or less an obscurity in the client-server protocol,  not very widely used, and for its intended purpose (connection pools), COM_RESET_CONNECTION is a much better candidate.

> even the answer to the second question is "no", please, add tests for
> change_user to document the behavior from the first question.

I added a test for it.
 
...
> > 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.

I moved it to plugins suite.
...
> > +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 :)

Not very likely, but ok, I added the check for that too 😊

> 1. Space before "Value"
> 2. Why 0 disables blocking? max_connect_errors doesn't have a special 0
>    value.

Alright, I removed a special value 0 , and made 1 the minimal value, like max_connect_errors does, so it does not appear different from anything else and does not create any confusion, but let me explain the original intention, which was

1. Zero performance overhead, if the option is not used.
Without special value , there will be overhead in case of invalid password - extra scanning of the acl_users array (under mutex protection).

2. No side-effects, if the option is not used. Technically, even if unlikely, if someone supplies 4294967295 of invalid passwords in a row, the account will be locked. It only takes 50 days, if connection attempt is made every millisecond.
With special value account won't be blocked.


> >  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

> > +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?

Maybe.  when we can be sure that auth_unix_socket or  auth_named_pipe are usable, and working for the root user.

On Windows, named pipe is not enabled by default (and to make it on, I believe we'd also need a better default value for named_pipe_name, so that multiple installations on the same box do not conflict), and auth_named_pipe is not compiled in yet.


> > @@ -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).

I added the condition, even if so far this condition is obsolete. mpvio->make_it_fail only produces CR_ERROR, not CR_AUTH_USER_CREDENTIALS. But l can imagine make_it_fail  could theoretically produce different CR_XXX errors in the future.