feat: add support to CRT HTTP Client for providing custom trust and key store #672

dayaffe · 2024-02-26T21:40:36Z

Issue #

Description of changes

Example Usage with STSClient on Apple devices:

let urlSessionTLSOptions = URLSessionTLSOptions(
    certificateFile: "localhost", // .cer
    keyStoreName: "certificate", // .p12
    keyStorePassword: "test"
)

let client = try await STSClient(
    config: .init(
        httpClientConfiguration: .init(
            tlsOptions: .init(urlSessionTLSOptions: urlSessionTLSOptions)
        )
    )
)

Example Usage with STSClient on Linux:

let crtTLSOptions = CRTClientTLSOptions(
    certificatePath: "/path/",
    certificateFilename: "trustStoreFile",
    keyStoreFilepath: "/path/file",
    keyStorePassword: "password"
)

let client = try await STSClient(
    config: .init(
        httpClientConfiguration: .init(
            tlsOptions: .init(crtTLSOptions: crtTLSOptions)
        )
    )
)

Directions for testing URLSession changes:

Step 1: Downgrade Openssl on mac from 3.x to 1.1. Mac's Keychain Access and XCode are not compatible with the latest openssl encryption algorithms

Step 2. Generate a key (.pem) and a certificate (.cer) and then include both in a keystore (.p12)

Step 3. Include both the certificate file (.cer) and keystore (.p12) in the project's bundle

Step 4. Create a local test server. You can use mine as an exmaple and run it using node server.js:

const express = require('express');
const https = require('https');
const fs = require('fs');

const app = express();

app.get('/', (req, res) => {
  res.send('Hello, Secure World!');
});

https.createServer({
  key: fs.readFileSync('key.pem'),
  cert: fs.readFileSync('certificate.pem'),
  passphrase: "test"
}, app)
.listen(3000, () => {
  console.log('Listening on https://localhost:3000');
});

Step 5. Run XCode proj that hits the server. Here is my example code:

let urlSessionTLSOptions = URLSessionTLSOptions(
    certificateFile: "localhost", // .cer
    keyStoreName: "certificate", // .p12
    keyStorePassword: "test"
)

// Create an instance of our URLSessionHTTPClient
let urlSessionClient = URLSessionHTTPClient(httpClientConfiguration: .init(urlSessionTLSOptions: urlSessionTLSOptions))

// Create an SdkHttpRequest
let sdkHttpRequest = SdkHttpRequest(
    method: .get,
    endpoint: Endpoint(host: "localhost", path: "/", port: 3000, protocolType: .https),
    body: .noStream
)
// Send the request
do {
    let response = try await urlSessionClient.send(request: sdkHttpRequest)
    print("Response received: \(response)")
} catch {
    print("Failed to make request: \(error)")
}

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sichanyoo

Minor suggestions

Sources/ClientRuntime/Networking/Http/HttpClientConfiguration.swift

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

waahm7 · 2024-03-28T17:10:06Z

Sources/ClientRuntime/Networking/Http/HttpClientConfiguration.swift

+    /// Custom TLS configuration for HTTPS connections.
+    ///
+    /// Enables specifying client certificates and trust stores for secure communication.
+    /// Defaults to system's TLS settings if `nil`.
+    public var tlsContext: TLSContext?
+
+    /// Custom TLS configuration for HTTPS connections with URLSession client.
+    ///
+    /// Enables specifying client certificates and trust stores for secure communication.
+    /// Defaults to system's TLS settings if `nil`.
+    public var urlSessionTLSOptions: URLSessionTLSOptions?


If this is the public API, Is it possible to have one TLSContext class and create either a URLSessionTLSOptions or a TLSContext internally from it?

It might be better to just expose a wrapper to TLSContextOptions and create a TLSContext from that. No one should be passing server mode to this TLSContext.init function.

waahm7 · 2024-03-28T17:18:07Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionTLSOptions.swift

+    /// Information is provided to use custom trust store
+    public var useSelfSignedCertificate: Bool
+
+    /// Information is provided to use custom key store
+    public var useProvidedKeystore: Bool


These are derived variables, but someone can modify them after the init function. Is that intended?

Will change so they cant modify

Should any of these properties be modifiable? Generally configuration objects are immutable once created.

xiazhvera · 2024-04-02T21:58:25Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

+
+        let options = [kSecImportExportPassphrase as String: password] as CFDictionary
+        var items: CFArray?
+        let status = SecPKCS12Import(p12Data as CFData, options, &items)


SecPKCS12Import: The latest aws-crt-swift disabled the API for iOS, tvOS, and watchOS, as we dont really have a way to test the API on those platforms. Would you have a test plan to the feature with pkcs12 key on the platforms? Maybe I could add the tests to CRT as well and enable the API there.

awslabs/aws-crt-swift#246 <= PR that disabled SecPKCS12Import

I agree there's no easy way to test on those platforms natively (would work via simulator for dev testing), I manually generated the p12 file using a combination of keychain access and openssl 1.1

jbelkins

See individual comments. Also it seems that we have some HTTP client config that is specific to only one client. Short of clearly documenting what affects what, is there a way that we can unify settings so that one setting affects both clients?

jbelkins · 2024-03-29T19:42:44Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

+                if try serverTrust.evaluateAllowing(rootCertificates: [customRoot]) {
+                    completionHandler(.useCredential, URLCredential(trust: serverTrust))
+                } else {
+                    logger.debug("Trust evaluation failed, cancelling authentication challenge.")


Should this be error log level?

jbelkins · 2024-03-29T20:09:42Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionTLSOptions.swift

+    /// Information is provided to use custom trust store
+    public var useSelfSignedCertificate: Bool
+
+    /// Information is provided to use custom key store
+    public var useProvidedKeystore: Bool


Should any of these properties be modifiable? Generally configuration objects are immutable once created.

jbelkins · 2024-03-29T20:10:10Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

+        var error: CFError?
+        let evaluationSucceeded = SecTrustEvaluateWithError(self, &error)
+        guard evaluationSucceeded else {
+            print(error.debugDescription)


Eliminate the print statement, or change it to a log statement

Also, should error be unwrapped here? Seems odd to log & throw an error you're not sure is there

Eliminating print statement, the error has to be there as part of description of SecTrustEvaluateWithError. If result is false, error will be populated with reason, if true error will be null

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

jbelkins · 2024-03-29T20:30:13Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

+        // Set the custom root certificates as trusted anchors.
+        let status = SecTrustSetAnchorCertificates(self, rootCertificates as CFArray)
+        guard status == errSecSuccess else {
+            throw TrustEvaluationError.evaluationFailed(error: nil)


The actual error here is that setting anchor certificates failed? Should the error we throw say that more explicitly?

…pper for TLS Options

jbelkins · 2024-04-16T18:35:36Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

@@ -330,4 +425,71 @@ public enum URLSessionHTTPClientError: Error {
    case unresumedConnection
 }

+extension Bundle {


Let's move these extensions to a separate source file

The URLSessionHTTPClient is getting big

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

jbelkins · 2024-04-16T18:39:21Z

Sources/ClientRuntime/Networking/Http/TLSOptions.swift

+        self.crtTLSContext = nil
+
+        // Set CRT client tls options with .client mode
+        if let _crtTLSOptions = crtTLSOptions {


Why unwrap to a local var with an underscore?

if let crtTLSOptions {

will unwrap the optional then "shadow" it inside the if.

jbelkins · 2024-04-16T18:41:15Z

Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift

@@ -39,13 +39,15 @@ public class CRTClientEngine: HTTPClient {
        private var http2ConnectionPools: [ConnectionPoolID: HTTP2StreamManager] = [:]
        private let sharedDefaultIO = SDKDefaultIO.shared
        private let connectTimeoutMs: UInt32?
+        private let tlsContext: TLSContext?


Is TLSContext a CRT type? If so, let's not expose it directly to customers. CRT is an "implementation detail" of the AWS SDK for Swift that should remain concealed from customers.

Will change this to CRTClientTLSOptions type that I created and provide a resolveContext() method on it

Can also rename that if we want to get rid of CRT from it

jbelkins · 2024-04-16T18:44:05Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

+        // - The `as? SecIdentity` casting causes a compiler error on Apple platforms as the cast is guaranteed.
+        // - Directly returning `identity` works on Linux but not on macOS due to strict type expectations.
+        // SwiftLint is temporarily disabled for the next line to allow a force cast, acknowledging the platform-specific behavior.
+        // swiftlint:disable:next force_cast


Eventually Swift Foundation will eliminate these differences, but for now we're stuck with swift-corelibs-foundation on Linux which is not always faithful to the original.

jbelkins · 2024-04-16T18:45:08Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionTLSOptions.swift

+        self.keyStorePassword = keyStorePassword
+
+        self.useSelfSignedCertificate = certificateFile != nil
+        self.useProvidedKeystore = keyStoreName != nil && keyStorePassword != nil


nit: can this be a computed property instead of stored?

same with the property above

jbelkins

Fix the swiftlint issue before merging

…tions

Sources/ClientRuntime/Networking/Http/CRT/CRTClientTLSOptions.swift

dayaffe added 11 commits February 26, 2024 16:40

add support to CRT HTTP Client for configuring trust and key store

e7a880d

add custom trust and keystore configuration to URLSession client

7038590

Merge branch 'main' into day/tls-key-store-config

1ea0487

remove #if to allow linux build

9e7ccc0

swiftlint

7b54f82

update how TLS options work on URL Session

4a76fd9

Merge branch 'main' into day/tls-key-store-config

b49c2ed

add comment and comma

99663d8

fix leftover merge conflicts

1334630

change argument order

1901cb1

swiftlint

ac8ec07

dayaffe requested a review from sichanyoo March 26, 2024 16:32

dayaffe added 5 commits March 26, 2024 12:57

swiftlint line lengths

28bea72

remove forced unwrap

2de0a6b

try moving cast inside guard

e0e1697

remove cast

534fd81

add explanation for force cast

b0712ac

sichanyoo requested changes Mar 28, 2024

View reviewed changes

waahm7 reviewed Mar 28, 2024

View reviewed changes

dayaffe requested a review from jbelkins March 29, 2024 16:09

xiazhvera reviewed Apr 2, 2024

View reviewed changes

jbelkins requested changes Apr 3, 2024

View reviewed changes

dayaffe added 3 commits April 9, 2024 15:34

address PR comments, make wrapper for CRT TLS Options and unified wra…

dc1be04

…pper for TLS Options

Merge branch 'main' into day/tls-key-store-config

c132d2e

add TLSOptions class

80cf15f

dayaffe requested review from sichanyoo, jbelkins and waahm7 April 9, 2024 20:08

some nit errors

804028a

sichanyoo approved these changes Apr 10, 2024

View reviewed changes

jbelkins requested changes Apr 16, 2024

View reviewed changes

dayaffe added 3 commits April 17, 2024 15:11

Merge branch 'main' into day/tls-key-store-config

f619ade

apply PR comments

68f2ad4

only render util class on apple devices

fbc331f

dayaffe requested a review from jbelkins April 17, 2024 21:26

provide correct argument

e4670ed

jbelkins approved these changes Apr 22, 2024

View reviewed changes

dayaffe added 8 commits April 23, 2024 09:14

Merge branch 'main' into day/tls-key-store-config

bcc6783

remove redundant initialization

34f9c7d

makeMtlsPkcs12FromPath changed to makeMTLS

bf3bad9

refactor CRT TLS options as CRT changed their underlying exposed func…

a8c8c33

…tions

Merge branch 'main' into day/tls-key-store-config

4a6cd71

set variable instead of return

b80bc42

initialize privateKeyFilepath

2fe88cb

Merge branch 'main' into day/tls-key-store-config

9c1d7d7

waahm7 approved these changes Apr 24, 2024

View reviewed changes

xiazhvera reviewed Apr 24, 2024

View reviewed changes

Sources/ClientRuntime/Networking/Http/CRT/CRTClientTLSOptions.swift Show resolved Hide resolved

dayaffe added 3 commits April 25, 2024 08:31

enable mac to use either makeMTLS method

4d76bc4

refactor slightly for readability

970d2ca

Merge branch 'main' into day/tls-key-store-config

9c1202b

dayaffe merged commit 84f036d into main Apr 25, 2024
12 checks passed

dayaffe deleted the day/tls-key-store-config branch April 25, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support to CRT HTTP Client for providing custom trust and key store #672

feat: add support to CRT HTTP Client for providing custom trust and key store #672

dayaffe commented Feb 26, 2024 •

edited

Loading

sichanyoo left a comment

waahm7 Mar 28, 2024

waahm7 Mar 28, 2024 •

edited

Loading

waahm7 Mar 28, 2024

dayaffe Mar 29, 2024

jbelkins Mar 29, 2024

xiazhvera Apr 2, 2024

dayaffe Apr 3, 2024

jbelkins left a comment

jbelkins Mar 29, 2024

jbelkins Mar 29, 2024

jbelkins Mar 29, 2024

jbelkins Mar 29, 2024

dayaffe Apr 9, 2024

jbelkins Mar 29, 2024

jbelkins Apr 16, 2024

jbelkins Apr 16, 2024

jbelkins Apr 16, 2024

jbelkins Apr 16, 2024

dayaffe Apr 17, 2024

dayaffe Apr 17, 2024

jbelkins Apr 16, 2024

jbelkins Apr 16, 2024

jbelkins Apr 16, 2024

jbelkins left a comment

feat: add support to CRT HTTP Client for providing custom trust and key store #672

feat: add support to CRT HTTP Client for providing custom trust and key store #672

Conversation

dayaffe commented Feb 26, 2024 • edited Loading

Issue #

Description of changes

Directions for testing URLSession changes:

Scope

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbelkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbelkins left a comment

Choose a reason for hiding this comment

dayaffe commented Feb 26, 2024 •

edited

Loading

waahm7 Mar 28, 2024 •

edited

Loading