| Message ID | a5d980e7501b1e0ab6f20a97136cd3a58427a139.1582759438.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HTTPS proxy SSL options (cert, key, cainfo) | expand |
"Jorge Lopez Silva via GitGitGadget" <gitgitgadget@gmail.com> writes: > +#if LIBCURL_VERSION_NUM >= 0x073400 > +static const char *http_proxy_ssl_cert; > +static const char *http_proxy_ssl_key; > +static const char *http_proxy_ssl_keypasswd; > +#endif > +static const char *http_proxy_ssl_ca_info; > + > static struct { > const char *name; > long curlauth_param; > @@ -365,6 +373,20 @@ static int http_options(const char *var, const char *value, void *cb) > if (!strcmp("http.proxyauthmethod", var)) > return git_config_string(&http_proxy_authmethod, var, value); > > +#if LIBCURL_VERSION_NUM >= 0x073400 > + if (!strcmp("http.proxycert", var)) > + return git_config_string(&http_proxy_ssl_cert, var, value); > + > + if (!strcmp("http.proxykey", var)) > + return git_config_string(&http_proxy_ssl_key, var, value); > + > + if (!strcmp("http.proxykeypass", var)) > + return git_config_string(&http_proxy_ssl_keypasswd, var, value); > + > + if (!strcmp("http.proxycainfo", var)) > + return git_config_string(&http_proxy_ssl_ca_info, var, value); > +#endif You may copy around your ~/.gitconfig to multiple hosts, some may have newer and others may have older versions of libcurl, so it would be OK for a version of Git built with older libcurl to at least see and parse configurations meant for newer one, if only to ignore and discard. The only two effects these #if/#endif have are (1) they save a tiny bit of memory, code and runtime cycle on an older platform and (2) they make the resuting code ugly and harder to read. I do not think that the tradeoff is worth it. > if (!strcmp("http.cookiefile", var)) > return git_config_pathname(&curl_cookie_file, var, value); > if (!strcmp("http.savecookies", var)) { > @@ -924,8 +946,14 @@ static CURL *get_curl_handle(void) > #if LIBCURL_VERSION_NUM >= 0x073400 > curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL); > #endif > - } else if (ssl_cainfo != NULL) > - curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); > + } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) { > + if (ssl_cainfo != NULL) > + curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); > +#if LIBCURL_VERSION_NUM >= 0x073400 > + if (http_proxy_ssl_ca_info != NULL) > + curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info); > +#endif > + } On this codepath, unlike the config and variable definitions, #if/#endif is absolutely necessary. In any case, the code around here is messy, but it is mostly due to the fact that the existing #if/#endif with if/elseif/... cascade was messy. The general idea is * We want to honor ssl_cainfo and http_proxy_ssl_ca_info, and use CAINFO when set, but * When http_schannel_use_ssl_cainfo is not in effect and http_ssl_backend is schannel, ssl_cainfo/http_proxy_ssl_ca_info business is completely skipped, and these two CAINFO are cleared instead. I do not know if the above is the best code structure to express that, but at least the way this patch adds code is the least noisy, I guess. > @@ -1018,9 +1046,19 @@ static CURL *get_curl_handle(void) > CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4); > #endif > #if LIBCURL_VERSION_NUM >= 0x073400 > - else if (starts_with(curl_http_proxy, "https")) > - curl_easy_setopt(result, > - CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); > + else if (starts_with(curl_http_proxy, "https")) { > + curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); > + > + if (http_proxy_ssl_cert != NULL) > + curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert); > + > + if (http_proxy_ssl_key != NULL) > + curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key); > + > + if (http_proxy_ssl_keypasswd != NULL) > + curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_keypasswd); This part is more or less straight-forward. This is a minor tangent, but I see many "var != NULL" instances used as the condition to if statements, which we tend to frown upon (instead just say "if (var) ..."). I know there are already many in the existing code in this file, but this patch is making it even worse. > + } > #endif > if (strstr(curl_http_proxy, "://")) > credential_from_url(&proxy_auth, curl_http_proxy);
> You may copy around your ~/.gitconfig to multiple hosts, some may > have newer and others may have older versions of libcurl, so it > would be OK for a version of Git built with older libcurl to at > least see and parse configurations meant for newer one, if only > to ignore and discard. > The only two effects these #if/#endif have are (1) they save a tiny > bit of memory, code and runtime cycle on an older platform and (2) > they make the resuting code ugly and harder to read. I do not think > that the tradeoff is worth it. I agree, thanks for the input. I'll remove the #if/#endif from the variables. > This part is more or less straight-forward. > This is a minor tangent, but I see many "var != NULL" instances used > as the condition to if statements, which we tend to frown upon > (instead just say "if (var) ..."). I know there are already many in > the existing code in this file, but this patch is making it even > worse. Understood, will fix! On Thu, Feb 27, 2020 at 10:31 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Jorge Lopez Silva via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > +#if LIBCURL_VERSION_NUM >= 0x073400 > > +static const char *http_proxy_ssl_cert; > > +static const char *http_proxy_ssl_key; > > +static const char *http_proxy_ssl_keypasswd; > > +#endif > > +static const char *http_proxy_ssl_ca_info; > > + > > static struct { > > const char *name; > > long curlauth_param; > > @@ -365,6 +373,20 @@ static int http_options(const char *var, const char *value, void *cb) > > if (!strcmp("http.proxyauthmethod", var)) > > return git_config_string(&http_proxy_authmethod, var, value); > > > > +#if LIBCURL_VERSION_NUM >= 0x073400 > > + if (!strcmp("http.proxycert", var)) > > + return git_config_string(&http_proxy_ssl_cert, var, value); > > + > > + if (!strcmp("http.proxykey", var)) > > + return git_config_string(&http_proxy_ssl_key, var, value); > > + > > + if (!strcmp("http.proxykeypass", var)) > > + return git_config_string(&http_proxy_ssl_keypasswd, var, value); > > + > > + if (!strcmp("http.proxycainfo", var)) > > + return git_config_string(&http_proxy_ssl_ca_info, var, value); > > +#endif > > You may copy around your ~/.gitconfig to multiple hosts, some may > have newer and others may have older versions of libcurl, so it > would be OK for a version of Git built with older libcurl to at > least see and parse configurations meant for newer one, if only > to ignore and discard. > > The only two effects these #if/#endif have are (1) they save a tiny > bit of memory, code and runtime cycle on an older platform and (2) > they make the resuting code ugly and harder to read. I do not think > that the tradeoff is worth it. > > > if (!strcmp("http.cookiefile", var)) > > return git_config_pathname(&curl_cookie_file, var, value); > > if (!strcmp("http.savecookies", var)) { > > @@ -924,8 +946,14 @@ static CURL *get_curl_handle(void) > > #if LIBCURL_VERSION_NUM >= 0x073400 > > curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL); > > #endif > > - } else if (ssl_cainfo != NULL) > > - curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); > > + } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) { > > + if (ssl_cainfo != NULL) > > + curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); > > +#if LIBCURL_VERSION_NUM >= 0x073400 > > + if (http_proxy_ssl_ca_info != NULL) > > + curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info); > > +#endif > > + } > > On this codepath, unlike the config and variable definitions, > #if/#endif is absolutely necessary. > > In any case, the code around here is messy, but it is mostly due to > the fact that the existing #if/#endif with if/elseif/... cascade was > messy. The general idea is > > * We want to honor ssl_cainfo and http_proxy_ssl_ca_info, and use > CAINFO when set, but > > * When http_schannel_use_ssl_cainfo is not in effect and > http_ssl_backend is schannel, ssl_cainfo/http_proxy_ssl_ca_info > business is completely skipped, and these two CAINFO are cleared > instead. > > I do not know if the above is the best code structure to express > that, but at least the way this patch adds code is the least noisy, > I guess. > > > @@ -1018,9 +1046,19 @@ static CURL *get_curl_handle(void) > > CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4); > > #endif > > #if LIBCURL_VERSION_NUM >= 0x073400 > > - else if (starts_with(curl_http_proxy, "https")) > > - curl_easy_setopt(result, > > - CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); > > + else if (starts_with(curl_http_proxy, "https")) { > > + curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); > > + > > + if (http_proxy_ssl_cert != NULL) > > + curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert); > > + > > + if (http_proxy_ssl_key != NULL) > > + curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key); > > + > > + if (http_proxy_ssl_keypasswd != NULL) > > + curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_keypasswd); > > This part is more or less straight-forward. > > This is a minor tangent, but I see many "var != NULL" instances used > as the condition to if statements, which we tend to frown upon > (instead just say "if (var) ..."). I know there are already many in > the existing code in this file, but this patch is making it even > worse. > > > + } > > #endif > > if (strstr(curl_http_proxy, "://")) > > credential_from_url(&proxy_auth, curl_http_proxy);
diff --git a/http.c b/http.c index 00a0e507633..88782d39f15 100644 --- a/http.c +++ b/http.c @@ -86,6 +86,14 @@ static long curl_low_speed_time = -1; static int curl_ftp_no_epsv; static const char *curl_http_proxy; static const char *http_proxy_authmethod; + +#if LIBCURL_VERSION_NUM >= 0x073400 +static const char *http_proxy_ssl_cert; +static const char *http_proxy_ssl_key; +static const char *http_proxy_ssl_keypasswd; +#endif +static const char *http_proxy_ssl_ca_info; + static struct { const char *name; long curlauth_param; @@ -365,6 +373,20 @@ static int http_options(const char *var, const char *value, void *cb) if (!strcmp("http.proxyauthmethod", var)) return git_config_string(&http_proxy_authmethod, var, value); +#if LIBCURL_VERSION_NUM >= 0x073400 + if (!strcmp("http.proxycert", var)) + return git_config_string(&http_proxy_ssl_cert, var, value); + + if (!strcmp("http.proxykey", var)) + return git_config_string(&http_proxy_ssl_key, var, value); + + if (!strcmp("http.proxykeypass", var)) + return git_config_string(&http_proxy_ssl_keypasswd, var, value); + + if (!strcmp("http.proxycainfo", var)) + return git_config_string(&http_proxy_ssl_ca_info, var, value); +#endif + if (!strcmp("http.cookiefile", var)) return git_config_pathname(&curl_cookie_file, var, value); if (!strcmp("http.savecookies", var)) { @@ -924,8 +946,14 @@ static CURL *get_curl_handle(void) #if LIBCURL_VERSION_NUM >= 0x073400 curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL); #endif - } else if (ssl_cainfo != NULL) - curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); + } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) { + if (ssl_cainfo != NULL) + curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); +#if LIBCURL_VERSION_NUM >= 0x073400 + if (http_proxy_ssl_ca_info != NULL) + curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info); +#endif + } if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) { curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT, @@ -1018,9 +1046,19 @@ static CURL *get_curl_handle(void) CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4); #endif #if LIBCURL_VERSION_NUM >= 0x073400 - else if (starts_with(curl_http_proxy, "https")) - curl_easy_setopt(result, - CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); + else if (starts_with(curl_http_proxy, "https")) { + curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS); + + if (http_proxy_ssl_cert != NULL) + curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert); + + if (http_proxy_ssl_key != NULL) + curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key); + + if (http_proxy_ssl_keypasswd != NULL) + curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_keypasswd); + + } #endif if (strstr(curl_http_proxy, "://")) credential_from_url(&proxy_auth, curl_http_proxy);