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
