Skip to content

Support request specific TLS configuration #358

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

Merged
merged 15 commits into from
May 7, 2021
3 changes: 2 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ let package = Package(
],
dependencies: [
.package(url: "https://github.com/apple/swift-nio.git", from: "2.27.0"),
.package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.12.0"),
.package(url: "https://github.com/apple/swift-nio-ssl.git", .branch("main")),
// .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.12.0"),
.package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.3.0"),
.package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"),
.package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"),
Expand Down
18 changes: 18 additions & 0 deletions Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import NIOSSL

/// Wrapper around `TLSConfiguration` from NIOSSL to provide a best effort implementation of `Hashable`
struct BestEffortHashableTLSConfiguration: Hashable {
let base: TLSConfiguration

init(wrapping base: TLSConfiguration) {
self.base = base
}

func hash(into hasher: inout Hasher) {
base.bestEffortHash(into: &hasher)
}

static func == (lhs: BestEffortHashableTLSConfiguration, rhs: BestEffortHashableTLSConfiguration) -> Bool {
lhs.base.bestEffortEquals(rhs.base)
}
}
4 changes: 4 additions & 0 deletions Sources/AsyncHTTPClient/ConnectionPool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,16 @@ final class ConnectionPool {
self.port = request.port
self.host = request.host
self.unixPath = request.socketPath
if let tls = request.tlsConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

@artemredkin should we add a test case that targets the pool directly here? Ie no actual connections?

self.tlsConfiguration = BestEffortHashableTLSConfiguration(wrapping: tls)
}
}

var scheme: Scheme
var host: String
var port: Int
var unixPath: String
var tlsConfiguration: BestEffortHashableTLSConfiguration?

enum Scheme: Hashable {
case http
Expand Down
11 changes: 8 additions & 3 deletions Sources/AsyncHTTPClient/HTTPHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ extension HTTPClient {
public var headers: HTTPHeaders
/// Request body, defaults to no body.
public var body: Body?
/// Request-specific TLS configuration, defaults to no request-specific TLS configuration.
public var tlsConfiguration: TLSConfiguration?

struct RedirectState {
var count: Int
Expand All @@ -203,17 +205,18 @@ extension HTTPClient {
/// - method: HTTP method.
/// - headers: Custom HTTP headers.
/// - body: Request body.
/// - tlsConfiguration: Request TLS configuration
/// - throws:
/// - `invalidURL` if URL cannot be parsed.
/// - `emptyScheme` if URL does not contain HTTP scheme.
/// - `unsupportedScheme` if URL does contains unsupported HTTP scheme.
/// - `emptyHost` if URL does not contains a host.
public init(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
public init(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil, tlsConfiguration: TLSConfiguration? = nil) throws {
guard let url = URL(string: url) else {
throw HTTPClientError.invalidURL
}

try self.init(url: url, method: method, headers: headers, body: body)
try self.init(url: url, method: method, headers: headers, body: body, tlsConfiguration: tlsConfiguration)
}

/// Create an HTTP `Request`.
Expand All @@ -223,12 +226,13 @@ extension HTTPClient {
/// - method: HTTP method.
/// - headers: Custom HTTP headers.
/// - body: Request body.
/// - tlsConfiguration: Request TLS configuration
/// - throws:
/// - `emptyScheme` if URL does not contain HTTP scheme.
/// - `unsupportedScheme` if URL does contains unsupported HTTP scheme.
/// - `emptyHost` if URL does not contains a host.
/// - `missingSocketPath` if URL does not contains a socketPath as an encoded host.
public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil, tlsConfiguration: TLSConfiguration? = nil) throws {
guard let scheme = url.scheme?.lowercased() else {
throw HTTPClientError.emptyScheme
}
Expand All @@ -244,6 +248,7 @@ extension HTTPClient {
self.scheme = scheme
self.headers = headers
self.body = body
self.tlsConfiguration = tlsConfiguration
}

/// Whether request will be executed using secure socket.
Expand Down
11 changes: 8 additions & 3 deletions Sources/AsyncHTTPClient/Utils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -150,17 +150,22 @@ extension NIOClientTCPBootstrap {
let key = destination

let requiresTLS = key.scheme.requiresTLS
// Override optional connection pool configuration.
var keyConfiguration = configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm finding the naming here somewhat confusing: keyConfiguration is not derived from the key but from configuration, and then we override it with the TLS configuration from key.

I think it'd be nice to wrap this logic up into something written as a function that clarifies what it does (merges config from two sources, preferring config in the key to the general configuration.

I'm also a bit uncertain as to why this is necessary. Why isn't configuration already carrying this TLS config?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should this merge perhaps be done at a higher level, say when we create the HTTP1ConnectionProvider? I am a bit nervous about having two separate configs from the perspective of the connection provider: it should always be creating the exact same connection each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

configuration is specific for the entire HTTPClient, so atm it passes down the configuration of client through all these methods. So, we need at some point to override the tlsConfiguration of it.

I moved the actual configuration "generation" to the place where we initialize the connection provider as you suggested, and added config(overriding:) to Key to retrieve key-specific configuration. Let me know, if this is better!

if let tlsConfiguration = key.tlsConfiguration {
keyConfiguration.tlsConfiguration = tlsConfiguration.base
}
let bootstrap: NIOClientTCPBootstrap
do {
bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: key.host, port: key.port, requiresTLS: requiresTLS, configuration: configuration)
bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: key.host, port: key.port, requiresTLS: requiresTLS, configuration: keyConfiguration)
} catch {
return channelEventLoop.makeFailedFuture(error)
}

let channel: EventLoopFuture<Channel>
switch key.scheme {
case .http, .https:
let address = HTTPClient.resolveAddress(host: key.host, port: key.port, proxy: configuration.proxy)
let address = HTTPClient.resolveAddress(host: key.host, port: key.port, proxy: keyConfiguration.proxy)
channel = bootstrap.connect(host: address.host, port: address.port)
case .unix, .http_unix, .https_unix:
channel = bootstrap.connect(unixDomainSocketPath: key.unixPath)
Expand All @@ -173,7 +178,7 @@ extension NIOClientTCPBootstrap {

if requiresLateSSLHandler {
let handshakePromise = channel.eventLoop.makePromise(of: Void.self)
channel.pipeline.syncAddLateSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, handshakePromise: handshakePromise)
channel.pipeline.syncAddLateSSLHandlerIfNeeded(for: key, tlsConfiguration: keyConfiguration.tlsConfiguration, handshakePromise: handshakePromise)
handshakeFuture = handshakePromise.futureResult
} else if requiresTLS {
do {
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ extension HTTPClientTests {
("testFileDownloadChunked", testFileDownloadChunked),
("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine),
("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
("testRequestSpecificTLS", testRequestSpecificTLS),
]
}
}
18 changes: 18 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2916,4 +2916,22 @@ class HTTPClientTests: XCTestCase {
XCTAssertEqual(error as? ExpectedError, .expected)
}
}

func testRequestSpecificTLS() throws {
let configuration = HTTPClient.Configuration(tlsConfiguration: nil,
timeout: .init(),
ignoreUncleanSSLShutdown: false,
decompression: .disabled)
let localHTTPBin = HTTPBin(ssl: true)
let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
configuration: configuration)
defer {
XCTAssertNoThrow(try localClient.syncShutdown())
XCTAssertNoThrow(try localHTTPBin.shutdown())
}

let request = try HTTPClient.Request(url: "https://localhost:\(localHTTPBin.port)/get", method: .GET, tlsConfiguration: .forClient(certificateVerification: .none))
let response = try localClient.execute(request: request).wait()
XCTAssertEqual(.ok, response.status)
}
}