Skip to content

HTTPClientRequest: allow custom TLS config #709

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 1 commit into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ extension HTTPClient {
// this loop is there to follow potential redirects
while true {
let preparedRequest = try HTTPClientRequest.Prepared(currentRequest, dnsOverride: configuration.dnsOverride)
let response = try await executeCancellable(preparedRequest, deadline: deadline, logger: logger)
let response = try await self.executeCancellable(preparedRequest, deadline: deadline, logger: logger)

guard var redirectState = currentRedirectState else {
// a `nil` redirectState means we should not follow redirects
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import struct Foundation.URL
import NIOCore
import NIOHTTP1
import NIOSSL

@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
extension HTTPClientRequest {
Expand All @@ -37,6 +38,7 @@ extension HTTPClientRequest {
var requestFramingMetadata: RequestFramingMetadata
var head: HTTPRequestHead
var body: Body?
var tlsConfiguration: TLSConfiguration?
}
}

Expand All @@ -58,15 +60,16 @@ extension HTTPClientRequest.Prepared {

self.init(
url: url,
poolKey: .init(url: deconstructedURL, tlsConfiguration: nil, dnsOverride: dnsOverride),
poolKey: .init(url: deconstructedURL, tlsConfiguration: request.tlsConfiguration, dnsOverride: dnsOverride),
requestFramingMetadata: metadata,
head: .init(
version: .http1_1,
method: request.method,
uri: deconstructedURL.uri,
headers: headers
),
body: request.body.map { .init($0) }
body: request.body.map { .init($0) },
tlsConfiguration: request.tlsConfiguration
)
}
}
Expand Down
8 changes: 8 additions & 0 deletions Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import Algorithms
import NIOCore
import NIOHTTP1
import NIOSSL

@usableFromInline
let bagOfBytesToByteBufferConversionChunkSize = 1024 * 1024 * 4
Expand All @@ -32,6 +33,9 @@ let byteBufferMaxSize = Int(UInt32.max)
/// A representation of an HTTP request for the Swift Concurrency HTTPClient API.
///
/// This object is similar to ``HTTPClient/Request``, but used for the Swift Concurrency API.
///
/// - note: For many ``HTTPClientRequest/body-swift.property`` configurations, this type is _not_ a value type
/// (https://github.com/swift-server/async-http-client/issues/708).
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
public struct HTTPClientRequest: Sendable {
/// The request URL, including scheme, hostname, and optionally port.
Expand All @@ -46,11 +50,15 @@ public struct HTTPClientRequest: Sendable {
/// The request body, if any.
public var body: Body?

/// Request-specific TLS configuration, defaults to no request-specific TLS configuration.
public var tlsConfiguration: TLSConfiguration?

public init(url: String) {
self.url = url
self.method = .GET
self.headers = .init()
self.body = .none
self.tlsConfiguration = nil
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/AsyncAwait/Transaction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ import NIOSSL
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
extension Transaction: HTTPSchedulableRequest {
var poolKey: ConnectionPool.Key { self.request.poolKey }
var tlsConfiguration: TLSConfiguration? { return nil }
var tlsConfiguration: TLSConfiguration? { return self.request.tlsConfiguration }
var requiredEventLoop: EventLoop? { return nil }

func requestWasQueued(_ scheduler: HTTPRequestScheduler) {
Expand Down
45 changes: 45 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3525,4 +3525,49 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass {
let response = try client.get(url: self.defaultHTTPBinURLPrefix + "get").wait()
XCTAssertEqual(.ok, response.status)
}

func testAsyncExecuteWithCustomTLS() async throws {
let httpsBin = HTTPBin(.http1_1(ssl: true))
defer {
XCTAssertNoThrow(try httpsBin.shutdown())
}

// A client with default TLS settings, i.e. it won't accept `httpsBin`'s fake self-signed cert
let client = HTTPClient(eventLoopGroup: MultiThreadedEventLoopGroup.singleton)
defer {
XCTAssertNoThrow(try client.shutdown().wait())
}

var request = HTTPClientRequest(url: "https://localhost:\(httpsBin.port)/get")

// For now, let's allow bad TLS certs
request.tlsConfiguration = TLSConfiguration.clientDefault
// ! is safe, assigned above
request.tlsConfiguration!.certificateVerification = .none
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets get ride of the force unwrap here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but that would hide issues. Force unwraps here are good because they'd catch that going wrong at the right place here

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, nil is the default configuration and it would be reasonable to normalise the input value to become nil after setting it to the default value.

Note that this shouldn't be an Optional property to begin with but that's the current API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that would not be reasonable, that would be user hostile. If this breaks, AHC is broken, let's not hide this


let response1 = try await client.execute(request, timeout: /* infinity */ .hours(99))
XCTAssertEqual(.ok, response1.status)

// For the second request, we reset the TLS config
request.tlsConfiguration = nil
do {
let response2 = try await client.execute(request, timeout: /* infinity */ .hours(99))
XCTFail("shouldn't succeed, self-signed cert: \(response2)")
} catch {
switch error as? NIOSSLError {
case .some(.handshakeFailed(_)):
() // ok
default:
XCTFail("unexpected error: \(error)")
}
}

// And finally we allow it again.
request.tlsConfiguration = TLSConfiguration.clientDefault
// ! is safe, assigned above
request.tlsConfiguration!.certificateVerification = .none

let response3 = try await client.execute(request, timeout: /* infinity */ .hours(99))
XCTAssertEqual(.ok, response3.status)
}
}