Re: [PATCH v2] git-imap-send: Add support for SSL.

Alam Arias [off-list ref] writes:

Allow SSL to be used when a new config setting, imap.ssl, is set to
true.

Also, automatically use TLS when imap.ssl is not set by using the IMAP
STARTTLS command, if the server supports it.

Tested with Courier and Gimap IMAP servers.

Signed-off-by: Robert Shearman <redacted>
---
 Documentation/git-imap-send.txt |    1 +
 Makefile                        |    4 +-
 imap-send.c                     |  163
+++++++++++++++++++++++++++++++++++---- 3 files changed, 152
insertions(+), 16 deletions(-)

Next time please do _not_ attach *.diff but follow the style of patch
submission other people do (see recent patch from Linus for example).

This is "start talking plain imap to the standard imap port and then say
STARTTLS to start SSL", not "imap over ssl (aka imaps = 993/tcp)", right?

Is support for the latter (1) widely needed, and/or (2) easy to add on top
of this?  I presume the latter would use imaps:// URL scheme (in which
case the user does not need an extra config)?

This looks enough to make it link both with and without NO_OPENSSL, but
has it actually been tested with both configurations?

The original code has tons of style violations like this, but please do
not introduce more of them.  I'd even like a follow-up patch after this
one to clean up the style of existing code (you can choose to do the other
way around, first clean up the style of existing code without adding
anything new, and then this patch without the style violations).

 * function name in definition does not start a new line, but follows its
   return type on the same line;

 * open and close parentheses for function parameter list and argument
   list are not followed/preceded by any space;

 static void
 socket_perror( const char *func, Socket_t *sock, int ret )
 {
-	if (ret < 0)
-		perror( func );
+#ifndef NO_OPENSSL
+	if (sock->ssl) {
+		int sslerr = SSL_get_error( sock->ssl, ret );
+		switch (sslerr) {
+			case SSL_ERROR_NONE:
+				break;
+			case SSL_ERROR_SYSCALL:
+				perror( "SSL_connect" );
+				break;
+			default:
+				ssl_socket_perror( "SSL_connect" );
+				break;
+		}

 * "case" arms of switch statement align with "switch" without extra
   indentation;

+	/* FIXME! Add a config option for this */
+	if (0)
+		SSL_CTX_set_verify( ctx, SSL_VERIFY_PEER, NULL );

Indeed ;-).

+	if (!SSL_CTX_set_default_verify_paths( ctx )) {
+		ssl_socket_perror( "SSL_CTX_set_default_verify_paths" );
+		return 1;
+	}
+	sock->ssl = SSL_new( ctx );
+	if (!sock->ssl) {
+		ssl_socket_perror( "SSL_new" );
+		return 1;
+	}

 * We usually signal error by returning negative (e.g. -1) unless there
   otherwise a reason not to.

Hmm.  If NO_OPENSSL compilation had ".ssl" member that is a dummy "int" or
something, you can use this ifndef and it might make it easier to read.