[RFC 0/3] imap-send curl tunnelling support

From 7.21.5, curl can be tricked into using an open fd.
This series uses this to allow using curl over a tunnel.

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)
- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a socketpair but I'm not sure either :)

This series also shows a "bug" in curl.
When trying out the tunnel example fro imap-send documentation, this happends:
Starting tunnel 'ssh -q -C localhost /usr/sbin/imapd ./Maildir'... ok
sending 3 messages
16:38:54.055221 http.c:639              == Info: Hostname was NOT found in DNS cache
16:38:54.059505 http.c:639              == Info:   Trying ::1...
16:38:54.059545 http.c:639              == Info: Connected to localhost () port 143 (#0)
16:38:54.354379 http.c:586              <= Recv header, 0000000332 bytes (0x0000014c)
16:38:54.354405 http.c:598              <= Recv header: * PREAUTH [CAPABILITY IMAP4REV1 I18NLEVEL=1 LITERAL+ IDLE UIDPLUS NAMESPACE CHILDREN MAILBOX-REFERRALS BINARY UNSELECT ESEARCH WITHIN SCAN SORT THREAD=REFERENCES THREAD=ORDEREDSUBJECT MULTIAPPEND] Pre-authenticated user nmorey portia.home.nicolas.morey-chaisemartin.com IMAP4rev1 2007e.404 at Wed, 9 Aug 2017 16:38:54 +0200 (CEST)
16:38:54.354425 http.c:639              == Info: Bad tagged response
16:38:54.354448 http.c:639              == Info: Closing connection 0
curl_easy_perform() failed: FTP: weird server reply

It appears curl do not support the PREAUTH tag.

However a test with "nc imap.server.ext 143" is working fine.

Nicolas Morey-Chaisemartin (3):
  imap-send: move tunnel setup to its own function
  imap-send: use a socketpair instead of pipe to communicate with the
    tunnel
  imap_send: add support for curl over tunnel

 Documentation/git-imap-send.txt |  4 +-
 imap-send.c                     | 91 +++++++++++++++++++++++++++++++----------
 2 files changed, 72 insertions(+), 23 deletions(-)

Signed-off-by: Nicolas Morey-Chaisemartin <redacted>
---
 imap-send.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..10f668eb7 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -926,6 +926,27 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }
 
+static void setup_tunnel(struct imap_server_conf *srvc, int fds[2])
+{
+	struct child_process tunnel = CHILD_PROCESS_INIT;
+
+	imap_info("Starting tunnel '%s'... ", srvc->tunnel);
+
+	argv_array_push(&tunnel.args, srvc->tunnel);
+	tunnel.use_shell = 1;
+	tunnel.in = -1;
+	tunnel.out = -1;
+	if (start_command(&tunnel))
+		die("cannot start proxy %s", srvc->tunnel);
+
+	fds[0] = tunnel.out;
+	fds[1] = tunnel.in;
+
+	imap_info("ok\n");
+
+	return;
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;

@@ -943,21 +964,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
 	/* open connection to IMAP server */
 
 	if (srvc->tunnel) {
-		struct child_process tunnel = CHILD_PROCESS_INIT;
-
-		imap_info("Starting tunnel '%s'... ", srvc->tunnel);
-
-		argv_array_push(&tunnel.args, srvc->tunnel);
-		tunnel.use_shell = 1;
-		tunnel.in = -1;
-		tunnel.out = -1;
-		if (start_command(&tunnel))
-			die("cannot start proxy %s", srvc->tunnel);
-
-		imap->buf.sock.fd[0] = tunnel.out;
-		imap->buf.sock.fd[1] = tunnel.in;
-
-		imap_info("ok\n");
+		setup_tunnel(srvc, imap->buf.sock.fd);
 	} else {
 #ifndef NO_IPV6
 		struct addrinfo hints, *ai0, *ai;

-- 
2.14.0.3.gb4ff627ec.dirty

Starting from libcurl 7.21.5, libcurl can be tricked into using
an already open socket.
This allows to use tunneling with libcurl instead of the legacy imap code.

Signed-off-by: Nicolas Morey-Chaisemartin <redacted>
---
 Documentation/git-imap-send.txt |  4 ++--
 imap-send.c                     | 45 +++++++++++++++++++++++++++++++++++------
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 5d1e4c80c..e765c08d7 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt

@@ -38,8 +38,8 @@ OPTIONS
 	Be quiet.
 
 --curl::
-	Use libcurl to communicate with the IMAP server, unless tunneling
-	into it.  Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
+	Use libcurl to communicate with the IMAP server.
+	Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
 	option set.
 
 --no-curl::

diff --git a/imap-send.c b/imap-send.c
index e5ff70096..31b93d873 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -1408,6 +1408,26 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
+static curl_socket_t curl_tunnel_socket(void *clientp,
+					curlsocktype purpose,
+					struct curl_sockaddr *address)
+{
+	return (unsigned long)clientp;
+}
+
+static int sockopt_callback(void *clientp, curl_socket_t curlfd,
+				 curlsocktype purpose)
+{
+	/* CURL_SOCKOPT_ALREADY_CONNECTED was intreocued in 7.21.5
+	 * and is needed to get curl working on an existing fd */
+#if LIBCURL_VERSION_NUM >= 0x071505
+	return CURL_SOCKOPT_ALREADY_CONNECTED;
+#else
+	return CURL_SOCKOPT_ERROR;
+#endif
+}
+
+
 static CURL *setup_curl(struct imap_server_conf *srvc)
 {
 	CURL *curl;

@@ -1424,8 +1444,21 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
 	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
-	strbuf_addstr(&path, server.use_ssl ? "imaps://" : "imap://");
-	strbuf_addstr(&path, server.host);
+	if (srvc->tunnel) {
+		int fds[2];
+
+		setup_tunnel(srvc, fds);
+		curl_easy_setopt(curl, CURLOPT_OPENSOCKETFUNCTION, curl_tunnel_socket);
+		curl_easy_setopt(curl, CURLOPT_OPENSOCKETDATA, (unsigned long)fds[0]);
+		curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);
+		/* Create a fake hostname to avoid resolution issue and in case
+		 * imap.host was not set */
+		strbuf_addstr(&path, "imap://localhost");
+	} else {
+		strbuf_addstr(&path, server.use_ssl ? "imaps://" : "imap://");
+		strbuf_addstr(&path, server.host);
+	}
+
 	if (!path.len || path.buf[path.len - 1] != '/')
 		strbuf_addch(&path, '/');
 	strbuf_addstr(&path, server.folder);

@@ -1570,12 +1603,12 @@ int cmd_main(int argc, const char **argv)
 
 	/* write it to the imap server */
 
-	if (server.tunnel)
-		return append_msgs_to_imap(&server, &all_msgs, total);
-
 #ifdef USE_CURL_FOR_IMAP_SEND
 	if (use_curl)
-		return curl_append_msgs_to_imap(&server, &all_msgs, total);
+#if LIBCURL_VERSION_NUM < 0x071505
+		if (!server.tunnel)
+#endif
+			return curl_append_msgs_to_imap(&server, &all_msgs, total);
 #endif
 
 	return append_msgs_to_imap(&server, &all_msgs, total);

-- 
2.14.0.3.gb4ff627ec.dirty

Signed-off-by: Nicolas Morey-Chaisemartin <redacted>
---
 imap-send.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 10f668eb7..e5ff70096 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -929,18 +929,27 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 static void setup_tunnel(struct imap_server_conf *srvc, int fds[2])
 {
 	struct child_process tunnel = CHILD_PROCESS_INIT;
+	int sock_fds[2];
 
 	imap_info("Starting tunnel '%s'... ", srvc->tunnel);
 
+	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_fds))
+		die("failed to create socketpair for proxy");
+
 	argv_array_push(&tunnel.args, srvc->tunnel);
 	tunnel.use_shell = 1;
-	tunnel.in = -1;
-	tunnel.out = -1;
+	tunnel.in = sock_fds[1];
+	/* Duplicate the fd as the child process requires
+	 * 1 for stdin, one for stdout */
+	tunnel.out = dup(sock_fds[1]);
+	if (tunnel.out < 0)
+		die("failed to create fd to proxy");
+
 	if (start_command(&tunnel))
 		die("cannot start proxy %s", srvc->tunnel);
 
-	fds[0] = tunnel.out;
-	fds[1] = tunnel.in;
+	fds[0] = sock_fds[0];
+	fds[1] = sock_fds[0];
 
 	imap_info("ok\n");
 

-- 
2.14.0.3.gb4ff627ec.dirty

Ping.

I'd like to get feedback from Windows developer on patch #2
Patch#3 will probably need some updates as I expected Jeff old curl drop patches to make it in.
As it seems to be going another way a few more ifdefs will be required

Nicolas

Le 09/08/2017 à 16:43, Nicolas Morey-Chaisemartin a écrit :

From 7.21.5, curl can be tricked into using an open fd.
This series uses this to allow using curl over a tunnel.

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)
- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a socketpair but I'm not sure either :)

This series also shows a "bug" in curl.
When trying out the tunnel example fro imap-send documentation, this happends:
Starting tunnel 'ssh -q -C localhost /usr/sbin/imapd ./Maildir'... ok
sending 3 messages
16:38:54.055221 http.c:639              == Info: Hostname was NOT found in DNS cache
16:38:54.059505 http.c:639              == Info:   Trying ::1...
16:38:54.059545 http.c:639              == Info: Connected to localhost () port 143 (#0)
16:38:54.354379 http.c:586              <= Recv header, 0000000332 bytes (0x0000014c)
16:38:54.354405 http.c:598              <= Recv header: * PREAUTH [CAPABILITY IMAP4REV1 I18NLEVEL=1 LITERAL+ IDLE UIDPLUS NAMESPACE CHILDREN MAILBOX-REFERRALS BINARY UNSELECT ESEARCH WITHIN SCAN SORT THREAD=REFERENCES THREAD=ORDEREDSUBJECT MULTIAPPEND] Pre-authenticated user nmorey portia.home.nicolas.morey-chaisemartin.com IMAP4rev1 2007e.404 at Wed, 9 Aug 2017 16:38:54 +0200 (CEST)
16:38:54.354425 http.c:639              == Info: Bad tagged response
16:38:54.354448 http.c:639              == Info: Closing connection 0
curl_easy_perform() failed: FTP: weird server reply

It appears curl do not support the PREAUTH tag.

However a test with "nc imap.server.ext 143" is working fine.

Nicolas Morey-Chaisemartin (3):
  imap-send: move tunnel setup to its own function
  imap-send: use a socketpair instead of pipe to communicate with the
    tunnel
  imap_send: add support for curl over tunnel

 Documentation/git-imap-send.txt |  4 +-
 imap-send.c                     | 91 +++++++++++++++++++++++++++++++----------
 2 files changed, 72 insertions(+), 23 deletions(-)

On Tue, Aug 15, 2017 at 10:49 AM, Nicolas Morey-Chaisemartin
[off-list ref] wrote:

Ping.

I'd like to get feedback from Windows developer on patch #2
Patch#3 will probably need some updates as I expected Jeff old curl drop patches to make it in.
As it seems to be going another way a few more ifdefs will be required

+cc Windows devs

Nicolas

Le 09/08/2017 à 16:43, Nicolas Morey-Chaisemartin a écrit :

quoted

From 7.21.5, curl can be tricked into using an open fd.
This series uses this to allow using curl over a tunnel.

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)
- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a socketpair but I'm not sure either :)

This series also shows a "bug" in curl.
When trying out the tunnel example fro imap-send documentation, this happends:
Starting tunnel 'ssh -q -C localhost /usr/sbin/imapd ./Maildir'... ok
sending 3 messages
16:38:54.055221 http.c:639              == Info: Hostname was NOT found in DNS cache
16:38:54.059505 http.c:639              == Info:   Trying ::1...
16:38:54.059545 http.c:639              == Info: Connected to localhost () port 143 (#0)
16:38:54.354379 http.c:586              <= Recv header, 0000000332 bytes (0x0000014c)
16:38:54.354405 http.c:598              <= Recv header: * PREAUTH [CAPABILITY IMAP4REV1 I18NLEVEL=1 LITERAL+ IDLE UIDPLUS NAMESPACE CHILDREN MAILBOX-REFERRALS BINARY UNSELECT ESEARCH WITHIN SCAN SORT THREAD=REFERENCES THREAD=ORDEREDSUBJECT MULTIAPPEND] Pre-authenticated user nmorey portia.home.nicolas.morey-chaisemartin.com IMAP4rev1 2007e.404 at Wed, 9 Aug 2017 16:38:54 +0200 (CEST)
16:38:54.354425 http.c:639              == Info: Bad tagged response
16:38:54.354448 http.c:639              == Info: Closing connection 0
curl_easy_perform() failed: FTP: weird server reply

It appears curl do not support the PREAUTH tag.

However a test with "nc imap.server.ext 143" is working fine.

Nicolas Morey-Chaisemartin (3):
  imap-send: move tunnel setup to its own function
  imap-send: use a socketpair instead of pipe to communicate with the
    tunnel
  imap_send: add support for curl over tunnel

 Documentation/git-imap-send.txt |  4 +-
 imap-send.c                     | 91 +++++++++++++++++++++++++++++++----------
 2 files changed, 72 insertions(+), 23 deletions(-)

On Wed, Aug 09, 2017 at 04:43:26PM +0200, Nicolas Morey-Chaisemartin wrote:

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)

I'm pretty sure the answer is no, after searching a bit for mingw and
socketpair. The big question is whether we could come up with a suitable
replacement. And that would depend on how libcurl works on Windows, I
think (because it's going to feed whatever we give it to other syscall
wrappers).

- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a socketpair but I'm not sure either :)

There's not much difference in practice. The obvious one is that
half-duplex shutdowns require shutdown() on a socket and just close() on
the write half of a pipe. I don't know if we do that or not.

I'd be inclined to leave the existing code alone, though, just because
of the risk of regression (and because I don't think the curl and
non-curl versions actually share that much code). But I haven't looked
deeply, so I may be wrong.

It appears curl do not support the PREAUTH tag.

Too bad. IMHO preauth is the main reason to use a tunnel in the first
place.

-Peff

Hi,

On Tue, 15 Aug 2017, Stefan Beller wrote:

On Tue, Aug 15, 2017 at 10:49 AM, Nicolas Morey-Chaisemartin
[off-list ref] wrote:

quoted

Ping.

I'd like to get feedback from Windows developer on patch #2
Patch#3 will probably need some updates as I expected Jeff old curl drop patches to make it in.
As it seems to be going another way a few more ifdefs will be required

+cc Windows devs

I can has easy-to-pull branch, please?

Thanks,
Dscho

(Sent a reply from my phone while out of town but couldn't find it so here it is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to add the proper ifdefs to handle older curl versions.

Nicolas

Le 16/08/2017 à 14:30, Johannes Schindelin a écrit :

Hi,

On Tue, 15 Aug 2017, Stefan Beller wrote:

quoted

On Tue, Aug 15, 2017 at 10:49 AM, Nicolas Morey-Chaisemartin
[off-list ref] wrote:

quoted

Ping.

I'd like to get feedback from Windows developer on patch #2
Patch#3 will probably need some updates as I expected Jeff old curl drop patches to make it in.
As it seems to be going another way a few more ifdefs will be required

+cc Windows devs

I can has easy-to-pull branch, please?

Thanks,
Dscho

Le 16/08/2017 à 10:34, Jeff King a écrit :

On Wed, Aug 09, 2017 at 04:43:26PM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)

I'm pretty sure the answer is no, after searching a bit for mingw and
socketpair. The big question is whether we could come up with a suitable
replacement. And that would depend on how libcurl works on Windows, I
think (because it's going to feed whatever we give it to other syscall
wrappers).

That's what I feared.
I'm not sure there is a portable "anonymous socket" API out there that'll work...

quoted

- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a socketpair but I'm not sure either :)

There's not much difference in practice. The obvious one is that
half-duplex shutdowns require shutdown() on a socket and just close() on
the write half of a pipe. I don't know if we do that or not.

I'd be inclined to leave the existing code alone, though, just because
of the risk of regression (and because I don't think the curl and
non-curl versions actually share that much code). But I haven't looked
deeply, so I may be wrong.

It's easy enough to keep the legacy working without socketpair.

quoted

It appears curl do not support the PREAUTH tag.

Too bad. IMHO preauth is the main reason to use a tunnel in the first
place.

It shouldn't be too hard to add support for this in curl.
If it's the main usecase, it'll simply means the curl tunnelling should be disabled by default for older curl (in this case, meaning every version until it gets supported) versions.

Nicolas

Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin:

(Sent a reply from my phone while out of town but couldn't find it so here it is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to add the proper ifdefs to handle older curl versions.

This does not build for me on Windows due to a missing socketpair() 
function. But I am working in an old environment, so I do not know 
whether this statement has much value.

-- Hannes

This was sadly kind of expected...
I need to look for another way to handle this on Windows.

Thanks for the test

Nicolas

Le 22/08/2017 à 19:10, Johannes Sixt a écrit :

Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin:

quoted

(Sent a reply from my phone while out of town but couldn't find it so here it is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to add the proper ifdefs to handle older curl versions.

This does not build for me on Windows due to a missing socketpair() function. But I am working in an old environment, so I do not know whether this statement has much value.

-- Hannes

On Mon, Aug 21, 2017 at 09:34:19AM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

quoted

It appears curl do not support the PREAUTH tag.

Too bad. IMHO preauth is the main reason to use a tunnel in the first
place.

It shouldn't be too hard to add support for this in curl.
If it's the main usecase, it'll simply means the curl tunnelling
should be disabled by default for older curl (in this case, meaning
every version until it gets supported) versions.

Yes, I agree. I was hoping when we started this discussion that we were
more ready to switch to curl-by-default. But sadly, that isn't close to
being the case. But hopefully we can at least end up with logic that
lets us use it in the easy cases (no tunneling) and falls back in the
harder ones.

-Peff

Hi Hannes,

On Tue, 22 Aug 2017, Johannes Sixt wrote:

Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin:

quoted

(Sent a reply from my phone while out of town but couldn't find it so here
it is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to
add the proper ifdefs to handle older curl versions.

This does not build for me on Windows due to a missing socketpair() function.
But I am working in an old environment, so I do not know whether this
statement has much value.

Same problem in Git for Windows' SDK:

imap-send.c: In function 'setup_tunnel':
imap-send.c:936:6: error: implicit declaration of function 'socketpair'; did you
 mean 'socket'? [-Werror=implicit-function-declaration]
  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_fds))
      ^~~~~~~~~~
      socket
imap-send.c: In function 'curl_tunnel_socket':
imap-send.c:1416:9: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  return (unsigned long)clientp;
         ^

Le 23/08/2017 à 23:43, Jeff King a écrit :

On Mon, Aug 21, 2017 at 09:34:19AM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

quoted

quoted

It appears curl do not support the PREAUTH tag.

Too bad. IMHO preauth is the main reason to use a tunnel in the first
place.

It shouldn't be too hard to add support for this in curl.
If it's the main usecase, it'll simply means the curl tunnelling
should be disabled by default for older curl (in this case, meaning
every version until it gets supported) versions.

Yes, I agree. I was hoping when we started this discussion that we were
more ready to switch to curl-by-default. But sadly, that isn't close to
being the case. But hopefully we can at least end up with logic that
lets us use it in the easy cases (no tunneling) and falls back in the
harder ones.

-Peff

I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

At least bleeding edge curl user should be able to use this.
I'm not sure where to go with these patches now.

1) There does not seem to be an easy/clean workaround for the lack of socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it means creating a socket file somewhere which pulls a lot of potential issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)

2) The PREAUTH support won't largely be available  for a while (curl, release, distro, etc.)
- If this is the main use case, it does not make much sense to puch curl; tunneling support without this. I could push the code and only enable the curl tunneling for the next curl release ?
  Meaning no one (or close to no one) would use this until some later
  This also means very little testing (apart from mine) until the next curl version gets widely available
- If this is not the main case (or at least the non PREAUTH is important enough), it would make sense to get this changes in.
  But it would probably need some more to code to either fallback to legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and directly use the legacy mode.

Nicolas

On Thu, Aug 24, 2017 at 10:00:47AM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

Yes, I agree. I was hoping when we started this discussion that we were
more ready to switch to curl-by-default. But sadly, that isn't close to
being the case. But hopefully we can at least end up with logic that
lets us use it in the easy cases (no tunneling) and falls back in the
harder ones.

I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

Cool! That's much faster than I had expected. :)

At least bleeding edge curl user should be able to use this.
I'm not sure where to go with these patches now.

1) There does not seem to be an easy/clean workaround for the lack of socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
means creating a socket file somewhere which pulls a lot of potential
issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)

Even if you create a non-anonymous socket and connect to both ends, I'm
not sure how it works to pass that to the spawned child. IIRC, our
run_command emulation cannot pass arbitrary descriptors to the child
processes (but I don't know the details of why that is the case, or if
there are windows-specific calls we could be making to work around it).

2) The PREAUTH support won't largely be available  for a while (curl,
release, distro, etc.)
- If this is the main use case, it does not make much sense to puch
curl; tunneling support without this. I could push the code and only
enable the curl tunneling for the next curl release ?
  Meaning no one (or close to no one) would use this until some later
  This also means very little testing (apart from mine) until the next
curl version gets widely available
- If this is not the main case (or at least the non PREAUTH is
important enough), it would make sense to get this changes in.
  But it would probably need some more to code to either fallback to
legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and
directly use the legacy mode.

It seems like we should be able to hit the cases that we know work out
of the box, and just hold back the default for the others. Like:

  static int use_curl_auto(void)
  {
  #ifndef USE_CURL_FOR_IMAP_SEND
	/* not built; we cannot use it */
	return 0;
  #else
	if (srvc->tunnel) {
  #if LIBCURL_VERSION < ...
		/* no preauth support */
		return 0;
  #else
		return 1;
  #endif /* LIBCURL_VERSION < ... */
	}
	... other checks go here ...
  #endif /* USE_CURL */
  }

  ...
  int use_curl = -1; /* auto */
  ... set use_curl to 0/1 from --curl/--no-curl command line */
  if (use_curl < 0)
      use_curl = use_curl_auto();

I'm not sure what other cases are left. But over time we'd hope that
use_curl_auto() would shrink to just "return 1", at which point
everybody is using it (and we can drop the fallback code).

-Peff

On Thu, 24 Aug 2017, Jeff King wrote:

quoted

I opened a bug upstream and they already fixed this. 
https://github.com/curl/curl/pull/1820

Cool! That's much faster than I had expected. :)

Your wish is our command! =)

-- 

  / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)

Le 24/08/2017 à 15:53, Jeff King a écrit :

On Thu, Aug 24, 2017 at 10:00:47AM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

quoted

Yes, I agree. I was hoping when we started this discussion that we were
more ready to switch to curl-by-default. But sadly, that isn't close to
being the case. But hopefully we can at least end up with logic that
lets us use it in the easy cases (no tunneling) and falls back in the
harder ones.

I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

Cool! That's much faster than I had expected. :)

quoted

At least bleeding edge curl user should be able to use this.
I'm not sure where to go with these patches now.

1) There does not seem to be an easy/clean workaround for the lack of socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
means creating a socket file somewhere which pulls a lot of potential
issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)

Even if you create a non-anonymous socket and connect to both ends, I'm
not sure how it works to pass that to the spawned child. IIRC, our
run_command emulation cannot pass arbitrary descriptors to the child
processes (but I don't know the details of why that is the case, or if
there are windows-specific calls we could be making to work around it).

Well as long as we can map it on a fd, the dup2 trickery should allow to remap whatever solution we pick to stdin/stdout.
Could this code be put in a #ifndef WINDOWS ?

quoted

2) The PREAUTH support won't largely be available  for a while (curl,
release, distro, etc.)
- If this is the main use case, it does not make much sense to puch
curl; tunneling support without this. I could push the code and only
enable the curl tunneling for the next curl release ?
  Meaning no one (or close to no one) would use this until some later
  This also means very little testing (apart from mine) until the next
curl version gets widely available
- If this is not the main case (or at least the non PREAUTH is
important enough), it would make sense to get this changes in.
  But it would probably need some more to code to either fallback to
legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and
directly use the legacy mode.

It seems like we should be able to hit the cases that we know work out
of the box, and just hold back the default for the others. Like:

  static int use_curl_auto(void)
  {
  #ifndef USE_CURL_FOR_IMAP_SEND
	/* not built; we cannot use it */
	return 0;
  #else
	if (srvc->tunnel) {
  #if LIBCURL_VERSION < ...
		/* no preauth support */
		return 0;
  #else
		return 1;
  #endif /* LIBCURL_VERSION < ... */
	}
	... other checks go here ...
  #endif /* USE_CURL */
  }

  ...
  int use_curl = -1; /* auto */
  ... set use_curl to 0/1 from --curl/--no-curl command line */
  if (use_curl < 0)
      use_curl = use_curl_auto();

I'm not sure what other cases are left. But over time we'd hope that
use_curl_auto() would shrink to just "return 1", at which point
everybody is using it (and we can drop the fallback code).

This code works but I'm not that confortable getting code into master that will have been pretty much untested (I doubt there are many git pu/next user that run the bleeding edge curl on their setup)
and that may just break down once curl gets updated.
It has only been tested using the example line from imap-send man page which is a tiny coverage and I'm sure there are some IMAP server with funky interpreation of the standard out there (who said Exchange?)

Nicolas

On Thu, Aug 24, 2017 at 04:15:04PM +0200, Nicolas Morey-Chaisemartin wrote:

quoted

quoted

1) There does not seem to be an easy/clean workaround for the lack of socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
means creating a socket file somewhere which pulls a lot of potential
issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)

Even if you create a non-anonymous socket and connect to both ends, I'm
not sure how it works to pass that to the spawned child. IIRC, our
run_command emulation cannot pass arbitrary descriptors to the child
processes (but I don't know the details of why that is the case, or if
there are windows-specific calls we could be making to work around it).

Well as long as we can map it on a fd, the dup2 trickery should allow to remap whatever solution we pick to stdin/stdout.
Could this code be put in a #ifndef WINDOWS ?

Good point. So yeah, in theory you could emulate socketpair() with a
temporary path to do the rendezvous. Just bind/listen/accept in a
non-blocking way, then connect() from the same process, then close() the
listener and delete the socket path.

Of course that doesn't work if you don't have AF_UNIX in the first
place. You could always do the same trick with TCP sockets over the
loopback, but now you get the added bonus of wondering whether whoever
connected is the other half of your process. ;)

I dunno. I am well out of my range of Windows knowledge, and I don't
have a system to test on to determine whether my suggestions are going
completely off the deep end.

-Peff

On Thu, Aug 24, 2017 at 04:02:19PM +0200, Daniel Stenberg wrote:

On Thu, 24 Aug 2017, Jeff King wrote:

quoted

quoted

I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

Cool! That's much faster than I had expected. :)

Your wish is our command! =)

Oh good. While I have you here, have you given any thought to a curl
handle that has two half-duplex file descriptors, rather than a single
full-duplex socket? That would let us tunnel over pipes rather than
worrying about the portability of socketpair().

I suspect it would be quite complicated, because I imagine that lots of
internal bits of curl assume there's a single descriptor.

 / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)

Don't you land most of the fixes in curl? :)

-Peff

On Thu, 24 Aug 2017, Jeff King wrote:

Oh good. While I have you here, have you given any thought to a curl handle 
that has two half-duplex file descriptors, rather than a single full-duplex 
socket? That would let us tunnel over pipes rather than worrying about the 
portability of socketpair().

I suspect it would be quite complicated, because I imagine that lots of 
internal bits of curl assume there's a single descriptor.

Yeah, it would take quite some surgery deep down in the heart of curl to 
implement something like that. It wouldn't call it impossible but it would 
take a certain level of determination and amount of time. I presume the 
descriptor-pair would be passed in via an API so it wouldn't affect the 
connect phase. We also have decent test coverage, making an overhaul like this 
a less scary thought - as if the existing tests say OK we can be fairly 
certain there aren't any major regressions...

(I may also have forgotten some tiny detail for the moment that makes it very 
hard.)

quoted

 / daniel.haxx.se (who landed the IMAP PREAUTH fix in curl)

Don't you land most of the fixes in curl? :)

I do, but I don't expect readers of the git list to know that!

-- 

  / daniel.haxx.se