List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:March 22 2011 10:48am
Subject:Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3301)
Bug#11756377
View as plain text  
Hi Nirbhay,

this patch looks better. At least we agree on direction. See
comments inline.

On Fri, Mar 18, 2011 at 12:00:20PM +0000, Nirbhay Choubey wrote:
> #At file:///home/nirbhay/Project/mysql/repo/bugs/source/mysql-trunk.48287/ based on
> revid:mattias.jonsson@stripped
> 
>  3301 Nirbhay Choubey	2011-03-18
>       BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY
>       
>       In interactive mode, MySQL client maintains a history file
>       (.mysql_history) to log all the commands executed through
>       it. Hence, eventually, it also logs the 'password' carrying
>       statements.
>       
>       Currently there is no way to disallow the logging of such
>       statements.
>       
>       This patch changes the default logging behavior. The
>       statements carrying password/identified words will
>       not be logged in the history file. However, a new
>       option 'log-password' is introduced to force the 
>       logging of such statements. The above scenarios can
>       further be customized by using an environment variable
>       'MYSQL_HISTIGNORE', a colon-separated list of strings
>       used to decide which statements should be saved on the
>       history list. This variable can shadow the use of 
>       log-password option.
>      @ client/client_priv.h
>         BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY
>         
>         Added a client option to control logging of password
>         carrying statements in the history file.
>      @ client/mysql.cc
>         BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY
>         
>         Added function check_histignore() to check for the presence
>         of some specific string patterns that is usually found in
>         the statements that carry passwords along.
>         In this case, the entire statement is not logged in order
>         to improve the efficiency.
> 
>     modified:
>       client/client_priv.h
>       client/mysql.cc
> === modified file 'client/client_priv.h'
> --- a/client/client_priv.h	2011-02-21 11:33:20 +0000
> +++ b/client/client_priv.h	2011-03-18 12:00:17 +0000
> @@ -86,7 +86,8 @@ enum options_client
>    OPT_DEFAULT_PLUGIN,
>    OPT_RAW_OUTPUT, OPT_WAIT_SERVER_ID, OPT_STOP_NEVER,
>    OPT_BINLOG_ROWS_EVENT_MAX_SIZE,
> -  OPT_MAX_CLIENT_OPTION
> +  OPT_MAX_CLIENT_OPTION,
> +  OPT_LOG_PASSWORD
>  };
>  
>  /**
> 
Recurring: OPT_MAX_CLIENT_OPTION is supposed to be last in enum
options_client.

> === modified file 'client/mysql.cc'
> --- a/client/mysql.cc	2011-03-09 20:54:55 +0000
> +++ b/client/mysql.cc	2011-03-18 12:00:17 +0000
> @@ -136,7 +136,7 @@ static my_bool ignore_errors=0,wait_flag
>  	       vertical=0, line_numbers=1, column_names=1,opt_html=0,
>                 opt_xml=0,opt_nopager=1, opt_outfile=0, named_cmds= 0,
>  	       tty_password= 0, opt_nobeep=0, opt_reconnect=1,
> -	       opt_secure_auth= 0,
> +	       opt_secure_auth= 0, opt_log_password= 0,
>                 default_pager_set= 0, opt_sigint_ignore= 0,
>                 auto_vertical_output= 0,
>                 show_warnings= 0, executing_query= 0, interrupted_query= 0,
> @@ -154,6 +154,7 @@ static char *current_host,*current_db,*c
>              *current_prompt=0, *delimiter_str= 0,
>              *default_charset= (char*) MYSQL_AUTODETECT_CHARSET_NAME,
>              *opt_init_command= 0;
> +static char *histignore= NULL, *histignore_buf= NULL;
>  static char *histfile;
>  static char *histfile_tmp;
>  static String glob_buffer,old_buffer;
> @@ -276,6 +277,9 @@ static void init_username();
>  static void add_int_to_prompt(int toadd);
>  static int get_result_width(MYSQL_RES *res);
>  static int get_field_disp_length(MYSQL_FIELD * field);
> +static my_bool check_histignore(const char *string);
> +static void add_filtered_history(const char *string);
> +
>  
>  /* A structure which contains information on the commands this program
>     can understand. */
> @@ -1224,6 +1228,21 @@ int main(int argc,char *argv[])
>    initialize_readline((char*) my_progname);
>    if (!status.batch && !quick && !opt_html && !opt_xml)
>    {
> +    if (getenv("MYSQL_HISTIGNORE"))
> +      histignore= my_strdup(getenv("MYSQL_HISTIGNORE"), MYF(MY_WME));
So you suggest tristate:
If MYSQL_HISTIGNORE environment variable is set, use it.
Else if log-password command line option was given, use none.
Else use default "IDENTIFIED:PASSWORD".

Why not to solve it via single "histignore" command line option,
which is "IDENTIFIED:PASSWORD" by default? Optionally you may
provide additional control via MYSQL_HISTIGNORE environment
variable (command line option will have precedence).

> +    else if (!opt_log_password)
> +    {
> +      histignore= (char*) my_malloc((uint) strlen("IDENTIFIED:PASSWORD"),
> +				    MYF(MY_WME));
> +      /* If 'MYSQL_HISTIGNORE' is not set, we assign it a default value. */
> +      if (histignore)
> +        sprintf(histignore, "IDENTIFIED:PASSWORD");
You mean histignore= my_strdup("IDENTIFIED:PASSWORD")? :)

> +    }
> +
> +    /* To be used by strtok. */
> +    if (histignore)
> +      histignore_buf= my_strdup(histignore, MYF(MY_WME));
You already have one copy of histignore string. Oh, well. You
do it for strtok(). Well, I'd suggest not to use strtok() at all.
Instead parse string into dynamic array (see mysys/array.c) of
something like LEX_STRING.

> +    
>      /* read-history from file, default ~/.mysql_history*/
>      if (getenv("MYSQL_HISTFILE"))
>        histfile=my_strdup(getenv("MYSQL_HISTFILE"),MYF(MY_WME));
> @@ -1303,6 +1322,8 @@ sig_handler mysql_end(int sig)
>    my_free(server_version);
>    my_free(opt_password);
>    my_free(opt_mysql_unix_port);
> +  my_free(histignore);
> +  my_free(histignore_buf);
>    my_free(histfile);
>    my_free(histfile_tmp);
>    my_free(current_db);
> @@ -1616,6 +1637,12 @@ static struct my_option my_long_options[
>      "Default authentication client-side plugin to use.",
>     (uchar**) &opt_default_auth, (uchar**) &opt_default_auth, 0,
>     GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> +  {"log-password", OPT_LOG_PASSWORD,
> +   "Log statements containing 'IDENTIFIED' and 'PASSWORD' to the history "
> +   "file. By default, such statements are not logged to the history file. "
> +   "However, this behavior can be altered by setting MYSQL_HISTIGNORE.",
> +   &opt_log_password, &opt_log_password, 0, GET_BOOL, NO_ARG,
> +   0, 0, 0, 0, 0, 0},
>    { 0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
>  };
>  
> @@ -1966,7 +1993,7 @@ static int read_and_execute(bool interac
>  	in_string=0;
>  #ifdef HAVE_READLINE
>        if (interactive && status.add_to_history &&
> not_in_history(line))
> -	add_history(line);
> +	add_filtered_history(line);
>  #endif
>        continue;
>      }
> @@ -2068,7 +2095,7 @@ static bool add_line(String &buffer,char
>      DBUG_RETURN(0);
>  #ifdef HAVE_READLINE
>    if (status.add_to_history && line[0] && not_in_history(line))
> -    add_history(line);
> +    add_filtered_history(line);
>  #endif
>    char *end_of_line=line+(uint) strlen(line);
>  
> @@ -2406,7 +2433,7 @@ static void fix_history(String *final_co
>      ptr++;
>    }
>    if (total_lines > 1)			
> -    add_history(fixed_buffer.ptr());
> +    add_filtered_history(fixed_buffer.ptr());
>  }
>  
>  /*	
> @@ -5074,3 +5101,38 @@ static int com_prompt(String *buffer __a
>      tee_fprintf(stdout, "PROMPT set to '%s'\n", current_prompt);
>    return 0;
>  }
> +
> +
> +static void add_filtered_history(const char *string)
> +{
> +  if (!histignore || !check_histignore(string))
> +    add_history(string);
> +}
> +
> +#ifdef instr
> +#undef instr
> +#endif
Why?

> +
> +/*
> +  Perform a check on the current statement if it
> +  contains any of the words stored in histignore.
> +*/
> +static my_bool check_histignore(const char *string)
> +{
> +  char *token;
> +  my_match_t match;
> +
> +  strncpy(histignore_buf, histignore, strlen(histignore));
> +  token= strtok(histignore_buf, ":");
> +
> +  while (token != NULL)
> +  {
> +    if(charset_info->coll->instr(charset_info,
> +                                 string, (uint) strlen(string),
> +                                 token, (uint) strlen(token),
> +                                 &match, 1))
Taking into account that bash offers shell expressions,
why wouldn't we offer LIKE expressions? Just use wildcmp()
instead of instr().

> +    return 1;
> +    token= strtok(NULL, ":");
> +  }
> +  return 0;
> +}
> 

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-trunk branch (nirbhay.choubey:3301) Bug#11756377Nirbhay Choubey18 Mar
  • Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3301)Bug#11756377Sergey Vojtovich22 Mar