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

[PATCH v3] 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 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::
+	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

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

[Eric Sunshine]

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

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

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

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

[Elia Pinto]

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

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


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

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

[Torsten Bögershausen]

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


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

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

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

[Elia Pinto]

2015-08-13 18:01 GMT+02:00 Torsten Bögershausen <tboegi@web.de>:
> (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.
>
>
>> ++
>> +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 !

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

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

[Eric Sunshine]

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.

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

[Elia Pinto]

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

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

[Ilari Liusvaara]

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.

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

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

[Elia Pinto]

2015-08-13 18:24 GMT+02:00 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>:
> 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.


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

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

[Eric Sunshine]

On Thu, Aug 13, 2015 at 12:15 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> 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

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

[Eric Sunshine]

On Thu, Aug 13, 2015 at 12:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

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

[Junio C Hamano]

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?

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

[Elia Pinto]

2015-08-14 19:21 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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