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

From: Jeff King <peff@peff.net>
Date: Aug 24, 2017, 13:53 UTC
Message-ID: <20170824135331.27wtwicjuoiyremx@sigill.intra.peff.net>
In-Reply-To: <e11d4449-8377-dbd7-3ad5-441baf7446b6@morey-chaisemartin.com>

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

Show 8 quoted lines

> > 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. :)

Show 7 quoted lines

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

Show 13 quoted lines

> 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