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

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

[Elia Pinto]

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

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

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

[Junio C Hamano]

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?

> +	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

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

[Eric Sunshine]

On Wed, Aug 12, 2015 at 04:24:51PM +0200, Elia Pinto wrote:
> 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(+)

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

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

[Elia Pinto]

2015-08-12 17:16 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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.
>
>> +     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.
>
>  - 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?
>
>
>>       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
>
>>       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

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

[Junio C Hamano]

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.