Skip to content
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

crypto/tls: TestHandshakeClientECDSATLS13 fails with "wsarecv: An existing connection was forcibly closed by the remote host. FAIL" #28852

Closed
alexbrainman opened this issue Nov 18, 2018 · 12 comments
Assignees
Milestone

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 18, 2018

Windows builders occasionally

https://build.golang.org/log/834476e2373d60d85854feea03232afcea7610a7
https://build.golang.org/log/f60e1b5dfbf4752a6dcd60be96e7c2e311328734
https://build.golang.org/log/6bb82fe2e703056d8c34cb9d2e2f44d2237eb3ed

break with

--- FAIL: TestHandshakeClientECDSATLS13 (0.00s)
    --- FAIL: TestHandshakeClientECDSATLS13/TLSv13 (0.12s)
        handshake_client_test.go:478: TLSv13-ECDSA #4: read tcp 127.0.0.1:49195->127.0.0.1:49229: wsarecv: An existing connection was forcibly closed by the remote host.
FAIL
FAIL	crypto/tls	3.947s

Alex

@FiloSottile FiloSottile self-assigned this Nov 18, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Nov 18, 2018
@FiloSottile FiloSottile changed the title net: TestHandshakeClientECDSATLS13 fails with "wsarecv: An existing connection was forcibly closed by the remote host. FAIL" crypto/tls: TestHandshakeClientECDSATLS13 fails with "wsarecv: An existing connection was forcibly closed by the remote host. FAIL" Nov 18, 2018
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 18, 2018

Looks like the same issue that I encountered in dc0be72.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Nov 21, 2018

Looks like the same issue that I encountered in dc0be72.

https://go-review.googlesource.com/c/go/+/147419 trybot failure https://storage.googleapis.com/go-build-log/47c24d6c/windows-amd64-2016_c500855c.log has sligtly different error message:

handshake_client_test.go:479: TLSv13-X25519-ECDHE #4: read tcp 127.0.0.1:49697->127.0.0.1:49703: wsarecv: An established connection was aborted by the software in your host machine.

This error message must be WSAECONNABORTED according to https://docs.microsoft.com/en-us/windows/desktop/debug/system-error-codes--9000-11999-

And original issue message is WSAECONNRESET.

I suspect both messages indicate that there is some not-read data on connection, and the connection is closed. This is from my past experience, I did not debug this.

Alex

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 27, 2018

Another one in https://storage.googleapis.com/go-build-log/8413fe9f/windows-386-2008_bacc8b63.log.

wsarecv: An existing connection was forcibly closed by the remote host.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Nov 28, 2018

Another one in https://storage.googleapis.com/go-build-log/8413fe9f/windows-386-2008_bacc8b63.log.

It is happening pretty regularly. I took a snapshot of all windows builders at this moment

image

all reds, except windows/arm, are because of this issue.

Alex

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 28, 2018

I still find it incredible that there is no way to tell the kernel "finish sending and THEN close the connection, pretty please", but here we control both sides when not recording traces, so I can probably fix this without increasing time.Sleeps. Sending a CL now.

(Just this week I noticed a similar issue in Tor: https://lists.torproject.org/pipermail/tor-dev/2018-November/013556.html)

@FiloSottile FiloSottile added the Soon label Nov 28, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 28, 2018

@FiloSottile Likely I misunderstand, but by default when closing a socket the kernel (Unix or Windows) will continue sending the data in the background. You can control this using https://golang.org/pkg/net/#TCPConn.SetLinger .

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 28, 2018

Upon re-reading https://go-review.googlesource.com/c/net/+/71372/ it's more nuanced than I understood it: the issue seems to be that if there is data in the receive buffer when Close is called, a RST is generated, which cuts short the send as it gets priority. Still annoying, but not as incredible.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

Ok, this makes sense: when the client closes the connection immediately after the handshake without reading anything from the server, the session tickets are sitting unread on the wire, causing the client to send a RST. I guess it's flaky instead of breaking every time because then the RST and the closeVerify race to the server.

I don't want the session tickets in there anyway, but there is no way to tell openssl not to send them. openssl/openssl#7727

The better fix would be not to record stuff we ignored by recording on the client side instead of the server side, I suppose, but I don't want to risk ignoring a server alert letting us know something went wrong.

The quick patch would be to do a read on the client side to drain the tickets, but since they don't show up on the application side, it would have to be a blind read with a timeout, either flaky or slow.

So I guess I'm patching OpenSSL until they upstream a fix.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

Turns out by reading the code that there is an undocumented -num_tickets 0 option for TLS 1.3.

CL incoming.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 29, 2018

Change https://golang.org/cl/151659 mentions this issue: crypto/tls: prevent the test server from sending session ticket

@FiloSottile FiloSottile added the Testing label Nov 29, 2018
@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Nov 29, 2018

I still find it incredible that there is no way to tell the kernel "finish sending and THEN close the connection, pretty please",

I think you want net.(*TCPConn).CloseWrite combined together with reading from connection until io.EOF arrives.

I could be wrong, but the idea of TCP was connection between two parties where data "does not get lost". So you cannot just "ignore" some data without the system gets errors somewhere. If you want to ignore some data, you still have to read it from the connection.

Alex

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

@alexbrainman yes, I was missing the part where the RST is caused by data in the receive buffer, not the send buffer, which is much more reasonable.

@gopherbot gopherbot closed this in 4c51c93 Nov 29, 2018
@golang golang locked and limited conversation to collaborators Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.