Hi Jonathan, thanks for the feedback and newb counseling, this is my
first attempt at a git contribution.

On Tue, 2010-11-09 at 09:09 -0600, Jonathan Nieder wrote: 
quoted text
> 
> Mike Miller wrote:
> 
> > 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.)

Fair enough.  That's a better division grouping functionality with
library dependencies, as gcrypt is needed for CRAM-MD5 and gnutls for
SSL socket layer.

quoted text
> > When compiling, GnuTLS is only used when NO_OPENSSL is defined.
> > ---
> 
> Sign-off?  (See Documentation/SubmittingPatches for what I'm talking
> about.)

rtfm, got it :)

quoted text
> 
> > --- 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"?

Sounds good.

quoted text
> > @@ -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.

Yes.

quoted text
> > +	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.

Right again.

quoted text
> [...]
> > --- /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.

As I was working through this part I was debating whether these should
be in a separate file, where to put them, etc.  Compat looked like it
was mostly libc-type functions so I stayed out of there.

Deferring this to gcrypt to handle sounds like a great idea.

quoted text
> > --- /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?

gnutls has generic base64 encode/decode routines but they are not
exported as callable symbols.  The only callable base64 routines in
gnutls are specific to PEM encoding/decoding.  There are so many base64
implementations out there but I don't know of any that are in the public
API of a relatively common library.  Other than openssl.  I'm open to
suggestions.

Or as you suggested above, merge with base85.c.

Another alternative would be, as with gcrypt, request gnutls to export
the base64 encode/decode functions, and provide an openssl compatible
interface while we're at it.

quoted text
> > --- 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. ;-))

Something like that.

quoted text
> > +#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".

Yeah, I think I changed that one back and forth.  At one point I had
changed all "NO_OPENSSL"s to "NO_OPENSSL_API" but I switched back to
minimize the changes and went with this check for the
SSL_CTX_set_default_verify_paths when that ended up being the only
difference.

quoted text
> > @@ -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...)?

Yeah I honestly don't know enough about it to be boldly making that
change.  This could be conditional for gnutls, which only provides
X_client_method and X_server_method but not X_method, which I suppose
can act as either end of the connection?

quoted text
> > @@ -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?

Looks good.

quoted text
> >  	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.

Amusingly enough, the need for this loop only showed up as the patches I
was testing with started to get longer...

Looking at it now I think it would be better to fix this in gnutls,
since the SSL_write manpage does state that the data will always be
written in full.

Thanks again, I'm looking forward to some more comments on this.  I'll
refine it some more, but it's starting to look like we may need some
features added to both gcrypt and gnutls before this can be done
cleanly.  I'm definitely willing to pull out the relevant parts and
forward them to the respective projects to keep this moving.

-- 
mike :: mtmiller at ieee dot org

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html