Ignore uncleanShutdown error when state is .head or .body #77

vkill · 2019-08-08T07:51:55Z

Hi there,

I think we should ignore uncleanShutdown when state is .head or .body, please review it. thx.

swift-server-bot · 2019-08-08T07:51:57Z

Can one of the admins verify this patch?

Lukasa · 2019-08-08T08:51:41Z

I am...unsure about this. This is the only place where .uncleanShutdown actually does communicate useful information: specifically, that we didn't know where EOF was, and the remote server didn't use TLS to tell us, so we can't tell whether the response body got truncated or not. I don't think signalling an error here is necessary the wrong thing to do.

What it may be worth doing is making this behaviour configurable, so for users who don't care they can choose to ignore this error and always treat it as a clean EOF.

vkill · 2019-08-08T09:21:13Z

Please read testNoContentLengthWithNIOSSLUncleanShutdown and testWrongContentLengthWithNIOSSLUncleanShutdown .

We know EOF when tcp connection close even if we ignore .uncleanShutdown.

Lukasa · 2019-08-08T09:44:40Z

No we don't, we only know that when content-length is set: that is, we know it in testWrongContentLengthWithNIOSSLUncleanShutdown, but testNoContentLengthWithNIOSSLUncleanShutdown does not error, and in my view it should.

The problem is that when content-length is not set, the complete response body is signalled by shutting the connection down as discussed in RFC 7230 § 3.3.3 Message Body Length:

Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

In this case, the .uncleanShutdown error is warning you that a network attacker could have injected a TCP FIN to forcibly truncate the response body. In this case, async-http-client cannot be confident that this has not happened, so we warn against it.

I am open to having settings put in place to allow you to ignore this error, but defaulting to reporting this error is the correct thing to do.

vkill · 2019-08-08T10:40:23Z

@Lukasa Yes, you are right. we can configurate it now, please continue to review.

Lukasa · 2019-08-08T16:09:38Z

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -276,19 +276,23 @@ public class HTTPClient {
        public var timeout: Timeout
        /// Upstream proxy, defaults to no proxy.
        public var proxy: Proxy?
+        /// Enables ignore NIOSSLError.uncleanShutdown when state is .head or .body, defaults to `false`.


I think we need a better description. Maybe: "Ignore TLS unclean shutdown errors".

It was updated.

Lukasa · 2019-08-08T16:12:37Z

Sources/AsyncHTTPClient/HTTPClient.swift


-        public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+        public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool = false) {


This change needs to be done differently, I'm afraid. This is a breaking API change, as you are allowed to refer to functions in Swift by their complete set of labels, and this changes the set of labels here. You need to do this:

public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) { self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreNIOSSLUncleanShutdownError: true) } public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool) { self.tlsConfiguration = tlsConfiguration self.followRedirects = followRedirects self.timeout = timeout self.proxy = proxy self.ignoreNIOSSLUncleanShutdownError = ignoreNIOSSLUncleanShutdownError }

It was reverted.

@Lukasa we are not tagged 1.0.0 yet, do you think we can update the signature? It's unlikely imho that someone will be referencing those particular functions, wdyt?

Sure, if we're not maintaining a stable API we can do that.

Lukasa · 2019-08-08T16:15:51Z

Sources/AsyncHTTPClient/HTTPClient.swift

        }

-        public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
+        public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreNIOSSLUncleanShutdownError: Bool = false) {


This requires a similar change as the one above: we need two functions, not one.

It was reverted too.

Lukasa · 2019-08-08T16:21:48Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+                    /// we can also ignore this error like .end .
+                    break
+                }
+                fallthrough


Can I propose using a where clause here? That would be:

case .head where self.ignoreNIOSSLUncleanShutdownError, .body where self.ignoreNIOSSLUncleanShutdownError: break

That avoids the need for a fall through or the internal if.

Yes, it's better to use internal if. it was updated.

Lukasa

Ok, I'm happy with this, but we'll need review from at least one other person.

tomerd · 2019-08-09T17:11:23Z

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -276,19 +276,31 @@ public class HTTPClient {
        public var timeout: Timeout
        /// Upstream proxy, defaults to no proxy.
        public var proxy: Proxy?
+        /// Ignore TLS unclean shutdown error, defaults to `false`.
+        public var ignoreNIOSSLUncleanShutdownError: Bool


=> ignoreNIOSSLUncleanShutdownError is a mouthful imo, maybe ignoreUncleanSSLShutdown? or something even shorter if someone can come up with a suggestion?

=> what are the arguments for the default to be false vs. true?

I’m in favour of a shorter name.

False is a good default because we already ignore it in the cases where it’s 100% safe to do so: setting this switch to true would ignore it in the cases where it potentially opens your application up to some theoretical risks.

maybe ignoreSSLUncleanShutdown?

ignoreUncleanSSLShutdown seems more natural? subjective obviously, so up to @artemredkin

+1 for ignoreUncleanSSLShutdown

@tomerd It was renamed.

artemredkin · 2019-08-14T10:07:51Z

@swift-server-bot test this please

Lukasa · 2019-08-14T12:52:39Z

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+        }
+
+        XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/wrongcontentlength").wait(), "Should fail") { error in
+            guard case let error = error as? HTTPParserError, case .invalidEOFState = error else {
