List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 13 2011 10:55am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (tatjana.nuernberg:3229)
Bug#21287 Bug#11745920
View as plain text  
Hi Tatjana,

On 5/11/11 8:46 PM, Tatjana Azundris Nuernberg wrote:
> #At file:///Users/tnurnberg/forest/21287/55-21287/ based on
> revid:dao-gang.qu@stripped
>
>   3229 Tatjana Azundris Nuernberg	2011-05-12
>        Bug#11745920/Bug#21287: "SSL connection error" is not helpful!
> (ssl-verify-server-cert=true vs localhos)
>
>        SSL errors on client and now more specific to aid end-user
>        with debugging.

Please expand this a bit to make more clear what is being provided to 
the end-user. This should make it easier for the bug to be clearly 
documented.

>       @ include/violite.h
>          new_VioSSLConnectorFd/sslaccept/sslconnect return more elaborate status
>       @ libmysql/errmsg.c
>          SSL errors now extended, more specific
>       @ mysql-test/r/openssl_1.result
>          SSL error messages now more specific
>       @ sql-common/client.c
>          ssl_verify_server_cert: we work out what's wrong, might as
>          well tell the user.
>

[...]

> === modified file 'sql/sql_acl.cc'
> --- a/sql/sql_acl.cc	2010-12-17 11:11:34 +0000
> +++ b/sql/sql_acl.cc	2011-05-11 23:46:04 +0000
> @@ -8394,16 +8394,24 @@ static ulong parse_client_handshake_pack
>     DBUG_PRINT("info", ("client capabilities: %lu", mpvio->client_capabilities));
>     if (mpvio->client_capabilities&  CLIENT_SSL)
>     {
> -    char error_string[1024] __attribute__((unused));
> +    unsigned long errptr;
>
>       /* Do the SSL layering. */
>       if (!ssl_acceptor_fd)
>         return packet_error;
>
>       DBUG_PRINT("info", ("IO layer change in progress..."));
> -    if (sslaccept(ssl_acceptor_fd, net->vio, net->read_timeout))
> +    if (sslaccept(ssl_acceptor_fd, net->vio, net->read_timeout,&errptr))
>       {
>         DBUG_PRINT("error", ("Failed to accept new SSL connection"));
> +
> +      if (global_system_variables.log_warnings>  1)
> +      {
> +        char buf[512];
> +        ERR_error_string_n(errptr, buf, 512);
> +        buf[511]= 0;
> +        sql_print_warning("SSL connection error: %s", buf);

Suggestion:

"Failed to accept an SSL connection: %s"

> +      }
>         return packet_error;
>       }
>
>
> === modified file 'vio/test-ssl.c'
> --- a/vio/test-ssl.c	2010-07-15 11:13:30 +0000
> +++ b/vio/test-ssl.c	2011-05-11 23:46:04 +0000
> @@ -59,6 +59,9 @@ main(int argc, char**	argv)
>     struct st_VioSSLFd* ssl_acceptor= 0;
>     struct st_VioSSLFd* ssl_connector= 0;
>     Vio* client_vio=0, *server_vio=0;
> +  enum enum_ssl_init_error ssl_init_error;
> +  unsigned long ssl_error;
> +
>     MY_INIT(argv[0]);
>     DBUG_PROCESS(argv[0]);
>     DBUG_PUSH(default_dbug_option);
> @@ -91,16 +94,16 @@ main(int argc, char**	argv)
>     ssl_acceptor = new_VioSSLAcceptorFd(server_key, server_cert, ca_file,
>   				      ca_path, cipher);
>     ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file,
> -					ca_path, cipher);
> +					ca_path, cipher,&ssl_init_error);
>
>     client_vio = (struct st_vio*)my_malloc(sizeof(struct st_vio),MYF(0));
>     client_vio->sd = sv[0];
>     client_vio->vioblocking(client_vio, 0,&unused);
> -  sslconnect(ssl_connector,client_vio,60L);
> +  sslconnect(ssl_connector,client_vio,60L,&ssl_error);
>     server_vio = (struct st_vio*)my_malloc(sizeof(struct st_vio),MYF(0));
>     server_vio->sd = sv[1];
>     server_vio->vioblocking(client_vio, 0,&unused);
> -  sslaccept(ssl_acceptor,server_vio,60L);
> +  sslaccept(ssl_acceptor,server_vio,60L,&ssl_error);
>
>     printf("Socketpair: %d , %d\n", client_vio->sd, server_vio->sd);
>
>
> === modified file 'vio/test-sslclient.c'
> --- a/vio/test-sslclient.c	2010-07-08 21:20:08 +0000
> +++ b/vio/test-sslclient.c	2011-05-11 23:46:04 +0000
> @@ -50,6 +50,9 @@ main(	int	argc __attribute__((unused)),
>   	Vio* client_vio=0;
>   	int err;
>   	char	xbuf[100]="Ohohhhhoh1234";
> +        enum enum_ssl_init_error ssl_init_error;
> +        unsigned long ssl_error;
> +
>   	MY_INIT(argv[0]);
>           DBUG_PROCESS(argv[0]);
>           DBUG_PUSH(default_dbug_option);
> @@ -60,7 +63,8 @@ main(	int	argc __attribute__((unused)),
>   	if (ca_path!=0)
>   		printf("CApath          : %s\n", ca_path);
>
> -	ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file, ca_path,
> cipher);
> +	ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file, ca_path,
> cipher,
> +&ssl_init_error);
>   	if(!ssl_connector) {
>                    fatal_error("client:new_VioSSLConnectorFd failed");
>   	}
> @@ -81,7 +85,7 @@ main(	int	argc __attribute__((unused)),
>   	/* ----------------------------------------------- */
>   	/* Now we have TCP conncetion. Start SSL negotiation. */
>   	read(client_vio->sd,xbuf, sizeof(xbuf));
> -        sslconnect(ssl_connector,client_vio,60L);
> +        sslconnect(ssl_connector,client_vio,60L,&ssl_error);
>   	err = vio_read(client_vio,xbuf, sizeof(xbuf));
>   	if (err<=0) {
>   		my_free(ssl_connector);
>
> === modified file 'vio/test-sslserver.c'
> --- a/vio/test-sslserver.c	2010-07-08 21:20:08 +0000
> +++ b/vio/test-sslserver.c	2011-05-11 23:46:04 +0000
> @@ -52,6 +52,7 @@ do_ssl_stuff(	TH_ARGS*	args)
>   	const char*	s = "Huhuhuhuuu";
>   	Vio*		server_vio;
>   	int		err;
> +        unsigned long   ssl_error;
>   	DBUG_ENTER("do_ssl_stuff");
>
>   	server_vio = vio_new(args->sd, VIO_TYPE_TCPIP, TRUE);
> @@ -60,7 +61,7 @@ do_ssl_stuff(	TH_ARGS*	args)
>   	/* TCP connection is ready. Do server side SSL. */
>
>   	err = write(server_vio->sd,(uchar*)s, strlen(s));
> -	sslaccept(args->ssl_acceptor,server_vio,60L);
> +	sslaccept(args->ssl_acceptor,server_vio,60L,&ssl_error);
>   	err = server_vio->write(server_vio,(uchar*)s, strlen(s));
>   	DBUG_VOID_RETURN;
>   }
>
> === modified file 'vio/vio_priv.h'
> --- a/vio/vio_priv.h	2010-06-07 14:01:39 +0000
> +++ b/vio/vio_priv.h	2011-05-11 23:46:04 +0000
> @@ -65,5 +65,7 @@ int vio_ssl_blocking(Vio *vio, my_bool s
>
>   my_bool vio_ssl_has_data(Vio *vio);
>
> +unsigned long ssl_report_errors();

The function definition is "ssl_report_errors(void)".

Also, suggest to rename to ssl_get_error(), or keep the functions 
independent.

> +
>   #endif /* HAVE_OPENSSL */
>   #endif /* VIO_PRIV_INCLUDED */
>
> === modified file 'vio/viossl.c'
> --- a/vio/viossl.c	2010-08-16 12:50:27 +0000
> +++ b/vio/viossl.c	2011-05-11 23:46:04 +0000
> @@ -24,31 +24,92 @@
>
>   #ifdef HAVE_OPENSSL
>
> -static void
> -report_errors(SSL* ssl)
> +#include<openssl/err.h>
> +
> +/**
> +  Clears SSL error queue. In debug builds, also reports its contents.
> +  OpenSSL doc requires we empty the error Q before doing SSL I/O (at
> +  least in those cases where we wish to use SSL_get_error()).
> +
> +SYNOPSIS
> +    ssl_report_errors()

You used a doxygen marker (/**) for the comment, yet the comment itself 
is not in doxygen format.

> +
> +  SIDE EFFECTS
> +      DBUG_PRINT()s and clears error queue
> +
> +  RETURN VALUES
> +      last error code in queue
> + */
> +
> +unsigned long
> +ssl_report_errors(void)
>   {
> -  unsigned long	l;
> +  unsigned long	e, l= 0;
> +#ifndef DBUG_OFF
>     const char *file;
>     const char *data;
>     int line, flags;
> -#ifndef DBUG_OFF
>     char buf[512];
>   #endif
>
> -  DBUG_ENTER("report_errors");
> +  DBUG_ENTER("ssl_report_errors");
>
> -  while ((l= ERR_get_error_line_data(&file,&line,&data,&flags)))
> +#ifndef DBUG_OFF
> +  while ((e= ERR_get_error_line_data(&file,&line,&data,&flags)))
>     {
> -    DBUG_PRINT("error", ("OpenSSL: %s:%s:%d:%s\n", ERR_error_string(l,buf),
> -			 file,line,(flags&ERR_TXT_STRING)?data:"")) ;
> +    DBUG_PRINT("error", ("OpenSSL: %s:%s:%d:%s\n", ERR_error_string(e, buf),
> +			 file, line, (flags&ERR_TXT_STRING)?data:""));
> +    l= e;
>     }
> +#else
> +  while ((e= ERR_get_error()))
> +    l= e;
> +#endif
> +
> +  DBUG_PRINT("info", ("socket_errno: %d", socket_errno));
> +  DBUG_RETURN(l);
> +}
> +
> +
> +/**
> +  report SSL errors.
> +  We call this because one of the TLS/SSL I/O routines -- SSL_connect(),
> +  SSL_accept(), SSL_do_handshake(), SSL_read(), SSL_peek(), or SSL_write() --
> +  failed.
> +
> +  SYNOPSIS
> +    ssl_report_io_errors()
> +      ssl     Current SSL (non-NULL) connection
> +      r       return value from last ssl_*() call
> +
> +  SIDE EFFECTS
> +      DBUG_PRINT()s
> +
> +  RETURN VALUES
> +      an error code
> + */
> +
> +unsigned long
> +ssl_report_io_errors(SSL *ssl, int r)
> +{
> +  unsigned long	io= 0;
> +#ifndef DBUG_OFF
> +  char buf[512];
> +#endif
> +
> +  DBUG_ENTER("ssl_report_io_errors");
> +
> +  DBUG_ASSERT(ssl);
>
>     if (ssl)
> -    DBUG_PRINT("error", ("error: %s",
> -                         ERR_error_string(SSL_get_error(ssl, l), buf)));
> +  {
> +    io= SSL_get_error(ssl, r);
> +    DBUG_PRINT("error", ("SSL I/O error: %lu - %s",
> +                         io, ERR_error_string(io, buf)));
> +  }
>
>     DBUG_PRINT("info", ("socket_errno: %d", socket_errno));
> -  DBUG_VOID_RETURN;
> +  DBUG_RETURN(io);
>   }
>
>
> @@ -59,10 +120,14 @@ size_t vio_ssl_read(Vio *vio, uchar* buf
>     DBUG_PRINT("enter", ("sd: %d  buf: 0x%lx  size: %u  ssl: 0x%lx",
>   		       vio->sd, (long) buf, (uint) size, (long) vio->ssl_arg));
>
> +#ifndef DBUG_OFF
> +  ssl_report_errors();
> +#endif
> +
>     r= SSL_read((SSL*) vio->ssl_arg, buf, size);
>   #ifndef DBUG_OFF
>     if (r == (size_t) -1)
> -    report_errors((SSL*) vio->ssl_arg);
> +    ssl_report_io_errors((SSL*) vio->ssl_arg, r);
>   #endif
>     DBUG_PRINT("exit", ("%u", (uint) r));
>     DBUG_RETURN(r);
> @@ -75,11 +140,15 @@ size_t vio_ssl_write(Vio *vio, const uch
>     DBUG_ENTER("vio_ssl_write");
>     DBUG_PRINT("enter", ("sd: %d  buf: 0x%lx  size: %u", vio->sd,
>                          (long) buf, (uint) size));
> +
> +#ifndef DBUG_OFF
> + ssl_report_errors();
> +#endif

No need for this. Let's keep it simple, we just need a way to pass errors.

Regards,

Davi

Thread
bzr commit into mysql-5.5-bugteam branch (tatjana.nuernberg:3229) Bug#21287Bug#11745920Tatjana Azundris Nuernberg12 May
  • Re: bzr commit into mysql-5.5-bugteam branch (tatjana.nuernberg:3229)Bug#21287 Bug#11745920Davi Arnaut13 May