Skip to contentSkip to conversation

git-imap-send: Add support for SSL.

PATCH New Rob Shearman 3 participants 3 messages Jun 15, 2016

Subsystems (2)
Documentation Kbuild
lore
Share & tools
lore

Rob Shearman

author
#0 lore raw

PATCH 1/1 git-imap-send: Add support for SSL.

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 <[email protected]>
---
 Documentation/git-imap-send.txt |    1 +
 Makefile                        |    4 +-
 imap-send.c                     |  165 +++++++++++++++++++++++++++++++++++----
 3 files changed, 153 insertions(+), 17 deletions(-)
Documentation/git-imap-send.txt +1 -0
index 522b73c..1c93339 100644
@@ -41,6 +41,7 @@ configuration file (shown with examples):
     User = bob
     Pass = pwd
     Port = 143
+    Ssl  = false
 ..........................
Makefile +3 -1
index cce5a6e..e3950ac 100644
@@ -1118,7 +1118,9 @@ endif
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
-git-imap-send$X: imap-send.o $(LIB_FILE)
+git-imap-send$X: imap-send.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
 
 http.o http-walker.o http-push.o transport.o: http.h
imap-send.c +149 -16

272 lines not on this page · raw message

#1 lore raw

PATCH v2 1/1 git-imap-send: Add support for SSL.

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 <[email protected]>
---
 Documentation/git-imap-send.txt |    1 +
 Makefile                        |    4 +-
 imap-send.c                     |  163

+++++++++++++++++++++++++++++++++++---- 3 files changed, 152 insertions(+), 16 deletions(-)

Documentation/git-imap-send.txt +1 -0
index f4fdc24..ecf2958 100644
@@ -41,6 +41,7 @@ configuration file (shown with examples):
     User = bob
     Pass = pwd
     Port = 143
+    Ssl  = false
 ..........................
Makefile +3 -1
index 6a31c9f..0bd18fa 100644
@@ -1157,7 +1157,9 @@ endif
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
-git-imap-send$X: imap-send.o $(LIB_FILE)
+git-imap-send$X: imap-send.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
 
 http.o http-walker.o http-push.o transport.o: http.h
imap-send.c +148 -15

271 lines not on this page · raw message

#2 lore raw
Quoted 8 lines · Alam Arias

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.

Quoted 8 lines
Signed-off-by Robert Shearman <[email protected]>
---
 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).

Documentation/git-imap-send.txt configuration file (shown with examples): · line 44

Quoted 10 lines
Documentation/git-imap-send.txt +1 -0
index f4fdc24..ecf2958 100644
@@ -41,6 +41,7 @@ configuration file (shown with examples):
     User = bob
     Pass = pwd
     Port = 143
+    Ssl  = false
..........................

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)?

Makefile endif · line 1162

Quoted 12 lines
Makefile +3 -1
index 6a31c9f..0bd18fa 100644
@@ -1157,7 +1157,9 @@ endif
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
-git-imap-send$X: imap-send.o $(LIB_FILE)
+git-imap-send$X: imap-send.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)

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

Quoted 5 lines
imap-send.c +0 -0
index 1ec1310..7c95c5c 100644

imap-send.c static const char *Flags[] = { · line 244

Quoted 10 lines
imap-send.c +7 -0
@@ -225,19 +235,104 @@ static const char *Flags[] = {
 	"Deleted",
 };
 
+#ifndef NO_OPENSSL
+static void
+ssl_socket_perror( const char *func )
+{
+	fprintf( stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), 0));
+}
+#endif

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;

Quoted 8 lines
imap-send.c +3 -2
 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 );

imap-send.c static const char *Flags[] = { · line 261

Quoted 10 lines
imap-send.c +10 -0
+		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;

imap-send.c static const char *Flags[] = { · line 300

Quoted 3 lines
imap-send.c +3 -0
+	/* FIXME! Add a config option for this */
+	if (0)
+		SSL_CTX_set_verify( ctx, SSL_VERIFY_PEER, NULL );

Indeed ;-).

imap-send.c static const char *Flags[] = { · line 310

Quoted 9 lines
imap-send.c +9 -0
+	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.

imap-send.c imap_open_store( imap_server_conf_t *srvc ) · line 1147

Quoted 8 lines
imap-send.c +3 -1
@@ -1014,7 +1142,10 @@ imap_open_store( imap_server_conf_t *srvc )
 			fprintf( stderr, "Skipping account %s@%s, server forbids LOGIN\n", srvc->user, srvc->host );
 			goto bail;
 		}
-		imap_warn( "*** IMAP Warning *** Password is being sent in the clear\n" );
+#ifndef NO_OPENSSL
+		if (!imap->buf.sock.ssl)
+#endif

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.