threads / patch / 40080

v3http: add support for specifying the SSL version

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

## tl;dr

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

replies: 12people: 5as markdown or json

Elia Pinto· Aug 13, 2015, 15:28 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 third version of the patch. The changes compared to the previous version are:
- Eliminated the unnecessary blank (Junio)
- Place a structure to associate mnemonic names with the curl enum constant (Junio)
- Eliminated the invocation to curl_easy_setopt to set the default SSL value. Also removed the static global variable.
  (Junio)
- Slight correction in config.txt (Eric)
 Documentation/config.txt               | 22 ++++++++++++++++++++++
 contrib/completion/git-completion.bash |  1 +
 http.c                                 | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 1 deletion(-)
Show changes to 3 files +54 −1

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 315f271..b23b01a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1595,6 +1595,28 @@ 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..d5fecd6 100644
--- a/http.c
+++ b/http.c
@@ -37,6 +37,21 @@ 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 struct {
+	const char *name;
+	long ssl_version;
+	} sslversions[] = {
+		{ "sslv2", CURL_SSLVERSION_SSLv2 },
+		{ "sslv3", CURL_SSLVERSION_TLSv1 },
+		{ "tlsv1", CURL_SSLVERSION_TLSv1 },
+#if LIBCURL_VERSION_NUM >= 0x072200
+		{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
+		{ "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
+		{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
+#endif
+		{ NULL }
+};
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -190,6 +205,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,9 +381,22 @@ 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) {
+		int i;
+		for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
+			if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
+				curl_easy_setopt(result, CURLOPT_SSLVERSION,
+					sslversions[i].ssl_version);
+				break;
+		}
+		if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
+							ssl_version);
+	}
+
 	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);
-- 
2.5.0.234.gefc8a62.dirty
Eric Sunshine· Aug 13, 2015, 15:47 UTC · re: Elia Pinto · lore

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

On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
Show 8 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>
> ---
> This is the third version of the patch. The changes compared to the previous version are:
Looks better. A few comments below...
Show 14 quoted lines
> 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
> @@ -364,9 +381,22 @@ 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) {
> +               int i;
> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {

This sort of loop is normally either handled by indexing up to a limit (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel (NULL, in this case). It is redundant to use both, as this code does. The former (using ARRAY_SIZE) is typically employed when you know the number of items upfront, such as when the item list is local and compiled in; the latter (NULL sentinel) is typically used when receiving an item list as an argument to a function where you don't know the item count upfront (and the item count is not passed to the function as a separate argument).

In this case, the item list is local and its size is known to the compiler, so that suggests using ARRAY_SIZE, and dropping the NULL sentinel.

Style aside: This 'if' statement is very wide and likely should be wrapped over multiple lines (trying to keep the code within an 80-column limit).

Show 6 quoted lines
> +                               curl_easy_setopt(result, CURLOPT_SSLVERSION,
> +                                       sslversions[i].ssl_version);
> +                               break;
> +               }
> +               if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
> +                                                       ssl_version);

Style: Drop spaces inside 'if' parentheses. Place warning() on its own line.

Show 10 quoted lines
> +       }
> +
>         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);
> --
> 2.5.0.234.gefc8a62.dirty
Elia Pinto· Aug 13, 2015, 15:58 UTC · re: Eric Sunshine · lore

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

2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
Show 30 quoted lines
> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@gmail.com> 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>
>> ---
>> This is the third version of the patch. The changes compared to the previous version are:
>
> Looks better. A few comments below...
>
>> 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
>> @@ -364,9 +381,22 @@ 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) {
>> +               int i;
>> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>
> This sort of loop is normally either handled by indexing up to a limit
> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
> (NULL, in this case). It is redundant to use both, as this code does.

I do not think. sslversions[i].name can be null, see how the structure is initialized. No ?

The other your observations written below are ok for me, but i will wait for your answer on this before you send another revision. Thank you very much.

Show 14 quoted lines
> The former (using ARRAY_SIZE) is typically employed when you know the
> number of items upfront, such as when the item list is local and
> compiled in; the latter (NULL sentinel) is typically used when
> receiving an item list as an argument to a function where you don't
> know the item count upfront (and the item count is not passed to the
> function as a separate argument).
>
> In this case, the item list is local and its size is known to the
> compiler, so that suggests using ARRAY_SIZE, and dropping the NULL
> sentinel.
>
> Style aside: This 'if' statement is very wide and likely should be
> wrapped over multiple lines (trying to keep the code within an
> 80-column limit).
ok.
Show 11 quoted lines
>
>> +                               curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> +                                       sslversions[i].ssl_version);
>> +                               break;
>> +               }
>> +               if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
>> +                                                       ssl_version);
>
> Style:
> Drop spaces inside 'if' parentheses.
> Place warning() on its own line.
ok.
Show 11 quoted lines
>
>> +       }
>> +
>>         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);
>> --
>> 2.5.0.234.gefc8a62.dirty
Eric Sunshine· Aug 13, 2015, 16:11 UTC · re: Elia Pinto · lore

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

On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
Show 33 quoted lines
> 2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@gmail.com> 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>
>>> ---
>>> This is the third version of the patch. The changes compared to the previous version are:
>>
>> Looks better. A few comments below...
>>
>>> 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
>>> @@ -364,9 +381,22 @@ 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) {
>>> +               int i;
>>> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>
>> This sort of loop is normally either handled by indexing up to a limit
>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>> (NULL, in this case). It is redundant to use both, as this code does.
> I do not think. sslversions[i].name can be null, see how the structure
> is initialized. No ?
The initialization:
    static struct {
       const char *name;
       long ssl_version;
       } sslversions[] = {
           { "sslv2", CURL_SSLVERSION_SSLv2 },
           ...
           { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
           { NULL }
    };

terminates the list with a NULL sentinel entry, which does indeed set sslversions[i].name to NULL. When you know the item count ahead of time (as you do in this case), this sort of end-of-list sentinel is redundant, and complicates the code unnecessarily. For instance, the 'sslversions[i].name != NULL' expression in the 'if':

    if (sslversions[i].name != NULL && *sslversions[i].name ...

is an unwanted complication. In fact, the '*sslversions[i].name' expression is also unnecessary.

Elia Pinto· Aug 13, 2015, 16:15 UTC · re: Eric Sunshine · lore

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

2015-08-13 18:11 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
Show 57 quoted lines
> On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
>> 2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>>> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@gmail.com> 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>
>>>> ---
>>>> This is the third version of the patch. The changes compared to the previous version are:
>>>
>>> Looks better. A few comments below...
>>>
>>>> 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
>>>> @@ -364,9 +381,22 @@ 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) {
>>>> +               int i;
>>>> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>>> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>>
>>> This sort of loop is normally either handled by indexing up to a limit
>>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>>> (NULL, in this case). It is redundant to use both, as this code does.
>> I do not think. sslversions[i].name can be null, see how the structure
>> is initialized. No ?
>
> The initialization:
>
>     static struct {
>        const char *name;
>        long ssl_version;
>        } sslversions[] = {
>            { "sslv2", CURL_SSLVERSION_SSLv2 },
>            ...
>            { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>            { NULL }
>     };
>
> terminates the list with a NULL sentinel entry, which does indeed set
> sslversions[i].name to NULL. When you know the item count ahead of
> time (as you do in this case), this sort of end-of-list sentinel is
> redundant, and complicates the code unnecessarily. For instance, the
> 'sslversions[i].name != NULL' expression in the 'if':
>
>     if (sslversions[i].name != NULL && *sslversions[i].name ...
>
> is an unwanted complication. In fact, the '*sslversions[i].name'
> expression is also unnecessary.

I agree. But this is what suggested me Junio: =). What do I have to do ? It becomes difficult to keep everyone happy: =)

Junio ?
Thanks
Eric Sunshine· Aug 13, 2015, 16:37 UTC · re: Elia Pinto · lore

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

On Thu, Aug 13, 2015 at 12:15 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:
Show 38 quoted lines
> 2015-08-13 18:11 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
>>> 2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>>>>> +       if (ssl_version != NULL && *ssl_version) {
>>>>> +               int i;
>>>>> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>>>> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>>>
>>>> This sort of loop is normally either handled by indexing up to a limit
>>>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>>>> (NULL, in this case). It is redundant to use both, as this code does.
>>> I do not think. sslversions[i].name can be null, see how the structure
>>> is initialized. No ?
>>
>> The initialization:
>>
>>     static struct {
>>        const char *name;
>>        long ssl_version;
>>        } sslversions[] = {
>>            { "sslv2", CURL_SSLVERSION_SSLv2 },
>>            ...
>>            { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>>            { NULL }
>>     };
>>
>> terminates the list with a NULL sentinel entry, which does indeed set
>> sslversions[i].name to NULL. When you know the item count ahead of
>> time (as you do in this case), this sort of end-of-list sentinel is
>> redundant, and complicates the code unnecessarily. For instance, the
>> 'sslversions[i].name != NULL' expression in the 'if':
>>
>>     if (sslversions[i].name != NULL && *sslversions[i].name ...
>>
>> is an unwanted complication. In fact, the '*sslversions[i].name'
>> expression is also unnecessary.
> I agree. But this is what  suggested me Junio: =). What do I have to do ?
> It becomes difficult to keep everyone happy: =)

You're referring to [1] in which Junio's example table initialization had the NULL sentinel. That approach is fine, and my earlier comment:

    This sort of loop is normally either handled by indexing up to a
    limit (ARRAY_SIZE, in this case) or by iterating until hitting a
    sentinel (NULL, in this case). It is redundant to use both...

wasn't saying that you shouldn't use the NULL sentinel. It said only that you should choose one approach rather than complicating the code unnecessarily by mixing the two.

So, your loop can either look like this, if you use the NULL sentinel:
    struct ssl_map *p = sslversions;
    while (p->name) {
        if (!strcmp(ssl_version, p->name))
            ...
    }
or like this, if you use ARRAY_SIZE:
    for (i = 0; i < ARRAY_SIZE(sslversions); i++) {
        if (!strcmp(ssl_version, sslversions[i].name))
            ...
    }

Each loop form is valid, and (other than the fact that the compiler knows the array size, thus slightly favoring the ARRAY_SIZE form) the choice of which of the above two forms to use isn't that important, and you can choose whichever you like, but please do choose one of the above two. If you feel that Junio would be happier with the NULL-sentinel form, then go with that.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275773
Eric Sunshine· Aug 13, 2015, 16:49 UTC · re: Eric Sunshine · lore

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

On Thu, Aug 13, 2015 at 12:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 7 quoted lines
> So, your loop can either look like this, if you use the NULL sentinel:
>
>     struct ssl_map *p = sslversions;
>     while (p->name) {
>         if (!strcmp(ssl_version, p->name))
>             ...
>     }
That's not quite correct. 'p' needs to be incremented, of course, so:
    struct ssl_map *p;
    for (p = sslversions; p->name; p++) {
        if (!strcmp(ssl_version, p->name))
            ...
    }
would be nicely idiomatic.
Torsten Bögershausen· Aug 13, 2015, 16:01 UTC · re: Elia Pinto · lore

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

(need to drop Eric from cc-list, no DNS from web.de)
On 2015-08-13 17.28, Elia Pinto wrote:
Show 29 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>
> ---
> This is the third version of the patch. The changes compared to the previous version are:
> 
> - Eliminated the unnecessary blank (Junio)
> - Place a structure to associate mnemonic names with the curl enum constant (Junio)
> - Eliminated the invocation to curl_easy_setopt to set the default SSL value. Also removed the static global variable.
>   (Junio)
> - Slight correction in config.txt (Eric)
> 
>  Documentation/config.txt               | 22 ++++++++++++++++++++++
>  contrib/completion/git-completion.bash |  1 +
>  http.c                                 | 32 +++++++++++++++++++++++++++++++-
>  3 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 315f271..b23b01a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1595,6 +1595,28 @@ 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::

should this be https.sslVersion ? (http doesn't use ssl)

Show 15 quoted lines
> +	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
> +

from https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0.2C_2.0_and_3.0 sslv2 and sslv3 are deprecated. Should there be a motivation in the commit message why we want to support them ?

Show 63 quoted lines
> ++
> +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..d5fecd6 100644
> --- a/http.c
> +++ b/http.c
> @@ -37,6 +37,21 @@ 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 struct {
> +	const char *name;
> +	long ssl_version;
> +	} sslversions[] = {
> +		{ "sslv2", CURL_SSLVERSION_SSLv2 },
> +		{ "sslv3", CURL_SSLVERSION_TLSv1 },
> +		{ "tlsv1", CURL_SSLVERSION_TLSv1 },
> +#if LIBCURL_VERSION_NUM >= 0x072200
> +		{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
> +		{ "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
> +		{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
> +#endif
> +		{ NULL }
> +};
>  #if LIBCURL_VERSION_NUM >= 0x070903
>  static const char *ssl_key;
>  #endif
> @@ -190,6 +205,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,9 +381,22 @@ 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");
> +	

Minor nit to shorten: if (ssl_version && *ssl_version) {

> +		int i;
> +		for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {

I think Git-style is not to have ' ' before/after ')' /'(' for (i = 0; i < ARRAY_SIZE(sslversions); i++)

> +			if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
> +				curl_easy_setopt(result, CURLOPT_SSLVERSION,
> +					sslversions[i].ssl_version);
This is what my man page says:
 CURLcode curl_easy_setopt(CURL *handle, CURLoption option, parameter);
[]
RETURN VALUE
       CURLE_OK (zero) means that the option was set properly...
Should the return value checked (and we die() if we fail ?
> +				break;
> +		}
> +		if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
> +							ssl_version);
Should we die() here to make things very clear to the user ?
Show 9 quoted lines
> +	}
> +
>  	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);
> 
Elia Pinto· Aug 13, 2015, 16:10 UTC · re: Torsten Bögershausen · lore

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

2015-08-13 18:01 GMT+02:00 Torsten Bögershausen <tboegi@web.de>:
Show 54 quoted lines
> (need to drop Eric from cc-list, no DNS from web.de)
>
> On 2015-08-13 17.28, 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>
>> ---
>> This is the third version of the patch. The changes compared to the previous version are:
>>
>> - Eliminated the unnecessary blank (Junio)
>> - Place a structure to associate mnemonic names with the curl enum constant (Junio)
>> - Eliminated the invocation to curl_easy_setopt to set the default SSL value. Also removed the static global variable.
>>   (Junio)
>> - Slight correction in config.txt (Eric)
>>
>>  Documentation/config.txt               | 22 ++++++++++++++++++++++
>>  contrib/completion/git-completion.bash |  1 +
>>  http.c                                 | 32 +++++++++++++++++++++++++++++++-
>>  3 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 315f271..b23b01a 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1595,6 +1595,28 @@ 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::
> should this be https.sslVersion ?
> (http doesn't use ssl)
>
>> +     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
>> +
> from
> https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0.2C_2.0_and_3.0
> sslv2 and sslv3 are deprecated.
> Should there be a motivation in the commit message why we want to support them ?

They are those provided by the documentation (TLS in particular). We let the underlying library to say what is deprecated or not. In this case the call fail.

Show 83 quoted lines
>
>
>> ++
>> +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..d5fecd6 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -37,6 +37,21 @@ 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 struct {
>> +     const char *name;
>> +     long ssl_version;
>> +     } sslversions[] = {
>> +             { "sslv2", CURL_SSLVERSION_SSLv2 },
>> +             { "sslv3", CURL_SSLVERSION_TLSv1 },
>> +             { "tlsv1", CURL_SSLVERSION_TLSv1 },
>> +#if LIBCURL_VERSION_NUM >= 0x072200
>> +             { "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
>> +             { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
>> +             { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>> +#endif
>> +             { NULL }
>> +};
>>  #if LIBCURL_VERSION_NUM >= 0x070903
>>  static const char *ssl_key;
>>  #endif
>> @@ -190,6 +205,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,9 +381,22 @@ 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");
>> +
> Minor nit to shorten:
> if (ssl_version && *ssl_version) {
>
>> +             int i;
>> +             for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
> I think Git-style is not to have  ' ' before/after ')' /'('
> for (i = 0; i < ARRAY_SIZE(sslversions); i++)
>
>> +                     if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>> +                             curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> +                                     sslversions[i].ssl_version);
> This is what my man page says:
>  CURLcode curl_easy_setopt(CURL *handle, CURLoption option, parameter);
> []
>
> RETURN VALUE
>        CURLE_OK (zero) means that the option was set properly...
> Should the return value checked (and we die() if we fail ?

It is not strictly necessary. If it fails the other curl call fail, try to use sslv2 for example (libcurl deprecated nss dunno)

Thanks !
Show 17 quoted lines
>
>> +                             break;
>> +             }
>> +             if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
>> +                                                     ssl_version);
> Should we die() here to make things very clear to the user ?
>
>> +     }
>> +
>>       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);
>>
>
Ilari Liusvaara· Aug 13, 2015, 16:24 UTC · re: Elia Pinto · lore

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

On Thu, Aug 13, 2015 at 06:10:48PM +0200, Elia Pinto wrote:
Show 9 quoted lines
> 2015-08-13 18:01 GMT+02:00 Torsten Bögershausen <tboegi@web.de>:
> >> +
> > from
> > https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0.2C_2.0_and_3.0
> > sslv2 and sslv3 are deprecated.
> > Should there be a motivation in the commit message why we want to support them ?
> They are those provided by the documentation (TLS in particular). We
> let the underlying library to say what is deprecated or not. In this
> case the call fail.

The statement from the relevant SDO is much stronger than "deprecated", it is "not to be used under any cirmumstances".

Option like this looks only useful for connecting to really broken servers, damn security.

It could be useful for connecting to buggy servers after TLS 1.3 comes out and is implemented, as there are lots of servers (IIRC, on order of 10%) that can't deal with TLS 1.3 properly (but very few, IIRC <<0.1%, that can't deal with TLS 1.2 correctly[1]).

Also, is this option settable globally for all HTTP servers? One definitely does not want that to be possible. Configurations like this need to be per-server if they exist at all.

[1] Where correctly includes secure downnegotiation, as TLS is intended to do when faced with version mismatch.

-Ilari
Elia Pinto· Aug 13, 2015, 16:33 UTC · re: Ilari Liusvaara · lore

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

2015-08-13 18:24 GMT+02:00 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>:
Show 16 quoted lines
> On Thu, Aug 13, 2015 at 06:10:48PM +0200, Elia Pinto wrote:
>> 2015-08-13 18:01 GMT+02:00 Torsten Bögershausen <tboegi@web.de>:
>> >> +
>> > from
>> > https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0.2C_2.0_and_3.0
>> > sslv2 and sslv3 are deprecated.
>> > Should there be a motivation in the commit message why we want to support them ?
>> They are those provided by the documentation (TLS in particular). We
>> let the underlying library to say what is deprecated or not. In this
>> case the call fail.
>
> The statement from the relevant SDO is much stronger than "deprecated",
> it is "not to be used under any cirmumstances".
>
> Option like this looks only useful for connecting to really broken
> servers, damn security.

I know very well this topic. https://securitypitfalls.wordpress.com/2015/07/29/july-2015-scan-results/ I prefer that the decision is from the libray not us.

Show 17 quoted lines
>
> It could be useful for connecting to buggy servers after TLS 1.3
> comes out and is implemented, as there are lots of servers (IIRC, on
> order of 10%) that can't deal with TLS 1.3 properly (but very few, IIRC
> <<0.1%, that can't deal with TLS 1.2 correctly[1]).
>
> Also, is this option settable globally for all HTTP servers? One
> definitely does not want that to be possible. Configurations like
> this need to be per-server if they exist at all.
>
>
>
> [1] Where correctly includes secure downnegotiation, as TLS
> is intended to do when faced with version mismatch.
>
>
> -Ilari
Junio C Hamano· Aug 14, 2015, 17:21 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 11 quoted lines
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 315f271..b23b01a 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1595,6 +1595,28 @@ 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::
> should this be https.sslVersion ?
> (http doesn't use ssl)

But there are sslCipherList, etc., already present, and more importantly, I think you want http.proxy to apply even if you happen to be talking http over SSL.

More importantly, given that we have the "limited to this URL" mechanism "http.<url>.<variable>" that overrides "http.<variable>", introducing "https.sslWhatEver" would force people to have two configuration sections for no real benefit, other than silencing pedants that want to say "these things should be defined only for https".

Show 11 quoted lines
>> + if (sslversions[i].name != NULL && *sslversions[i].name &&
>> !strcmp(ssl_version,sslversions[i].name)) {
>> +				curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> +					sslversions[i].ssl_version);
> This is what my man page says:
>  CURLcode curl_easy_setopt(CURL *handle, CURLoption option, parameter);
> []
>
> RETURN VALUE
>        CURLE_OK (zero) means that the option was set properly...
> Should the return value checked (and we die() if we fail ?
Probably.  Do we check status from other calls to setopt?
Elia Pinto· Aug 14, 2015, 19:51 UTC · re: Junio C Hamano · lore

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

2015-08-14 19:21 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 38 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
>
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index 315f271..b23b01a 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -1595,6 +1595,28 @@ 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::
>> should this be https.sslVersion ?
>> (http doesn't use ssl)
>
> But there are sslCipherList, etc., already present, and more
> importantly, I think you want http.proxy to apply even if you happen
> to be talking http over SSL.
>
> More importantly, given that we have the "limited to this URL"
> mechanism "http.<url>.<variable>" that overrides "http.<variable>",
> introducing "https.sslWhatEver" would force people to have two
> configuration sections for no real benefit, other than silencing
> pedants that want to say "these things should be defined only for
> https".
>
>>> + if (sslversions[i].name != NULL && *sslversions[i].name &&
>>> !strcmp(ssl_version,sslversions[i].name)) {
>>> +                            curl_easy_setopt(result, CURLOPT_SSLVERSION,
>>> +                                    sslversions[i].ssl_version);
>> This is what my man page says:
>>  CURLcode curl_easy_setopt(CURL *handle, CURLoption option, parameter);
>> []
>>
>> RETURN VALUE
>>        CURLE_OK (zero) means that the option was set properly...
>> Should the return value checked (and we die() if we fail ?
>
> Probably.  Do we check status from other calls to setopt?

No. In this case anyway is not important i think: we already check if the version is accepted by curl, and if it is deprecated ( sslv2 for eample) we have an error in any case. refs http://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html

Best Regards

← back to recent threads