Allow use of TLS 1.3
PATCH v2 1/1 Allow use of TLS 1.3
Add a tlsv1.3 option to http.sslVersion in addition to the existing tlsv1.[012] options. libcurl has supported this since 7.52.0.
Done during IETF 101 Hackathon
--- Documentation/config.txt | 2 +- http.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
Documentation/config.txt +1 -1
@@ -1957,7 +1957,7 @@ http.sslVersion:: - tlsv1.0 - tlsv1.1 - tlsv1.2 - + - tlsv1.3 + Can be overridden by the `GIT_SSL_VERSION` environment variable. To force git to use libcurl's default ssl version and ignore any
http.c +3 -0
@@ -62,6 +62,9 @@ static struct { { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 }, { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 }, #endif +#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif };#if LIBCURL_VERSION_NUM >=0x070903 static const char *ssl_key;
Signature · 1 line
2.16.2
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif
Unfortunately, CURL_SSLVERSION_TLSv1_3 is an enum so this construct won't work.
Also, let me just point out that 7.52.0 is 0x073400 in hex and not the one used for the first version of this patch.
Signature · 1 line
/ daniel.haxx.se
Quoted 11 lines · Daniel Stenberg
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif Unfortunately,
CURL_SSLVERSION_TLSv1_3is an enum so this construct won't work.Also, let me just point out that 7.52.0 is
0x073400in hex and not the one used for the first version of this patch.
Thanks!
Quoted 11 lines · Junio C Hamano
Quoted 9 lines · Daniel Stenberg
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif Unfortunately,
CURL_SSLVERSION_TLSv1_3is an enum so this construct won't work.
Quoted 3 lines
Quoted 2 lines
Also, let me just point out that 7.52.0 is
0x073400in hex and not the one used for the first version of this patch.
It's working with tls 1.3:
ldd for curl (showing linking to openssl 1.1.1 pre2 preview):
ldd /usr/local/bin/curl
linux-vdso.so.1 (0x00007ffd30599000)
libcurl.so.4 => /usr/local/lib/libcurl.so.4 (0x00007f5a81845000) libssl.so.1.1 => /usr/local/lib/libssl.so.1.1 (0x00007f5a815b5000) libcrypto.so.1.1 => /usr/local/lib/libcrypto.so.1.1 (0x00007f5a810dd000) libz.so.1 => /usr/lib/libz.so.1 (0x00007f5a80ec6000) libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f5a80ca7000) libc.so.6 => /usr/lib/libc.so.6 (0x00007f5a808f2000) libnghttp2.so.14 => /usr/lib/libnghttp2.so.14 (0x00007f5a806cd000) libdl.so.2 => /usr/lib/libdl.so.2 (0x00007f5a804c9000) /lib/ld-linux-x86-64.so.2 (0x00007f5a81ce5000)
handshake failure against a tls 1.2 server:
GIT_SSL_VERSION=tlsv1.3 ./git clone https://github.com/shuque/pydig
Cloning into 'pydig'...
warning: templates not found /usr/local/share/git-core/templates
fatal: unable to access 'https://github.com/shuque/pydig/':
error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure
With a local server running nginx using only tls 1.3 (had to disable ssl verification due to self-signed cert):
GIT_SSL_NO_VERIFY=true GIT_SSL_VERSION=tlsv1.2 ./git clone
https://192.168.1.214/git_test
error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol version
Now with TLS 1.3, it works:
GIT_SSL_NO_VERIFY=true GIT_SSL_VERSION=tlsv1.3 ./git clone
https://192.168.1.214/git_test
Quoted 1 line
Thanks!
Quoted 10 lines · Daniel Stenberg
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif Unfortunately,
CURL_SSLVERSION_TLSv1_3is an enum so this construct won't work.
Quoted 3 lines
Also, let me just point out that 7.52.0 is
0x073400in hex and not the one used for the first version of this patch.
Thanks, will fix it.
Quoted 3 lines
--
/ daniel.haxx.se
Quoted 10 lines · Daniel Stenberg
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif Unfortunately,
CURL_SSLVERSION_TLSv1_3is an enum so this construct won't work.
Quoted 3 lines
Also, let me just point out that 7.52.0 is
0x073400in hex and not the one used for the first version of this patch.
Yeah, v1 patch is broken. I'm sending a v3 patch which is properly tested with OpenSSL preview alpha.
Quoted 3 lines
--
/ daniel.haxx.se
Quoted 10 lines · Daniel Stenberg
http.c static struct { · line 67
Quoted 3 lines · Loganaden Velvindron
http.c +3
-0+#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif Unfortunately,
CURL_SSLVERSION_TLSv1_3is an enum so this construct won't work.
Quoted 3 lines
Also, let me just point out that 7.52.0 is
0x073400in hex and not the one used for the first version of this patch.
Here's the error i get when I use a recent libcurl, but the OpenSSL wasn't built with tls 1.3:
using : GIT_SSL_VERSION=tlsv1.3
Error:
OpenSSL was built without TLS 1.3 support
Quoted 3 lines
--
/ daniel.haxx.se
Quoted 1 line · Loganaden Velvindron
Subject: Re: [PATCH v2] Allow use of TLS 1.3
Let's retitle it to something like
Subject: [PATCH v2] http: allow use of TLS 1.3
Quoted 2 lines
Add a tlsv1.3 option to http.sslVersion in addition to the existing tlsv1.[012] options. libcurl has supported this since 7.52.0.
Good.
Quoted 2 lines
Done during IETF 101 Hackathon
I am on the fence wrt the value of this line, especially because I would strongly suspect that this version is not what you wrote and tested during your Hackathon. Even if it were, would it give value to future "git log" readers by supplying extra context?
Quoted 11 lines
Signed-off-by Loganaden Velvindron <[email protected]>--- Documentation/config.txt | 2 +- http.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)Documentation/config.txt +0
-0
Documentation/config.txt http.sslVersion:: · line 1961
Quoted 6 lines
Documentation/config.txt +1
-1@@ -1957,7 +1957,7 @@ http.sslVersion:: - tlsv1.0 - tlsv1.1 - tlsv1.2 - + - tlsv1.3 +
Before this change, the block that shows the list of versions had one blank line before and after it. Now we lost the blank line after the block. Is it intended? Possibilities that come to my mind as a reviewer are:
A. There is no difference in the rendered output if we have zero
blank line (i.e. with the patch), or one blank line (i.e. before the patch applied).
A.1) the submitter made this change on purpose, because it will make the source shorter without affecting the output, as a "clean-up while at it" change.
A.2) this was an accidental change, which did not break the output merely because the submitter was lucky.
B. The rendered output changes due to the lack of the blank line.
B.1) And it changes in a good way. The submitter made this change on purpose.
B.2) And it changes in a bad way, but the submitter did not notice it.
Please do not make reviewers wonder. Either avoid making unnecessary changes (e.g. you could have just added a new line with tlsv1.3 on it without touching the blank line), or make the change and explain why you made that change that is not essential for the purpose of adding tls1.3 which is the main focus of this patch.
Quoted 7 lines
Documentation/config.txt +0
-0Can be overridden by the `GIT_SSL_VERSION` environment variable. To force git to use libcurl's default ssl version and ignore anyhttp.c +0
-0
http.c static struct { · line 68
Quoted 7 lines
http.c +3
-0@@ -62,6 +62,9 @@ static struct { { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 }, { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 }, #endif +#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif };
It seems to me that
https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956
tells me that this #ifdef would not work. Did you test it with the "test not version but feature" change you made at the last minute?
I know it is not your fault but is Ævar's, but you're responsible for double-checking what you are told on the internet ;-)
Thanks.
Quoted 11 lines · Junio C. Hamano
http.c static struct { · line 68
Quoted 8 lines
http.c +3
-0@@ -62,6 +62,9 @@ static struct { { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 }, { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 }, #endif +#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif };It seems to me that
Quoted 7 lines
https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956tells me that this #ifdef would not work. Did you test it with the "test not version but feature" change you made at the last minute?
I know it is not your fault but is Ævar's, but you're responsible for double-checking what you are told on the internet ;-)
Yeah I should add some "I haven't actually tried this, but what do you think about this?" disclaimer.
But it's not a good sign that we have a v2 with an ifdef that'll never be true, indicating that it wasn't tested against TLSv1.3. Is there some way we could check for this in our test suite?
Quoted 14 lines · Junio C Hamano
Quoted 1 line · Loganaden Velvindron
Subject: Re: [PATCH v2] Allow use of TLS 1.3
Let's retitle it to something like
Subject: [PATCH v2] http: allow use of TLS 1.3
Quoted 2 lines
Add a tlsv1.3 option to http.sslVersion in addition to the existing tlsv1.[012] options. libcurl has supported this since 7.52.0.
Good.
Quoted 1 line
Quoted 7 lines
Quoted 1 line
Done during IETF 101 Hackathon
I am on the fence wrt the value of this line, especially because I would strongly suspect that this version is not what you wrote and tested during your Hackathon. Even if it were, would it give value to future "git log" readers by supplying extra context?
Will remove this line.
Quoted 39 lines
Quoted 11 lines
Signed-off-by Loganaden Velvindron <[email protected]>--- Documentation/config.txt | 2 +- http.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)Documentation/config.txt +0
-0Documentation/config.txt http.sslVersion:: · line 1961
Quoted 6 lines
Documentation/config.txt +1
-1@@ -1957,7 +1957,7 @@ http.sslVersion:: - tlsv1.0 - tlsv1.1 - tlsv1.2 - + - tlsv1.3 +Before this change, the block that shows the list of versions had one blank line before and after it. Now we lost the blank line after the block. Is it intended? Possibilities that come to my mind as a reviewer are:
A. There is no difference in the rendered output if we have zero
blank line (i.e. with the patch), or one blank line (i.e. before the patch applied).A.1) the submitter made this change on purpose, because it will make the source shorter without affecting the output, as a "clean-up while at it" change.
A.2) this was an accidental change, which did not break the output merely because the submitter was lucky.
B. The rendered output changes due to the lack of the blank line.
B.1) And it changes in a good way. The submitter made this change on purpose.
Quoted 8 lines
B.2) And it changes in a bad way, but the submitter did not notice it.
Please do not make reviewers wonder. Either avoid making unnecessary changes (e.g. you could have just added a new line with tlsv1.3 on it without touching the blank line), or make the change and explain why you made that change that is not essential for the purpose of adding tls1.3 which is the main focus of this patch.
Alright.
Quoted 18 lines
Quoted 8 lines
Documentation/config.txt +0
-0Can be overridden by the `GIT_SSL_VERSION` environment variable. To force git to use libcurl's default ssl version and ignore anyhttp.c +0
-0http.c static struct { · line 68
Quoted 7 lines
http.c +3
-0@@ -62,6 +62,9 @@ static struct { { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 }, { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 }, #endif +#ifdef CURL_SSLVERSION_TLSv1_3 + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 } +#endif };It seems to me that
Quoted 4 lines
https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956tells me that this #ifdef would not work. Did you test it with the "test not version but feature" change you made at the last minute?
I compiled it.
Quoted 3 lines
I know it is not your fault but is Ævar's, but you're responsible for double-checking what you are told on the internet ;-)
Yes, my fault, not Ævar Arnfjörð Bjarmason .
Quoted 2 lines
Thanks.