List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 11 2011 7:48pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (azundris:3229) Bug#21287
Bug#11745920
View as plain text  
Hi Tatiana,

On 4/28/11 7:52 AM, Tatiana Azundris Nurnberg wrote:
> #At file:///Users/tnurnberg/forest/21287/55-21287/ based on
> revid:dao-gang.qu@stripped
>
>   3229 Tatiana Azundris Nurnberg	2011-04-28
>        Bug#11745920/Bug#21287: "SSL connection error" is not helpful!
> (ssl-verify-server-cert=true vs localhos)
>
>        SSL errors on client now more specific to aid end-user with
>        debugging.
>       @ include/violite.h
>          new_VioSSLConnectorFd/sslaccept/sslconnect new 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.
>
>          Do more detailed error reporting for setup, connect, and
>          server cert verifying phases.
>       @ sql/sql_acl.cc
>          sslaccept() signature has changed
>       @ vio/viossl.c
>          don't just print low level SSL problems to debug trace,
>          save the error code and return it to callers of sslaccept
>          and sslconnect!
>       @ vio/viosslfactories.c
>          new_VioSSLConnectorFd() now returns error codes from
>          new_VioSSLFd() rather than discard them.

Looks good. Some comments below.

> === modified file 'include/violite.h'
> --- a/include/violite.h	2010-06-07 14:01:39 +0000
> +++ b/include/violite.h	2011-04-28 10:51:56 +0000
> @@ -132,13 +132,13 @@ struct st_VioSSLFd
>     SSL_CTX *ssl_context;
>   };
>
> -int sslaccept(struct st_VioSSLFd*, Vio *, long timeout);
> -int sslconnect(struct st_VioSSLFd*, Vio *, long timeout);
> +int sslaccept(struct st_VioSSLFd*, Vio *, long timeout, unsigned long *errptr);
> +int sslconnect(struct st_VioSSLFd*, Vio *, long timeout, unsigned long *errptr);
>
>   struct st_VioSSLFd
>   *new_VioSSLConnectorFd(const char *key_file, const char *cert_file,
>   		       const char *ca_file,  const char *ca_path,
> -		       const char *cipher);
> +		       const char *cipher, enum enum_ssl_init_error* error);

Asterisk position.

>   struct st_VioSSLFd
>   *new_VioSSLAcceptorFd(const char *key_file, const char *cert_file,
>   		      const char *ca_file,const char *ca_path,
>
> === modified file 'sql-common/client.c'
> --- a/sql-common/client.c	2010-11-10 15:21:51 +0000
> +++ b/sql-common/client.c	2011-04-28 10:51:56 +0000
> @@ -1850,6 +1850,7 @@ mysql_get_ssl_cipher(MYSQL *mysql __attr
>     ssl_verify_server_cert()
>       vio              pointer to a SSL connected vio
>       server_hostname  name of the server that we connected to
> +    errptr           if we fail, we'll return (a pointer to a string describing) the
> reason here

The maximum line width should be 80 characters.

>
> === modified file 'sql/sql_acl.cc'
> --- a/sql/sql_acl.cc	2010-12-17 11:11:34 +0000
> +++ b/sql/sql_acl.cc	2011-04-28 10:51:56 +0000
> @@ -8395,13 +8395,14 @@ static ulong parse_client_handshake_pack
>     if (mpvio->client_capabilities&  CLIENT_SSL)
>     {
>       char error_string[1024] __attribute__((unused));

This seems to be unused. Could you please remove it?

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

Should we perhaps log something if log_warnings > 1?

See similar sql_print_warning()s in the same file.

>       {
>         DBUG_PRINT("error", ("Failed to accept new SSL connection"));
>         return packet_error;
>
> === modified file 'vio/viossl.c'
> --- a/vio/viossl.c	2010-08-16 12:50:27 +0000
> +++ b/vio/viossl.c	2011-04-28 10:51:56 +0000
> @@ -24,10 +24,12 @@
>
>   #ifdef HAVE_OPENSSL
>
> -static void
> +#include<openssl/err.h>
> +
> +static unsigned long
>   report_errors(SSL* ssl)
>   {
> -  unsigned long	l;
> +  unsigned long	l, p= 0;
>     const char *file;
>     const char *data;
>     int line, flags;
> @@ -41,14 +43,18 @@ report_errors(SSL* ssl)
>     {
>       DBUG_PRINT("error", ("OpenSSL: %s:%s:%d:%s\n", ERR_error_string(l,buf),
>   			 file,line,(flags&ERR_TXT_STRING)?data:"")) ;
> +    p= l;
>     }
>
>     if (ssl)
> -    DBUG_PRINT("error", ("error: %s",
> -                         ERR_error_string(SSL_get_error(ssl, l), buf)));
> +  {
> +    p= SSL_get_error(ssl, p);
> +    DBUG_PRINT("error", ("SSL error: %lu - %s",
> +                         p, ERR_error_string(p, buf)));
> +  }
>
>     DBUG_PRINT("info", ("socket_errno: %d", socket_errno));
> -  DBUG_VOID_RETURN;
> +  DBUG_RETURN(p);
>   }
>
>
> @@ -144,7 +150,7 @@ void vio_ssl_delete(Vio *vio)
>
>
>   static int ssl_do(struct st_VioSSLFd *ptr, Vio *vio, long timeout,
> -                  int (*connect_accept_func)(SSL*))
> +                  int (*connect_accept_func)(SSL*), unsigned long *errptr)
>   {
>     SSL *ssl;
>     my_bool unused;
> @@ -160,7 +166,7 @@ static int ssl_do(struct st_VioSSLFd *pt
>     if (!(ssl= SSL_new(ptr->ssl_context)))
>     {
>       DBUG_PRINT("error", ("SSL_new failure"));
> -    report_errors(ssl);
> +    *errptr= report_errors(ssl);

Why not simply use ERR_peek_error?

Regards,

Davi
Thread
bzr commit into mysql-5.5-bugteam branch (azundris:3229) Bug#21287Bug#11745920Tatiana Azundris Nurnberg28 Apr
  • Re: bzr commit into mysql-5.5-bugteam branch (azundris:3229) Bug#21287Bug#11745920Davi Arnaut11 May
    • Re: bzr commit into mysql-5.5-bugteam branch (azundris:3229) Bug#21287Bug#11745920Tatjana Azundris Nuernberg12 May