Re: [PATCH] imap-send: Support SSL using GnuTLS

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Mike,

Mike Miller wrote:

> GnuTLS is an LGPL alternative to OpenSSL, required for IMAP over SSL
> support on Debian.
> 
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=434599
> 
> This patch uses the OpenSSL compatibility library of GnuTLS for the same
> behavior with minimal differences.

That's awesome.  Just as a quality-of-implementation issue, it is good
to be able to handle multiple backends so we can be sure we are not
relying on tiny implementation details.

> However, GnuTLS does not provide equivalents to the base64 and md5
> routines needed for CRAM-MD5 authentication.

Mm, that feels like it should be a separate patch.  It could be useful
when not using an SSL lib at all.

Maybe the base64 routine could share some code with base85.c?  (Just
a thought.)

> When compiling, GnuTLS is only used when NO_OPENSSL is defined.
> ---

Sign-off?  (See Documentation/SubmittingPatches for what I'm talking
about.)

> --- a/Makefile
> +++ b/Makefile
> @@ -24,6 +24,9 @@ all::
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
>  # This also implies BLK_SHA1.
>  #
> +# Define NO_GNUTLS if you do not have gnutls installed.  gnutls provides
> +# SSL when OpenSSL is not used.
> +#

Shouldn't this say "if NO_OPENSSL is defined and you do not have GnuTLS
installed"?

> @@ -1244,8 +1247,21 @@ ifndef NO_OPENSSL
>  else
>  	BASIC_CFLAGS += -DNO_OPENSSL
>  	BLK_SHA1 = 1
> +ifndef NO_GNUTLS
> +	OPENSSL_LIBSSL = -lgnutls-openssl -lgnutls -lgcrypt

Probably should be indented another level to convey structure.

> +	ifdef GNUTLSDIR
> +		BASIC_CFLAGS += -I$(GNUTLSDIR)/include
> +		OPENSSL_LINK = -L$(GNUTLSDIR)/$(lib) $(CC_LD_DYNPATH)$(GNUTLSDIR)/$(lib)
> +	else
> +		OPENSSL_LINK =
> +	endif
> +	LIB_OBJS += gnutls-base64.o gcrypt-hmac.o
> +	LIB_H += gnutls-base64.h gcrypt-hmac.h

This causes all git commands to link to base64 and hmac routines.
Maybe you meant for them to be linked in to imap-send only?  See
http.o for an example of this kind of thing.

On the other hand, see below...

[...]
> --- /dev/null
> +++ b/gcrypt-hmac.c
> @@ -0,0 +1,41 @@
> +/*
> + * gcrypt-hmac.c - interface wrapper to provide OpenSSL HMAC API using gcrypt.

Maybe this could be useful for inclusion in gcrypt?

In git, it might make sense to treat these as routines for compat/,
kind of like compat/basename.c.

> --- /dev/null
> +++ b/gnutls-base64.c
> @@ -0,0 +1,197 @@
> +/*
> + * gnutls-base64.c - base64 encode and decode
> + *                   adapted from GnuTLS, original copyright follows

Why is this needed?

> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -26,10 +26,19 @@
>  #include "exec_cmd.h"
>  #include "run-command.h"
>  #ifdef NO_OPENSSL
> +#ifdef NO_GNUTLS
>  typedef void *SSL;
>  #else
> +#include <gnutls/openssl.h>
> +#include "gnutls-base64.h"
> +#include "gcrypt-hmac.h"
> +#undef NO_OPENSSL /* gnutls is providing an openssl API */
> +#define SSL_VERIFY_PEER 1 /* doesn't matter */

If it doesn't matter, why set it?  (I assume it does matter but that
its value is unimportant. ;-))

> +#endif
> +#else
>  #include <openssl/evp.h>
>  #include <openssl/hmac.h>
> +#define USE_OPENSSL_REAL 1
>  #endif

Nit: this means something like "SSL_IS_OPENSSL", right?

i.e., it is not actually a request "use such-and-such" but a
statement of fact "using such-and-such".

> @@ -307,9 +316,9 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
>  	SSL_load_error_strings();
>  
>  	if (use_tls_only)
> -		meth = TLSv1_method();
> +		meth = TLSv1_client_method();
>  	else
> -		meth = SSLv23_method();
> +		meth = SSLv23_client_method();

Is there a semantic difference?  Will old versions of OpenSSL
continue to work (I assume so, but...)?

> @@ -321,10 +330,12 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
>  	if (verify)
>  		SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);
>  
> +#ifdef USE_OPENSSL_REAL
>  	if (!SSL_CTX_set_default_verify_paths(ctx)) {
>  		ssl_socket_perror("SSL_CTX_set_default_verify_paths");
>  		return -1;
>  	}
> +#endif

Why?  Could this be accomplished with a

#ifndef USE_OPENSSL_REAL
	static inline int SSL_CTX_set_default_verify_paths(...
	{
		/* Not needed by GnuTLS */
		return 0;
	}
#endif

wrapper?

>  	sock->ssl = SSL_new(ctx);
>  	if (!sock->ssl) {
>  		ssl_socket_perror("SSL_new");
> @@ -371,9 +382,19 @@ static int socket_write(struct imap_socket *sock, const char *buf, int len)
>  {
>  	int n;
>  #ifndef NO_OPENSSL
> -	if (sock->ssl)
> -		n = SSL_write(sock->ssl, buf, len);
> -	else
> +	if (sock->ssl) {
> +		/* loop based on write_in_full, the gnutls implementation of
> +		 * SSL_write may write a partial buffer. */
> +		int count = len;
> +		n = 0;
> +		while (count > 0) {
> +			int written = SSL_write(sock->ssl, buf, count);
> +			if (written <= 0) break;
> +			count -= written;
> +			buf += written;
> +			n += written;
> +		}
> +	} else

Probably a wrapper function would be nicer.

Thanks for a clean patch.

Hope that helps,
Jonathan