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
