Hi, RJ!
On Jun 01, RJ Silk wrote:
> Hello Sergei!
>
> I've uploaded the latest version of my patch for WL#1054 to
> http://web.mit.edu/rsilk/Public/MySQL/MySQL_6.0_PluginAuth/mysql_auth_plugin.patch
>
> The patch should include all of the changes requested in your previous
> review (please let me know if I?ve overlooked anything). In the patch I?ve
> also included extra comments with explanations and questions that I will
> remove before the final patch.
Thanks!
Please, find my review below.
> I'm also assembling a more detailed breakdown of the low-level changes made
> by my patch that I can post to the internals list for discussion.
> Additionally I'll be reviewing my implementation with the Kerberos team to
> see what other features would need to be added to the plugin API to make
> the Kerberos implementation as robust as possible.
Great!
Here's the review:
================================================
You've done a lot or work.
I think it's getting complicated to communicate sending patches around.
for a project of this complexity it totally makes sense to
create a branch on launchpad. Can you do that ? If you'd like I can create it
for you. Do you already use bzr for this project or you develop in the
unpacked copy of the MySQL source distribution ?
> === modified file 'sql/sql_parse.cc'
> --- sql/sql_parse.cc 2009-05-16 08:36:58 +0000
> +++ sql/sql_parse.cc 2009-05-28 13:43:37 +0000
> @@ -985,7 +985,7 @@ bool dispatch_command(enum enum_server_c
> /* Clear variables that are allocated */
> thd->user_connect= 0;
> thd->security_ctx->priv_user= thd->security_ctx->user;
> - res= check_user(thd, COM_CHANGE_USER, passwd, passwd_len, db, FALSE);
> + res= check_user(thd, COM_CHANGE_USER, passwd, passwd_len, db, "", FALSE);
I suppose COM_CHANGE_USER should also take a 'default_plugin' argument,
but for now let's mention this in a TODO comment, and continue
passing the empty string.
>
> if (res)
> {
> === modified file 'sql/CMakeLists.txt'
> --- sql/CMakeLists.txt 2009-05-04 20:51:05 +0000
> +++ sql/CMakeLists.txt 2009-05-28 14:09:47 +0000
> @@ -65,7 +65,8 @@ SET (SQL_SOURCE
> sql_cursor.cc sql_db.cc sql_delete.cc sql_derived.cc sql_do.cc
> sql_error.cc sql_handler.cc sql_help.cc sql_insert.cc
> sql_join_cache.cc sql_lex.cc sql_list.cc sql_load.cc sql_manager.cc
> - sql_map.cc sql_parse.cc sql_partition.cc sql_plugin.cc
> + sql_map.cc sql_parse.cc sql_partition.cc sql_plugin.cc
> + ../sql-common/client_plugin.c
why do you need client_plugin.c in the server ?
> sql_prepare.cc sql_rename.cc
> debug_sync.cc debug_sync.h
> sql_repl.cc sql_select.cc sql_show.cc sql_state.c sql_string.cc
> === modified file 'sql/sql_acl.h'
> --- sql/sql_acl.h 2009-04-02 08:50:24 +0000
> +++ sql/sql_acl.h 2009-05-28 13:43:32 +0000
> @@ -206,6 +206,8 @@ public:
> uint8 salt_len; // 0 - no password, 4 - 3.20, 8 - 3.23, 20 - 4.1.1
> enum SSL_type ssl_type;
> const char *ssl_cipher, *x509_issuer, *x509_subject;
> + LEX_STRING plugin;
use spaces, not tabs, please
> + char *auth_string;
> };
>
>
> === modified file 'include/mysql_com.h'
> --- include/mysql_com.h 2009-01-26 16:03:39 +0000
> +++ include/mysql_com.h 2009-05-29 16:16:34 +0000
> @@ -193,6 +196,9 @@ enum enum_server_command
> & ~CLIENT_COMPRESS) \
> &
> ~CLIENT_SSL_VERIFY_SERVER_CERT)
comment for the below ?
> +#define SERVER_REQ_SHORT_SCRAMBLE_TOK 254
> +#define SERVER_REQ_DIF_PLUGIN 253
> +
> #define SERVER_STATUS_IN_TRANS 1 /* Transaction has started */
> #define SERVER_STATUS_AUTOCOMMIT 2 /* Server in auto_commit mode */
> #define SERVER_MORE_RESULTS_EXISTS 8 /* Multi query - next query exists */
> === modified file 'sql/sql_yacc.yy'
> --- sql/sql_yacc.yy 2009-05-11 17:58:07 +0000
> +++ sql/sql_yacc.yy 2009-06-01 04:20:02 +0000
> @@ -13500,6 +13507,13 @@ grant_user:
> }
> }
> }
> + | user IDENTIFIED_SYM VIA_SYM ident_or_text AS TEXT_STRING
> + /* XXX TEXT_STRING or TEXT_STRING_sys ??? */
The difference: TEXT_STRING is in charset_client (whatever it is)
TEXT_STRING_sys is in utf8.
I think it should be TEXT_STRING_sys, always utf8, and we store it as such.
(which probably means that 'auth_string' column in the user table should be
TEXT, not BLOB, as we treat it as textual data, not binary)
The benefit, of course, we always know what the charset the data are in, and
how to interpret it.
The drawback of using TEXT_STRING_sys - one won't be able to use bytes that
aren't valid utf-8 characters, no matter what the current charset is. But I
suppose we can say that if some plugin requires binary data in the
auth_string, it's this plugin's job to decode them from a safe textual
representation.
Opinions ?
> + {
> + $$= $1;
> + $1->plugin= $4;
> + $1->auth= $6;
> + }
> | user IDENTIFIED_SYM BY PASSWORD TEXT_STRING
> { $$= $1; $1->password= $5; }
> | user
> === modified file 'sql/sql_connect.cc'
> --- sql/sql_connect.cc 2009-05-14 21:49:53 +0000
> +++ sql/sql_connect.cc 2009-05-31 19:55:09 +0000
> @@ -352,41 +352,8 @@ check_user(THD *thd, enum enum_server_co
> }
>
> USER_RESOURCES ur;
> - int res= acl_getroot(thd, &ur, passwd, passwd_len);
> + int res= acl_getroot(thd, &ur, passwd, passwd_len, default_plugin);
> -#ifndef EMBEDDED_LIBRARY
> - if (res == -1)
you removed this "request old scramble code" from here, fine.
but where did you add it instead ?
I couldn't find it :(
> - {
> - /*
> - This happens when client (new) sends password scrambled with
> - scramble(), but database holds old value (scrambled with
> - scramble_323()). Here we please client to send scrambled_password
> - in old format.
> - */
> - NET *net= &thd->net;
> - if (opt_secure_auth_local)
> - {
> - my_error(ER_SERVER_IS_IN_SECURE_AUTH_MODE, MYF(0),
> - thd->main_security_ctx.user,
> - thd->main_security_ctx.host_or_ip);
> - general_log_print(thd, COM_CONNECT, ER(ER_SERVER_IS_IN_SECURE_AUTH_MODE),
> - thd->main_security_ctx.user,
> - thd->main_security_ctx.host_or_ip);
> - DBUG_RETURN(1);
> - }
> - /* We have to read very specific packet size */
> - if (send_old_password_request(thd) ||
> - my_net_read(net) != SCRAMBLE_LENGTH_323 + 1)
> - {
> - inc_host_errors(&net->vio->remote);
>
> - my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
> - DBUG_RETURN(1);
> - }
> - /* Final attempt to check the user based on reply */
> - /* So as passwd is short, errcode is always >= 0 */
> - res= acl_getroot(thd, &ur, (char *) net->read_pos, SCRAMBLE_LENGTH_323);
> - }
> -#endif /*EMBEDDED_LIBRARY*/
> /* here res is always >= 0 */
> if (res == 0)
> {
> @@ -910,11 +877,27 @@ static int check_connection(THD *thd)
> user_len-= 2;
> }
>
> + char *default_plugin = thd->client_capabilities & CLIENT_PLUGIN_AUTH ?
> + passwd + passwd_len + db_len + 1 : 0;
> + uint default_plugin_len = default_plugin ? strlen(default_plugin) : 0;
> + char default_plugin_buff[NAME_LEN + 1];
> +
> + if (default_plugin && (passwd + passwd_len + db_len + default_plugin_len)
> <=
> + (char *)net->read_pos + pkt_len)
> + {
why not an error in the opposite case ? as in
if (passwd + passwd_len + db_len > (char *)net->read_pos + pkt_len)
{
inc_host_errors(&net->vio->remote);
my_error(ER_HANDSHAKE_ERROR, MYF(0), thd->main_security_ctx.host_or_ip);
return 1;
}
we used to issue an error in these cases
> + default_plugin_buff[copy_and_convert(default_plugin_buff,
> + sizeof(default_plugin_buff)-1,
> + system_charset_info,
> + default_plugin, default_plugin_len,
> + thd->charset(), &dummy_errors)]=
> '\0';
strictly speaking, it's unnecessary, as plugin names are
always 7-bit ascii.
> + default_plugin = default_plugin_buff[0] ? default_plugin_buff : 0;
> + }
> +
> if (thd->main_security_ctx.user)
> x_free(thd->main_security_ctx.user);
> if (!(thd->main_security_ctx.user= my_strdup(user, MYF(MY_WME))))
> return 1; /* The error is set by my_strdup(). */
> - return check_user(thd, COM_CONNECT, passwd, passwd_len, db, TRUE);
> + return check_user(thd, COM_CONNECT, passwd, passwd_len, db, default_plugin,
> TRUE);
> }
>
>
> === modified file 'sql/protocol.cc'
> --- sql/protocol.cc 2009-05-07 20:48:24 +0000
> +++ sql/protocol.cc 2009-06-01 03:56:17 +0000
> @@ -322,6 +323,59 @@ bool send_old_password_request(THD *thd)
> }
>
>
> +/**
> + Ask the client to use a different auth plugin.
> +
> + @param thd thread handle
> + @param plugin the plugin requested
> +
> + @retval
> + 0 ok
> + @retval
> + !0 error
> +*/
> +
> +bool send_plugin_request(THD *thd, char *plugin)
> +{
> + char buff[100];
> + NET *net= &thd->net;
> + size_t len= plugin ? strlen(plugin) : 0;
> + if (plugin && len > 0)
> + {
> + if (len == 1 && (plugin[0] == (char)SERVER_REQ_DIF_PLUGIN ||
> + plugin[0] == (char)SERVER_REQ_SHORT_SCRAMBLE_TOK))
why SERVER_REQ_DIF_PLUGIN
(shouldn't here be only SERVER_REQ_SHORT_SCRAMBLE_TOK as the only special
case) ?
> + {
> + /*
> + This special plugin request was made by the built-in password auth
> + plugin and should be sent as-is. (We have to handle password auth
> + specially, becuase we want to maintain backwards compatability).
> + */
> + strcpy((char*)&buff, plugin);
> + }
> + else
> + {
> + /* The plugin requests looks normal; format and send */
> + buff[0]= (char)SERVER_REQ_DIF_PLUGIN;
> + strcpy(&buff[1], plugin);
> + len++;
> + buff[len]= 0;
> + len++;
> + }
> + }
> + else
> + {
> + /*
> + Plugin name was not specified, tell the client that we don't need
> + it to use an auth plugin.
what does that mean ? what would client need to do ?
> + */
> + buff[0]= (char)SERVER_REQ_DIF_PLUGIN;
> + buff[1]= 0;
> + len= 2;
> + }
> +
> + return my_net_write(net, (uchar*)buff, len) || net_flush(net);
> +}
> +
> void net_send_error_packet(THD *thd, uint sql_errno, const char *err,
> const char* sqlstate)
> {
> @@ -505,6 +559,22 @@ void Protocol::send_ok(uint server_statu
>
>
> /**
> + Sends a simple "OK" reponse to the client during plugin auth.
> +
> + This method is exposed publicly but should only be used during
> + auth in sql_acl.cc.
> +*/
> +
> +void Protocol::send_plugin_ok()
> +{
> + DBUG_ENTER("Protocol::send_plugin_ok");
> +
> + net_send_ok(thd, 0, 0, 0, 0, NULL);
> +
> + DBUG_VOID_RETURN;
> +}
As you call it only in one place, and it's, anyway, just a convenience
wrapper on top of Protocol::send_ok, I think you don't need this method at
all, you can simply call
protocol->send_ok(0, 0, 0, 0, 0);
in sql_acl.cc
> +
> +/**
> A default implementation of "EOF" packet response to the client.
>
> Binary and text protocol do not differ in their EOF packet format.
> === modified file 'include/mysql/plugin.h'
> --- include/mysql/plugin.h 2009-05-14 08:56:34 +0000
> +++ include/mysql/plugin.h 2009-06-01 03:48:21 +0000
> @@ -475,6 +476,109 @@ struct Mysql_replication {
> int interface_version;
> };
Move the below to a separate file, please. include/mysql/plugin_auth.h
> +/*************************************************************************
> + API for Authentication plugin. (MYSQL_AUTHENTICATION_PLUGIN)
> +*/
> +
> +#define MYSQL_AUTHENTICATION_INTERFACE_VERSION 0x0001
> +
> +/**
> + Provides plugin access to communication channel
> +*/
> +typedef struct st_plugin_io
call it either "io" or "vio" everywhere, not a mix of both
> +{
> + /**
> + Plugin provides a buffer and this function fills it with the
> + contents of any incoming packet. Returns the packet length.
> + */
> + int (*read)(struct st_plugin_io*, char* buf, int len);
> +
> + /**
> + Plugin provides a buffer with data and the length and this
> + function sends it as a packet. Returns the length of the data sent.
> + */
> + int (*write)(struct st_plugin_io*, const unsigned char* buf, int len);
> +
> + void * vio_handle; /**< internal handle to vio */
> +} MYSQL_PLUGIN_VIO;
Why read/write methods take MYSQL_PLUGIN_VIO as the first argument,
I mean, why not vio_handle ?
> +
> +/* Provide a constant defining the max size of the info bufs */
> +#define MYSQL_AUTH_NAME_LEN 60
> +
> +/**
> + Provides plugin access to authentication information
> +*/
> +typedef struct st_mysql_server_auth_info
> +{
> + char *user_name; /**< username sent by client */
> + char *auth_string; /**< auth string from db */
> + char *passwd; /**< pass field from auth pkt */
> + unsigned int passwd_len; /**< length of pass field */
> + char *scramble; /**< random scramble */
> + unsigned int scramble_len; /**< length of scramble */
> + unsigned char *salt; /**< pass salt from db */
> + unsigned char salt_len; /**< pass salt length */
I'm not sure you need salt and scramble here. They're specific to
certain authentication methods. Like, you don't have "TGT" here, do you ?
.... few hours later ...
okay, you store the client auth data in the 'password', and it doesn't
necessarily have to be the password, as such. May be you need to rename
it to something more generic ? auth_data ?
> + char *default_plugin; /**< plugin loaded by client */
> + /**
> + Empty buffer that the plugin should fill with the name of the
> + client-side auth plugin it requires (if at all).
> + */
> + char req_plugin_name[MYSQL_AUTH_NAME_LEN];
> + /**
> + Empty buffer that the plugin can fill with the user name that
> + should be used for authorization.
> + */
> + char req_user_name[MYSQL_AUTH_NAME_LEN];
> +} MYSQL_SERVER_AUTH_INFO;
> +
> +/**
> + Server authentication plugin descriptor
> +*/
> +struct st_mysql_auth
> +{
> + int interface_version; /**< version plugin uses */
> + /**
> + Function provided by the plugin which should fill the info.req_plugin_name
> + field if it requires a different plugin than that specified in
> + info.default_plugin. Plugin should return 0 if the default plugin is
> + acceptable, otherwise return 1 and fill info.req_plugin_name.
> + */
> + int (*get_req_plugin)(MYSQL_SERVER_AUTH_INFO *info);
> +
> + /**
> + Function provided by the plugin which should perform authentication (using
> + the io functions if necessary) and return 0 if successful. The plugin can
> + also fill the info.req_user_name field if a different username other than
> + that provided in info.user_name should be used for authorization.
> + */
> + int (*authenticate_user)(MYSQL_PLUGIN_VIO *io,
> + MYSQL_SERVER_AUTH_INFO *info);
> +};
> +
> +/**
> + Client authentication plugin descriptor
> +*/
> +struct st_mysql_auth_client
> +{
> + int interface_version; /**< version plugin uses */
> +
> + /**
> + Function provided by the plugin which should fill dest_buf with data (up
> + to dest_buf_len) that will be send in the client auth packet. Returns
> + scramble length.
> + */
> + int (*get_scramble)(char* dest_buf,
> + unsigned int dest_buf_len,
> + char *server_scramble,
> + unsigned int server_scramble_len,
> + char *username, char *password);
> +
> + /**
> + Function provided by the plugin which should perform authentication (using
> + the io functions if necessary) and return 0 if successful.
again, consistent terminology. either it's "io" or "vio" everywhere.
> + */
> + int (*authenticate_user)(MYSQL_PLUGIN_VIO *io, char *host_name);
> +};
>
> /*************************************************************************
> st_mysql_value struct for reading values from mysqld.
> === modified file 'sql/sql_acl.cc'
> --- sql/sql_acl.cc 2009-05-07 20:48:24 +0000
> +++ sql/sql_acl.cc 2009-06-01 05:17:00 +0000
> @@ -832,43 +846,169 @@ static int acl_compare(ACL_ACCESS *a,ACL
> }
>
>
> /*
> + The following two functions are used by authentication plugins
> + in order to access the communication channel. They are duplicated
> + in client.c in case either the server or client requires
> + different proxied functionality from the other.
please clarify in a comment (and, perhaps, in function name) that this
function reads a *packet*. That is, it reads one complete next packet from
the wire - as opposite, for example - to reading 'n' bytes, as one could've
guessed judging from the prototype (and comparing it with, say, libc read()
or fread()).
and by 'function name' I also mean the callback name in the MYSQL_PLUGIN_VIO
structure. Remember one of the rules of the good API design "the obvious
usage is right" - so the name should tell how to use it.
> +*/
> +static int plg_io_read(MYSQL_PLUGIN_VIO *net, char *buf, int n)
> +{
> + int res = (int)my_net_read((NET*)net->vio_handle);
> + if (res > 0)
> + memcpy(buf, (char*)((NET*)net->vio_handle)->read_pos, min(n, res));
> + return min(n, res);
do you need a memcpy here ?
why not to return a pointer to the data instead ?
> +}
> +
> +
> +static int plg_io_write(MYSQL_PLUGIN_VIO *net, const unsigned char *buf, int n)
> +{
> + int res = (int)my_net_write((NET*)net->vio_handle, buf, n);
> + net_flush((NET*)net->vio_handle);
> + return n;
> +}
> +
> +
> +/**
> + Perform authentication for a given user, using an authentication plugin.
> +
> + This function attempts to load the required auth plugin
> + for the given user. If found, attempts to coordinate
> + authentication with the client if necessary.
> +
> + @param thd thread handle; no members of thd are changed.
> + @param acl_user user resources; supplies information about the
> + required authentication mechanism
> + @param passwd contents of scramble field from client auth packet
> + @param passwd_len length of scramble field contents
> + @param default_plugin null-terminted name of plugin the client already has
> + loaded; used to avoid round-trip of a request packet
> + @param user_name_buf name the plugin says should be used for auth; we
> + expect this buf to be at least of length NAME_LEN
> +
> + @return
> + @retval 0 success
> + @retval -1 authentication failed
> +*/
> +
> +static int acl_ext_auth(THD *thd, ACL_USER *acl_user,
> + const char *passwd, uint passwd_len,
> + char *default_plugin,
> + char *user_name_buf)
> +{
> + int res= 0;
> + plugin_ref plugin;
> + MYSQL_PLUGIN_VIO simple_io;
> + LEX_STRING passwd_str= {"passwd", 6};
what's this - the name of the default built-in auth plugin ?
I suppose it should be then defined in some shared place, so that
clients could see it too.
besides, to avoid specifying the length (6) manually - which is error-prone -
use STRING_WITH_LEN macro:
LEX_STRING passwd_str= {STRING_WITH_LEN("passwd")};
may be a name should be less ambigous like "mysql_password" ?
> + const LEX_STRING *req_plugin;
> + DBUG_ENTER("acl_ext_auth");
> +
> + /* Assemble the plugin IO structure for the authentication plugins. */
> + simple_io.vio_handle= &thd->net;
> + simple_io.read= plg_io_read;
> + simple_io.write= plg_io_write;
> +
> + /* Check if user requires custom authentication */
> + if (thd->client_capabilities & CLIENT_PLUGIN_AUTH &&
> + acl_user->plugin.str)
> + req_plugin= (const LEX_STRING*)&acl_user->plugin;
> + else
> + req_plugin= (const LEX_STRING*)&passwd_str;
> +
> + if (!(plugin= my_plugin_lock_by_name(thd, req_plugin,
> + MYSQL_AUTHENTICATION_PLUGIN)))
> + {
> + /* Server cannot load required plugin. */
> + my_error(ER_PLUGIN_IS_NOT_LOADED, MYF(0), req_plugin->str);
> + DBUG_RETURN(-1);
> + }
> +
> + st_mysql_auth *auth_desc=
> + (st_mysql_auth*)plugin_decl(plugin)->info;
> +
> + MYSQL_SERVER_AUTH_INFO info;
> + bzero(&info, sizeof(MYSQL_SERVER_AUTH_INFO));
> + info.user_name= acl_user->user;
> + info.auth_string= acl_user->auth_string;
> + info.passwd= (char*)passwd;
> + info.passwd_len= passwd_len;
> + info.scramble= (char*)&thd->scramble;
> + info.scramble_len= SCRAMBLE_LENGTH+1;
> + info.salt= (uint8*)&acl_user->salt;
> + info.salt_len= acl_user->salt_len;
> + info.default_plugin= default_plugin;
> +
> + if (auth_desc->get_req_plugin(&info))
> + {
> + send_plugin_request(thd, info.req_plugin_name);
> +
> + /* Read new scramble */
> + info.passwd_len= my_net_read(&thd->net)-1;
> + info.passwd= (char *)thd->net.read_pos;
> + }
> + else
> + {
> + if (req_plugin != &passwd_str)
> + {
> + /*
> + Tell client: default plugin is OK. This is required, because without
> + this message, the client will hang waiting to see whether or not
> + it should turn control over to its plugin for authentication. If we used
> + a builtin password plugin we can avoid this round-trip because on the
> + client the password auth method is empty (all we needed was the
> + scramble) and the server will automatically issue an OK packet once
> + execution goes back to sql_connect (if auth succeeded). Therefore the
> + new auth plugin API only adds an extra round trip when using a
> + third-party plugin. */
sorry, I failed to understand this. if get_req_plugin() returned 0
it means that the client already uses correct plugin. And the control
is already turned over to it. What do you need to ok here ?
> + thd->protocol->send_plugin_ok();
> + }
> + }
> +
> + res= auth_desc->authenticate_user(&simple_io, &info);
> + if (res || !info.req_user_name[0])
> + strcpy(user_name_buf, info.user_name);
strncpy, just in case.
> + else
> + strcpy(user_name_buf, info.req_user_name);
same
> +
> + plugin_unlock(thd, plugin);
> +
> + DBUG_RETURN(res);
> +}
> +
> +
> +/**
> Seek ACL entry for a user, check password, SSL cypher, and if
> everything is OK, update THD user data and USER_RESOURCES struct.
>
> - IMPLEMENTATION
> - This function does not check if the user has any sensible privileges:
> - only user's existence and validity is checked.
> - Note, that entire operation is protected by acl_cache_lock.
> -
> - SYNOPSIS
> - acl_getroot()
> - thd thread handle. If all checks are OK,
> - thd->security_ctx->priv_user/master_access are updated.
> - thd->security_ctx->host/ip/user are used for checks.
> - mqh user resources; on success mqh is reset, else
> - unchanged
> - passwd scrambled & crypted password, received from client
> - (to check): thd->scramble or thd->scramble_323 is
> - used to decrypt passwd, so they must contain
> - original random string,
> - passwd_len length of passwd, must be one of 0, 8,
> - SCRAMBLE_LENGTH_323, SCRAMBLE_LENGTH
> - 'thd' and 'mqh' are updated on success; other params are IN.
> + This function does not check if the user has any sensible privileges:
> + only user's existence and validity is checked.
> + Note, that entire operation is protected by acl_cache_lock.
> +
> + @param thd thread handle. If all checks are OK,
> + thd->security_ctx->priv_user/master_access are updated.
> + thd->security_ctx->host/ip/user are used for checks.
> + @param mqh user resources; on success mqh is reset, else
> + unchanged
> + @param passwd scrambled & crypted password, received from client
> + (to check): thd->scramble or thd->scramble_323 is
> + used to decrypt passwd, so they must contain
> + original random string,
> + @param passwd_len length of passwd
> + 'thd' and 'mqh' are updated on success; other params are IN.
>
> - RETURN VALUE
> - 0 success: thd->priv_user, thd->priv_host, thd->master_access, mqh
> are
> - updated
> - 1 user not found or authentication failure
> - 2 user found, has long (4.1.1) salt, but passwd is in old (3.23) format.
> - -1 user found, has short (3.23) salt, but passwd is in new (4.1.1) format.
> + @return
> + @retval 0 success: thd->priv_user, thd->priv_host, thd->master_access,
> mqh are
> + updated
> + @retval != 0 user not found or authentication failure
> */
>
> int acl_getroot(THD *thd, USER_RESOURCES *mqh,
> - const char *passwd, uint passwd_len)
> + const char *passwd, uint passwd_len, char *default_plugin)
> {
> ulong user_access= NO_ACCESS;
> int res= 1;
> + int req_ext_auth= 0;
> + char ext_user_name_buff[MYSQL_AUTH_NAME_LEN];
> ACL_USER *acl_user= 0;
> Security_context *sctx= thd->security_ctx;
> DBUG_ENTER("acl_getroot");
> @@ -1047,7 +1175,10 @@ int acl_getroot(THD *thd, USER_RESOURCES
> #endif /* HAVE_OPENSSL */
> }
> sctx->master_access= user_access;
> - sctx->priv_user= acl_user->user ? sctx->user : (char *) "";
> + if (req_ext_auth)
> + sctx->priv_user= strdup_root(&mem, (char*)&ext_user_name_buff);
> + else
> + sctx->priv_user= acl_user->user ? sctx->user : (char *) "";
how can req_ext_auth be 0 here ?
I mean, acl_user != 0 and req_ext_auth == 0, can it ever happen ?
> *mqh= acl_user->user_resource;
>
> if (acl_user->host.hostname)
> @@ -1189,6 +1322,9 @@ static void acl_update_user(const char *
> acl_user->host.hostname &&
> !my_strcasecmp(system_charset_info, host, acl_user->host.hostname))
> {
> + acl_user->plugin.str= plugin ? strdup_root(&mem,plugin) : 0;
> + acl_user->plugin.length= plugin ? strlen(acl_user->plugin.str) : 0;
indentation
> + acl_user->auth_string= auth ? strdup_root(&mem,auth) : 0;
> acl_user->access=privileges;
> if (mqh->specified_limits & USER_RESOURCES::QUERIES_PER_HOUR)
> acl_user->user_resource.questions=mqh->questions;
> @@ -1953,6 +2094,17 @@ static int replace_user_table(THD *thd,
> thd->security_ctx->user, thd->security_ctx->host_or_ip);
> goto end;
> }
> + else if (combo.plugin.str && combo.auth.str)
why do you require combo.auth.str != 0 ?
it's not too difficult to imagine an auth plugin that doesn't need
any additional arguments, and in that case user won't need to specify
auth str.
> + {
> + LEX_STRING plugin_name ={((char *) (combo.plugin.str)),
> + ((size_t) (strlen(combo.plugin.str)))};
strlen already returns size_t, doesn't it ?
> + if (!plugin_is_ready(&plugin_name, MYSQL_AUTHENTICATION_PLUGIN))
> + {
> + my_error(ER_PLUGIN_IS_NOT_LOADED, MYF(0), combo.plugin.str);
> + goto end;
> + }
> + }
> +
> old_row_exists = 0;
> restore_record(table,s->default_values);
> table->field[0]->store(combo.host.str,combo.host.length,
> @@ -2048,6 +2200,15 @@ static int replace_user_table(THD *thd,
> table->field[next_field+3]->store((longlong) mqh.user_conn, TRUE);
> mqh_used= mqh_used || mqh.questions || mqh.updates || mqh.conn_per_hour;
> }
> +
> + if (combo.plugin.str)
> + {
> + table->field[40]->store(combo.plugin.str,combo.plugin.length,
> + system_charset_info);
> + table->field[41]->store(combo.auth.str,combo.auth.length,
> + system_charset_info);
same here. it should be possible to specify plugin.str without auth.str
> + }
> +
> if (old_row_exists)
> {
> /*
> @@ -6864,3 +7029,86 @@ bool check_routine_level_acl(THD *thd, c
> }
>
> #endif
> +
> +/*
> + MySQL Server Password Authentication Plugin
> +*/
what about #ifndef EMBEDDED_LIBRARY somewhere here ?
> +static int mysql_auth_passwd_get_plugin(MYSQL_SERVER_AUTH_INFO *info)
> +{
> + if (!info->default_plugin || info->default_plugin[0] == 0)
> + {
> + /* No default plugin specified, some form of password auth was used */
> + if (info->passwd_len != info->salt_len){
> + if (info->passwd_len == SCRAMBLE_LENGTH &&
> + info->salt_len == SCRAMBLE_LENGTH_323)
> + {
> + /* Database has old salt; request old scramble */
> + info->req_plugin_name[0]= (char)253;
didn't you introduce named constants for all magical numbers ? :)
> + return 1;
> + }
> + else if (info->passwd_len == SCRAMBLE_LENGTH_323 &&
> + info->salt_len == SCRAMBLE_LENGTH)
> + {
> + /*
> + Client sent old scrambleby default,
> + but database has new; auth will fail.
> + */
> + return 0;
> + }
> + }
> + }
> + else
> + {
> + /* A default plugin was specified other than password */
> + if (info->salt_len == SCRAMBLE_LENGTH )
> + {
> + /* database has new salt; request new scramble */
> + info->req_plugin_name[0]= (char)253;
> + return 1;
> + }
> + else if (info->salt_len == SCRAMBLE_LENGTH_323)
> + {
> + /* database has old salt; request old scramble */
> + info->req_plugin_name[0]= (char)254;
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
I think you're over-engineering a bit. I can hardly imagine why any "normal"
plugin may need a complex get_plugin() callback. I suspect for most of them it
will be simply
{ return "some_fixed_plugin_name"; }
a hard-coded constant.
the only use case is the default mysql auth, where you use this callback to
request the shorter scramble. But even this is not very logical -
other plugins may need many roundtrips for the correct authentication -
but you call get_plugin() only once - so they'll do this back-and-forth
dialog in the authenticate_user() callback. Similarly, I think, default
auth plugin should request a short scramble in the authenticate_user()
and in this case you won't need get_plugin() callback at all,
you can make it a hard-coded fixed char* constant in the plugin structure.
Keeping is simple :)
> +
> +static int mysql_auth_passwd_authenticate(st_plugin_io * vio,
> + MYSQL_SERVER_AUTH_INFO *info)
> +{
> + if (info->salt_len == 0 ||
> + ((info->passwd_len == SCRAMBLE_LENGTH ?
> + check_scramble(info->passwd, info->scramble, info->salt) :
> + check_scramble_323(info->passwd, info->scramble,
> + (ulong *) info->salt)) == 0))
> + return 0;
> + else
> + return -1;
> +
> +}
> +
> +struct st_mysql_auth mysql_auth_passwd=
> + { MYSQL_AUTHENTICATION_INTERFACE_VERSION,
> + mysql_auth_passwd_get_plugin,
> + mysql_auth_passwd_authenticate};
> +
> +mysql_declare_plugin(passwd)
> +{
> + MYSQL_AUTHENTICATION_PLUGIN, /* type constant */
> + &mysql_auth_passwd, /* type descriptor */
> + "passwd", /* Name */
> + "MySQL", /* Author */
> + "Provides builtin password support", /* Description */
The name - more MySQL specific, e.g. "mysql_password",
and the description - "traditional MySQL authentication"
> + PLUGIN_LICENSE_GPL, /* License */
> + NULL, /* Init function */
> + NULL, /* Deinit function */
> + 0x0010, /* Version (1.0) */
> + NULL, /* status variables */
> + NULL, /* system variables */
> + NULL /* config options */
> +}
> +mysql_declare_plugin_end;
> === modified file 'libmysql/client_settings.h'
> --- libmysql/client_settings.h 2008-08-07 17:52:43 +0000
> +++ libmysql/client_settings.h 2009-03-24 04:53:44 +0000
> @@ -22,7 +22,8 @@ extern char * mysql_unix_port;
> CLIENT_PROTOCOL_41 | \
> CLIENT_SECURE_CONNECTION | \
> CLIENT_MULTI_RESULTS | \
> - CLIENT_PS_MULTI_RESULTS)
> + CLIENT_PS_MULTI_RESULTS | \
> + CLIENT_PLUGIN_AUTH)
it would be more correct to add CLIENT_PLUGIN_AUTH only if HAVE_DLOPEN is
defined. e.g.:
#ifdef HAVE_DLOPEN
#define CAN_CLIENT_PLUGIN_AUTH CLIENT_PLUGIN_AUTH
#else
#define CAN_CLIENT_PLUGIN_AUTH 0
#endif
and use CAN_CLIENT_PLUGIN_AUTH in the above CLIENT_CAPABILITIES define
>
> sig_handler my_pipe_sig_handler(int sig);
> void read_user_name(char *name);
> === modified file 'libmysql/libmysql.c'
> --- libmysql/libmysql.c 2009-05-07 20:48:24 +0000
> +++ libmysql/libmysql.c 2009-05-29 19:51:27 +0000
> @@ -198,6 +200,8 @@ void STDCALL mysql_server_end()
> if (!mysql_client_init)
> return;
>
> + mysql_client_plugin_deinit();
> +
> #ifdef EMBEDDED_LIBRARY
> end_embedded_server();
> #endif
I've stopped here, I'm concentrating on the server part for now,
will do client part after that.
================================================
Regards / Mit vielen GrÐÑen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB MÐnchen 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
GeschÐftsfÐhrer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin HÐring
| Thread |
|---|
| • Re: WL#1054 Pluggable Authentication Update | Sergei Golubchik | 15 Jul |
