{"thread":{"id":"48129","subject":"[PATCH v2] Allow use of TLS 1.3","startedAt":"2018-03-23T19:37:00Z","lastAt":"2018-03-24T07:53:02Z","messageCount":10,"participants":["Loganaden Velvindron","Daniel Stenberg","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"342671","messageId":"20180323193435.GA21971@voidlinux","threadId":"48129","inReplyTo":null,"subject":"[PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-23T19:34:35Z","receivedAt":"2018-03-23T19:37:00Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"Add a tlsv1.3 option to http.sslVersion in addition to the existing \ntlsv1.[012] options. libcurl has supported this since 7.52.0.\n\nDone during IETF 101 Hackathon\n\nSigned-off-by: Loganaden Velvindron \n---\n Documentation/config.txt | 2 +-\n http.c | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ce9102cea..b18cb9104 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1957,7 +1957,7 @@ http.sslVersion::\n \t- tlsv1.0\n \t- tlsv1.1\n \t- tlsv1.2\n-\n+\t- tlsv1.3\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\ndiff --git a/http.c b/http.c\nindex a5bd5d62c..25eb84c11 100644\n--- a/http.c\n+++ b/http.c\n@@ -62,6 +62,9 @@ static struct {\n \t{ \"tlsv1.1\", CURL_SSLVERSION_TLSv1_1 },\n \t{ \"tlsv1.2\", CURL_SSLVERSION_TLSv1_2 },\n #endif\n+#ifdef CURL_SSLVERSION_TLSv1_3\n+\t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n+#endif\n };\n #if LIBCURL_VERSION_NUM >= 0x070903\n static const char *ssl_key;\n-- \n2.16.2\n\n"},{"id":"342696","messageId":"alpine.DEB.2.20.1803232246020.16250@tvnag.unkk.fr","threadId":"48129","inReplyTo":"20180323193435.GA21971@voidlinux","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2018-03-23T21:47:41Z","receivedAt":"2018-03-23T21:55:00Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n\n> +#ifdef CURL_SSLVERSION_TLSv1_3\n> +\t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n> +#endif\n\nUnfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct won't \nwork.\n\nAlso, let me just point out that 7.52.0 is 0x073400 in hex and not the one \nused for the first version of this patch.\n\n-- \n\n / daniel.haxx.se\n"},{"id":"342697","messageId":"xmqqy3iih2xi.fsf@gitster-ct.c.googlers.com","threadId":"48129","inReplyTo":"20180323193435.GA21971@voidlinux","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T21:55:53Z","receivedAt":"2018-03-23T21:56:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Loganaden Velvindron writes:\n\n> Subject: Re: [PATCH v2] Allow use of TLS 1.3\n\nLet's retitle it to something like\n\n\tSubject: [PATCH v2] http: allow use of TLS 1.3\n\n> Add a tlsv1.3 option to http.sslVersion in addition to the existing \n> tlsv1.[012] options. libcurl has supported this since 7.52.0.\n\nGood.\n\n>\n> Done during IETF 101 Hackathon\n\nI am on the fence wrt the value of this line, especially because I\nwould strongly suspect that this version is not what you wrote and\ntested during your Hackathon. Even if it were, would it give value\nto future \"git log\" readers by supplying extra context?\n\n> Signed-off-by: Loganaden Velvindron \n> ---\n> Documentation/config.txt | 2 +-\n> http.c | 3 +++\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ce9102cea..b18cb9104 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1957,7 +1957,7 @@ http.sslVersion::\n> \t- tlsv1.0\n> \t- tlsv1.1\n> \t- tlsv1.2\n> -\n> +\t- tlsv1.3\n> +\n\nBefore this change, the block that shows the list of versions had\none blank line before and after it. Now we lost the blank line\nafter the block. Is it intended? Possibilities that come to my\nmind as a reviewer are:\n\n A. There is no difference in the rendered output if we have zero\n blank line (i.e. with the patch), or one blank line (i.e. before\n the patch applied).\n\n A.1) the submitter made this change on purpose, because it will\n make the source shorter without affecting the output, as a\n \"clean-up while at it\" change.\n\n A.2) this was an accidental change, which did not break the\n output merely because the submitter was lucky.\n\n B. The rendered output changes due to the lack of the blank line.\n\n B.1) And it changes in a good way. The submitter made this\n change on purpose.\n\n B.2) And it changes in a bad way, but the submitter did not\n notice it.\n\nPlease do not make reviewers wonder. Either avoid making\nunnecessary changes (e.g. you could have just added a new line with\ntlsv1.3 on it without touching the blank line), or make the change\nand explain why you made that change that is not essential for the\npurpose of adding tls1.3 which is the main focus of this patch.\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> diff --git a/http.c b/http.c\n> index a5bd5d62c..25eb84c11 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -62,6 +62,9 @@ static struct {\n> \t{ \"tlsv1.1\", CURL_SSLVERSION_TLSv1_1 },\n> \t{ \"tlsv1.2\", CURL_SSLVERSION_TLSv1_2 },\n> #endif\n> +#ifdef CURL_SSLVERSION_TLSv1_3\n> +\t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n> +#endif\n> };\n\nIt seems to me that\n\n https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956\n\ntells me that this #ifdef would not work. Did you test it with the\n\"test not version but feature\" change you made at the last minute?\n\nI know it is not your fault but is Ævar's, but you're responsible\nfor double-checking what you are told on the internet ;-)\n\nThanks.\n"},{"id":"342698","messageId":"xmqqtvt6h2ic.fsf@gitster-ct.c.googlers.com","threadId":"48129","inReplyTo":"alpine.DEB.2.20.1803232246020.16250@tvnag.unkk.fr","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T22:04:59Z","receivedAt":"2018-03-23T22:05:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Stenberg writes:\n\n> On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n>\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> +\t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>\n> Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct\n> won't work.\n>\n> Also, let me just point out that 7.52.0 is 0x073400 in hex and not the\n> one used for the first version of this patch.\n\nThanks!\n"},{"id":"342703","messageId":"87h8p6xw7t.fsf@evledraar.gmail.com","threadId":"48129","inReplyTo":"xmqqy3iih2xi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-23T22:28:54Z","receivedAt":"2018-03-23T22:29:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 23 2018, Junio C. Hamano wrote:\n\n>> @@ -62,6 +62,9 @@ static struct {\n>> \t{ \"tlsv1.1\", CURL_SSLVERSION_TLSv1_1 },\n>> \t{ \"tlsv1.2\", CURL_SSLVERSION_TLSv1_2 },\n>> #endif\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> +\t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>> };\n>\n> It seems to me that\n>\n> https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956\n>\n> tells me that this #ifdef would not work. Did you test it with the\n> \"test not version but feature\" change you made at the last minute?\n>\n> I know it is not your fault but is Ævar's, but you're responsible\n> for double-checking what you are told on the internet ;-)\n\nYeah I should add some \"I haven't actually tried this, but what do you\nthink about this?\" disclaimer.\n\nBut it's not a good sign that we have a v2 with an ifdef that'll never\nbe true, indicating that it wasn't tested against TLSv1.3. Is there some\nway we could check for this in our test suite?\n"},{"id":"342718","messageId":"CAFDEUTeggKUbjAHdO6wG9uOk01o2atbqf8MB=VHF_XGgXmBQzQ@mail.gmail.com","threadId":"48129","inReplyTo":"alpine.DEB.2.20.1803232246020.16250@tvnag.unkk.fr","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-24T04:21:46Z","receivedAt":"2018-03-24T04:21:54Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"On Sat, Mar 24, 2018 at 1:47 AM, Daniel Stenberg wrote:\n> On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n>\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> + { \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>\n>\n> Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct won't\n> work.\n>\n> Also, let me just point out that 7.52.0 is 0x073400 in hex and not the one\n> used for the first version of this patch.\n>\n\nThanks, will fix it.\n\n> --\n>\n> / daniel.haxx.se\n"},{"id":"342719","messageId":"CAFDEUTfMk+9mfJoX+116eW-e2o_rBiVeDZ1dfEbxaLGLign1AQ@mail.gmail.com","threadId":"48129","inReplyTo":"xmqqy3iih2xi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-24T04:23:14Z","receivedAt":"2018-03-24T04:23:19Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"On Sat, Mar 24, 2018 at 1:55 AM, Junio C Hamano wrote:\n> Loganaden Velvindron writes:\n>\n>> Subject: Re: [PATCH v2] Allow use of TLS 1.3\n>\n> Let's retitle it to something like\n>\n> Subject: [PATCH v2] http: allow use of TLS 1.3\n>\n>> Add a tlsv1.3 option to http.sslVersion in addition to the existing\n>> tlsv1.[012] options. libcurl has supported this since 7.52.0.\n>\n> Good.\n>\n>>\n>> Done during IETF 101 Hackathon\n>\n> I am on the fence wrt the value of this line, especially because I\n> would strongly suspect that this version is not what you wrote and\n> tested during your Hackathon. Even if it were, would it give value\n> to future \"git log\" readers by supplying extra context?\n>\n\nWill remove this line.\n\n>> Signed-off-by: Loganaden Velvindron \n>> ---\n>> Documentation/config.txt | 2 +-\n>> http.c | 3 +++\n>> 2 files changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index ce9102cea..b18cb9104 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1957,7 +1957,7 @@ http.sslVersion::\n>> - tlsv1.0\n>> - tlsv1.1\n>> - tlsv1.2\n>> -\n>> + - tlsv1.3\n>> +\n>\n> Before this change, the block that shows the list of versions had\n> one blank line before and after it. Now we lost the blank line\n> after the block. Is it intended? Possibilities that come to my\n> mind as a reviewer are:\n>\n> A. There is no difference in the rendered output if we have zero\n> blank line (i.e. with the patch), or one blank line (i.e. before\n> the patch applied).\n>\n> A.1) the submitter made this change on purpose, because it will\n> make the source shorter without affecting the output, as a\n> \"clean-up while at it\" change.\n>\n> A.2) this was an accidental change, which did not break the\n> output merely because the submitter was lucky.\n>\n> B. The rendered output changes due to the lack of the blank line.\n>\n> B.1) And it changes in a good way. The submitter made this\n> change on purpose.\n>\n> B.2) And it changes in a bad way, but the submitter did not\n> notice it.\n>\n> Please do not make reviewers wonder. Either avoid making\n> unnecessary changes (e.g. you could have just added a new line with\n> tlsv1.3 on it without touching the blank line), or make the change\n> and explain why you made that change that is not essential for the\n> purpose of adding tls1.3 which is the main focus of this patch.\n\nAlright.\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>> diff --git a/http.c b/http.c\n>> index a5bd5d62c..25eb84c11 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -62,6 +62,9 @@ static struct {\n>> { \"tlsv1.1\", CURL_SSLVERSION_TLSv1_1 },\n>> { \"tlsv1.2\", CURL_SSLVERSION_TLSv1_2 },\n>> #endif\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> + { \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>> };\n>\n> It seems to me that\n>\n> https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956\n>\n> tells me that this #ifdef would not work. Did you test it with the\n> \"test not version but feature\" change you made at the last minute?\n\nI compiled it.\n\n>\n> I know it is not your fault but is Ævar's, but you're responsible\n> for double-checking what you are told on the internet ;-)\n\nYes, my fault, not Ævar Arnfjörð Bjarmason .\n\n\n>\n> Thanks.\n"},{"id":"342720","messageId":"CAFDEUTd_t554b=7xWcO=bY21YTRfbz8SwLVjEmdbCRb=56v6FA@mail.gmail.com","threadId":"48129","inReplyTo":"alpine.DEB.2.20.1803232246020.16250@tvnag.unkk.fr","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-24T04:31:47Z","receivedAt":"2018-03-24T04:31:52Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"On Sat, Mar 24, 2018 at 1:47 AM, Daniel Stenberg wrote:\n> On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n>\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> + { \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>\n>\n> Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct won't\n> work.\n>\n> Also, let me just point out that 7.52.0 is 0x073400 in hex and not the one\n> used for the first version of this patch.\n>\nYeah, v1 patch is broken. I'm sending a v3 patch which is properly\ntested with OpenSSL preview alpha.\n\n\n\n> --\n>\n> / daniel.haxx.se\n"},{"id":"342726","messageId":"CAFDEUTfxnJdVunuEJ9VWPy_T4yByv3cCGZZbW7iDfoq3kYy17Q@mail.gmail.com","threadId":"48129","inReplyTo":"alpine.DEB.2.20.1803232246020.16250@tvnag.unkk.fr","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-24T05:43:26Z","receivedAt":"2018-03-24T05:43:32Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"On Sat, Mar 24, 2018 at 1:47 AM, Daniel Stenberg wrote:\n> On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n>\n>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>> + { \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>> +#endif\n>\n>\n> Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct won't\n> work.\n>\n> Also, let me just point out that 7.52.0 is 0x073400 in hex and not the one\n> used for the first version of this patch.\n>\n\nHere's the error i get when I use a recent libcurl, but the OpenSSL\nwasn't built with tls 1.3:\n\n using : GIT_SSL_VERSION=tlsv1.3\n\nError:\nOpenSSL was built without TLS 1.3 support\n\n\n> --\n>\n> / daniel.haxx.se\n"},{"id":"342806","messageId":"CAFDEUTf_KCn7Sqs+O8JJfBXo_+QfvAmiT2uzmM_ROWfXUdF0Sg@mail.gmail.com","threadId":"48129","inReplyTo":"xmqqtvt6h2ic.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Allow use of TLS 1.3","fromName":"Loganaden Velvindron","fromEmail":"logan@hackers.mu","sentAt":"2018-03-24T07:52:57Z","receivedAt":"2018-03-24T07:53:02Z","isPatch":true,"sender":{"key":"logan@hackers.mu","avatar":"https://avatars.githubusercontent.com/u/1688420?v=4"},"body":"On Sat, Mar 24, 2018 at 2:04 AM, Junio C Hamano wrote:\n> Daniel Stenberg writes:\n>\n>> On Fri, 23 Mar 2018, Loganaden Velvindron wrote:\n>>\n>>> +#ifdef CURL_SSLVERSION_TLSv1_3\n>>> + { \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 }\n>>> +#endif\n>>\n>> Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct\n>> won't work.\n>>\n>> Also, let me just point out that 7.52.0 is 0x073400 in hex and not the\n>> one used for the first version of this patch.\n>\n\nIt's working with tls 1.3:\n\nldd for curl (showing linking to openssl 1.1.1 pre2 preview):\n ldd /usr/local/bin/curl\nlinux-vdso.so.1 (0x00007ffd30599000)\nlibcurl.so.4 => /usr/local/lib/libcurl.so.4 (0x00007f5a81845000)\nlibssl.so.1.1 => /usr/local/lib/libssl.so.1.1 (0x00007f5a815b5000)\nlibcrypto.so.1.1 => /usr/local/lib/libcrypto.so.1.1 (0x00007f5a810dd000)\nlibz.so.1 => /usr/lib/libz.so.1 (0x00007f5a80ec6000)\nlibpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f5a80ca7000)\nlibc.so.6 => /usr/lib/libc.so.6 (0x00007f5a808f2000)\nlibnghttp2.so.14 => /usr/lib/libnghttp2.so.14 (0x00007f5a806cd000)\nlibdl.so.2 => /usr/lib/libdl.so.2 (0x00007f5a804c9000)\n/lib/ld-linux-x86-64.so.2 (0x00007f5a81ce5000)\n\nhandshake failure against a tls 1.2 server:\n\nGIT_SSL_VERSION=tlsv1.3 ./git clone https://github.com/shuque/pydig\nCloning into 'pydig'...\nwarning: templates not found /usr/local/share/git-core/templates\nfatal: unable to access 'https://github.com/shuque/pydig/':\nerror:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake\nfailure\n\nWith a local server running nginx using only tls 1.3 (had to disable\nssl verification due to self-signed cert):\nGIT_SSL_NO_VERIFY=true GIT_SSL_VERSION=tlsv1.2 ./git clone\nhttps://192.168.1.214/git_test\nerror:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol version\n\n\nNow with TLS 1.3, it works:\nGIT_SSL_NO_VERIFY=true GIT_SSL_VERSION=tlsv1.3 ./git clone\nhttps://192.168.1.214/git_test\n\n\n\n> Thanks!\n"}]}