{"thread":{"id":"40069","subject":"[PATCH v2] http: add support for specifying the SSL version","startedAt":"2015-08-12T14:24:51Z","lastAt":"2015-08-13T14:58:41Z","messageCount":5,"participants":["Elia Pinto","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"267907","messageId":"1439389491-21669-1-git-send-email-gitter.spiros@gmail.com","threadId":"40069","inReplyTo":null,"subject":"[PATCH v2] http: add support for specifying the SSL version","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2015-08-12T14:24:51Z","receivedAt":"2015-08-12T14:24:51Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Teach git about a new option, \"http.sslVersion\", which permits one to\nspecify the SSL version to use when negotiating SSL connections. The\nsetting can be overridden by the GIT_SSL_VERSION environment\nvariable.\n\nSigned-off-by: Elia Pinto \n---\nThis is the second version. I moved out of the else clause from the #ifdef.\n\n\n Documentation/config.txt | 21 +++++++++++++++++++++\n contrib/completion/git-completion.bash | 1 +\n http.c | 31 +++++++++++++++++++++++++++++--\n 3 files changed, 51 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 315f271..76a4f2b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1595,6 +1595,27 @@ http.saveCookies::\n \tIf set, store cookies received during requests to the file specified by\n \thttp.cookieFile. Has no effect if http.cookieFile is unset.\n \n+http.sslVersion::\n+\tThe SSL version to use when negotiating an SSL connection, if you\n+\twant to force the default. The available and default version depend on\n+\twhether libcurl was built against NSS or OpenSSL and the particular configuration\n+\tof the crypto library in use. Internally this sets the 'CURLOPT_SSL_VERSION'\n+\toption; see the libcurl documentation for more details on the format\n+\tof this option and for the ssl version supported. Actually the possible values\n+\tof this option are:\n+\n+\t- sslv2\n+\t- sslv3\n+\t- tlsv1\n+\t- tlsv1.0\n+\t- tlsv1.1\n+\t- tlsv1.2\n++\n+Can be overridden by the 'GIT_SSL_VERSION' environment variable.\n+To force git to use libcurl's default ssl version and ignore any\n+explicit http.sslversion option, set 'GIT_SSL_VERSION' to the\n+empty string.\n+\n http.sslCipherList::\n A list of SSL ciphers to use when negotiating an SSL connection.\n The available ciphers depend on whether libcurl was built against\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex c97c648..6e9359c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2118,6 +2118,7 @@ _git_config ()\n \t\thttp.postBuffer\n \t\thttp.proxy\n \t\thttp.sslCipherList\n+\t\thttp.sslVersion\n \t\thttp.sslCAInfo\n \t\thttp.sslCAPath\n \t\thttp.sslCert\ndiff --git a/http.c b/http.c\nindex e9c6fdd..1504005 100644\n--- a/http.c\n+++ b/http.c\n@@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n static const char *ssl_cert;\n static const char *ssl_cipherlist;\n+static const char *ssl_version;\n+static long sslversion = CURL_SSLVERSION_DEFAULT;\n #if LIBCURL_VERSION_NUM >= 0x070903\n static const char *ssl_key;\n #endif\n@@ -190,6 +192,8 @@ static int http_options(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(\"http.sslcipherlist\", var))\n \t\treturn git_config_string(&ssl_cipherlist, var, value);\n+\tif (!strcmp(\"http.sslversion\", var))\n+\t\treturn git_config_string(&ssl_version, var, value);\n \tif (!strcmp(\"http.sslcert\", var))\n \t\treturn git_config_string(&ssl_cert, var, value);\n #if LIBCURL_VERSION_NUM >= 0x070903\n@@ -364,13 +368,36 @@ static CURL *get_curl_handle(void)\n \tif (http_proactive_auth)\n \t\tinit_curl_http_auth(result);\n \n+\n+\tif (getenv(\"GIT_SSL_VERSION\"))\n+\t\tssl_version = getenv(\"GIT_SSL_VERSION\");\n+\tif (ssl_version != NULL && *ssl_version) {\n+\t\tif (!strcmp(ssl_version,\"tlsv1\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_TLSv1;\n+\t\t} else if (!strcmp(ssl_version,\"sslv2\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_SSLv2;\n+\t\t} else if (!strcmp(ssl_version,\"sslv3\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_SSLv3;\n+#if LIBCURL_VERSION_NUM >= 0x072200\n+\t\t} else if (!strcmp(ssl_version,\"tlsv1.0\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_TLSv1_0;\n+\t\t} else if (!strcmp(ssl_version,\"tlsv1.1\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_TLSv1_1;\n+\t\t} else if (!strcmp(ssl_version,\"tlsv1.2\")) {\n+\t\t\tsslversion = CURL_SSLVERSION_TLSv1_2;\n+#endif\n+\t\t} else {\n+\t\t\twarning(\"unsupported ssl version %s : using default\",\n+\t\t\tssl_version);\n+\t\t}\n+ }\n+\tcurl_easy_setopt(result, CURLOPT_SSLVERSION,\n+\t\t\t\tsslversion);\n \tif (getenv(\"GIT_SSL_CIPHER_LIST\"))\n \t\tssl_cipherlist = getenv(\"GIT_SSL_CIPHER_LIST\");\n-\n \tif (ssl_cipherlist != NULL && *ssl_cipherlist)\n \t\tcurl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,\n \t\t\t\tssl_cipherlist);\n-\n \tif (ssl_cert != NULL)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n \tif (has_cert_password())\n-- \n2.5.0.234.gefc8a62\n"},{"id":"267911","messageId":"xmqqoaicmtac.fsf@gitster.dls.corp.google.com","threadId":"40069","inReplyTo":"1439389491-21669-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v2] http: add support for specifying the SSL version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T15:16:11Z","receivedAt":"2015-08-12T15:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> diff --git a/http.c b/http.c\n> index e9c6fdd..1504005 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;\n> static int curl_ssl_try;\n> static const char *ssl_cert;\n> static const char *ssl_cipherlist;\n> +static const char *ssl_version;\n> +static long sslversion = CURL_SSLVERSION_DEFAULT;\n\nI think you shouldn't have to initialize this variable.\nSee below.\n\n> \t\tinit_curl_http_auth(result);\n> \n> +\n\nAn unnecessary double blank?\n\n> +\tif (getenv(\"GIT_SSL_VERSION\"))\n> +\t\tssl_version = getenv(\"GIT_SSL_VERSION\");\n> +\tif (ssl_version != NULL && *ssl_version) {\n> +\t\tif (!strcmp(ssl_version,\"tlsv1\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_TLSv1;\n> +\t\t} else if (!strcmp(ssl_version,\"sslv2\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_SSLv2;\n> +\t\t} else if (!strcmp(ssl_version,\"sslv3\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_SSLv3;\n> +#if LIBCURL_VERSION_NUM >= 0x072200\n> +\t\t} else if (!strcmp(ssl_version,\"tlsv1.0\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_TLSv1_0;\n> +\t\t} else if (!strcmp(ssl_version,\"tlsv1.1\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_TLSv1_1;\n> +\t\t} else if (!strcmp(ssl_version,\"tlsv1.2\")) {\n> +\t\t\tsslversion = CURL_SSLVERSION_TLSv1_2;\n> +#endif\n> +\t\t} else {\n> +\t\t\twarning(\"unsupported ssl version %s : using default\",\n> +\t\t\tssl_version);\n> +\t\t}\n> + }\n> +\tcurl_easy_setopt(result, CURLOPT_SSLVERSION,\n> +\t\t\t\tsslversion);\n\nA few problems:\n\n - Why do we even have to call this when sslversion is not given by\n the end user, either from the environment or from the config?\n Wouldn't we use the default version if we do not make this call?\n\n - It is true that 0x072200 is described as introducing these three\n in [*1*] but the structure is maintenance burden waiting to\n happen. If you #if/#endif based on defined(CURL_SSLVERSION_$v),\n it will be obvious to other people how to add their future\n favourite versions in their patches. Also it may be read better\n if you separated the logic and the code by using a table like\n this:\n\n static struct {\n \tconst char *name;\n long ssl_version;\n } sslversions[] = {\n\t{ \"tlsv1\", CURL_SSLVERSION_TLSv1 },\n\t{ \"sslv2\", CURL_SSLVERSION_SSLv2 },\n ...\n #ifdef CURL_SSLVERSION_TLSv1_0\n\t{ \"tlsv1.0\", CURL_SSLVERSION_TLSv1_0 },\n #endif\n \t...,\n { NULL }\n };\n\n\n\n> \tif (getenv(\"GIT_SSL_CIPHER_LIST\"))\n> \t\tssl_cipherlist = getenv(\"GIT_SSL_CIPHER_LIST\");\n> -\n\nThis blank removal is understandable (i.e. group related things\ntogether).\n\n> \tif (ssl_cipherlist != NULL && *ssl_cipherlist)\n> \t\tcurl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,\n> \t\t\t\tssl_cipherlist);\n> -\n\nThis is not.\n\n> \tif (ssl_cert != NULL)\n> \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n> \tif (has_cert_password())\n\n[References]\n\n*1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n"},{"id":"267927","messageId":"20150812182011.GA3803@flurp.local","threadId":"40069","inReplyTo":"1439389491-21669-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v2] http: add support for specifying the SSL version","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-12T18:20:11Z","receivedAt":"2015-08-12T18:20:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2015 at 04:24:51PM +0200, Elia Pinto wrote:\n> Teach git about a new option, \"http.sslVersion\", which permits one to\n> specify the SSL version to use when negotiating SSL connections. The\n> setting can be overridden by the GIT_SSL_VERSION environment\n> variable.\n> \n> Signed-off-by: Elia Pinto \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 315f271..76a4f2b 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1595,6 +1595,27 @@ http.saveCookies::\n> +http.sslVersion::\n> +\tThe SSL version to use when negotiating an SSL connection, if you\n> +\twant to force the default. The available and default version depend on\n> +\twhether libcurl was built against NSS or OpenSSL and the particular configuration\n> +\tof the crypto library in use. Internally this sets the 'CURLOPT_SSL_VERSION'\n> +\toption; see the libcurl documentation for more details on the format\n> +\tof this option and for the ssl version supported. Actually the possible values\n> +\tof this option are:\n> +\n> +\t- sslv2\n> +\t- sslv3\n> +\t- tlsv1\n> +\t- tlsv1.0\n> +\t- tlsv1.1\n> +\t- tlsv1.2\n> ++\n> +Can be overridden by the 'GIT_SSL_VERSION' environment variable.\n> +To force git to use libcurl's default ssl version and ignore any\n> +explicit http.sslversion option, set 'GIT_SSL_VERSION' to the\n> +empty string.\n\nUnfortunately, this won't format properly in Asciidoc; the final\nparagraph will be indented as part of the itemized list supported SSL\nversions. Here's a squash-in to fix it:\n\n---- 8< ----\nSubject: [PATCH] fixup! http: add support for specifying the SSL version\n\n---\n Documentation/config.txt | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 76a4f2b..b23b01a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1610,6 +1610,7 @@ http.sslVersion::\n \t- tlsv1.0\n \t- tlsv1.1\n \t- tlsv1.2\n+\n +\n Can be overridden by the 'GIT_SSL_VERSION' environment variable.\n To force git to use libcurl's default ssl version and ignore any\n-- \n2.5.0.276.gf5e568e\n"},{"id":"267979","messageId":"CA+EOSBmWqpT6zSZuCyBdW0cL0OLWbq1yvYmwqgTCUVF5T51K-w@mail.gmail.com","threadId":"40069","inReplyTo":"xmqqoaicmtac.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] http: add support for specifying the SSL version","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2015-08-13T14:30:21Z","receivedAt":"2015-08-13T14:30:21Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2015-08-12 17:16 GMT+02:00 Junio C Hamano :\n> Elia Pinto writes:\n>\n>> diff --git a/http.c b/http.c\n>> index e9c6fdd..1504005 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;\n>> static int curl_ssl_try;\n>> static const char *ssl_cert;\n>> static const char *ssl_cipherlist;\n>> +static const char *ssl_version;\n>> +static long sslversion = CURL_SSLVERSION_DEFAULT;\n>\n> I think you shouldn't have to initialize this variable.\n> See below.\n>\n>> init_curl_http_auth(result);\n>>\n>> +\n>\n> An unnecessary double blank?\nI will fix this and the typo in the next patch.\n>\n>> + if (getenv(\"GIT_SSL_VERSION\"))\n>> + ssl_version = getenv(\"GIT_SSL_VERSION\");\n>> + if (ssl_version != NULL && *ssl_version) {\n>> + if (!strcmp(ssl_version,\"tlsv1\")) {\n>> + sslversion = CURL_SSLVERSION_TLSv1;\n>> + } else if (!strcmp(ssl_version,\"sslv2\")) {\n>> + sslversion = CURL_SSLVERSION_SSLv2;\n>> + } else if (!strcmp(ssl_version,\"sslv3\")) {\n>> + sslversion = CURL_SSLVERSION_SSLv3;\n>> +#if LIBCURL_VERSION_NUM >= 0x072200\n>> + } else if (!strcmp(ssl_version,\"tlsv1.0\")) {\n>> + sslversion = CURL_SSLVERSION_TLSv1_0;\n>> + } else if (!strcmp(ssl_version,\"tlsv1.1\")) {\n>> + sslversion = CURL_SSLVERSION_TLSv1_1;\n>> + } else if (!strcmp(ssl_version,\"tlsv1.2\")) {\n>> + sslversion = CURL_SSLVERSION_TLSv1_2;\n>> +#endif\n>> + } else {\n>> + warning(\"unsupported ssl version %s : using default\",\n>> + ssl_version);\n>> + }\n>> + }\n>> + curl_easy_setopt(result, CURLOPT_SSLVERSION,\n>> + sslversion);\n>\n> A few problems:\n>\n> - Why do we even have to call this when sslversion is not given by\n> the end user, either from the environment or from the config?\n> Wouldn't we use the default version if we do not make this call?\nRight. I will fix.\n>\n> - It is true that 0x072200 is described as introducing these three\n> in [*1*] but the structure is maintenance burden waiting to\n> happen. If you #if/#endif based on defined(CURL_SSLVERSION_$v),\n> it will be obvious to other people how to add their future\n> favourite versions in their patches. Also it may be read better\n> if you separated the logic and the code by using a table like\n> this:\n>\n> static struct {\n> const char *name;\n> long ssl_version;\n> } sslversions[] = {\n> { \"tlsv1\", CURL_SSLVERSION_TLSv1 },\n> { \"sslv2\", CURL_SSLVERSION_SSLv2 },\n> ...\n> #ifdef CURL_SSLVERSION_TLSv1_0\n> { \"tlsv1.0\", CURL_SSLVERSION_TLSv1_0 },\n> #endif\n> ...,\n> { NULL }\n> };\n>\nunfortunately they are enum constant (not #defined ) and I don't know\na smart way in C to make this change with the preprocessor. Would you\nhave any idea?\n>\n>\n>> if (getenv(\"GIT_SSL_CIPHER_LIST\"))\n>> ssl_cipherlist = getenv(\"GIT_SSL_CIPHER_LIST\");\n>> -\n>\n> This blank removal is understandable (i.e. group related things\n> together).\n>\n>> if (ssl_cipherlist != NULL && *ssl_cipherlist)\n>> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,\n>> ssl_cipherlist);\n>> -\n>\n> This is not.\nI will fix.\n\nThank you very much\n>\n>> if (ssl_cert != NULL)\n>> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n>> if (has_cert_password())\n>\n> [References]\n>\n> *1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n"},{"id":"267981","messageId":"CAPc5daUZFEDN9OqQVwYFxsyHZYtvw8D-Ost-bopreKZmV0epxw@mail.gmail.com","threadId":"40069","inReplyTo":"CA+EOSBmWqpT6zSZuCyBdW0cL0OLWbq1yvYmwqgTCUVF5T51K-w@mail.gmail.com","subject":"Re: [PATCH v2] http: add support for specifying the SSL version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-13T14:58:41Z","receivedAt":"2015-08-13T14:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Aug 13, 2015 at 7:30 AM, Elia Pinto wrote:\n>>\n> unfortunately they are enum constant (not #defined)\n\nAhh, then checking LIBCURL_VERSION_NUM is unfortunately the\nbest we could do. Sorry for the noise.\n\nYou still can go with the table-driven approach, though.\n"}]}