New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--tcp-fastopen with https URLs cause SSL Protocol error #907

Closed
arnd opened this Issue Jul 7, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@arnd

arnd commented Jul 7, 2016

For some reason curl requests for https URLs with --tcp-fast-open
cause a SSL protocol error.
When doing tcpdump, I don't even see a connection attempt from curl, despite curl
claiming in verbose output it "Connected to" the host.
Without HTTPs, TFO works fine. Without TFO HTTPs works fine.

I did this

./curl -v --tcp-fastopen https://www.google.de
* Rebuilt URL to: https://www.google.de/
*   Trying 2a00:1450:4008:802::2003...
* Connected to www.google.de () port 443 (#0)
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: none
* Unknown SSL protocol error in connection to www.google.de:443 
* Closing connection 0
curl: (35) Unknown SSL protocol error in connection to www.google.de:443 

I expected the following (but with --tcp-fastopen)

./curl -v  https://www.google.de
* Rebuilt URL to: https://www.google.de/
*   Trying 2a00:1450:4008:802::2003...
* Connected to www.google.de (2a00:1450:4008:802::2003) port 443 (#0)
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: none
* TLSv1.2 (OUT), TLS header, Certificate Status (22):
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use http/1.1

curl/libcurl version

arnd@kallisto:~/packages/curl/curl-7.49.1/src$ ./curl -V
curl 7.49.1 (x86_64-pc-linux-gnu) libcurl/7.49.1 OpenSSL/1.0.2g zlib/1.2.8 libidn/1.32 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp 
Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP UnixSockets 

operating system

Ubuntu 16.04, amd64

@arnd

This comment has been minimized.

arnd commented Jul 7, 2016

Ouch, after debugging this further, I realize there is no (easy) fix.
Linux kernel API for using TFO expects that instead of connect(), the connection request happens with sendto() of the first data.
In case of https that first data is generated by the SSL library, thus the SSL library itself would need to perform that sendto().

@bagder

This comment has been minimized.

Member

bagder commented Jul 10, 2016

So I figure the correct fix here (short term at least) is to make sure we don't try to do TFO with TLS enabled, right?

@bagder bagder added the SSL/TLS label Jul 10, 2016

@ghedo

This comment has been minimized.

Member

ghedo commented Jul 10, 2016

The OS X implementation should work with TLS though (they have a saner API), and it shouldn't be too difficult to actually implement with OpenSSL (don't know about the other TLS libraries).

Anyway, I can make a patch for disabling Linux TFO+TLS for now when I have a bit of time.

@ghedo

This comment has been minimized.

Member

ghedo commented Jul 10, 2016

@arnd

This comment has been minimized.

arnd commented Jul 11, 2016

As TFO needs to be explicitly requested by the user. Wouldn't it be better to fail with a sensible error message, like "Not implemented yet."?
If it is preferred that the command doesn't fail, maybe a warning can inform the user, that TFO was ignored/disabled to make the connection work, because of TLS?

@ghedo

This comment has been minimized.

Member

ghedo commented Jul 11, 2016

There are other scenarios where TFO is requested but not actually used (e.g. the server doesn't support it) and we can't really do anything in those cases (not even print a warning) since it's handled transparently by the kernel, so I'd say this is a similar case.

@jay

This comment has been minimized.

Member

jay commented Jul 16, 2016

I agree with @ghedo, I think we just make it work for now. I have some questions I will address in the branch

jay added a commit that referenced this issue Jul 20, 2016

connect: disable TFO on Linux when using SSL
- Linux TFO + TLS is not implemented yet.

Bug: #907
@jay

This comment has been minimized.

Member

jay commented Jul 20, 2016

Since there is a release tomorrow (July 21) and since I'm unclear on the behavior of connectx (asked in the proposed PR) I've instead committed my own version which sidesteps the issue. Landed in 4ee2035.

@stale

This comment has been minimized.

stale bot commented Apr 8, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Apr 29, 2017

@Alexander--

This comment has been minimized.

Alexander-- commented Feb 7, 2018

@bagder in 2017 there have been some interesting changes, both in openssl and Linux kernel.

The new socket flag — TCP_FASTOPEN_CONNECT — is supposed to make fast open "just work", like in Mac OS X. Meanwhile, it sounds like recent versions of openssl should be able to better cope with fastopen (that commit looks like it would benefit servers more than clients, but at least there is some kind of official support).

Does it make sense to revisit this issue now?

@bagder

This comment has been minimized.

Member

bagder commented Feb 7, 2018

see #2056 and commit 979b012 (shipped in 7.57.0).

What is there to revisit?

@Alexander--

This comment has been minimized.

Alexander-- commented Feb 7, 2018

@bagder Ah, so it has already been done. I quickly searched for "fastopen" in logs, didn't notice it was added under slightly different wording.

In my defense — the consequences of this ticket (e.g. that curl couldn't use fast open with SSL on old kernels) have never been properly documented, so I just assumed that this ticket is the authority on issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.