Re: [PATCH v3] http: add support for specifying the SSL version
- From
Torsten Bögershausen <tboegi@web.de>
- Date
- Aug 13, 2015, 16:01 UTC
- Message-ID
- <55CCBF6F.3070808@web.de>
- In-Reply-To
- <1439479731-16018-1-git-send-email-gitter.spiros@gmail.com>
(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);
>