Skip to content

add secondary 'tls-rustls-webpki' and 'rustls-webpki' features that don't implicitly enable rustls-native-certs #4017

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

Closed
wants to merge 1 commit into from

Conversation

jlizen
Copy link

@jlizen jlizen commented Feb 14, 2025

Motivation and Context

I'm trying to remove openssl entirely from my package's dependency closure.

Currently aws-smithy-runtime and aws-config both implicitly bring in openssl, even under their respective rustls features, because they have default hyper-rustls features enabled. That in turn enables rustls-native-certs, which then brings in OpenSsl.

Description

I've handled this in a non-breaking manner by adding a new feature to both packages, tls-rustls-webpki, and rustls-webki, respectively. I made sure to explicitly enable all default features for hyper-rustls besides rustls-native-certs (which now is moved to the tls-rustls feature only).

I also added a __rustls feature that both rustls/rustls-webpki and tls-rustls/tls-rustls-webpki resolve to, which all the conditional compilation inside application code now checks for. This is the convention I see in the hyper ecosystem.

Testing

I'm publishing this as a draft for initial feedback, as I need to do more testing. Still sorting out my local build. It looks like I should check in Cargo.lock files as well?

Related - are there any CI/CD updates needed to test the new featureset?

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.
  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

It's not clear if this merits a changelog entry... probably?


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

@jlizen
Copy link
Author

jlizen commented Feb 14, 2025

Here's a sample dependency closure that illustrates what I'm trying to fix:

> cargo tree -e features -i openssl-probe

openssl-probe v0.1.6
└── openssl-probe feature "default"
    └── rustls-native-certs v0.6.3
        └── rustls-native-certs feature "default"
            └── hyper-rustls v0.24.2
                └── reqwest v0.11.27
                    ├── reqwest feature "__rustls"
                    │   └── reqwest feature "rustls-tls-webpki-roots"
                    │       └── reqwest feature "rustls-tls"
                    │           └── oauth2 feature "rustls-tls"
                    │               └── oauth2 feature "default"
                    │                   └── censored v0.1.0 (/workplace/jlizen/censored/test_7234421309/src/censored)
                    │                       ├── censored feature "censored" 
                    │                       │   └── censored feature "default" (command-line)
                    │                       ├── censored feature "default" (command-line)
                    │                       └── censored feature "local-introspection"
                    │                           └── censored feature "default" (command-line)
                    ├── reqwest feature "__tls"
                    │   └── reqwest feature "__rustls" (*)
                    ├── reqwest feature "blocking"
                    │   └── oauth2 v4.4.2
                    │       ├── oauth2 feature "default" (*)
                    │       ├── oauth2 feature "reqwest"
                    │       │   ├── oauth2 feature "default" (*)
                    │       │   └── oauth2 feature "rustls-tls" (*)
                    │       └── oauth2 feature "rustls-tls" (*)
                    ├── reqwest feature "hyper-rustls"
                    │   └── reqwest feature "__rustls" (*)
                    ├── reqwest feature "rustls"
                    │   └── reqwest feature "__rustls" (*)
                    ├── reqwest feature "rustls-tls" (*)
                    ├── reqwest feature "rustls-tls-webpki-roots" (*)
                    ├── reqwest feature "tokio-rustls"
                    │   └── reqwest feature "__rustls" (*)
                    └── reqwest feature "webpki-roots"
                        └── reqwest feature "rustls-tls-webpki-roots" (*)
                ├── hyper-rustls feature "acceptor"
                │   └── hyper-rustls feature "default"
                │       └── aws-smithy-runtime v1.7.8
                │           ├── aws-smithy-runtime feature "client"
                │           │   ├── aws-config v1.5.16
                │           │   │   ├── aws-config feature "behavior-version-latest"
                │           │   │   │   └── censored v0.1.0 (/workplace/jlizen/censored/test_7234421309/src/censored) (*)
                │           │   │   ├── aws-config feature "client-hyper"
                │           │   │   │   ├── aws-config feature "default"
                │           │   │   │   │   └── censored v0.1.0 (/workplace/jlizen/censored/test_7234421309/src/censored) (*)
                │           │   │   │   └── aws-config feature "rustls"
                │           │   │   │       └── aws-config feature "default" (*)
                │           │   │   ├── aws-config feature "credentials-process"
                │           │   │   │   └── aws-config feature "default" (*)
                │           │   │   ├── aws-config feature "default" (*)
                │           │   │   ├── aws-config feature "rt-tokio"
                │           │   │   │   └── aws-config feature "default" (*)
                │           │   │   ├── aws-config feature "rustls" (*)
                │           │   │   └── aws-config feature "sso"
                │           │   │       └── aws-config feature "default" (*)
                │           │   ├── aws-runtime v1.5.5
                │           │   │   └── aws-runtime feature "default"
                │           │   │       ├── aws-config v1.5.16 (*)
                │           │   │       ├── aws-sdk-sso v1.58.0
                │           │   │       │   └── aws-config v1.5.16 (*)
                │           │   │       ├── aws-sdk-ssooidc v1.59.0
                │           │   │       │   └── aws-config v1.5.16 (*)
                │           │   │       └── aws-sdk-sts v1.59.0
                │           │   │           └── aws-config v1.5.16 (*)
                │           │   ├── aws-sdk-sso v1.58.0 (*)
                │           │   ├── aws-sdk-ssooidc v1.59.0 (*)
                │           │   └── aws-sdk-sts v1.59.0 (*)
                │           ├── aws-smithy-runtime feature "connector-hyper-0-14-x"
                │           │   ├── aws-config feature "client-hyper" (*)
                │           │   └── aws-smithy-runtime feature "tls-rustls"
                │           │       └── aws-config feature "rustls" (*)
                │           ├── aws-smithy-runtime feature "default"
                │           │   ├── aws-config v1.5.16 (*)
                │           │   ├── aws-runtime v1.5.5 (*)
                │           │   ├── aws-sdk-sso v1.58.0 (*)
                │           │   ├── aws-sdk-ssooidc v1.59.0 (*)
                │           │   └── aws-sdk-sts v1.59.0 (*)
                │           ├── aws-smithy-runtime feature "rt-tokio"
                │           │   └── aws-config feature "rt-tokio" (*)
                │           └── aws-smithy-runtime feature "tls-rustls" (*)
                ├── hyper-rustls feature "default" (*)
                ├── hyper-rustls feature "http1"
                │   └── hyper-rustls feature "default" (*)
                ├── hyper-rustls feature "http2"
                │   └── aws-smithy-runtime v1.7.8 (*)
                ├── hyper-rustls feature "log"
                │   └── hyper-rustls feature "logging"
                │       └── hyper-rustls feature "default" (*)
                ├── hyper-rustls feature "logging" (*)
                ├── hyper-rustls feature "native-tokio"
                │   └── hyper-rustls feature "default" (*)
                ├── hyper-rustls feature "rustls-native-certs"
                │   └── aws-smithy-runtime v1.7.8 (*)
                │   └── hyper-rustls feature "native-tokio" (*)
                ├── hyper-rustls feature "tls12"
                │   └── hyper-rustls feature "default" (*)
                └── hyper-rustls feature "tokio-runtime"
                    ├── hyper-rustls feature "acceptor" (*)
                    └── hyper-rustls feature "native-tokio" (*)

@rcoh
Copy link
Collaborator

rcoh commented Feb 14, 2025

Cc @aajtodd

@jlizen jlizen changed the title add secondary 'tls-rustls-webpki' and 'rustls-webpki' features that don't on't implicitly enable rustls-native-certs add secondary 'tls-rustls-webpki' and 'rustls-webpki' features that don't implicitly enable rustls-native-certs Feb 14, 2025
@jlizen
Copy link
Author

jlizen commented Feb 14, 2025

Hmm, just now seeing #3985

I had to introduce a new crate aws-smithy-default-tls to make this work. This is because we can't modify dependencies or add new features based on a feature. Doing it without a new crate seems only possible with rust-lang/cargo#1197.

Sounds like there is some context I'm missing (unless maybe this was a typo, and you are referring to modifying features based on *a config flag / platform target?

I'll drop you a line tomorrow, Aaron. Sounds like you have some wheels in motion that might satisfy this use case along with some grander ones. Glad to close this out if so. A feature flag just for this is a bit awkward, though still worth it if it's the best path out of requiring rustls to use openssl for pki (in my opinion).

@aajtodd
Copy link
Contributor

aajtodd commented Feb 14, 2025

Hmm, just now seeing #3985

I had to introduce a new crate aws-smithy-default-tls to make this work. This is because we can't modify dependencies or add new features based on a feature. Doing it without a new crate seems only possible with rust-lang/cargo#1197.

Sounds like there is some context I'm missing (unless maybe this was a typo, and you are referring to modifying features based on *a config flag / platform target?

I'll drop you a line tomorrow, Aaron. Sounds like you have some wheels in motion that might satisfy this use case along with some grander ones. Glad to close this out if so. A feature flag just for this is a bit awkward, though still worth it if it's the best path out of requiring rustls to use openssl for pki (in my opinion).

The context is we were/are trying to default the TLS provider to be s2n-tls for unix platforms and rustls+aws-lc for windows since s2n doesn't work on Windows. You can't do this today with feature flags alone (i.e. you can't conditionally enable different features of a dependency based on a feature flag). Instead I introduced a new crate that acts as a proxy, or you could think of it as representing the feature itself.


I haven't looked at the PR in detail but I would really prefer to work on this in the context of the hyper1 work. One of the biggest changes is moving all the HTTP/TLS concerns we can into a new aws-smithy-http-client crate with the idea being that out of the box you get a default HTTPS stack that should work for most users. If you want to do anything else you need to pull in the aws-smithy-http-client crate yourself and enable whatever flags you want and explicitly pass in an HTTP client to the SDK (or bring your own of course by implementing the appropriate traits). This should hopefully alleviate aws-smithy-runtime and by extension SDK crates from having to introduce feature flags all over the place and be load bearing for all HTTPS concerns. It almost has to be this way because features are supposed to be additive and most of these TLS concerns are not so instead we offer a default and if you want something else BYOH (Bring Your Own HTTP).

@jlizen
Copy link
Author

jlizen commented Feb 14, 2025

Thanks for the explanation! That makes a lot of sense to wait for hyper 1 since it solves this class of problem. I'm fine to go ahead and close this out in the meantime as it's just a small paper cut.

Thanks for your eyes!

@jlizen jlizen closed this Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants