Add unix domain socket support to HTTP transport.

From: Leslie Cheng <[email protected]>

This changeset introduces an http.unixSocket option so that users can proxy their git over HTTP remotes to a unix domain socket. In terms of why, since UDS are local and git already has a local protocol: some corporate environments use a UDS to proxy requests to internal resources (ie. source control), so this change would support those use-cases. This proxy can occasionally be necessary to attach MFA tokens or client certificates for CLI tools.

The implementation leverages --unix-socket option [0] via the CURLOPT_UNIX_SOCKET_PATH flag available with libcurl [1].

GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH and NO_UNIX_SOCKETS were kept separate so that we can spit out better error messages for users if git was compiled with NO_UNIX_SOCKETS.

[0] https://curl.se/docs/manpage.html#--unix-socket
[1] https://curl.se/libcurl/c/CURLOPT_UNIX_SOCKET_PATH.html

---
Add unix domain socket support to HTTP transport.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1681%2Flcfyi%2Flcfyi%2Fadd-unix-socket-support-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1681/lcfyi/lcfyi/add-unix-socket-support-v1
Pull-Request: https://github.com/git/git/pull/1681

 Documentation/config/http.txt      |  5 ++
 git-curl-compat.h                  |  7 +++
 http.c                             | 23 +++++++++
 t/t5565-http-unix-domain-socket.sh | 80 ++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+)
 create mode 100755 t/t5565-http-unix-domain-socket.sh

index 2d4e0c9b869..bf48cbd599a 100644
@@ -277,6 +277,11 @@ http.followRedirects::
 	the base for the follow-up requests, this is generally
 	sufficient. The default is `initial`.
 
+http.unixSocket::
+	Connect through this Unix domain socket via HTTP, instead of using the
+	network. If set, this config takes precendence over `http.proxy` and
+	is incompatible with the proxy options (see `curl(1)`).
+
 http.<url>.*::
 	Any of the http.* options above can be applied selectively to some URLs.
 	For a config key to match a URL, each element of the config key is

index fd96b3cdffd..f0f3bec0e17 100644
@@ -74,6 +74,13 @@
 #define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
 #endif
 
+/**
+ * CURLOPT_UNIX_SOCKET_PATH was added in 7.40.0, released in January 2015.
+ */
+#if LIBCURL_VERSION_NUM >= 0x074000
+#define GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH 1
+#endif
+
 /**
  * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
  *

index e73b136e589..8cfdcaeac82 100644
@@ -79,6 +79,9 @@ static const char *http_proxy_ssl_ca_info;
 static struct credential proxy_cert_auth = CREDENTIAL_INIT;
 static int proxy_ssl_cert_password_required;
 
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+static const char *curl_unix_socket_path;
+#endif
 static struct {
 	const char *name;
 	long curlauth_param;

@@ -455,6 +458,20 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp("http.unixsocket", var)) {
+#ifdef GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH
+#ifndef NO_UNIX_SOCKETS
+		return git_config_string(&curl_unix_socket_path, var, value);
+#else
+		warning(_("Unix socket support unavailable in this build of Git"));
+		return 0;
+#endif
+#else
+		warning(_("Unix socket support is not supported with cURL < 7.40.0"));
+		return 0;
+#endif
+	}
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_pathname(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {

@@ -1203,6 +1220,12 @@ static CURL *get_curl_handle(void)
 	}
 	init_curl_proxy_auth(result);
 
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+	if (curl_unix_socket_path) {
+		curl_easy_setopt(result, CURLOPT_UNIX_SOCKET_PATH, curl_unix_socket_path);
+	}
+#endif
+
 	set_curl_keepalive(result);
 
 	return result;

base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b

gitgitgadget

No need for trailing `.' in commit message titles

<snip>

<snip>

Will fix in the next patch, sorry!

I considered extracting it out, but the other conditionals in this function follow a similar pattern so I didn't want to change it. However, my use here is also the first time there's both an #ifdef and nested #ifndef, which I agree makes it a bit confusing to grok.

I'm open to changing it, but I'll let it sit and marinate for a bit.

Good catch, I'll fix in the next patch. I've subbed nc out for a simple Perl script to pipe back and forth, just making sure CI is happy about this before submitting.

From: Leslie Cheng <[email protected]>

This changeset introduces an http.unixSocket option so that users can proxy their git over HTTP remotes to a unix domain socket. In terms of why, since UDS are local and git already has a local protocol: some corporate environments use a UDS to proxy requests to internal resources (ie. source control), so this change would support those use-cases. This proxy can occasionally be necessary to attach MFA tokens or client certificates for CLI tools.

The implementation leverages --unix-socket option [0] via the CURLOPT_UNIX_SOCKET_PATH flag available with libcurl [1].

GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH and NO_UNIX_SOCKETS were kept separate so that we can spit out better error messages for users if git was compiled with NO_UNIX_SOCKETS.

[0] https://curl.se/docs/manpage.html#--unix-socket
[1] https://curl.se/libcurl/c/CURLOPT_UNIX_SOCKET_PATH.html

---
Add unix domain socket support to HTTP transport

Changes since v1:

* Updated test to use Perl instead of nc to proxy between UDS and TCP socket; I chose not to split this out into a library since its use is hyper-specific and has a dependency on lib-httpd.sh

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1681%2Flcfyi%2Flcfyi%2Fadd-unix-socket-support-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1681/lcfyi/lcfyi/add-unix-socket-support-v2
Pull-Request: https://github.com/git/git/pull/1681

Range-diff vs v1:

1: 3e531632329 ! 1: 2af5cc8089b Add unix domain socket support to HTTP transport.

     @@ Metadata
      Author: Leslie Cheng <[email protected]>
      
       ## Commit message ##
     -    Add unix domain socket support to HTTP transport.
     +    Add unix domain socket support to HTTP transport
      
          This changeset introduces an `http.unixSocket` option so that users can
          proxy their git over HTTP remotes to a unix domain socket. In terms of
     @@ t/t5565-http-unix-domain-socket.sh (new)
      +	test_done
      +}
      +
     -+UDS_TO_TCP_FIFO=uds_to_tcp
     -+TCP_TO_UDS_FIFO=tcp_to_uds
     -+UDS_PID=
     -+TCP_PID=
     ++if ! test_have_prereq PERL
     ++then
     ++	skip_all='skipping http-unix-socket tests; perl not available'
     ++	test_done
     ++fi
     ++
     ++SOCKET_PROXY_PIDFILE="$(pwd)/proxy.pid"
      +UDS_SOCKET="$(pwd)/uds.sock"
     -+UNRESOLVABLE_ENDPOINT=http://localhost:4242
     ++UNRESOLVABLE_ENDPOINT=http://unresolved
      +
      +start_proxy_unix_to_tcp() {
     -+    local socket_path="$UDS_SOCKET"
     -+    local host=127.0.0.1
     -+    local port=$LIB_HTTPD_PORT
     -+
     -+    rm -f "$UDS_TO_TCP_FIFO"
     -+    rm -f "$TCP_TO_UDS_FIFO"
     -+    rm -f "$socket_path"
     -+    mkfifo "$UDS_TO_TCP_FIFO"
     -+    mkfifo "$TCP_TO_UDS_FIFO"
     -+    nc -klU "$socket_path" <tcp_to_uds >uds_to_tcp &
     -+    UDS_PID=$!
     -+
     -+    nc "$host" "$port" >tcp_to_uds <uds_to_tcp &
     -+    TCP_PID=$!
     -+
     -+    test_atexit 'stop_proxy_unix_to_tcp'
     ++	test_atexit 'stop_proxy_unix_to_tcp'
     ++
     ++	perl -Mstrict -MIO::Select -MIO::Socket::INET -MIO::Socket::UNIX -e '
     ++		my $uds_path = $ARGV[0];
     ++		my $host = $ARGV[1];
     ++		my $port = $ARGV[2];
     ++		my $pidfile = $ARGV[3];
     ++
     ++		open(my $fh, ">", $pidfile) or die "failed to create pidfile";
     ++		print $fh "$$";
     ++		close($fh);
     ++
     ++		my $uds = IO::Socket::UNIX->new(
     ++			Local => $uds_path,
     ++			Type => SOCK_STREAM,
     ++			Listen => 5,
     ++		) or die "failed to create unix domain socket";
     ++
     ++		while (my $conn = $uds->accept()) {
     ++			my $tcp_client = IO::Socket::INET->new(
     ++				PeerAddr => $host,
     ++				PeerPort => $port,
     ++				Proto => "tcp",
     ++			) or die "failed to create TCP socket";
     ++
     ++			my $sel = IO::Select->new($conn, $tcp_client);
     ++
     ++			while (my @ready = $sel->can_read(10)) {
     ++				foreach my $socket (@ready) {
     ++					my $other = ($socket == $conn) ? $tcp_client : $conn;
     ++					my $data;
     ++					my $bytes = $socket->sysread($data, 4096);
     ++
     ++					if ($bytes) {
     ++						$other->syswrite($data, $bytes);
     ++					} else {
     ++						$socket->close();
     ++					}
     ++				}
     ++			}
     ++		}
     ++	' "$UDS_SOCKET" "127.0.0.1" "$LIB_HTTPD_PORT" "$SOCKET_PROXY_PIDFILE" &
     ++	SOCKET_PROXY_PID=$!
      +}
      +
      +stop_proxy_unix_to_tcp() {
     -+    kill "$UDS_PID"
     -+    kill "$TCP_PID"
     -+    rm -f "$UDS_TO_TCP_FIFO"
     -+    rm -f "$TCP_TO_UDS_FIFO"
     ++	kill -9 "$(cat "$SOCKET_PROXY_PIDFILE")"
     ++	rm -f "$SOCKET_PROXY_PIDFILE"
     ++	rm -f "$UDS_SOCKET"
      +}
      +
      +start_httpd
     @@ t/t5565-http-unix-domain-socket.sh (new)
      +
      +# sanity check that we can't clone normally
      +test_expect_success 'cloning without UDS fails' '
     -+    test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
     ++	test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
      +'
      +
      +test_expect_success 'cloning with UDS succeeds' '
     -+    test_when_finished "rm -rf clone" &&
     ++	test_when_finished "rm -rf clone" &&
      +	test_config_global http.unixsocket "$UDS_SOCKET" &&
      +	git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
      +'
      +
      +test_expect_success 'cloning with a non-existent http proxy fails' '
     -+    git clone $HTTPD_URL/smart/repo.git clone &&
     -+    rm -rf clone &&
     -+    test_config_global http.proxy 127.0.0.1:0 &&
     -+    test_must_fail git clone $HTTPD_URL/smart/repo.git clone
     ++	git clone $HTTPD_URL/smart/repo.git clone &&
     ++	rm -rf clone &&
     ++	test_config_global http.proxy 127.0.0.1:0 &&
     ++	test_must_fail git clone $HTTPD_URL/smart/repo.git clone
      +'
      +
      +test_expect_success 'UDS socket takes precedence over http proxy' '
     -+    test_when_finished "rm -rf clone" &&
     -+    test_config_global http.proxy 127.0.0.1:0 &&
     -+    test_config_global http.unixsocket "$UDS_SOCKET" &&
     -+    git clone $HTTPD_URL/smart/repo.git clone
     ++	test_when_finished "rm -rf clone" &&
     ++	test_config_global http.proxy 127.0.0.1:0 &&
     ++	test_config_global http.unixsocket "$UDS_SOCKET" &&
     ++	git clone $HTTPD_URL/smart/repo.git clone
      +'
      +
      +test_done

 Documentation/config/http.txt      |   5 ++
 git-curl-compat.h                  |   7 ++
 http.c                             |  23 ++++++
 t/t5565-http-unix-domain-socket.sh | 109 +++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+)
 create mode 100755 t/t5565-http-unix-domain-socket.sh

index 2d4e0c9b869..bf48cbd599a 100644
@@ -277,6 +277,11 @@ http.followRedirects::
 	the base for the follow-up requests, this is generally
 	sufficient. The default is `initial`.
 
+http.unixSocket::
+	Connect through this Unix domain socket via HTTP, instead of using the
+	network. If set, this config takes precendence over `http.proxy` and
+	is incompatible with the proxy options (see `curl(1)`).
+
 http.<url>.*::
 	Any of the http.* options above can be applied selectively to some URLs.
 	For a config key to match a URL, each element of the config key is

index fd96b3cdffd..f0f3bec0e17 100644
@@ -74,6 +74,13 @@
 #define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
 #endif
 
+/**
+ * CURLOPT_UNIX_SOCKET_PATH was added in 7.40.0, released in January 2015.
+ */
+#if LIBCURL_VERSION_NUM >= 0x074000
+#define GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH 1
+#endif
+
 /**
  * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
  *

index e73b136e589..8cfdcaeac82 100644
@@ -79,6 +79,9 @@ static const char *http_proxy_ssl_ca_info;
 static struct credential proxy_cert_auth = CREDENTIAL_INIT;
 static int proxy_ssl_cert_password_required;
 
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+static const char *curl_unix_socket_path;
+#endif
 static struct {
 	const char *name;
 	long curlauth_param;

@@ -455,6 +458,20 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp("http.unixsocket", var)) {
+#ifdef GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH
+#ifndef NO_UNIX_SOCKETS
+		return git_config_string(&curl_unix_socket_path, var, value);
+#else
+		warning(_("Unix socket support unavailable in this build of Git"));
+		return 0;
+#endif
+#else
+		warning(_("Unix socket support is not supported with cURL < 7.40.0"));
+		return 0;
+#endif
+	}
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_pathname(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {

@@ -1203,6 +1220,12 @@ static CURL *get_curl_handle(void)
 	}
 	init_curl_proxy_auth(result);
 
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+	if (curl_unix_socket_path) {
+		curl_easy_setopt(result, CURLOPT_UNIX_SOCKET_PATH, curl_unix_socket_path);
+	}
+#endif
+
 	set_curl_keepalive(result);
 
 	return result;

base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b

gitgitgadget

Perhaps

Subject: [PATCH] http: enable proxying via unix-domain socket

to follow the usual "<area>: <description>" format?

"This changeset introduces" -> "Introduce". There may be other gotchas that might use help from Documentation/SubmittingPatches, but I didn't read too carefully.

Besides, it is a single patch, not a set of changes ;-).

http.unixSocket is a configuration variable. It may be confusing to use the word "option". Speaking of options, shouldn't there be a command line option that overrides the configured value?

We should honor the usual http.<url>.VARIABLE convention where http.<url>.VARIABLE that is destination-specific overrides a more generic http.VARIABLE configuration variable.

"ie." -> "i.e.,"?

There is a feature in libcURL library, that is enabled by setting the CURLOPT_UNIX_SOCKET_PATH option via the curl_easy_setopt() call, and their command line utility. You do the same to implement this feature. But when you are not adding "--unix-socket" option to any of our commands, mention of that option name makes it more confusing than necessary.

The usual way to compose a log message of this project is to

- Give an observation on how the current system work in the present tense (so no need to say "Currently X is Y", just "X is Y"), and discuss what you perceive as a problem in it.

- Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds).

- Give commands to the codebase to "become like so".

in this order.

How about following that convention, perhaps like:

In some corporate environments, the proxy server listens to a local unix domain socket for requests, instead of listening to a network port. Even though we have http.proxy (and more destination specific http.<url>.proxy) configuration variables to specify the network address/port of a proxy, that would not help if your proxy does not listen to the network.

Introduce an http.unixSocket (and http.<url>.unixSocket) configuration variables that specify the path to a unix domain socket for such a proxy. Recent versions of libcURL library added CURLOPT_UNIX_SOCKET_PATH to support "curl --unix-socket <path>"---use the same mechanism to implement it.

Unlike NO_UNIX_SOCKETS, GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is entirely internal to your implementation and not surfaced to neither the end-users or the binary packagers. Because of that, I suspect that any description that has to use that name probably falls on the other side of "too much implementation details" to be useful to help future developers..

Besides, I suspect that GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH might not be the optimum approach. See below.

	#if !defined(NO_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
	#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH)
        #define USE_CURLOPT_UNIX_SOCKET_PATH
	#endif
	#endif

		if (curl_unix_socket_path) {
	#ifdef USE_CURLOPT_UNIX_SOCKET_PATH
			curl_easy_setopt(...);
	#elif defined(NO_CURLOPT_UNIX_SOCKET_PATH) || defined(NO_UNIX_SOCKETS)
			warning(_("this build disables the unix-domain-socket feature"));
	#elif
			warning(_("your cURL library is too old"));
	#endif
		}

Clarification. I know the above is automatically achieved, given the way we have laid urlmatch foundation to allow easy parsing for configuration variables structured this way. I did not mean that you'd need to do anything special; rather, I meant that we should advertise that we do in the commit log message.

This is excellent, thanks for the guidance (and all the other suggestions prior)! I'll update in the next patch.

That's reasonable, I figured it would fit as a cover letter detail but I agree it's not relevant as a commit message that lives in the history of the project. I'll also update this in the next patch.

I like that wording, I'll update in the next patch.

Do you think this functionality is worth adding another macro to conditionally include it in the build? It felt out-of-the-way enough that we could just use the same NO_UNIX_SOCKETS macro to control for environments that don't support unix domain sockets.

!defined(NO_UNIX_SOCKETS)

Agreed in general, I was looking to other patterns for conditional variables in file, e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L66-L68

But, revisiting, this looks like an exception rather than the norm.

Similar to above, I followed what was already done for certain configuration variables (e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L485-L501),
but I agree with your feedback since this would result in constant warnings.

To summarize, I'll do the following in the next patch:
- change the wording of the commit message
- move the conditional variables and parsing to a check at
curl_easy_setopt() time

I'm still undecided on whether I should introduce another macro specifically for this functionality, and I'd like to hear your thoughts on why it might make sense.