Skip to content

uncleanShutdown should be handled automatically (where possible) #238

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

Closed
weissi opened this issue Jun 12, 2020 · 1 comment · Fixed by #472
Closed

uncleanShutdown should be handled automatically (where possible) #238

weissi opened this issue Jun 12, 2020 · 1 comment · Fixed by #472
Labels
kind/enhancement Improvements to existing feature.
Milestone

Comments

@weissi
Copy link
Contributor

weissi commented Jun 12, 2020

TLS (SSL) encrypts, validates, and authenticates all the data that goes through the connection. That is a fantastic property to have and solves most issues. TLS however still runs over TCP and the control packets of TCP are not encrypted. This means that the one thing an attacker can still do on a TLS connection is to close it. The attacker could send RST or FIN packets the other peer and the receiving peer has no means to verify if this RST/FIN packet is actually coming from the other peer (as opposed to an attacker).
To fix this problem, TLS introduces a close_notify message that is send over connection as encrypted data. So if the other peer receives a close_notify it knows that it was truly sent by the other peer (and not an attacker). A well behaving peer would then reply to the close_notify with its own close_notify and after that both peers can close the TCP connection because they know that the respective other peer knows they're okay closing it.

Okay, but what's the issue with having a connection just close. Wouldn't I notice that part of the data is missing? The answer is it depends. Many protocols actually send to the other peer how much data they will send before sending the data. And they also have a well defined "end message". If you're using such a protocol, then an "unclean shutdown" (which is you have received a RST/FIN without a close_notify) is totally harmless. The higher level protocol allows you to distinguish between a truncated message (when there's still outstanding data) and a close after a completed message.

The reason SwiftNIO sends you a NIOSSLError.uncleanShutdown if it sees a connection closing without a prior close_notify is because it doesn't know what protocol you're implementing. Maybe the protocol you speak doesn't transmit length information so a truncated message cannot be told apart from a complete message.

Let's go into some example protocols and their behaviour regarding framing:

  • With HTTP/2 the other peer always knows how much data to expect, so an unclean shutdown is totally harmless, the error can be ignored.
  • With HTTP/1, the situation is much more complicated: HTTP/1 when using the content-length header is unaffected by truncation attacks because we know the content length. So if the connection closes before we have received that many bytes, we know it was a truncated message (either by an attacker injecting a FIN/RST, or by a software/network problem somewhere along the way)
  • HTTP/1 with transfer-encoding: chunked is also unaffected by truncation attacks because each chunk sends its length and there's a special "END" chunk (0\r\n\r\n). Unfortunately HTTP/1 can be used without either transfer-encoding: chunked or content-length. It then runs in the "EOF framing" mode which means that the message ends when we receive a connection close 😢 . Very bad, if HTTP/1 is used in "EOF framing" mode, then an unclean shutdown is actually a real error because we cannot tell a truncated message apart from a complete message.

Why should AHC do better here? Well, AHC knows if its speaking HTTP/1 or HTTP/2 and whether the server sent a content-length or transfer-encoding: chunked or not. Therefore, AHC should suppress the uncleanShutdown error in all cases apart from when HTTP/1 is run in "EOF framing mode".

The good news is that this will almost never happen. Nobody in their right mind uses EOF framing mode.

@weissi
Copy link
Contributor Author

weissi commented Jun 12, 2020

This ticket is very related to #231 . The above explanation is really the reason why the "ignoreUncleanShutdown" parameter should be deprecated (#231).

@artemredkin artemredkin added this to the 1.2.1 milestone Jun 13, 2020
@artemredkin artemredkin added kind/enhancement Improvements to existing feature. help wanted and removed help wanted labels Jun 13, 2020
@artemredkin artemredkin modified the milestones: 1.2.1, 1.2.2 Aug 20, 2020
@artemredkin artemredkin modified the milestones: 1.2.2, 1.2.3 Nov 11, 2020
fabianfett added a commit that referenced this issue Nov 11, 2021
### Motivation

Fixes #238 and #231.

### Changes

- Extracted the unclean shutdown test from `HTTPClientTests` into their own file `HTTPClientUncleanSSLConnectionShutdownTests`
- Copy and pasted @weissi great explanation from #238 into the test file
- Removed property `ignoreUncleanSSLShutdown` everywhere

### Result

`ignoreUncleanSSLShutdown` on `HTTPClient.Configuration` is deprecated and ignored.

Co-authored-by: Johannes Weiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants