Skip to content

Commit 7617c35

Browse files
fabianfettweissi
fabianfett
and
weissi
authored
Handle NIOSSLError.uncleanShutdown correctly (#472)
### 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 <johannesweiss@apple.com>
1 parent 170fd53 commit 7617c35

16 files changed

+450
-362
lines changed

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ClientChannelHandler.swift‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
153153

154154
let action = self.state.runNewRequest(
155155
head: req.requestHead,
156-
metadata: req.requestFramingMetadata,
157-
ignoreUncleanSSLShutdown: req.requestOptions.ignoreUncleanSSLShutdown
156+
metadata: req.requestFramingMetadata
158157
)
159158
self.run(action, context: context)
160159
}

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -156,17 +156,13 @@ struct HTTP1ConnectionStateMachine {
156156

157157
mutating func runNewRequest(
158158
head: HTTPRequestHead,
159-
metadata: RequestFramingMetadata,
160-
ignoreUncleanSSLShutdown: Bool
159+
metadata: RequestFramingMetadata
161160
) -> Action {
162161
guard case .idle = self.state else {
163162
preconditionFailure("Invalid state")
164163
}
165164

166-
var requestStateMachine = HTTPRequestStateMachine(
167-
isChannelWritable: self.isChannelWritable,
168-
ignoreUncleanSSLShutdown: ignoreUncleanSSLShutdown
169-
)
165+
var requestStateMachine = HTTPRequestStateMachine(isChannelWritable: self.isChannelWritable)
170166
let action = requestStateMachine.startRequest(head: head, metadata: metadata)
171167

172168
// by default we assume a persistent connection. however in `requestVerified`, we read the

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
2424

2525
private let eventLoop: EventLoop
2626

27-
private var state: HTTPRequestStateMachine = .init(isChannelWritable: false, ignoreUncleanSSLShutdown: false) {
27+
private var state: HTTPRequestStateMachine = .init(isChannelWritable: false) {
2828
willSet {
2929
self.eventLoop.assertInEventLoop()
3030
}

‎Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,8 @@ struct HTTPRequestStateMachine {
103103

104104
private var isChannelWritable: Bool
105105

106-
private let ignoreUncleanSSLShutdown: Bool
107-
108-
init(isChannelWritable: Bool, ignoreUncleanSSLShutdown: Bool) {
106+
init(isChannelWritable: Bool) {
109107
self.isChannelWritable = isChannelWritable
110-
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
111108
}
112109

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

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

208+
case .running(.streaming, .receivingBody(let responseHead, _)),
209+
.running(.endSent, .receivingBody(let responseHead, _)) where error as? NIOSSLError == .uncleanShutdown:
210+
// This code is only reachable for request and responses, which we expect to have a body.
211+
// We depend on logic from the HTTPResponseDecoder here. The decoder will emit an
212+
// HTTPResponsePart.end right after the HTTPResponsePart.head, for every request with a
213+
// CONNECT or HEAD method and every response with a 1xx, 204 or 304 response status.
214+
//
215+
// For this reason we only need to check the "content-length" or "transfer-encoding"
216+
// headers here to determine if we are potentially in an EOF terminated response.
217+
218+
if responseHead.headers.contains(name: "content-length") || responseHead.headers.contains(name: "transfer-encoding") {
219+
// If we have already received the response head, the parser will ensure that we
220+
// receive a complete response, if the content-length or transfer-encoding header
221+
// was set. In this case we can ignore the NIOSSLError.uncleanShutdown. We will see
222+
// a HTTPParserError very soon.
223+
return .wait
224+
}
225+
226+
// If the response is EOF terminated, we need to rely on a clean tls shutdown to be sure
227+
// we have received all necessary bytes. For this reason we forward the uncleanShutdown
228+
// error to the user.
229+
self.state = .failed(error)
230+
return .failRequest(error, .close)
231+
209232
case .running:
210233
self.state = .failed(error)
211234
return .failRequest(error, .close)
235+
212236
case .finished, .failed:
213237
// ignore error
214238
return .wait
239+
215240
case .modifying:
216241
preconditionFailure("Invalid state: \(self.state)")
217242
}

‎Sources/AsyncHTTPClient/ConnectionPool/RequestOptions.swift‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,15 @@ struct RequestOptions {
1818
/// The maximal `TimeAmount` that is allowed to pass between `channelRead`s from the Channel.
1919
var idleReadTimeout: TimeAmount?
2020

21-
/// Should `NIOSSLError.uncleanShutdown` be forwarded to the user in HTTP/1 mode.
22-
var ignoreUncleanSSLShutdown: Bool
23-
24-
init(idleReadTimeout: TimeAmount?, ignoreUncleanSSLShutdown: Bool) {
21+
init(idleReadTimeout: TimeAmount?) {
2522
self.idleReadTimeout = idleReadTimeout
26-
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
2723
}
2824
}
2925

3026
extension RequestOptions {
3127
static func fromClientConfiguration(_ configuration: HTTPClient.Configuration) -> Self {
3228
RequestOptions(
33-
idleReadTimeout: configuration.timeout.read,
34-
ignoreUncleanSSLShutdown: configuration.ignoreUncleanSSLShutdown
29+
idleReadTimeout: configuration.timeout.read
3530
)
3631
}
3732
}

‎Sources/AsyncHTTPClient/HTTPClient.swift‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,11 @@ public class HTTPClient {
601601
/// Enables automatic body decompression. Supported algorithms are gzip and deflate.
602602
public var decompression: Decompression
603603
/// Ignore TLS unclean shutdown error, defaults to `false`.
604-
public var ignoreUncleanSSLShutdown: Bool
604+
@available(*, deprecated, message: "AsyncHTTPClient now correctly supports handling unexpected SSL connection drops. This property is ignored")
605+
public var ignoreUncleanSSLShutdown: Bool {
606+
get { false }
607+
set {}
608+
}
605609

606610
// TODO: make public
607611
// TODO: set to automatic by default
@@ -645,7 +649,6 @@ public class HTTPClient {
645649
self.timeout = timeout
646650
self.connectionPool = connectionPool
647651
self.proxy = proxy
648-
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
649652
self.decompression = decompression
650653
self.httpVersion = httpVersion
651654
}

‎Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
2525

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

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

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

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

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

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

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

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

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

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

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

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

@@ -234,7 +234,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
234234
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
235235
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
236236
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
237-
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
237+
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
238238
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
239239
let responseHead = HTTPResponseHead(version: .http1_1, status: .ok)
240240
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
@@ -249,7 +249,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
249249
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
250250
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
251251
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
252-
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
252+
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
253253
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
254254
let responseHead = HTTPResponseHead(version: .http1_1, status: .switchingProtocols)
255255
XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false))
@@ -261,7 +261,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
261261
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
262262
let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
263263
let metadata = RequestFramingMetadata(connectionClose: false, body: .none)
264-
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata, ignoreUncleanSSLShutdown: false)
264+
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
265265
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false))
266266
let responseHead = HTTPResponseHead(version: .http1_1, status: .init(statusCode: 103, reasonPhrase: "Early Hints"))
267267
XCTAssertEqual(state.channelRead(.head(responseHead)), .wait)

‎Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift‎

Lines changed: 0 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -931,103 +931,6 @@ final class ConnectionsCountHandler: ChannelInboundHandler {
931931
}
932932
}
933933

934-
internal class HttpBinForSSLUncleanShutdown {
935-
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
936-
let serverChannel: Channel
937-
938-
var port: Int {
939-
return Int(self.serverChannel.localAddress!.port!)
940-
}
941-
942-
init(channelPromise: EventLoopPromise<Channel>? = nil) {
943-
self.serverChannel = try! ServerBootstrap(group: self.group)
944-
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
945-
.childChannelOption(ChannelOptions.socket(IPPROTO_TCP, TCP_NODELAY), value: 1)
946-
.childChannelInitializer { channel in
947-
let requestDecoder = HTTPRequestDecoder()
948-
return channel.pipeline.addHandler(ByteToMessageHandler(requestDecoder)).flatMap {
949-
let configuration = TLSConfiguration.makeServerConfiguration(certificateChain: [.certificate(try! NIOSSLCertificate(bytes: Array(cert.utf8), format: .pem))],
950-
privateKey: .privateKey(try! NIOSSLPrivateKey(bytes: Array(key.utf8), format: .pem)))
951-
let context = try! NIOSSLContext(configuration: configuration)
952-
return channel.pipeline.addHandler(NIOSSLServerHandler(context: context), name: "NIOSSLServerHandler", position: .first).flatMap {
953-
channel.pipeline.addHandler(HttpBinForSSLUncleanShutdownHandler(channelPromise: channelPromise))
954-
}
955-
}
956-
}.bind(host: "127.0.0.1", port: 0).wait()
957-
}
958-
959-
func shutdown() {
960-
try! self.group.syncShutdownGracefully()
961-
}
962-
}
963-
964-
internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler {
965-
typealias InboundIn = HTTPServerRequestPart
966-
typealias OutboundOut = ByteBuffer
967-
968-
let channelPromise: EventLoopPromise<Channel>?
969-
970-
init(channelPromise: EventLoopPromise<Channel>? = nil) {
971-
self.channelPromise = channelPromise
972-
}
973-
974-
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
975-
switch self.unwrapInboundIn(data) {
976-
case .head(let req):
977-
self.channelPromise?.succeed(context.channel)
978-
979-
let response: String?
980-
switch req.uri {
981-
case "/nocontentlength":
982-
response = """
983-
HTTP/1.1 200 OK\r\n\
984-
Connection: close\r\n\
985-
\r\n\
986-
foo
987-
"""
988-
case "/nocontent":
989-
response = """
990-
HTTP/1.1 204 OK\r\n\
991-
Connection: close\r\n\
992-
\r\n
993-
"""
994-
case "/noresponse":
995-
response = nil
996-
case "/wrongcontentlength":
997-
response = """
998-
HTTP/1.1 200 OK\r\n\
999-
Connection: close\r\n\
1000-
Content-Length: 6\r\n\
1001-
\r\n\
1002-
foo
1003-
"""
1004-
default:
1005-
response = """
1006-
HTTP/1.1 404 OK\r\n\
1007-
Connection: close\r\n\
1008-
Content-Length: 9\r\n\
1009-
\r\n\
1010-
Not Found
1011-
"""
1012-
}
1013-
1014-
if let response = response {
1015-
var buffer = context.channel.allocator.buffer(capacity: response.count)
1016-
buffer.writeString(response)
1017-
context.writeAndFlush(self.wrapOutboundOut(buffer), promise: nil)
1018-
}
1019-
1020-
context.channel.pipeline.removeHandler(name: "NIOSSLServerHandler").whenSuccess {
1021-
context.close(promise: nil)
1022-
}
1023-
case .body:
1024-
()
1025-
case .end:
1026-
()
1027-
}
1028-
}
1029-
}
1030-
1031934
internal final class CloseWithoutClosingServerHandler: ChannelInboundHandler {
1032935
typealias InboundIn = HTTPServerRequestPart
1033936
typealias OutboundOut = HTTPServerResponsePart

0 commit comments

Comments
 (0)