Custom TLS config #4032

aajtodd · 2025-02-21T16:36:28Z

Motivation and Context

Description

Introduces a new TlsContext that can be used to customize TLS configuration. Currently that only includes supplying custom root certificates. This PR also refactors the way caching works. Previously we cached complete connectors with default resolver when the connector settings were known to be defaulted. The reason given in code and past commits for this caching resolves around the fact that hyper-rustls with_native_roots() configuration option re-loads platform root certificates on each invocation. Rather than cache the connector, instead I've switched the caching to only cache loading the certificates. This reduces a bit of complexity and required context to understand when we can cache.

Questions/Discussion

There are a number of places we call unwrap/expect in this code. It was this way previously but I've introduced a few more. This stems from building a connector not returning a result. I don't know that I love this but I also suppose I can understand why we chose this. Now is the time to consider whether we'd approach it differently or not though or we are simply saying a valid HTTPS configuration.
Loading native certs is all blocking file calls though and not async friendly. I haven't changed anything here but it caught my eye. It would be nice if this were more async friendly. The mitigation we chose to this seemed to be caching the connector and paying that cost once up front at construction time if possible.
Both rustls and s2n take in PEM encoded certs. Rustls (via rustls-pki-types) provides a way to actually parse and load the certs as an independent configuration object (trust store). The issue is it converts them to DER format which makes using this for s2n not possible. It would be nice if we could load and validate certs before passing them off to rustls/s2n in a uniform way.
Should TlsContext and/or TrustStore provider builders? Currently they just self mutate which is a pattern we use in some places (e.g. stalled stream protection config comes to mind).

Testing

New unit tests + existing TLS tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2025-02-21T19:37:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-25T16:37:01Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-25T17:05:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-25T18:30:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-25T19:55:46Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-25T21:16:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aajtodd · 2025-02-26T15:24:28Z

rust-runtime/aws-smithy-http-client/src/client/tls.rs

+    }
+
+    /// Build a new [TlsContext]
+    pub fn build(self) -> Result<TlsContext, BoxError> {


Do we want a custom error type here?

Yeah, we probably should.

github-actions · 2025-02-26T15:42:12Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-26T16:24:05Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2025-02-26T17:17:17Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aajtodd · 2025-02-26T17:43:12Z

rust-runtime/aws-smithy-http-client/src/client/tls.rs

+    /// This may be called more than once to add multiple certificates.
+    /// NOTE: PEM certificate contents are not validated until passed to the configured
+    /// TLS provider.
+    pub fn with_pem_certificate(mut self, pem_bytes: &[u8]) -> Self {


Maybe impl AsRef<u8> or Into<Vec<u8>>?

Maybe we should have this and add_pem_certificate return a Result to allow for validation errors in the future to be pulled forward?

Maybe we should have this and add_pem_certificate return a Result to allow for validation errors in the future to be pulled forward?

I think that is reasonable, and likely much easier to debug for a user than succeeding here and failing in rustls

Maybe impl AsRef or Into<Vec>?

I probably prefer the latter since that's being honest about the function needing an owned value.

Yeah it's only that way really because we have to store it until we convert it to either rustls or s2n configuration objects. I think if we were able to move to a common format and validate up front it maybe wouldn't be required to own it. Not 100% sure it will/would work out that way though.

I think that is reasonable, and likely much easier to debug for a user than succeeding here and failing in rustls

Theres no way to move this forward (this being the possible validation errors). I'm going to leave it as is and if we ever have that capability we can at least validate in TlsContextBuilder::build() that the trust store configured is valid.

landonxjames · 2025-02-26T19:32:11Z

rust-runtime/aws-smithy-http-client/src/client/tls.rs

+    /// This may be called more than once to add multiple certificates.
+    /// NOTE: PEM certificate contents are not validated until passed to the configured
+    /// TLS provider.
+    pub fn with_pem_certificate(mut self, pem_bytes: &[u8]) -> Self {


Maybe we should have this and add_pem_certificate return a Result to allow for validation errors in the future to be pulled forward?

I think that is reasonable, and likely much easier to debug for a user than succeeding here and failing in rustls

rust-runtime/aws-smithy-http-client/src/client/tls.rs

ysaito1001 · 2025-02-26T20:07:50Z

rust-runtime/aws-smithy-http-client/src/client/tls.rs

+    }
+
+    /// Build a new [TlsContext]
+    pub fn build(self) -> Result<TlsContext, BoxError> {


Yeah, we probably should.

ysaito1001 · 2025-02-26T20:12:22Z

rust-runtime/aws-smithy-http-client/src/client/tls.rs

+    /// This may be called more than once to add multiple certificates.
+    /// NOTE: PEM certificate contents are not validated until passed to the configured
+    /// TLS provider.
+    pub fn with_pem_certificate(mut self, pem_bytes: &[u8]) -> Self {


Maybe impl AsRef or Into<Vec>?

I probably prefer the latter since that's being honest about the function needing an owned value.

github-actions · 2025-02-27T15:12:21Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aajtodd added 5 commits February 19, 2025 15:07

initial test suite

c25f94e

custom TLS context

7b406fb

cleanup

2120dc7

fix cycle

594bc27

update TLS stub for CI to use latest hyper1 client

749e2c2

fix tls tests

3e9c21f

maybe now

81f656e

Merge branch 'hyper1' into hyper1-tls-context

b038b72

more ci fixes

3dc0f17

aajtodd marked this pull request as ready for review February 25, 2025 20:55

aajtodd requested review from a team as code owners February 25, 2025 20:55

this CI is a real pain

49df0da

fix minimal versions check

2d278a6

aajtodd commented Feb 26, 2025

View reviewed changes

update lock files

8c8e711

aajtodd added 2 commits February 26, 2025 11:35

fix udeps

c5de81f

fix test

ed066cc

aajtodd commented Feb 26, 2025

View reviewed changes

landonxjames approved these changes Feb 26, 2025

View reviewed changes

ysaito1001 approved these changes Feb 26, 2025

View reviewed changes

aajtodd added 3 commits February 27, 2025 09:10

remove once_cell in favor of std

fe94915

add custom error type

528ddd1

api updates

9df531a

aajtodd merged commit 33b8247 into hyper1 Feb 27, 2025
42 of 45 checks passed

aajtodd deleted the hyper1-tls-context branch February 27, 2025 15:29

aajtodd mentioned this pull request Mar 3, 2025

change default HTTPS client to hyper1 #4040

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom TLS config #4032

Custom TLS config #4032

aajtodd commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

aajtodd Feb 26, 2025

ysaito1001 Feb 26, 2025

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

aajtodd Feb 26, 2025

aajtodd Feb 26, 2025

landonxjames Feb 26, 2025 •

edited

Loading

ysaito1001 Feb 26, 2025

aajtodd Feb 26, 2025

aajtodd Feb 27, 2025

landonxjames Feb 26, 2025 •

edited

Loading

ysaito1001 Feb 26, 2025

ysaito1001 Feb 26, 2025

github-actions bot commented Feb 27, 2025

Custom TLS config #4032

Custom TLS config #4032

Conversation

aajtodd commented Feb 21, 2025 • edited Loading

Motivation and Context

Description

Questions/Discussion

Testing

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonxjames Feb 26, 2025 • 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

landonxjames Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2025

aajtodd commented Feb 21, 2025 •

edited

Loading

landonxjames Feb 26, 2025 •

edited

Loading

landonxjames Feb 26, 2025 •

edited

Loading