threads / patch / 40069

v2http: add support for specifying the SSL version

Subject: [PATCH v2] http: add support for specifying the SSL version

## tl;dr

5 messages between Aug 12, 2015 and Aug 13, 2015. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Elia Pinto· Aug 12, 2015, 14:24 UTC · lore

Teach git about a new option, "http.sslVersion", which permits one to specify the SSL version to use when negotiating SSL connections. The setting can be overridden by the GIT_SSL_VERSION environment variable.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
This is the second version. I moved out of the else clause from the #ifdef.
 Documentation/config.txt               | 21 +++++++++++++++++++++
 contrib/completion/git-completion.bash |  1 +
 http.c                                 | 31 +++++++++++++++++++++++++++++--
 3 files changed, 51 insertions(+), 2 deletions(-)
Show changes to 3 files +51 −2

Documentation/config.txt, contrib/completion/git-completion.bash, http.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 315f271..76a4f2b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1595,6 +1595,27 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslVersion::
+	The SSL version to use when negotiating an SSL connection, if you
+	want to force the default.  The available and default version depend on
+	whether libcurl was built against NSS or OpenSSL and the particular configuration
+	of the crypto library in use. Internally this sets the 'CURLOPT_SSL_VERSION'
+	option; see the libcurl documentation for more details on the format
+	of this option and for the ssl version supported. Actually the possible values
+	of this option are:
+
+	- sslv2
+	- sslv3
+	- tlsv1
+	- tlsv1.0
+	- tlsv1.1
+	- tlsv1.2
++
+Can be overridden by the 'GIT_SSL_VERSION' environment variable.
+To force git to use libcurl's default ssl version and ignore any
+explicit http.sslversion option, set 'GIT_SSL_VERSION' to the
+empty string.
+
 http.sslCipherList::
   A list of SSL ciphers to use when negotiating an SSL connection.
   The available ciphers depend on whether libcurl was built against
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c97c648..6e9359c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2118,6 +2118,7 @@ _git_config ()
 		http.postBuffer
 		http.proxy
 		http.sslCipherList
+		http.sslVersion
 		http.sslCAInfo
 		http.sslCAPath
 		http.sslCert
diff --git a/http.c b/http.c
index e9c6fdd..1504005 100644
--- a/http.c
+++ b/http.c
@@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
 static const char *ssl_cipherlist;
+static const char *ssl_version;
+static long sslversion = CURL_SSLVERSION_DEFAULT;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -190,6 +192,8 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.sslcipherlist", var))
 		return git_config_string(&ssl_cipherlist, var, value);
+	if (!strcmp("http.sslversion", var))
+		return git_config_string(&ssl_version, var, value);
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -364,13 +368,36 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+
+	if (getenv("GIT_SSL_VERSION"))
+		ssl_version = getenv("GIT_SSL_VERSION");
+	if (ssl_version != NULL && *ssl_version) {
+		if (!strcmp(ssl_version,"tlsv1")) {
+			sslversion = CURL_SSLVERSION_TLSv1;
+		} else if (!strcmp(ssl_version,"sslv2")) {
+			sslversion = CURL_SSLVERSION_SSLv2;
+		} else if (!strcmp(ssl_version,"sslv3")) {
+			sslversion = CURL_SSLVERSION_SSLv3;
+#if LIBCURL_VERSION_NUM >= 0x072200
+		} else if (!strcmp(ssl_version,"tlsv1.0")) {
+			sslversion = CURL_SSLVERSION_TLSv1_0;
+		} else if (!strcmp(ssl_version,"tlsv1.1")) {
+			sslversion = CURL_SSLVERSION_TLSv1_1;
+		} else if (!strcmp(ssl_version,"tlsv1.2")) {
+			sslversion = CURL_SSLVERSION_TLSv1_2;
+#endif
+		} else {
+			warning("unsupported ssl version %s : using default",
+			ssl_version);
+		}
+        }
+	curl_easy_setopt(result, CURLOPT_SSLVERSION,
+				sslversion);
 	if (getenv("GIT_SSL_CIPHER_LIST"))
 		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
-
 	if (ssl_cipherlist != NULL && *ssl_cipherlist)
 		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
 				ssl_cipherlist);
-
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.5.0.234.gefc8a62
Junio C Hamano· Aug 12, 2015, 15:16 UTC · re: Elia Pinto · lore

Re: [PATCH v2] http: add support for specifying the SSL version

Elia Pinto <gitter.spiros@gmail.com> writes:
Show 10 quoted lines
> diff --git a/http.c b/http.c
> index e9c6fdd..1504005 100644
> --- a/http.c
> +++ b/http.c
> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
>  static int curl_ssl_try;
>  static const char *ssl_cert;
>  static const char *ssl_cipherlist;
> +static const char *ssl_version;
> +static long sslversion = CURL_SSLVERSION_DEFAULT;

I think you shouldn't have to initialize this variable. See below.

>  		init_curl_http_auth(result);
>  
> +
An unnecessary double blank?
Show 24 quoted lines
> +	if (getenv("GIT_SSL_VERSION"))
> +		ssl_version = getenv("GIT_SSL_VERSION");
> +	if (ssl_version != NULL && *ssl_version) {
> +		if (!strcmp(ssl_version,"tlsv1")) {
> +			sslversion = CURL_SSLVERSION_TLSv1;
> +		} else if (!strcmp(ssl_version,"sslv2")) {
> +			sslversion = CURL_SSLVERSION_SSLv2;
> +		} else if (!strcmp(ssl_version,"sslv3")) {
> +			sslversion = CURL_SSLVERSION_SSLv3;
> +#if LIBCURL_VERSION_NUM >= 0x072200
> +		} else if (!strcmp(ssl_version,"tlsv1.0")) {
> +			sslversion = CURL_SSLVERSION_TLSv1_0;
> +		} else if (!strcmp(ssl_version,"tlsv1.1")) {
> +			sslversion = CURL_SSLVERSION_TLSv1_1;
> +		} else if (!strcmp(ssl_version,"tlsv1.2")) {
> +			sslversion = CURL_SSLVERSION_TLSv1_2;
> +#endif
> +		} else {
> +			warning("unsupported ssl version %s : using default",
> +			ssl_version);
> +		}
> +        }
> +	curl_easy_setopt(result, CURLOPT_SSLVERSION,
> +				sslversion);
A few problems:
 - Why do we even have to call this when sslversion is not given by
   the end user, either from the environment or from the config?
   Wouldn't we use the default version if we do not make this call?
 - It is true that 0x072200 is described as introducing these three
   in [*1*] but the structure is maintenance burden waiting to
   happen.  If you #if/#endif based on defined(CURL_SSLVERSION_$v),
   it will be obvious to other people how to add their future
   favourite versions in their patches.  Also it may be read better
   if you separated the logic and the code by using a table like
   this:
   static struct {
   	const char *name;
        long ssl_version;
   } sslversions[] = {
	{ "tlsv1", CURL_SSLVERSION_TLSv1 },
	{ "sslv2", CURL_SSLVERSION_SSLv2 },
        ...
   #ifdef CURL_SSLVERSION_TLSv1_0
	{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
   #endif
   	...,
        { NULL }
   };
>  	if (getenv("GIT_SSL_CIPHER_LIST"))
>  		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -

This blank removal is understandable (i.e. group related things together).

>  	if (ssl_cipherlist != NULL && *ssl_cipherlist)
>  		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
>  				ssl_cipherlist);
> -
This is not.
>  	if (ssl_cert != NULL)
>  		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>  	if (has_cert_password())
[References]
*1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
Elia Pinto· Aug 13, 2015, 14:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] http: add support for specifying the SSL version

2015-08-12 17:16 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 21 quoted lines
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> diff --git a/http.c b/http.c
>> index e9c6fdd..1504005 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
>>  static int curl_ssl_try;
>>  static const char *ssl_cert;
>>  static const char *ssl_cipherlist;
>> +static const char *ssl_version;
>> +static long sslversion = CURL_SSLVERSION_DEFAULT;
>
> I think you shouldn't have to initialize this variable.
> See below.
>
>>               init_curl_http_auth(result);
>>
>> +
>
> An unnecessary double blank?
I will fix this and the typo in the next patch.
Show 31 quoted lines
>
>> +     if (getenv("GIT_SSL_VERSION"))
>> +             ssl_version = getenv("GIT_SSL_VERSION");
>> +     if (ssl_version != NULL && *ssl_version) {
>> +             if (!strcmp(ssl_version,"tlsv1")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1;
>> +             } else if (!strcmp(ssl_version,"sslv2")) {
>> +                     sslversion = CURL_SSLVERSION_SSLv2;
>> +             } else if (!strcmp(ssl_version,"sslv3")) {
>> +                     sslversion = CURL_SSLVERSION_SSLv3;
>> +#if LIBCURL_VERSION_NUM >= 0x072200
>> +             } else if (!strcmp(ssl_version,"tlsv1.0")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_0;
>> +             } else if (!strcmp(ssl_version,"tlsv1.1")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_1;
>> +             } else if (!strcmp(ssl_version,"tlsv1.2")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_2;
>> +#endif
>> +             } else {
>> +                     warning("unsupported ssl version %s : using default",
>> +                     ssl_version);
>> +             }
>> +        }
>> +     curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> +                             sslversion);
>
> A few problems:
>
>  - Why do we even have to call this when sslversion is not given by
>    the end user, either from the environment or from the config?
>    Wouldn't we use the default version if we do not make this call?
Right. I will fix.
Show 23 quoted lines
>
>  - It is true that 0x072200 is described as introducing these three
>    in [*1*] but the structure is maintenance burden waiting to
>    happen.  If you #if/#endif based on defined(CURL_SSLVERSION_$v),
>    it will be obvious to other people how to add their future
>    favourite versions in their patches.  Also it may be read better
>    if you separated the logic and the code by using a table like
>    this:
>
>    static struct {
>         const char *name;
>         long ssl_version;
>    } sslversions[] = {
>         { "tlsv1", CURL_SSLVERSION_TLSv1 },
>         { "sslv2", CURL_SSLVERSION_SSLv2 },
>         ...
>    #ifdef CURL_SSLVERSION_TLSv1_0
>         { "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
>    #endif
>         ...,
>         { NULL }
>    };
>

unfortunately they are enum constant (not #defined ) and I don't know a smart way in C to make this change with the preprocessor. Would you have any idea?

Show 15 quoted lines
>
>
>>       if (getenv("GIT_SSL_CIPHER_LIST"))
>>               ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
>> -
>
> This blank removal is understandable (i.e. group related things
> together).
>
>>       if (ssl_cipherlist != NULL && *ssl_cipherlist)
>>               curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
>>                               ssl_cipherlist);
>> -
>
> This is not.
I will fix.
Thank you very much
Show 8 quoted lines
>
>>       if (ssl_cert != NULL)
>>               curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>>       if (has_cert_password())
>
> [References]
>
> *1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
Junio C Hamano· Aug 13, 2015, 14:58 UTC · re: Elia Pinto · lore

Re: [PATCH v2] http: add support for specifying the SSL version

On Thu, Aug 13, 2015 at 7:30 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
>>
> unfortunately they are enum constant (not #defined)

Ahh, then checking LIBCURL_VERSION_NUM is unfortunately the best we could do. Sorry for the noise.

You still can go with the table-driven approach, though.
Eric Sunshine· Aug 12, 2015, 18:20 UTC · re: Elia Pinto · lore

Re: [PATCH v2] http: add support for specifying the SSL version

On Wed, Aug 12, 2015 at 04:24:51PM +0200, Elia Pinto wrote:
Show 32 quoted lines
> Teach git about a new option, "http.sslVersion", which permits one to
> specify the SSL version  to use when negotiating SSL connections.  The
> setting can be overridden by the GIT_SSL_VERSION environment
> variable.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 315f271..76a4f2b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1595,6 +1595,27 @@ http.saveCookies::
> +http.sslVersion::
> +	The SSL version to use when negotiating an SSL connection, if you
> +	want to force the default.  The available and default version depend on
> +	whether libcurl was built against NSS or OpenSSL and the particular configuration
> +	of the crypto library in use. Internally this sets the 'CURLOPT_SSL_VERSION'
> +	option; see the libcurl documentation for more details on the format
> +	of this option and for the ssl version supported. Actually the possible values
> +	of this option are:
> +
> +	- sslv2
> +	- sslv3
> +	- tlsv1
> +	- tlsv1.0
> +	- tlsv1.1
> +	- tlsv1.2
> ++
> +Can be overridden by the 'GIT_SSL_VERSION' environment variable.
> +To force git to use libcurl's default ssl version and ignore any
> +explicit http.sslversion option, set 'GIT_SSL_VERSION' to the
> +empty string.

Unfortunately, this won't format properly in Asciidoc; the final paragraph will be indented as part of the itemized list supported SSL versions. Here's a squash-in to fix it:

---- 8< ----
Subject: [PATCH] fixup! http: add support for specifying the SSL version
---
 Documentation/config.txt | 1 +
 1 file changed, 1 insertion(+)
Show changes to Documentation/config.txt +1 −0
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 76a4f2b..b23b01a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1610,6 +1610,7 @@ http.sslVersion::
 	- tlsv1.0
 	- tlsv1.1
 	- tlsv1.2
+
 +
 Can be overridden by the 'GIT_SSL_VERSION' environment variable.
 To force git to use libcurl's default ssl version and ignore any
-- 
2.5.0.276.gf5e568e

← back to recent threads