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

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

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