-
Notifications
You must be signed in to change notification settings - Fork 33
Af/optional min tls version #1037
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
base: main
Are you sure you want to change the base?
Changes from all commits
8be8091
203d18a
3c4cad2
91a35f7
095a075
aa99257
627b8fe
343c926
d7d2d9b
0f910b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -447,6 +447,21 @@ public final class URLSessionHTTPClient: HTTPClient, @unchecked Sendable { | |
| self.connectionTimeout = httpClientConfiguration.connectTimeout ?? 60.0 | ||
| var urlsessionConfiguration = URLSessionConfiguration.default | ||
| urlsessionConfiguration = URLSessionConfiguration.from(httpClientConfiguration: httpClientConfiguration) | ||
|
|
||
| if let tlsConfig = tlsConfiguration, let minVersion = tlsConfig.minimumTLSVersion { | ||
| switch minVersion { | ||
| case .tls10, | ||
| .tls11: | ||
| // Enforce a secure minimum; do not allow TLS 1.0 or 1.1, they have been deprecated. | ||
| logger.error("This `minimumTLSVersion` (\(minVersion)) is not supported. Falling back to TLS 1.2.") | ||
| urlsessionConfiguration.tlsMinimumSupportedProtocolVersion = .TLSv12 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the minimum TLS 1.2 enforced on the URLSession-based client but not on the CRT-based client? Also, we should provide some sort of notice to the customer that their TLS setting is not supported when they choose a version before TLS 1.2. Logging would probably be the best choice.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding the Logger.error now, as for the enforcement difference between URLSession and CRT, my reasoning was that URLSession uses a security protocol version (from Apple I suspect) that already enforces it for macOS 12.0 so your systems platform should be able to trust but if you're using a macOS before 12.0 then this should stop the user as an added safety measure. CRT on the other hand does not set any minimum because it trust your OS set TLS. |
||
| case .tls12: | ||
| urlsessionConfiguration.tlsMinimumSupportedProtocolVersion = .TLSv12 | ||
| case .tls13: | ||
| urlsessionConfiguration.tlsMinimumSupportedProtocolVersion = .TLSv13 | ||
| } | ||
| } | ||
|
|
||
| self.session = SessionType.init(configuration: urlsessionConfiguration, delegate: delegate, delegateQueue: nil) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| // | ||
| // Copyright Amazon.com Inc. or its affiliates. | ||
| // All Rights Reserved. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
|
|
||
| import Foundation | ||
| import XCTest | ||
| @testable import ClientRuntime | ||
| import SmithyHTTPClientAPI | ||
| import AwsCommonRuntimeKit | ||
|
|
||
| class CRTClientTLSOptionsTests: XCTestCase { | ||
|
|
||
| override func setUp() { | ||
| super.setUp() | ||
| CommonRuntimeKit.initialize() | ||
| } | ||
|
|
||
| func testMinimumTLSVersionInitialization() { | ||
| let tlsOptions = CRTClientTLSOptions(minimumTLSVersion: .tls12) | ||
| XCTAssertEqual(tlsOptions.minimumTLSVersion, .tls12) | ||
| } | ||
|
|
||
| func testMinimumTLSVersionDefaultsToNil() { | ||
| let tlsOptions = CRTClientTLSOptions() | ||
| XCTAssertNil(tlsOptions.minimumTLSVersion) | ||
| } | ||
|
|
||
| func testAllTLSVersionValues() { | ||
| let tls10 = CRTClientTLSOptions(minimumTLSVersion: .tls10) | ||
| let tls11 = CRTClientTLSOptions(minimumTLSVersion: .tls11) | ||
| let tls12 = CRTClientTLSOptions(minimumTLSVersion: .tls12) | ||
| let tls13 = CRTClientTLSOptions(minimumTLSVersion: .tls13) | ||
|
|
||
| XCTAssertEqual(tls10.minimumTLSVersion, .tls10) | ||
| XCTAssertEqual(tls11.minimumTLSVersion, .tls11) | ||
| XCTAssertEqual(tls12.minimumTLSVersion, .tls12) | ||
| XCTAssertEqual(tls13.minimumTLSVersion, .tls13) | ||
| } | ||
|
|
||
| func testResolveContextSucceedsWithMinimumTLSVersion() { | ||
| let tlsOptions = CRTClientTLSOptions(minimumTLSVersion: .tls12) | ||
|
|
||
| // The resolveContext() method should succeed without throwing | ||
| let context = tlsOptions.resolveContext() | ||
|
|
||
| // Verify a context was created | ||
| XCTAssertNotNil(context) | ||
| } | ||
|
|
||
| func testResolveContextSucceedsWithAllTLSVersions() { | ||
| #if os(Linux) | ||
| let versions: [SmithyHTTPClientAPI.TLSVersion] = [.tls12] | ||
| #else | ||
| let versions: [SmithyHTTPClientAPI.TLSVersion] = [.tls10, .tls11, .tls12, .tls13] | ||
| #endif | ||
|
|
||
| for version in versions { | ||
| let tlsOptions = CRTClientTLSOptions(minimumTLSVersion: version) | ||
| let context = tlsOptions.resolveContext() | ||
|
|
||
| XCTAssertNotNil(context, "Failed to create context for TLS version: \(version)") | ||
| } | ||
| } | ||
|
|
||
| func testResolveContextSucceedsWithoutMinimumTLSVersion() { | ||
| let tlsOptions = CRTClientTLSOptions() | ||
|
|
||
| let context = tlsOptions.resolveContext() | ||
|
|
||
| XCTAssertNotNil(context) | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // | ||
| // Copyright Amazon.com Inc. or its affiliates. | ||
| // All Rights Reserved. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
|
|
||
| import Foundation | ||
| import XCTest | ||
| @testable import ClientRuntime | ||
| import SmithyHTTPClientAPI | ||
|
|
||
| class URLSessionTLSOptionsTests: XCTestCase { | ||
|
|
||
| func testMinimumTLSVersionInitialization() { | ||
| let tlsOptions = URLSessionTLSOptions(minimumTLSVersion: .tls12) | ||
| XCTAssertEqual(tlsOptions.minimumTLSVersion, .tls12) | ||
| } | ||
|
|
||
| func testMinimumTLSVersionDefaultsToNil() { | ||
| let tlsOptions = URLSessionTLSOptions() | ||
| XCTAssertNil(tlsOptions.minimumTLSVersion) | ||
| } | ||
|
|
||
| func testAllTLSVersionValues() { | ||
| let tls10 = URLSessionTLSOptions(minimumTLSVersion: .tls10) | ||
| let tls11 = URLSessionTLSOptions(minimumTLSVersion: .tls11) | ||
| let tls12 = URLSessionTLSOptions(minimumTLSVersion: .tls12) | ||
| let tls13 = URLSessionTLSOptions(minimumTLSVersion: .tls13) | ||
|
|
||
| XCTAssertEqual(tls10.minimumTLSVersion, .tls10) | ||
| XCTAssertEqual(tls11.minimumTLSVersion, .tls11) | ||
| XCTAssertEqual(tls12.minimumTLSVersion, .tls12) | ||
| XCTAssertEqual(tls13.minimumTLSVersion, .tls13) | ||
| } | ||
|
|
||
| #if os(iOS) || os(macOS) || os(watchOS) || os(tvOS) || os(visionOS) | ||
|
|
||
| func testMinimumTLSVersionAppliedToURLSessionConfiguration() { | ||
| let tlsOptions = URLSessionTLSOptions(minimumTLSVersion: .tls12) | ||
| let httpConfig = HttpClientConfiguration(tlsConfiguration: tlsOptions) | ||
| let client = URLSessionHTTPClient(httpClientConfiguration: httpConfig) | ||
|
|
||
| // Access the URLSession's configuration to verify TLS version was set | ||
| XCTAssertEqual( | ||
| client.session.configuration.tlsMinimumSupportedProtocolVersion, | ||
| .TLSv12 | ||
| ) | ||
| } | ||
|
|
||
| func testMinimumTLSVersionTLS13AppliedToURLSessionConfiguration() { | ||
| let tlsOptions = URLSessionTLSOptions(minimumTLSVersion: .tls13) | ||
| let httpConfig = HttpClientConfiguration(tlsConfiguration: tlsOptions) | ||
| let client = URLSessionHTTPClient(httpClientConfiguration: httpConfig) | ||
|
|
||
| XCTAssertEqual( | ||
| client.session.configuration.tlsMinimumSupportedProtocolVersion, | ||
| .TLSv13 | ||
| ) | ||
| } | ||
|
|
||
| func testNoMinimumTLSVersionUsesDefault() { | ||
| let tlsOptions = URLSessionTLSOptions() | ||
| let httpConfig = HttpClientConfiguration(tlsConfiguration: tlsOptions) | ||
| let client = URLSessionHTTPClient(httpClientConfiguration: httpConfig) | ||
|
|
||
| // When not set, should use system default (typically .TLSv12) | ||
| XCTAssertNotNil(client.session.configuration.tlsMinimumSupportedProtocolVersion) | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these 3 lines from
.gitignore, we actually do, in some cases, want to commit files in Xcode shared data (Not routinely, but in specific situations.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines removed