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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html