Re: [PATCH v2] Add unix domain socket support to HTTP transport
From: Junio C Hamano <hidden>
Date: 2024-02-23 08:38:04
"Leslie Cheng via GitGitGadget" [off-list ref] writes:
Subject: Re: [PATCH v2] Add unix domain socket support to HTTP transport
Perhaps Subject: [PATCH] http: enable proxying via unix-domain socket to follow the usual "<area>: <description>" format?
From: Leslie Cheng <redacted> This changeset introduces an `http.unixSocket` option so that users can
"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.
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
"ie." -> "i.e.,"?
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].
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.
`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`.
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.
quoted hunk
diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt index 2d4e0c9b869..bf48cbd599a 100644 --- a/Documentation/config/http.txt +++ b/Documentation/config/http.txt@@ -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)`).
Talking about precedence between this and http.proxy is good thing, but one very important piece of information is missing. What value does it take? The absolute path of a unix-domain socket to pass the HTTP traffic over, instead of using the network. or something, perhaps?
quoted hunk
diff --git a/git-curl-compat.h b/git-curl-compat.h index fd96b3cdffd..f0f3bec0e17 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h@@ -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
The "HAVE" part in GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is a statement of a fact. If the version of cURL library we have is certain value, we have it. OK.
quoted hunk
diff --git a/http.c b/http.c index e73b136e589..8cfdcaeac82 100644 --- a/http.c +++ b/http.c@@ -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;
It might make the code easier to follow if you did:
#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
The points are
(1) the users can decline to use CURLOPT_UNIX_SOCKET_PATH while
still using unix domain socket for other purposes, and
(2) you do not have to care if you HAVE it or not most of time;
what matters more often is if the user told you to USE it.
Hmm?
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS) +static const char *curl_unix_socket_path; +#endif
The guard here would become "#ifdef USE_CURLOPT_UNIX_SOCKET_PATH" if we wanted this to be conditional, but I think it is easier to make the variable unconditionally available; see below.
quoted hunk
@@ -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 + }
In general, it is inadvisable to issue a warning in the codepath that parses configuration variables, as the values we read may not be necessarily used. We could instead accept the given path into a variable unconditionally, and complain only before it gets used, near the call to curl_easy_setopt().
quoted hunk
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
Here, the guard may become more like
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
}