Skip to content

Commit 236b1de

Browse files
artemredkinLukasa
andauthored
Fixes propagation of errors during TLS handshakre (#316)
Motivation: Right now we only handle one type of SSL error: `.handshakeFailed`, but in reality a multitude of errors can happen, for example, remote party might just close connection that will in turn raise `.uncleanShutdown` error, that will be dropped on the floor and users will only get non-descriptive `NoResult` error. Modifications: Handle all types of SSL errors during handshake instead of just one. Adds a test. Result: Closes #313 Co-authored-by: Cory Benfield <[email protected]>
1 parent 1bc2e1a commit 236b1de

File tree

3 files changed

+41
-10
lines changed

3 files changed

+41
-10
lines changed

Sources/AsyncHTTPClient/HTTPClient.swift

+3-10
Original file line numberDiff line numberDiff line change
@@ -935,16 +935,9 @@ class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler {
935935
}
936936

937937
func errorCaught(context: ChannelHandlerContext, error: Error) {
938-
if let sslError = error as? NIOSSLError {
939-
switch sslError {
940-
case .handshakeFailed:
941-
self.completionPromise?.fail(error)
942-
self.completionPromise = nil
943-
context.pipeline.removeHandler(self, promise: nil)
944-
default:
945-
break
946-
}
947-
}
938+
self.completionPromise?.fail(error)
939+
self.completionPromise = nil
940+
context.pipeline.removeHandler(self, promise: nil)
948941
context.fireErrorCaught(error)
949942
}
950943

Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

+1
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ extension HTTPClientTests {
125125
("testBodyUploadAfterEndFails", testBodyUploadAfterEndFails),
126126
("testNoBytesSentOverBodyLimit", testNoBytesSentOverBodyLimit),
127127
("testDoubleError", testDoubleError),
128+
("testSSLHandshakeErrorPropagation", testSSLHandshakeErrorPropagation),
128129
]
129130
}
130131
}

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+37
Original file line numberDiff line numberDiff line change
@@ -2675,4 +2675,41 @@ class HTTPClientTests: XCTestCase {
26752675
// we need to verify that second error on write after timeout does not lead to double-release.
26762676
XCTAssertThrowsError(try self.defaultClient.execute(request: request, deadline: .now() + .milliseconds(2)).wait())
26772677
}
2678+
2679+
func testSSLHandshakeErrorPropagation() throws {
2680+
class CloseHandler: ChannelInboundHandler {
2681+
typealias InboundIn = Any
2682+
2683+
func channelActive(context: ChannelHandlerContext) {
2684+
context.close(promise: nil)
2685+
}
2686+
}
2687+
2688+
let server = try ServerBootstrap(group: self.serverGroup)
2689+
.childChannelOption(ChannelOptions.autoRead, value: false)
2690+
.childChannelInitializer { channel in
2691+
channel.pipeline.addHandler(CloseHandler())
2692+
}
2693+
.bind(host: "127.0.0.1", port: 0)
2694+
.wait()
2695+
2696+
defer {
2697+
XCTAssertNoThrow(try server.close().wait())
2698+
}
2699+
2700+
let request = try Request(url: "https://localhost:\(server.localAddress!.port!)", method: .GET)
2701+
let task = self.defaultClient.execute(request: request, delegate: TestHTTPDelegate())
2702+
2703+
XCTAssertThrowsError(try task.wait()) { error in
2704+
#if os(Linux)
2705+
XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown)
2706+
#else
2707+
if isTestingNIOTS() {
2708+
XCTAssertEqual((error as? AsyncHTTPClient.HTTPClient.NWTLSError).map { $0.status }, errSSLClosedNoNotify)
2709+
} else {
2710+
XCTAssertEqual((error as? IOError).map { $0.errnoCode }, 54)
2711+
}
2712+
#endif
2713+
}
2714+
}
26782715
}

0 commit comments

Comments
 (0)