Skip to content

Handle NIOSSLError.uncleanShutdown correctly #472

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Handle NIOSSLError.uncleanShutdown correctly #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47c7a7b
Extract HTTPBinForUncleanShutdown
fabianfett Nov 10, 2021
df1217e
Handle NIOSSLError.uncleanShutdown correctly
fabianfett Nov 10, 2021
3e03472
Fix comment
fabianfett Nov 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ClientChannelHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {

let action = self.state.runNewRequest(
head: req.requestHead,
metadata: req.requestFramingMetadata,
ignoreUncleanSSLShutdown: req.requestOptions.ignoreUncleanSSLShutdown
metadata: req.requestFramingMetadata
)
self.run(action, context: context)
}
Expand Down
8 changes: 2 additions & 6 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,17 +156,13 @@ struct HTTP1ConnectionStateMachine {

mutating func runNewRequest(
head: HTTPRequestHead,
metadata: RequestFramingMetadata,
ignoreUncleanSSLShutdown: Bool
metadata: RequestFramingMetadata
) -> Action {
guard case .idle = self.state else {
preconditionFailure("Invalid state")
}

var requestStateMachine = HTTPRequestStateMachine(
isChannelWritable: self.isChannelWritable,
ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown
)
var requestStateMachine = HTTPRequestStateMachine(isChannelWritable: self.isChannelWritable)
let action = requestStateMachine.startRequest(head: head, metadata: metadata)

// by default we assume a persistent connection. however in `requestVerified`, we read the
Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {

private let eventLoop: EventLoop

private var state: HTTPRequestStateMachine = .init(isChannelWritable: false, ignoreUncleanSSLShutdown: false) {
private var state: HTTPRequestStateMachine = .init(isChannelWritable: false) {
willSet {
self.eventLoop.assertInEventLoop()
}
Expand Down
39 changes: 32 additions & 7 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,8 @@ struct HTTPRequestStateMachine {

private var isChannelWritable: Bool

private let ignoreUncleanSSLShutdown: Bool

init(isChannelWritable: Bool, ignoreUncleanSSLShutdown: Bool) {
init(isChannelWritable: Bool) {
self.isChannelWritable = isChannelWritable
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}

mutating func startRequest(head: HTTPRequestHead, metadata: RequestFramingMetadata) -> Action {
Expand Down Expand Up @@ -201,17 +198,45 @@ struct HTTPRequestStateMachine {
self.state = .failed(error)
return .failRequest(error, .none)

case .running(.streaming, .receivingBody),
.running(.endSent, .receivingBody)
where error as? NIOSSLError == .uncleanShutdown && self.ignoreUncleanSSLShutdown == true:
case .running(.streaming, .waitingForHead),
.running(.endSent, .waitingForHead) where error as? NIOSSLError == .uncleanShutdown:
// if we received a NIOSSL.uncleanShutdown before we got an answer we should handle
// this like a normal connection close. We will receive a call to channelInactive after
// this error.
return .wait

case .running(.streaming, .receivingBody(let responseHead, _)),
.running(.endSent, .receivingBody(let responseHead, _)) where error as? NIOSSLError == .uncleanShutdown:
// This code is only reachable for request and responses, which we expect to have a body.
// We depend on logic from the HTTPResponseDecoder here. The decoder will emit an
// HTTPResponsePart.end right after the HTTPResponsePart.head, for every request with a
// CONNECT or HEAD method and every response with a 1xx, 204 or 304 response status.
//
// For this reason we only need to check the "content-length" or "transfer-encoding"
// headers here to determine if we are potentially in an EOF terminated response.

if responseHead.headers.contains(name: "content-length") || responseHead.headers.contains(name: "transfer-encoding") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a sufficient check: there are cases where these headers are not present, but we are nonetheless confident of the length of the response. Specifically:

  1. Responses to a HEAD request
  2. Responses with a 204 or 304 status code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTPResponseDecoder will always emit a response .end after having received a response head, if the request had a HEAD or CONNECT method or the response status is 204 or 304.

https://github.com/apple/swift-nio/blob/main/Sources/NIOHTTP1/HTTPDecoder.swift#L301-L314

For this reason, we can expect that we will only be in the streaming state, if we have a "content-length" or "transfer-encoding" header or if the response is EOF terminated. I'm well aware that this is very implicit. If we want to have explicit checks here, I will need to come up with a way to preserve the request method a little longer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can we get this logic into a comment then? The correctness of this code is not apparent merely from reading it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added.

// If we have already received the response head, the parser will ensure that we
// receive a complete response, if the content-length or transfer-encoding header
// was set. In this case we can ignore the NIOSSLError.uncleanShutdown. We will see
// a HTTPParserError very soon.
return .wait
}

// If the response is EOF terminated, we need to rely on a clean tls shutdown to be sure
// we have received all necessary bytes. For this reason we forward the uncleanShutdown
// error to the user.
self.state = .failed(error)
return .failRequest(error, .close)

case .running:
self.state = .failed(error)
return .failRequest(error, .close)

case .finished, .failed:
// ignore error
return .wait

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
}
Expand Down
9 changes: 2 additions & 7 deletions Sources/AsyncHTTPClient/ConnectionPool/RequestOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,15 @@ struct RequestOptions {
/// The maximal `TimeAmount` that is allowed to pass between `channelRead`s from the Channel.
var idleReadTimeout: TimeAmount?

/// Should `NIOSSLError.uncleanShutdown` be forwarded to the user in HTTP/1 mode.
var ignoreUncleanSSLShutdown: Bool

init(idleReadTimeout: TimeAmount?, ignoreUncleanSSLShutdown: Bool) {
init(idleReadTimeout: TimeAmount?) {
self.idleReadTimeout = idleReadTimeout
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}
}

extension RequestOptions {
static func fromClientConfiguration(_ configuration: HTTPClient.Configuration) -> Self {
RequestOptions(
idleReadTimeout: configuration.timeout.read,
ignoreUncleanSSLShutdown: configuration.ignoreUncleanSSLShutdown
idleReadTimeout: configuration.timeout.read
)
}
}
7 changes: 5 additions & 2 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,11 @@ public class HTTPClient {
/// Enables automatic body decompression. Supported algorithms are gzip and deflate.
public var decompression: Decompression
/// Ignore TLS unclean shutdown error, defaults to `false`.
public var ignoreUncleanSSLShutdown: Bool
@available(*, deprecated, message: "AsyncHTTPClient now correctly supports handling unexpected SSL connection drops. This property is ignored")
public var ignoreUncleanSSLShutdown: Bool {
get { false }
set {}
}

// TODO: make public
// TODO: set to automatic by default
Expand Down Expand Up @@ -645,7 +649,6 @@ public class HTTPClient {
self.timeout = timeout
self.connectionPool = connectionPool
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
self.decompression = decompression
self.httpVersion = httpVersion
}
Expand Down
24 changes: 12 additions & 12 deletions Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {

let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"])
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4))
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false), .wait)
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata), .wait)
XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, startBody: true))

let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0]))
Expand Down Expand Up @@ -63,7 +63,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {

let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["content-length": "12"])
Expand Down Expand Up @@ -91,7 +91,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["connection": "close"])
let metadata = RequestFramingMetadata(connectionClose: true, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

let responseHead = HTTPResponseHead(version: .http1_1, status: .ok)
Expand All @@ -107,7 +107,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

let responseHead = HTTPResponseHead(version: .http1_0, status: .ok, headers: ["content-length": "4"])
Expand All @@ -123,7 +123,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

let responseHead = HTTPResponseHead(version: .http1_0, status: .ok, headers: ["content-length": "4", "connection": "keep-alive"])
Expand All @@ -140,7 +140,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.writabilityChanged(writable: true), .wait)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["connection": "close"])
Expand Down Expand Up @@ -169,7 +169,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {

let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))

XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none))
Expand All @@ -181,7 +181,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {

let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"])
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4))
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false), .wait)
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata), .wait)
XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, startBody: true))

let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0]))
Expand Down Expand Up @@ -225,7 +225,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: false), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"])
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4))
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false), .wait)
XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata), .wait)
XCTAssertEqual(state.requestCancelled(closeConnection: false), .failRequest(HTTPClientError.cancelled, .informConnectionIsIdle))
}

Expand All @@ -234,7 +234,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
let responseHead = HTTPResponseHead(version: .http1_1, status: .ok)
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
Expand All @@ -249,7 +249,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
let responseHead = HTTPResponseHead(version: .http1_1, status: .switchingProtocols)
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
Expand All @@ -261,7 +261,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
let responseHead = HTTPResponseHead(version: .http1_1, status: .init(statusCode: 103, reasonPhrase: "Early Hints"))
XCTAssertEqual(state.channelRead(.head(responseHead)), .wait)
Expand Down
97 changes: 0 additions & 97 deletions Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,103 +931,6 @@ final class ConnectionsCountHandler: ChannelInboundHandler {
}
}

internal class HttpBinForSSLUncleanShutdown {
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
let serverChannel: Channel

var port: Int {
return Int(self.serverChannel.localAddress!.port!)
}

init(channelPromise: EventLoopPromise<Channel>? = nil) {
self.serverChannel = try! ServerBootstrap(group: self.group)
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
.childChannelOption(ChannelOptions.socket(IPPROTO_TCP, TCP_NODELAY), value: 1)
.childChannelInitializer { channel in
let requestDecoder = HTTPRequestDecoder()
return channel.pipeline.addHandler(ByteToMessageHandler(requestDecoder)).flatMap {
let configuration = TLSConfiguration.makeServerConfiguration(certificateChain: [.certificate(try! NIOSSLCertificate(bytes: Array(cert.utf8), format: .pem))],
privateKey: .privateKey(try! NIOSSLPrivateKey(bytes: Array(key.utf8), format: .pem)))
let context = try! NIOSSLContext(configuration: configuration)
return channel.pipeline.addHandler(NIOSSLServerHandler(context: context), name: "NIOSSLServerHandler", position: .first).flatMap {
channel.pipeline.addHandler(HttpBinForSSLUncleanShutdownHandler(channelPromise: channelPromise))
}
}
}.bind(host: "127.0.0.1", port: 0).wait()
}

func shutdown() {
try! self.group.syncShutdownGracefully()
}
}

internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler {
typealias InboundIn = HTTPServerRequestPart
typealias OutboundOut = ByteBuffer

let channelPromise: EventLoopPromise<Channel>?

init(channelPromise: EventLoopPromise<Channel>? = nil) {
self.channelPromise = channelPromise
}

func channelRead(context: ChannelHandlerContext, data: NIOAny) {
switch self.unwrapInboundIn(data) {
case .head(let req):
self.channelPromise?.succeed(context.channel)

let response: String?
switch req.uri {
case "/nocontentlength":
response = """
HTTP/1.1 200 OK\r\n\
Connection: close\r\n\
\r\n\
foo
"""
case "/nocontent":
response = """
HTTP/1.1 204 OK\r\n\
Connection: close\r\n\
\r\n
"""
case "/noresponse":
response = nil
case "/wrongcontentlength":
response = """
HTTP/1.1 200 OK\r\n\
Connection: close\r\n\
Content-Length: 6\r\n\
\r\n\
foo
"""
default:
response = """
HTTP/1.1 404 OK\r\n\
Connection: close\r\n\
Content-Length: 9\r\n\
\r\n\
Not Found
"""
}

if let response = response {
var buffer = context.channel.allocator.buffer(capacity: response.count)
buffer.writeString(response)
context.writeAndFlush(self.wrapOutboundOut(buffer), promise: nil)
}

context.channel.pipeline.removeHandler(name: "NIOSSLServerHandler").whenSuccess {
context.close(promise: nil)
}
case .body:
()
case .end:
()
}
}
}

internal final class CloseWithoutClosingServerHandler: ChannelInboundHandler {
typealias InboundIn = HTTPServerRequestPart
typealias OutboundOut = HTTPServerResponsePart
Expand Down
Loading