maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #02727
Re: Review of 5.2 pluggable-auth tree
Hi, Monty!
See the answers below:
(no reply means "ok, changed")
On Mar 25, Michael Widenius wrote:
> > === modified file 'client/mysql.cc'
> > + /**
> > + An example of mysql_authentication_dialog_ask callback.
> > +
> > + The C function with the name "mysql_authentication_dialog_ask", if exists,
> > + will be used by the "dialog" client authentication plugin when user
> > + input is needed. This function should be of mysql_authentication_dialog_ask_t
> > + type. If the function does not exists, a built-in implementation will be
> > + used.
> > +
> > + @param mysql mysql
> > + @param type type of the input
> > + 1 - normal string input
> > + 2 - password string
> > + @param prompt prompt
> > + @param buf a buffer to store the use input
> > + @param buf_len the length of the buffer
> > +
> > + @retval a pointer to the user input string.
> > + It may be equal to 'buf' or to 'mysql->password'.
> > + In all other cases it is assumed to be an allocated
> > + string, and the "dialog" plugin will free() it.
> > + */
> > + extern "C" char *mysql_authentication_dialog_ask(MYSQL *mysql, int type,
>
> I would prefer to have type as my_bool as there is only two options
> for it and rename it to 'ask_for_password'. This would make the code
> more self_explanatory.
it cannot be my_bool because my_bool is internal MySQL typedef, not part
of the API.
and it's not really "ask_for_password" it's just "ask" for anything.
Password, your dog's name, the mainden name of your neighbour's wife -
whatever the plugin wants to ask for.
> > + const char *prompt,
> > + char *buf, int buf_len)
> > + {
> > + int ch;
> > + char *s=buf, *end=buf+buf_len-1;
> > +
> > + fputs("[mysql] ", stdout);
>
> > === modified file 'include/my_global.h'
> > *** include/my_global.h 2009-12-03 11:19:05 +0000
> > --- include/my_global.h 2010-02-19 08:18:09 +0000
> > *************** int __void__;
> > *** 578,583 ****
> > --- 578,591 ----
> > #define IF_VALGRIND(A,B) (B)
> > #endif
> >
> > + #ifdef _WIN32
>
> In the rest of my_global.h we use __WIN__
> Wouldn't it be better to use the same constant everywhere?
Yes, it would. And this constant should be _WIN32, __WIN__ was
historically used, but it's incorrect - as was explained by our windows
developers. I've just used the "recommended" symbol.
> > + #define SO_EXT ".dll"
> > + #elif defined(__APPLE__)
> > + #define SO_EXT ".dylib"
> > + #else
> > + #define SO_EXT ".so"
> > + #endif
>
> > + /** the max allowed length for a user name */
> > + #define MYSQL_USERNAME_LENGTH 48
>
> Shouldn't this be the define USERNAME_LENGTH ?
> Otherwise there will be a crash if someone redefines USERNAME_CHAR_LENGTH
USERNAME_LENGTH is in mysql_com.h
Instead of including one file in another, I've ensured that there two
defines are exactly equal with compile time asserts. I'm sure you've
seen them when you've reviewed a little further down.
> > *************** void init_embedded_mysql(MYSQL *mysql, i
> > *** 584,589 ****
> > --- 582,588 ----
> > THD *thd = (THD *)mysql->thd;
> > thd->mysql= mysql;
> > mysql->server_version= server_version;
> > + mysql->client_flag= client_flag;
> > init_alloc_root(&mysql->field_alloc, 8192, 0);
> > }
>
> Was this a bug in the old code ?
I don't remember anymore. May be mysql->client_flag were not really used
in embedded. Or may be there was a bug.
> > + mysql->client_flag|= mysql->options.client_flag;
> > + mysql->client_flag|= CLIENT_CAPABILITIES;
>
> The code would be easier to read (and a bit faster/shorter) if you have
> client_flag as a static variable and set mysql->client_flag from this
> when all options are set.
don't worry, compiler does pretty much the same automatically at -O3,
I've just checked.
> > + if (data_len)
>
> Add a comment that data_len is always password length here.
it's not the length of the "password", it's the length of whatever data
client plugin wants to send.
> > + /* otherwise read the data */
> > + pkt_len= (*mysql->methods->read_change_user_result)(mysql);
> > + mpvio->last_read_packet_len= pkt_len;
> > + *buf= mysql->net.read_pos;
> > +
> > + /* was it a request to change plugins ? */
> > + if (**buf == 254)
> > + return (int)packet_error; /* if yes, this plugin shan't continue */
>
> shan't -> can't
The plugin shall not continue.
> > + /**
> > + vio->write_packet() callback method for client authentication plugins
> > +
> > + This function is called by a client authentication plugin, when it wants
> > + to send data to the server.
> > +
> > + It transparently wraps the data into a change user or authentication
> > + handshake packet, if neccessary.
> > + */
> > + static int client_mpvio_write_packet(struct st_plugin_vio *mpv,
> > + const uchar *pkt, int pkt_len)
> > + {
> > + int res;
> > + MCPVIO_EXT *mpvio= (MCPVIO_EXT*)mpv;
> > +
> > + if (mpvio->packets_written == 0)
> > + {
> > + if (mpvio->mysql_change_user)
> > + res= send_change_user_packet(mpvio, pkt, pkt_len);
> > + else
> > + res= send_client_reply_packet(mpvio, pkt, pkt_len);
>
> It would be nice to not have this if.
>
> As a trick, it's possible to remove all of the above if's.
>
> This would be to set write_packet directly to
> send_change_user_packet & send_client_reply_packet
Yes, it's a nice trick.
But I think it'll obfuscate the code flow, now you can look at one
function and see what it does where, with your trick you first look at
send_change_user_packet, then you see that the next function will be
client_mpvio_write_packet, and so on.
But if you insist, I can do that.
> And then end both of these with
> mpvio->write_packet=client_mpvio_write_packet;
> mpvio->packets_written++;
>
> In this case the packets_written may not even be neccessary to have.
I prefer to have them anyway, even if we will use your trick.
They make the code much simpler, you can any time check what stage
you're in. Or add asserts or anything.
> > + /**
> > + fills MYSQL_PLUGIN_VIO_INFO structure with the information about the
> > + connection
> > + */
> > + void mpvio_info(Vio *vio, MYSQL_PLUGIN_VIO_INFO *info)
> > + {
> > + bzero(info, sizeof(*info));
> > + switch (vio->type) {
...
> > + default: DBUG_ASSERT(0);
> > + }
> > + }
>
> As this is not used by MariaDBL by default, add a comment that for now
> this is only used by the socket_peercred plugin.
I'd rather not. We don't track third-party plugins, as soon as
another plugin starts using it, the comment will be obsolete.
And why do we care who uses it ? it's enough that it's part of the API.
> > + int run_plugin_auth(MYSQL *mysql, char *data, uint data_len,
> > + char *data_plugin, const char *db)
> > + {
> <cut>
>
> > + res= auth_plugin->authenticate_user((struct st_plugin_vio *)&mpvio, mysql);
> > +
> > + compile_time_assert(CR_OK == -1);
> > + compile_time_assert(CR_ERROR == 0);
> > + if (res > CR_OK && mysql->net.read_pos[0] != 254)
> > + {
> > + /*
> > + the plugin returned an error. write it down in mysql,
> > + unless the error code is CR_ERROR and mysql->net.last_errno
> > + is already set (the plugin has done it)
> > + */
> > + if (res > CR_ERROR)
> > + set_mysql_error(mysql, res, unknown_sqlstate);
> > + else
> > + if (!mysql->net.last_errno)
> > + set_mysql_error(mysql, CR_UNKNOWN_ERROR, unknown_sqlstate);
> > + return 1;
> > + }
> > +
> > + /* read the OK packet (or use the cached value in mysql->net.read_pos */
> > + if (res == CR_OK)
> > + pkt_length= (*mysql->methods->read_change_user_result)(mysql);
> > + else /* res == CR_OK_HANDSHAKE_COMPLETE */
> > + pkt_length= mpvio.last_read_packet_len;
>
> Note that here res may be CR_ERROR as mysql->net.read_pos[] can
> contain 254 as part of some old 'garbage' data, so the comment is
> wrong. I assume that in this case last_read_packet_len is packet_error.
I don't think mysql->net.read_pos[0] can be 254 here, there can be no
garbage in it. It is not uninitialized because we are in the middle of
authentication, the server packet is already received.
> > --- sql-common/client_plugin.c 2010-02-19 08:18:09 +0000
> > ***************
> > *** 0 ****
> > --- 1,427 ----
> > + int mysql_client_plugin_init()
> > + {
> > + MYSQL mysql;
> > + struct st_mysql_client_plugin **builtin;
> > +
> > + if (initialized)
> > + return 0;
> > + bzero(&mysql, sizeof(mysql)); /* dummy mysql for set_mysql_extended_error */
> > +
> > + pthread_mutex_init(&LOCK_load_client_plugin, MY_MUTEX_INIT_SLOW);
> > + init_alloc_root(&mem_root, 128, 128);
> > +
> > + bzero(&plugin_list, sizeof(plugin_list));
> > +
> > + initialized= 1;
> > +
> > + pthread_mutex_lock(&LOCK_load_client_plugin);
> > +
> > + for (builtin= mysql_client_builtins; *builtin; builtin++)
> > + add_plugin(&mysql, *builtin, 0, 0, 0);
> > +
> > + pthread_mutex_unlock(&LOCK_load_client_plugin);
> > +
> > + load_env_plugins(&mysql);
>
> You don't need mutex_lock() here, as no other thread can access data
> until this is setup. You can make this safe by setting initialized to
> 1 at end of function.
I have safe_mutex_assert_owner() inside add_plugin().
> > + return 0;
> > + }
> > +
> > + /**
> > + Deinitializes the client plugin layer.
> > +
> > + Unloades all client plugins and frees any associated resources.
> > + */
> > + void mysql_client_plugin_deinit()
> > + {
> > + int i;
> > + struct st_client_plugin_int *p;
> > +
> > + if (!initialized)
> > + return;
>
> Shouldn't this be a DBUG_ASSERT() to detect wrong usage?
it's safer that way, we have clients that call mysql_close many times,
I'd rather not be too strict here either.
> > --- sql/mysqld.cc 2010-02-19 08:18:09 +0000
> > *************** static int init_common_variables(const c
> > *** 3490,3495 ****
> > --- 3492,3498 ----
> > if (init_errmessage()) /* Read error messages from file */
> > return 1;
> > init_client_errs();
> > + mysql_library_init(un,us,ed); /* for replication */
>
> Please add comment. As this can never work, why have it here?
This works. mysql_library_init() here is defined to be
#define mysql_server_init(a,b,c) mysql_client_plugin_init()
so, mysql_client_plugin_init() is called, for replication, as comment
says, and having bogus parameters ensures that this never gets compiled
with the real client mysql_library_init() function.
> > === modified file 'sql/sql_acl.cc'
> > *** sql/sql_acl.cc 2010-02-01 06:14:12 +0000
>
> <cut>
>
> > --- 6238,6256 ----
> > }
> > else
> > {
> > ! push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, ER_PASSWD_LENGTH,
> > ! ER(ER_PASSWD_LENGTH), SCRAMBLED_PASSWORD_CHAR_LENGTH);
> > return TRUE;
> > }
> > combo->password.str= passwd_buff;
> > }
> > !
> > ! if (au->plugin.str != native_password_plugin_name.str &&
> > ! au->plugin.str != old_password_plugin_name.str)
>
> Also here it would be nice with just:
> if (!au->plugin.native_plugin)
most of the time I want to know whether it's native_password_plugin or
old_password_plugin. Only rarely I only care whether it's either one,
no matter which one.
> > + /****************************************************************************
> > + AUTHENTICATION CODE
> > + including initial connect handshake, invoking appropriate plugins,
> > + client-server plugin negotiation, COM_CHANGE_USER, and native
> > + MySQL authentication plugins.
> > + ****************************************************************************/
...
> > + /**
> > + The internal version of what plugins know as MYSQL_PLUGIN_VIO,
> > + basically the context of the authentication session
> > + */
> > + struct MPVIO_EXT : public MYSQL_PLUGIN_VIO
> > + {
> > + MYSQL_SERVER_AUTH_INFO auth_info;
> > + THD *thd;
> > + ACL_USER *acl_user; ///< a copy, independent from acl_users array
> > + plugin_ref plugin; ///< what plugin we're under
> > + LEX_STRING db; ///< db name from the handshake packet
> > + /** when restarting a plugin this caches the last client reply */
> > + struct {
> > + char *plugin, *pkt; ///< pointers into NET::buff
> > + uint pkt_len;
> > + } cached_client_reply;
> > + /** this caches the first plugin packet for restart request on the client */
> > + struct {
> > + char *pkt;
> > + uint pkt_len;
> > + } cached_server_packet;
> > + int packets_read, packets_written; ///< counters for send/received packets
> > + uint connect_errors; ///< if there were connect errors for this host
> > + /** when plugin returns a failure this tells us what really happened */
> > + enum { SUCCESS, FAILURE, RESTART } status;
> > + };
>
> Change comments from ///< to //
> (No reason for new style here)
> Also change /** to /*
no, they are doxygen comments, when this file is processed with Doxygen
there comments automatically become descriptions of the appropriate
fields in a structure.
> > + /**
> > + a helper function to report an access denied error in all the proper places
> > + */
> > + static void login_failed_error(THD *thd, bool passwd_used)
> > + {
> > + my_error(ER_ACCESS_DENIED_ERROR, MYF(0),
> > + thd->main_security_ctx.user,
> > + thd->main_security_ctx.host_or_ip,
> > + passwd_used ? ER(ER_YES) : ER(ER_NO));
> > + general_log_print(thd, COM_CONNECT, ER(ER_ACCESS_DENIED_ERROR),
> > + thd->main_security_ctx.user,
> > + thd->main_security_ctx.host_or_ip,
> > + passwd_used ? ER(ER_YES) : ER(ER_NO));
> > + status_var_increment(thd->status_var.access_denied_errors);
> > + /*
> > + Log access denied messages to the error log when log-warnings = 2
> > + so that the overhead of the general query log is not required to track
> > + failed connections.
> > + */
> > + if (global_system_variables.log_warnings > 1)
> > + {
> > + sql_print_warning(ER(ER_ACCESS_DENIED_ERROR),
> > + thd->main_security_ctx.user,
> > + thd->main_security_ctx.host_or_ip,
> > + passwd_used ? ER(ER_YES) : ER(ER_NO));
> > + }
> > + }
>
> Both general_log_print() and sql_print_warning() prints error to log
> file. Shouldn't we remove the first general_log_print() from above?
> (I couldn't find the sql_print_warning() in the old code).
sql_print_warning prints to the error log, general_log_print prints to
the general log. It wasn't in the 5.1, but I think it was in 6.0 - as
this was first implemented in 6.0 and then backported.
> > + /**
> > + sends a server handshake initialization packet, the very first packet
> > + after the connection was established
> > +
> > + Packet format:
> > +
> > + Bytes Content
> > + ----- ----
> > + 1 protocol version (always 10)
> > + n server version string, \0-terminated
> > + 4 thread id
> > + 8 first 8 bytes of the plugin provided data (scramble)
> > + 1 \0 byte, terminating the first part of a scramble
> > + 2 server capabilities (two lower bytes)
> > + 1 server character set
> > + 2 server status
> > + 2 server capabilities (two upper bytes)
> > + 1 length of the scramble
> > + 10 reserved, always 0
> > + n rest of the plugin provided data (at least 12 bytes)
> > + 1 \0 byte, terminating the second part of a scramble
> > +
> > + @retval 0 ok
> > + @retval 1 error
> > + */
>
> Shouldn't this function be in sql_connect.c ?
It could, but it'd force me to make many declarations public, I prefer
it contained to one file, with as much static and local as possible.
> > + THD *thd= mpvio->thd;
> > + char *buff= (char *)my_alloca(1 + SERVER_VERSION_LENGTH + data_len + 64);
> > + char scramble_buf[SCRAMBLE_LENGTH];
> > + char *end= buff;
> > +
> > + *end++= protocol_version;
> > +
> > + thd->client_capabilities= CLIENT_BASIC_FLAGS;
> > +
> > + if (data_len)
> > + {
> > + mpvio->cached_server_packet.pkt= (char*)thd->memdup(data, data_len);
> > + mpvio->cached_server_packet.pkt_len= data_len;
> > + }
> > +
> > + if (data_len < SCRAMBLE_LENGTH)
> > + {
> > + if (data_len)
> > + { /*
> > + the first packet *must* have at least 20 bytes of a scramble.
> > + if a plugin provided less, we pad it to 20 with zeros
> > + */
> > + memcpy(scramble_buf, data, data_len);
> > + bzero(scramble_buf+data_len, SCRAMBLE_LENGTH-data_len);
> > + data= scramble_buf;
> > + }
> > + else
> > + {
> > + /*
> > + if the default plugin does not provide the data for the scramble at
> > + all, we generate a scramble internally anyway, just in case the
> > + user account (that will be known only later) uses a
> > + native_password_plugin (which needs a scramble). If we don't send a
> > + scramble now - wasting 20 bytes in the packet -
> > + native_password_plugin will have to send it in a separate packet,
> > + adding one more round trip.
> > + */
> > + create_random_string(thd->scramble, SCRAMBLE_LENGTH, &thd->rand);
> > + data= thd->scramble;
> > + }
>
> You could move the above to an else of the first if (data_len) and
> move the first part inside the first if.
> This saves one-two if's during execution and makes code shorter.
I think it will be very confusing. Note tha after an if() I set
data_len. It would not make the code easier to read if data will be set
above in completely unrelated if(), but data_len - here.
> > + data_len= SCRAMBLE_LENGTH;
> > + }
> > +
> > + end= (char*)memcpy(end, data + SCRAMBLE_LENGTH_323,
> > + data_len - SCRAMBLE_LENGTH_323);
>
> Note that in old code, we had an end zero after scramble, that you
> don't have here. I checked the client code and at least in the .c
> client it's safe to nto have the end \0. You should probable check
> with other native drivers that it's safe with them too.
> (Especially the php driver).
php driver uses libmysqlclient, so it's safe.
And I do have \0 after a scramble. native_password_authenticate()
function has
if (mpvio->write_packet(mpvio, pkt, pkt_len + 1))
because of this +1 the \0 at the end is included.
> > + end+= data_len - SCRAMBLE_LENGTH_323;
> > + end= strmake(end, plugin_name(mpvio->plugin)->str,
> > + plugin_name(mpvio->plugin)->length);
> > +
> > + int res= my_net_write(&mpvio->thd->net, (uchar*) buff, (size_t) (end-buff)) ||
> > + net_flush(&mpvio->thd->net);
>
> Please move declaration of 'res' to function start.
why ?
it's fine in C++.
And I prefer to declare variables where they are used, not a hundred
lines in advance, especially if a variable is only needed for a few
lines, at the end of the function.
> > + my_afree(buff);
> > + return res;
> > + }
...
> > + static bool send_plugin_request_packet(MPVIO_EXT *mpvio,
> > + const uchar *data, uint data_len)
> > + {
> > + DBUG_ASSERT(mpvio->packets_written == 1);
> > + DBUG_ASSERT(mpvio->packets_read == 1);
> > + NET *net= &mpvio->thd->net;
> > +
> > + mpvio->status= MPVIO_EXT::FAILURE; // the status is no longer RESTART
> > +
> > + const char *client_auth_plugin=
> > + ((st_mysql_auth *)(plugin_decl(mpvio->plugin)->info))->client_auth_plugin;
> > +
> > + DBUG_ASSERT(client_auth_plugin);
>
> Wouldn't it be better to make this a LEX_STRING *?
no, because it comes straight from a plugin structure.
> > + static bool find_mpvio_user(MPVIO_EXT *mpvio, Security_context *sctx)
> > + {
...
> > + /* user account requires non-default plugin and the client is too old */
> > + if (my_strcasecmp(system_charset_info, mpvio->acl_user->plugin.str,
> > + native_password_plugin_name.str) &&
> > + my_strcasecmp(system_charset_info, mpvio->acl_user->plugin.str,
> > + old_password_plugin_name.str) &&
>
> Having a 'native' plugin field, would make this test:
>
> if (!mpvio->acl_user->plugin.native_plugin)
>
> I think we really should do this as it makes code shorter and we don't
> have to do a lot of name comparisons everywhere.
eh, plugin is a LEX_STRING here.
But I think I can remove these strcmp's and compare pointers instead.
> > + }
> > + #endif
> > +
> > + static bool parse_com_change_user_packet(MPVIO_EXT *mpvio, uint packet_length)
> > + {
> > + uint passwd_len= (thd->client_capabilities & CLIENT_SECURE_CONNECTION ?
> > + (uchar)(*passwd++) : strlen(passwd));
>
> Move declaration first
>
> > +
> > + db+= passwd_len + 1;
>
> Better to do:
>
> db= passwd + passwd_len + 1;
>
> And remove initialization from start.
no :)
It's the old code and it's written that way for a reason. Note that
passwd can be incremented in the passwd_len= expression, but db is the
original value of passwd. It's probably you who wrote it this way in the
first place :)
> > + if (ptr+1 < end)
> > + {
> > + uint cs_number= uint2korr(ptr);
> > + thd_init_client_charset(thd, cs_number);
> > + thd->update_charset();
>
> Add ptr+= 2 here (Makes rest of code simpler).
no, it'll be incorrect. It will be conditional ptr+=2, which happens
only if ptr+1 < end. Later I set client_plugin=ptr+2, and I don't want
to replace it with client_plugin=ptr, where ptr depends on ptrr+1<end.
> > + }
>
> We should first set the character set and then do copy_and_convert()
ouch!
Thanks.
> > + /* remember the data part of the packet, to present it to plugin in read_packet() */
> > + mpvio->cached_client_reply.pkt= passwd;
> > + mpvio->cached_client_reply.pkt_len= passwd_len;
> > + mpvio->cached_client_reply.plugin= client_plugin;
>
> Consider adding here
> mpvio->cached_client_reply.native_plugin= 0 | 1;
it'll be useless, I never need to know whether
cached_client_reply.plugin is one of the builtin two :)
In one condition I need to know whether it's native_password_plugin, in
another - whether it's old_password_plugin. But never - whether it's one
of either.
Instead I removed strcmp's with native_password_plugin_name.str and
old_password_plugin_name.str, and compare pointers instead
> > + mpvio->status= MPVIO_EXT::RESTART;
> > + #endif
> > +
> > + return 0;
> > + }
> > +
> > + static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio,
> > + uchar **buff, ulong pkt_len)
> > + {
> > + /* Disable those bits which are not supported by the client. */
>
> Should be 'server' not 'client' as client_capabilities are initially
> set by server.
No, it's 'client', not 'server'. client_capabilities are initially set
by server, and here they are AND'ed with what client has sent, to
remove all bits that server supports but client does not.
> > +
> > + if (passwd + passwd_len + db_len > (char *)net->read_pos + pkt_len)
> > + return packet_error;
> > +
> > + char *client_plugin= passwd + passwd_len + (db ? db_len + 1 : 0);
>
> client_plugin= passwd + passwd_len + db_len + test(db_len);
No, please.
I know that it works, but I don't like exploiting the fact that test() -
which is logically a boolean test - returns 1 or 0 and can be used in
arithmetics.
> > + static int server_mpvio_write_packet(MYSQL_PLUGIN_VIO *param,
> > + const uchar *packet, int packet_len)
> > + {
> > + MPVIO_EXT *mpvio= (MPVIO_EXT*)param;
> > + int res;
>
> > + static int server_mpvio_read_packet(MYSQL_PLUGIN_VIO *param, uchar **buf)
> > + {
> > + MPVIO_EXT *mpvio= (MPVIO_EXT*)param;
> > + ulong pkt_len;
> > +
> > + if (mpvio->packets_written == 0)
> > + {
> > + /*
> > + plugin wants to read the data without sending anything first.
> > + send an empty packet, to start a handshake
> > + */
>
> Change 'empty packet' to 'server_handshake_packet'.
done
> Here, the code would be much clearer if we would just use:
>
> mpvio->cached_client_reply.pkt= 0;
> mpvio->packets_written++;
> if (send_server_handshake_packet(mpvio, 0,0))
I'd rather not. server_mpvio_write_packet() is responsible for sending,
and I prefer to use it to send, instead of picking pieces of it to use
elsewhere. If we need to add some code to server_mpvio_write_packet()
later, we want to have all code paths affected, and not to track down
all copy-pasted pieces.
> > + static void server_mpvio_info(MYSQL_PLUGIN_VIO *vio,
> > + MYSQL_PLUGIN_VIO_INFO *info)
> > + {
> > + MPVIO_EXT *mpvio= (MPVIO_EXT*)vio;
> > + mpvio_info(mpvio->thd->net.vio, info);
> > + }
> > +
> > + static int acl_check_ssl(THD *thd, ACL_USER *acl_user)
>
> int -> bool
>
> Add function comment (at least regarding return values)
it's usual 0 - ok, 1 - error. As everywhere. I am trying to avoid
trivial comments like
i++; // increment i
> > + int acl_authenticate(THD *thd, uint connect_errors, uint com_change_user_pkt_len)
> > + {
> > + DBUG_ENTER("acl_authenticate");
> > + int res, old_status;
> > + plugin_ref plugin;
> > + MPVIO_EXT mpvio;
> > + st_mysql_auth *auth;
> > + LEX_STRING *auth_plugin_name= default_auth_plugin_name;
> > + bool unlock_plugin;
> > + enum enum_server_command command= com_change_user_pkt_len ? COM_CHANGE_USER
> > + : COM_CONNECT;
> > +
> > + compile_time_assert(MYSQL_USERNAME_LENGTH == USERNAME_LENGTH);
> > +
> > + bzero(&mpvio, sizeof(mpvio));
> > + mpvio.read_packet= server_mpvio_read_packet;
> > + mpvio.write_packet= server_mpvio_write_packet;
> > + mpvio.info= server_mpvio_info;
> > + mpvio.thd= thd;
> > + mpvio.connect_errors= connect_errors;
> > + mpvio.status= MPVIO_EXT::FAILURE;
> > +
> > + if (com_change_user_pkt_len == 0)
> > + thd->scramble[SCRAMBLE_LENGTH]= 1; // it means - there is no scramble yet
>
> Wouldn't it be better to use a separate flag or state for this?
> Using part of scramble, even if it's safe, looks strange and hard to
> remember. Having a state in MPVIO_EXT of what has taken place could
> be a good addition. This could be better than just incrementing
> packets_written / packets_read (in the case we ever want to do more
> round trips than 2)
no, it doesn't work. Note that it's a state of THD, not a state of
MPVIO_EXT. Because in COM_CHANGE_USER we also need to know if a scramble
was already generated - during initial connect or earlier
COM_CHANGE_USER's - or not.
> > + else
> > + {
> > + mpvio.packets_written++; // pretend that a server handshake packet was sent
> > + mpvio.packets_read++; // take COM_CHANGE_USER packet into account
> > +
> > + if (parse_com_change_user_packet(&mpvio, com_change_user_pkt_len))
> > + DBUG_RETURN(1);
> > +
> > + DBUG_ASSERT(mpvio.status == MPVIO_EXT::RESTART ||
> > + mpvio.status == MPVIO_EXT::SUCCESS);
> > + /*
> > + we skip the first step of the authentication -
> > + the one that starts a default plugin, sends the handshake packet with the
> > + scramble and reads the packet with the user name.
> > + in COM_CHANGE_USER the client already has the scramble and we already
> > + know the user name
> > + */
> > + goto skip_first;
>
> Note that 'res' and 'old_status' variables are not set here,
> which may be a problem as they could be tested below.
Good catch.
It's already fixed because I've got test failures in the optimized
builds because of that :)
> > + }
...
> > + mpvio.plugin= plugin;
> > + auth= (st_mysql_auth*)plugin_decl(plugin)->info;
> > +
> > + old_status= mpvio.status;
> > + res= auth->authenticate_user(&mpvio, &mpvio.auth_info);
> > +
> > + if (unlock_plugin)
> > + plugin_unlock(thd, plugin);
> > +
> > + skip_first:
>
> <cut>
>
> > +
> > + if (res > CR_OK && mpvio.status != MPVIO_EXT::SUCCESS)
>
> How about res != CR_OK ? (More clear)
You forgot about res == CR_OK_HANDSHAKE_COMPLETE
> > + /*
> > + This is the default access rights for the current database. It's
> > + set to 0 here because we don't have an active database yet (and we
> > + may not have an active database to set.
> > + */
> > + sctx->db_access=0;
> > +
> > + /* Change a database if necessary */
> > + if (mpvio.db.length)
> > + {
> > + if (mysql_change_db(thd, &mpvio.db, FALSE))
> > + {
> > + /* mysql_change_db() has pushed the error message. */
> > + if (thd->user_connect)
> > + {
> > + decrease_user_connections(thd->user_connect);
> > + status_var_increment(thd->status_var.access_denied_errors);
> > + thd->user_connect= 0;
>
> Move this just after the test (easier to read)
it cannot be "just after", because decrease_user_connections() takes it
as an argument.
> > + }
> > + DBUG_RETURN(1);
> > + }
> > + }
>
> > + static int old_password_authenticate(MYSQL_PLUGIN_VIO *vio,
> > + MYSQL_SERVER_AUTH_INFO *info)
> > + {
> > + uchar *pkt;
> > + int pkt_len;
> > + MPVIO_EXT *mpvio=(MPVIO_EXT*)vio;
> > + THD *thd=mpvio->thd;
> > +
> > + if (thd->scramble[SCRAMBLE_LENGTH])
> > + {
> > + /* no scramble was sent to the client yet, do it now */
> > + create_random_string(thd->scramble,
> > + pkt_len= SCRAMBLE_LENGTH, &thd->rand);
> > + pkt= (uchar*)thd->scramble;
> > + if (mpvio->write_packet(mpvio, pkt, pkt_len))
> > + return CR_ERROR;
> > + }
> > +
> > + /* ok, the client has got the scramble. read the reply and authenticate */
> > + if ((pkt_len= mpvio->read_packet(mpvio, &pkt)) < 0)
> > + return CR_ERROR;
> > +
> > + #ifdef NO_EMBEDDED_ACCESS_CHECKS
> > + return CR_OK;
> > + #endif
> > +
> > + /*
> > + legacy: if switch_from_long_to_short_scramble,
> > + the password is sent \0-terminated, the pkt_len is always 9 bytes.
> > + We need to figure out the correct scramble length here.
> > + */
> > + if (pkt_len == SCRAMBLE_LENGTH_323+1)
> > + pkt_len= strnlen((char*)pkt, pkt_len);
> > + if (pkt_len == 0) /* no password */
> > + return info->auth_string[0] ? CR_ERROR : CR_OK;
>
> If we want to do things like in the old code, we should here check the
> packet length.
>
> if (pkt_len != SCRAMBLE_LENGTH_323)
> {
> inc_host_errors(&mpvio->thd->net.vio->remote.sin_addr);
> my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
> return CR_ERROR;
> }
Yes, I know. It was intentional.
I thought that having the old_password_authenticate() and
native_password_authenticate() similar, with the same code structure, is
more important that having the same error in the case when the client
sends the scramble of incorrect length, the user account has a short
scramble, and a secure-auth is enabled.
> > + If the server is running in secure auth mode, short scrambles are
> > + forbidden. Extra juggling to report the same error as the old code.
> > + */
> > + if (opt_secure_auth)
> > + {
> > + if (thd->client_capabilities & CLIENT_PROTOCOL_41)
> > + {
> > + my_error(ER_SERVER_IS_IN_SECURE_AUTH_MODE, MYF(0),
> > + thd->security_ctx->user,
> > + thd->security_ctx->host_or_ip);
> > + general_log_print(thd, COM_CONNECT, ER(ER_SERVER_IS_IN_SECURE_AUTH_MODE),
> > + thd->security_ctx->user,
> > + thd->security_ctx->host_or_ip);
> > + }
> > + else
> > + {
> > + my_error(ER_NOT_SUPPORTED_AUTH_MODE, MYF(0));
> > + general_log_print(thd, COM_CONNECT, ER(ER_NOT_SUPPORTED_AUTH_MODE));
> > + }
> > + return CR_ERROR;
> > + }
>
> The old code (sql_connect.cc:check_user()) tested first for:
>
> if (opt_secure_auth_local && passwd_len == SCRAMBLE_LENGTH_323)
>
> And the other error condition next.
>
> Here things are reversed so we may get other errors than from MySQL.
Yes, but see above - I don't think it's very important to try to issue
the same error in that very exotic case when a scramble length is
incorrect AND secure auth mode is enabled AND a user account has a
short scramble.
> > + info->password_used = 1;
> > +
> > + if (pkt_len == SCRAMBLE_LENGTH_323)
> > + return check_scramble_323(pkt, thd->scramble,
> > + (ulong *)mpvio->acl_user->salt) ? CR_ERROR : CR_OK;
> > +
> > + inc_host_errors(&mpvio->thd->net.vio->remote.sin_addr);
> > + my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
> > + return CR_ERROR;
> > + }
>
> <cut>
>
> Other things.
>
> You need to add creation of auth_string to:
> scripts/mysql_fix_privilege_tables.sql
This is, of course, done. It's in the patch.
> In old_password_auth_client(), if we get an error from write_packet,
>
> it would be good to do:
> set_mysql_extended_error(mysql, CR_SERVER_LOST, unknown_sqlstate,
> ER(CR_SERVER_LOST_EXTENDED),
> "sending password information",
> errno);
there's no need to, because write_packet and read_packet set the error
message. It only makes sense to use set_mysql_extended_error() for
plugin internal errors, not for communication errors.
> See if you can combine common code from parse_com_change_user_packet()
> and parse_client_handshake_packet().
I know, I tried to, that's why find_mpvio_user() was created - it's one
of the last minute changes, trying to get rid of code duplication.
> It would also be nice to avoid comparison of strings and even string
> pointers and instead compare objects (like pointer to plugin) and
> values in plugin object (like if it's native or not).
How comparing pointers to plugins is better than comparing pointers to
strings ?
But I'm removed all string comparisons now - there's no single strcmp
with native_password_plugin_name or old_password_plugin_name during
authentication (only one is left - in acl_load, but it's unavoidable).
> Other than the above suggestions, it looked good and definitely
> something we need!
Thanks!
Regards,
Sergei
Follow ups
References
