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

From: Elia Pinto <gitter.spiros@gmail.com>
Date: Aug 13, 2015, 14:30 UTC
Message-ID: <CA+EOSBmWqpT6zSZuCyBdW0cL0OLWbq1yvYmwqgTCUVF5T51K-w@mail.gmail.com>
In-Reply-To: <xmqqoaicmtac.fsf@gitster.dls.corp.google.com>

2015-08-12 17:16 GMT+02:00 Junio C Hamano <gitster@pobox.com>:

Show 21 quoted lines

> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> diff --git a/http.c b/http.c
>> index e9c6fdd..1504005 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
>>  static int curl_ssl_try;
>>  static const char *ssl_cert;
>>  static const char *ssl_cipherlist;
>> +static const char *ssl_version;
>> +static long sslversion = CURL_SSLVERSION_DEFAULT;
>
> I think you shouldn't have to initialize this variable.
> See below.
>
>>               init_curl_http_auth(result);
>>
>> +
>
> An unnecessary double blank?

I will fix this and the typo in the next patch.

Show 31 quoted lines

>
>> +     if (getenv("GIT_SSL_VERSION"))
>> +             ssl_version = getenv("GIT_SSL_VERSION");
>> +     if (ssl_version != NULL && *ssl_version) {
>> +             if (!strcmp(ssl_version,"tlsv1")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1;
>> +             } else if (!strcmp(ssl_version,"sslv2")) {
>> +                     sslversion = CURL_SSLVERSION_SSLv2;
>> +             } else if (!strcmp(ssl_version,"sslv3")) {
>> +                     sslversion = CURL_SSLVERSION_SSLv3;
>> +#if LIBCURL_VERSION_NUM >= 0x072200
>> +             } else if (!strcmp(ssl_version,"tlsv1.0")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_0;
>> +             } else if (!strcmp(ssl_version,"tlsv1.1")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_1;
>> +             } else if (!strcmp(ssl_version,"tlsv1.2")) {
>> +                     sslversion = CURL_SSLVERSION_TLSv1_2;
>> +#endif
>> +             } else {
>> +                     warning("unsupported ssl version %s : using default",
>> +                     ssl_version);
>> +             }
>> +        }
>> +     curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> +                             sslversion);
>
> A few problems:
>
>  - Why do we even have to call this when sslversion is not given by
>    the end user, either from the environment or from the config?
>    Wouldn't we use the default version if we do not make this call?

Right. I will fix.

Show 23 quoted lines

>
>  - It is true that 0x072200 is described as introducing these three
>    in [*1*] but the structure is maintenance burden waiting to
>    happen.  If you #if/#endif based on defined(CURL_SSLVERSION_$v),
>    it will be obvious to other people how to add their future
>    favourite versions in their patches.  Also it may be read better
>    if you separated the logic and the code by using a table like
>    this:
>
>    static struct {
>         const char *name;
>         long ssl_version;
>    } sslversions[] = {
>         { "tlsv1", CURL_SSLVERSION_TLSv1 },
>         { "sslv2", CURL_SSLVERSION_SSLv2 },
>         ...
>    #ifdef CURL_SSLVERSION_TLSv1_0
>         { "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
>    #endif
>         ...,
>         { NULL }
>    };
>

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

Show 15 quoted lines

>
>
>>       if (getenv("GIT_SSL_CIPHER_LIST"))
>>               ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
>> -
>
> This blank removal is understandable (i.e. group related things
> together).
>
>>       if (ssl_cipherlist != NULL && *ssl_cipherlist)
>>               curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
>>                               ssl_cipherlist);
>> -
>
> This is not.

I will fix.

Thank you very much

Show 8 quoted lines

>
>>       if (ssl_cert != NULL)
>>               curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>>       if (has_cert_password())
>
> [References]
>
> *1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions