[v2,1/2] http: add client cert for HTTPS proxies.
diff mbox series

Message ID a5d980e7501b1e0ab6f20a97136cd3a58427a139.1582759438.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Add HTTPS proxy SSL options (cert, key, cainfo)
Related show

Commit Message

Garima Singh via GitGitGadget Feb. 26, 2020, 11:23 p.m. UTC
From: Jorge Lopez Silva <jalopezsilva@gmail.com>

Git currently supports performing connections to HTTPS proxies but we
don't support doing mutual authentication with them (through TLS). This
commit adds the necessary options to be able to send a client
certificate to the HTTPS proxy.

A client certificate can provide an alternative way of authentication
instead of using 'ProxyAuthorization' or other more common methods of
authentication.

Libcurl supports this functionality already. The feature is guarded by
the first available libcurl version that supports these options.

Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>
---
 http.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Feb. 27, 2020, 6:31 p.m. UTC | #1
"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);
Jorge A López Silva March 3, 2020, 1:41 a.m. UTC | #2
> 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);

Patch
diff mbox series

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