{"thread":{"id":"25687","subject":"Re: [PATCH] imap-send: Support SSL using GnuTLS","startedAt":"2010-11-09T15:09:41Z","lastAt":"2010-11-10T07:39:04Z","messageCount":3,"participants":["Jonathan Nieder","Mike Miller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"155494","messageId":"20101109150941.GD18960@burratino","threadId":"25687","inReplyTo":"AANLkTim=hL6ONwu1i+xN=N0vJaF21g5PSj5wdjqoEm5c@mail.gmail.com","subject":"Re: [PATCH] imap-send: Support SSL using GnuTLS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-09T15:09:41Z","receivedAt":"2010-11-09T15:09:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Mike,\n\nMike Miller wrote:\n\n> GnuTLS is an LGPL alternative to OpenSSL, required for IMAP over SSL\n> support on Debian.\n> \n> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=434599\n> \n> This patch uses the OpenSSL compatibility library of GnuTLS for the same\n> behavior with minimal differences.\n\nThat's awesome.  Just as a quality-of-implementation issue, it is good\nto be able to handle multiple backends so we can be sure we are not\nrelying on tiny implementation details.\n\n> However, GnuTLS does not provide equivalents to the base64 and md5\n> routines needed for CRAM-MD5 authentication.\n\nMm, that feels like it should be a separate patch.  It could be useful\nwhen not using an SSL lib at all.\n\nMaybe the base64 routine could share some code with base85.c?  (Just\na thought.)\n\n> When compiling, GnuTLS is only used when NO_OPENSSL is defined.\n> ---\n\nSign-off?  (See Documentation/SubmittingPatches for what I'm talking\nabout.)\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -24,6 +24,9 @@ all::\n>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n>  # This also implies BLK_SHA1.\n>  #\n> +# Define NO_GNUTLS if you do not have gnutls installed.  gnutls provides\n> +# SSL when OpenSSL is not used.\n> +#\n\nShouldn't this say \"if NO_OPENSSL is defined and you do not have GnuTLS\ninstalled\"?\n\n> @@ -1244,8 +1247,21 @@ ifndef NO_OPENSSL\n>  else\n>  \tBASIC_CFLAGS += -DNO_OPENSSL\n>  \tBLK_SHA1 = 1\n> +ifndef NO_GNUTLS\n> +\tOPENSSL_LIBSSL = -lgnutls-openssl -lgnutls -lgcrypt\n\nProbably should be indented another level to convey structure.\n\n> +\tifdef GNUTLSDIR\n> +\t\tBASIC_CFLAGS += -I$(GNUTLSDIR)/include\n> +\t\tOPENSSL_LINK = -L$(GNUTLSDIR)/$(lib) $(CC_LD_DYNPATH)$(GNUTLSDIR)/$(lib)\n> +\telse\n> +\t\tOPENSSL_LINK =\n> +\tendif\n> +\tLIB_OBJS += gnutls-base64.o gcrypt-hmac.o\n> +\tLIB_H += gnutls-base64.h gcrypt-hmac.h\n\nThis causes all git commands to link to base64 and hmac routines.\nMaybe you meant for them to be linked in to imap-send only?  See\nhttp.o for an example of this kind of thing.\n\nOn the other hand, see below...\n\n[...]\n> --- /dev/null\n> +++ b/gcrypt-hmac.c\n> @@ -0,0 +1,41 @@\n> +/*\n> + * gcrypt-hmac.c - interface wrapper to provide OpenSSL HMAC API using gcrypt.\n\nMaybe this could be useful for inclusion in gcrypt?\n\nIn git, it might make sense to treat these as routines for compat/,\nkind of like compat/basename.c.\n\n> --- /dev/null\n> +++ b/gnutls-base64.c\n> @@ -0,0 +1,197 @@\n> +/*\n> + * gnutls-base64.c - base64 encode and decode\n> + *                   adapted from GnuTLS, original copyright follows\n\nWhy is this needed?\n\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -26,10 +26,19 @@\n>  #include \"exec_cmd.h\"\n>  #include \"run-command.h\"\n>  #ifdef NO_OPENSSL\n> +#ifdef NO_GNUTLS\n>  typedef void *SSL;\n>  #else\n> +#include <gnutls/openssl.h>\n> +#include \"gnutls-base64.h\"\n> +#include \"gcrypt-hmac.h\"\n> +#undef NO_OPENSSL /* gnutls is providing an openssl API */\n> +#define SSL_VERIFY_PEER 1 /* doesn't matter */\n\nIf it doesn't matter, why set it?  (I assume it does matter but that\nits value is unimportant. ;-))\n\n> +#endif\n> +#else\n>  #include <openssl/evp.h>\n>  #include <openssl/hmac.h>\n> +#define USE_OPENSSL_REAL 1\n>  #endif\n\nNit: this means something like \"SSL_IS_OPENSSL\", right?\n\ni.e., it is not actually a request \"use such-and-such\" but a\nstatement of fact \"using such-and-such\".\n\n> @@ -307,9 +316,9 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve\n>  \tSSL_load_error_strings();\n>  \n>  \tif (use_tls_only)\n> -\t\tmeth = TLSv1_method();\n> +\t\tmeth = TLSv1_client_method();\n>  \telse\n> -\t\tmeth = SSLv23_method();\n> +\t\tmeth = SSLv23_client_method();\n\nIs there a semantic difference?  Will old versions of OpenSSL\ncontinue to work (I assume so, but...)?\n\n> @@ -321,10 +330,12 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve\n>  \tif (verify)\n>  \t\tSSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);\n>  \n> +#ifdef USE_OPENSSL_REAL\n>  \tif (!SSL_CTX_set_default_verify_paths(ctx)) {\n>  \t\tssl_socket_perror(\"SSL_CTX_set_default_verify_paths\");\n>  \t\treturn -1;\n>  \t}\n> +#endif\n\nWhy?  Could this be accomplished with a\n\n#ifndef USE_OPENSSL_REAL\n\tstatic inline int SSL_CTX_set_default_verify_paths(...\n\t{\n\t\t/* Not needed by GnuTLS */\n\t\treturn 0;\n\t}\n#endif\n\nwrapper?\n\n>  \tsock->ssl = SSL_new(ctx);\n>  \tif (!sock->ssl) {\n>  \t\tssl_socket_perror(\"SSL_new\");\n> @@ -371,9 +382,19 @@ static int socket_write(struct imap_socket *sock, const char *buf, int len)\n>  {\n>  \tint n;\n>  #ifndef NO_OPENSSL\n> -\tif (sock->ssl)\n> -\t\tn = SSL_write(sock->ssl, buf, len);\n> -\telse\n> +\tif (sock->ssl) {\n> +\t\t/* loop based on write_in_full, the gnutls implementation of\n> +\t\t * SSL_write may write a partial buffer. */\n> +\t\tint count = len;\n> +\t\tn = 0;\n> +\t\twhile (count > 0) {\n> +\t\t\tint written = SSL_write(sock->ssl, buf, count);\n> +\t\t\tif (written <= 0) break;\n> +\t\t\tcount -= written;\n> +\t\t\tbuf += written;\n> +\t\t\tn += written;\n> +\t\t}\n> +\t} else\n\nProbably a wrapper function would be nicer.\n\nThanks for a clean patch.\n\nHope that helps,\nJonathan\n"},{"id":"155564","messageId":"1289355416.3762.67.camel@tesla","threadId":"25687","inReplyTo":"20101109150941.GD18960@burratino","subject":"Re: [PATCH] imap-send: Support SSL using GnuTLS","fromName":"Mike Miller","fromEmail":"mtmiller@ieee.org","sentAt":"2010-11-10T02:16:56Z","receivedAt":"2010-11-10T02:16:56Z","isPatch":true,"sender":{"key":"mtmiller@ieee.org","avatar":null},"body":"Hi Jonathan, thanks for the feedback and newb counseling, this is my\nfirst attempt at a git contribution.\n\nOn Tue, 2010-11-09 at 09:09 -0600, Jonathan Nieder wrote: \n> \n> Mike Miller wrote:\n> \n> > However, GnuTLS does not provide equivalents to the base64 and md5\n> > routines needed for CRAM-MD5 authentication.\n> \n> Mm, that feels like it should be a separate patch.  It could be useful\n> when not using an SSL lib at all.\n> \n> Maybe the base64 routine could share some code with base85.c?  (Just\n> a thought.)\n\nFair enough.  That's a better division grouping functionality with\nlibrary dependencies, as gcrypt is needed for CRAM-MD5 and gnutls for\nSSL socket layer.\n\n> > When compiling, GnuTLS is only used when NO_OPENSSL is defined.\n> > ---\n> \n> Sign-off?  (See Documentation/SubmittingPatches for what I'm talking\n> about.)\n\nrtfm, got it :)\n\n> \n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -24,6 +24,9 @@ all::\n> >  # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n> >  # This also implies BLK_SHA1.\n> >  #\n> > +# Define NO_GNUTLS if you do not have gnutls installed.  gnutls provides\n> > +# SSL when OpenSSL is not used.\n> > +#\n> \n> Shouldn't this say \"if NO_OPENSSL is defined and you do not have GnuTLS\n> installed\"?\n\nSounds good.\n\n> > @@ -1244,8 +1247,21 @@ ifndef NO_OPENSSL\n> >  else\n> >  \tBASIC_CFLAGS += -DNO_OPENSSL\n> >  \tBLK_SHA1 = 1\n> > +ifndef NO_GNUTLS\n> > +\tOPENSSL_LIBSSL = -lgnutls-openssl -lgnutls -lgcrypt\n> \n> Probably should be indented another level to convey structure.\n\nYes.\n\n> > +\tifdef GNUTLSDIR\n> > +\t\tBASIC_CFLAGS += -I$(GNUTLSDIR)/include\n> > +\t\tOPENSSL_LINK = -L$(GNUTLSDIR)/$(lib) $(CC_LD_DYNPATH)$(GNUTLSDIR)/$(lib)\n> > +\telse\n> > +\t\tOPENSSL_LINK =\n> > +\tendif\n> > +\tLIB_OBJS += gnutls-base64.o gcrypt-hmac.o\n> > +\tLIB_H += gnutls-base64.h gcrypt-hmac.h\n> \n> This causes all git commands to link to base64 and hmac routines.\n> Maybe you meant for them to be linked in to imap-send only?  See\n> http.o for an example of this kind of thing.\n\nRight again.\n\n> [...]\n> > --- /dev/null\n> > +++ b/gcrypt-hmac.c\n> > @@ -0,0 +1,41 @@\n> > +/*\n> > + * gcrypt-hmac.c - interface wrapper to provide OpenSSL HMAC API using gcrypt.\n> \n> Maybe this could be useful for inclusion in gcrypt?\n> \n> In git, it might make sense to treat these as routines for compat/,\n> kind of like compat/basename.c.\n\nAs I was working through this part I was debating whether these should\nbe in a separate file, where to put them, etc.  Compat looked like it\nwas mostly libc-type functions so I stayed out of there.\n\nDeferring this to gcrypt to handle sounds like a great idea.\n\n> > --- /dev/null\n> > +++ b/gnutls-base64.c\n> > @@ -0,0 +1,197 @@\n> > +/*\n> > + * gnutls-base64.c - base64 encode and decode\n> > + *                   adapted from GnuTLS, original copyright follows\n> \n> Why is this needed?\n\ngnutls has generic base64 encode/decode routines but they are not\nexported as callable symbols.  The only callable base64 routines in\ngnutls are specific to PEM encoding/decoding.  There are so many base64\nimplementations out there but I don't know of any that are in the public\nAPI of a relatively common library.  Other than openssl.  I'm open to\nsuggestions.\n\nOr as you suggested above, merge with base85.c.\n\nAnother alternative would be, as with gcrypt, request gnutls to export\nthe base64 encode/decode functions, and provide an openssl compatible\ninterface while we're at it.\n\n> > --- a/imap-send.c\n> > +++ b/imap-send.c\n> > @@ -26,10 +26,19 @@\n> >  #include \"exec_cmd.h\"\n> >  #include \"run-command.h\"\n> >  #ifdef NO_OPENSSL\n> > +#ifdef NO_GNUTLS\n> >  typedef void *SSL;\n> >  #else\n> > +#include <gnutls/openssl.h>\n> > +#include \"gnutls-base64.h\"\n> > +#include \"gcrypt-hmac.h\"\n> > +#undef NO_OPENSSL /* gnutls is providing an openssl API */\n> > +#define SSL_VERIFY_PEER 1 /* doesn't matter */\n> \n> If it doesn't matter, why set it?  (I assume it does matter but that\n> its value is unimportant. ;-))\n\nSomething like that.\n\n> > +#endif\n> > +#else\n> >  #include <openssl/evp.h>\n> >  #include <openssl/hmac.h>\n> > +#define USE_OPENSSL_REAL 1\n> >  #endif\n> \n> Nit: this means something like \"SSL_IS_OPENSSL\", right?\n> \n> i.e., it is not actually a request \"use such-and-such\" but a\n> statement of fact \"using such-and-such\".\n\nYeah, I think I changed that one back and forth.  At one point I had\nchanged all \"NO_OPENSSL\"s to \"NO_OPENSSL_API\" but I switched back to\nminimize the changes and went with this check for the\nSSL_CTX_set_default_verify_paths when that ended up being the only\ndifference.\n\n> > @@ -307,9 +316,9 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve\n> >  \tSSL_load_error_strings();\n> >  \n> >  \tif (use_tls_only)\n> > -\t\tmeth = TLSv1_method();\n> > +\t\tmeth = TLSv1_client_method();\n> >  \telse\n> > -\t\tmeth = SSLv23_method();\n> > +\t\tmeth = SSLv23_client_method();\n> \n> Is there a semantic difference?  Will old versions of OpenSSL\n> continue to work (I assume so, but...)?\n\nYeah I honestly don't know enough about it to be boldly making that\nchange.  This could be conditional for gnutls, which only provides\nX_client_method and X_server_method but not X_method, which I suppose\ncan act as either end of the connection?\n\n> > @@ -321,10 +330,12 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve\n> >  \tif (verify)\n> >  \t\tSSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);\n> >  \n> > +#ifdef USE_OPENSSL_REAL\n> >  \tif (!SSL_CTX_set_default_verify_paths(ctx)) {\n> >  \t\tssl_socket_perror(\"SSL_CTX_set_default_verify_paths\");\n> >  \t\treturn -1;\n> >  \t}\n> > +#endif\n> \n> Why?  Could this be accomplished with a\n> \n> #ifndef USE_OPENSSL_REAL\n> \tstatic inline int SSL_CTX_set_default_verify_paths(...\n> \t{\n> \t\t/* Not needed by GnuTLS */\n> \t\treturn 0;\n> \t}\n> #endif\n> \n> wrapper?\n\nLooks good.\n\n> >  \tsock->ssl = SSL_new(ctx);\n> >  \tif (!sock->ssl) {\n> >  \t\tssl_socket_perror(\"SSL_new\");\n> > @@ -371,9 +382,19 @@ static int socket_write(struct imap_socket *sock, const char *buf, int len)\n> >  {\n> >  \tint n;\n> >  #ifndef NO_OPENSSL\n> > -\tif (sock->ssl)\n> > -\t\tn = SSL_write(sock->ssl, buf, len);\n> > -\telse\n> > +\tif (sock->ssl) {\n> > +\t\t/* loop based on write_in_full, the gnutls implementation of\n> > +\t\t * SSL_write may write a partial buffer. */\n> > +\t\tint count = len;\n> > +\t\tn = 0;\n> > +\t\twhile (count > 0) {\n> > +\t\t\tint written = SSL_write(sock->ssl, buf, count);\n> > +\t\t\tif (written <= 0) break;\n> > +\t\t\tcount -= written;\n> > +\t\t\tbuf += written;\n> > +\t\t\tn += written;\n> > +\t\t}\n> > +\t} else\n> \n> Probably a wrapper function would be nicer.\n\nAmusingly enough, the need for this loop only showed up as the patches I\nwas testing with started to get longer...\n\nLooking at it now I think it would be better to fix this in gnutls,\nsince the SSL_write manpage does state that the data will always be\nwritten in full.\n\nThanks again, I'm looking forward to some more comments on this.  I'll\nrefine it some more, but it's starting to look like we may need some\nfeatures added to both gcrypt and gnutls before this can be done\ncleanly.  I'm definitely willing to pull out the relevant parts and\nforward them to the respective projects to keep this moving.\n\n-- \nmike :: mtmiller at ieee dot org\n"},{"id":"155573","messageId":"20101110073904.GB9211@burratino","threadId":"25687","inReplyTo":"1289355416.3762.67.camel@tesla","subject":"Re: [PATCH] imap-send: Support SSL using GnuTLS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-10T07:39:04Z","receivedAt":"2010-11-10T07:39:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Mike,\n\nMike Miller wrote:\n\n>           I'm definitely willing to pull out the relevant parts and\n> forward them to the respective projects to keep this moving.\n\nThat's great to hear.  FWIW, although feedback from that direction\nwould indeed help keep this moving, there is no need to wait for a\nrelease including your changes before moving forward on the git side.\nIt might not be a bad idea to include workarounds in git until\nupstream changes have had some years to percolate to user systems\nanyway.\n\nCheers,\nJonathan\n"}]}