Re: [PATCH] imap-send: Support SSL using GnuTLS
flat view
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:50:00
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.)
quoted hunk
--- 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"?
quoted hunk
@@ -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.
quoted hunk
+ 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... [...]
quoted hunk
--- /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.
quoted hunk
--- /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?
quoted hunk
--- 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. ;-))
quoted hunk
+#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".
quoted hunk
@@ -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...)?
quoted hunk
@@ -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?
quoted hunk
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