Don't compile aws-lc-rs when the ring provider is requested - #966
Don't compile aws-lc-rs when the ring provider is requested#966ShanireZ wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates pingora-rustls’s dependency feature selection so that enabling the ring crypto provider does not also compile aws-lc-rs via transitive default features, reducing build time/toolchain requirements for pingora-core consumers using the rustls backend.
Changes:
- Disable
rustlsdefault features and explicitly re-enable the needed defaults (ring,logging,std,tls12) without pulling inaws_lc_rs. - Disable
tokio-rustlsdefault features and explicitly enablering,logging, andtls12to avoid reintroducingaws_lc_rsvia that dependency path.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce86e204f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rustls = { version = "0.23.12", default-features = false, features = [ | ||
| "ring", |
There was a problem hiding this comment.
Disable rustls defaults in the core dev dependency
When pingora-core test or benchmark targets are built with the rustls feature, its direct rustls = "0.23" dev-dependency in pingora-core/Cargo.toml:97 still enables the default aws_lc_rs feature, which is unified with this dependency and causes aws-lc-rs/aws-lc-sys to be compiled despite selecting ring here. Apply the same default-features = false and explicit ring-related feature set to that dev-dependency so test builds also receive the intended toolchain and compile-time reduction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — adopted, and it turned out to be a little broader than described.
Confirmed by measurement on main (0046038), unique crates in cargo tree -p pingora-core:
--features rustls, -e normal |
--features rustls, -e normal,dev |
no features, -e normal,dev |
|
|---|---|---|---|
main |
178, aws-lc present | 260, present | 228, present |
| this PR before your comment | 176, gone | 260, still present | 228, still present |
| this PR now | 176, gone | 258, gone | 226, gone |
Two things worth noting:
- The dev-dependency is unconditional, so it is not limited to
rustls-feature builds — the
third column showsaws-lc-rs/aws-lc-sysbeing compiled by the default (openssl) test
build as well. - My original verification could not have seen this: I measured with
-e normal, which by
definition excludes dev-dependencies. The gate was narrower than the claim it was supporting.
The dev-dependency now gets default-features = false with ["logging", "std", "tls12"] and
no provider feature at all. It is only used for types in connectors/http tests
(ServerCertVerifier, CertificateDer, DigitallySignedStruct), and the provider it needs at
runtime already arrives through pingora-rustls; naming one there would repeat exactly the
mistake this PR fixes, and would also conflict with the provider-selection work in #630/#887.
I measured the ["ring", ...] variant too — same graph, same result — so nothing is lost by
leaving the provider unspecified.
cargo check -p pingora-core --all-targets passes with and without the rustls feature, and
cargo fmt / cargo check --workspace / cargo clippy --all-targets --all -- --allow=unknown-lints --deny=warnings / cargo test -p pingora-core --lib --features rustls
(566 passed, same 7 environment-dependent failures as unpatched main, both-way set difference
empty) / cargo +1.85.0 check --workspace --exclude pingora-foundations all behave the same
before and after.
Unrelated to this PR: the cargo audit job is currently failing on h2 0.3.27
(RUSTSEC-2026-0258, published 2026-08-17). It arrives through hyper 0.14 under
pingora-foundations → foundations → aws-smithy-http-client, and the advisory has no patched
release on the 0.3 line, so it is not something this PR can address — every PR opened against
main today fails that job.
ce86e20 to
73600fd
Compare
pingora-rustls asks rustls and tokio-rustls for the `ring` provider, but
neither dependency disables default features. Both crates enable
`aws_lc_rs` by default, so *both* crypto providers end up in the build:
rustls 0.23.43 default = ["aws_lc_rs", "logging",
"prefer-post-quantum", "std", "tls12"]
tokio-rustls 0.26.4 default = ["logging", "tls12", "aws_lc_rs"]
pingora-core's own `rustls = "0.23"` dev-dependency is a third such
entry, and because it is unconditional it reaches even the default
(openssl) test build.
The aws-lc-rs provider is never used. pingora-rustls installs ring
explicitly (`CryptoProvider::install_default(rustls::crypto::ring::
default_provider())` in `install_default_crypto_provider()`), and the
only other crypto call in the crate is `ring::digest`.
What it costs: aws-lc-sys vendors ~69 MB of C source (ring is ~8.5 MB)
and builds it through cmake, so every consumer of pingora-core's
`rustls` feature pays for a C toolchain and the resulting compile time.
For anyone linking statically against musl this is a real obstacle, and
it is a needless increase in the amount of compiled-in crypto code.
Fixing it means naming the defaults we do want, since
`default-features = false` also drops `logging`, `std` and `tls12`.
tokio-rustls has no `std` feature; its `ring`, `logging` and `tls12`
features forward to the rustls features of the same name. The only
default that is deliberately not restored is `prefer-post-quantum`,
which is defined as `["aws_lc_rs"]` and whose every `#[cfg]` site lives
under `src/crypto/aws_lc_rs/` — it does nothing under the ring provider.
The dev-dependency deliberately names no provider at all. It is only
used for types (`ServerCertVerifier`, `CertificateDer`,
`DigitallySignedStruct` in connectors/http tests), and the provider it
needs at runtime already arrives through pingora-rustls; selecting one
there would repeat the mistake this commit fixes.
Measured on this commit's parent with
`cargo tree -p pingora-core -e <edges>`, counting unique crates:
--features rustls -e normal 178 -> 176 aws-lc gone
--features rustls -e normal,dev 260 -> 258 aws-lc gone
(no features) -e normal,dev 228 -> 226 aws-lc gone
`cargo check -p pingora-core --all-targets`, with and without the
`rustls` feature, passes before and after.
Signed-off-by: Shanire <shanire86@gmail.com>
Refs #965
pingora-rustlsasksrustlsandtokio-rustlsfor theringprovider, but neitherdependency disables default features, and both crates enable
aws_lc_rsby default:So
features = ["ring"]adds ring alongside aws-lc-rs rather than instead of it, and bothproviders are compiled into every build that enables
pingora-core/rustls. The aws-lc-rs oneis never used:
install_default_crypto_provider()installs ring explicitly, and the crate'sonly other crypto call is
ring::digest.pingora-core's ownrustls = "0.23"dev-dependency is a third such entry. Since it isunconditional, it reaches even the default (openssl) test build:
cargo tree -p pingora-core -e normal,devwith no features showsaws-lc-rsandaws-lc-systoday.That entry is only used for types in
connectors/httptests (ServerCertVerifier,CertificateDer,DigitallySignedStruct), so this patch gives itdefault-features = falseand names no provider at all — selecting one there wouldrepeat the mistake the rest of the patch fixes.
aws-lc-sysvendors ~69 MB of C source and builds it through cmake (ring is ~8.5 MB), so thiscosts every
rustls-feature consumer a C toolchain and the compile time, and it is aparticular obstacle for static musl builds.
This patch names the defaults that are actually wanted, since
default-features = falsealsodrops
logging,stdandtls12. (tokio-rustlshas nostdfeature; itsring,loggingandtls12features forward to the rustls features of the same name.) The onedefault not restored is
prefer-post-quantum, which is defined as["aws_lc_rs"]and whoseevery
#[cfg]site is undersrc/crypto/aws_lc_rs/— it does nothing under ring, andrestoring it would pull aws-lc-rs back in.
Relation to #630 and #887
Both of those make the provider selectable, which is a larger design question. This PR is
deliberately orthogonal and manifest-only: it stops aws-lc-rs being compiled on today's
main, whichever way that question is eventually settled.The reason it is not redundant with either is that
aws_lc_rsarrives through twoindependent doors, and both PRs change only the
rustlsline. Measured onmain(
0046038), unique crates incargo tree -p pingora-core -e normal:aws-lc-rs/aws-lc-sysmainas-is,--features rustlsrustlsline applied alone,--features rustls--features rustls--features rustls-no-providerringis gone)The fourth row is worth a look from #887's side:
rustls-no-providerdoes dropring, but aconsumer who picks it in order to bring their own
CryptoProviderstill compiles aws-lc-rsand its 69 MB of C.
If #887 lands first, what remains of this patch is the
tokio-rustlsentry, and itsringfeature should then follow the same optionality that #887 gives the
rustlsentry. Happy torebase it into that shape, or to close this in favour of a combined change — whichever the
maintainers prefer.
Verification
All run against
main(0046038) in a container, unpatched vs patched, using the commandsfrom
.github/workflows/build.yml:cargo tree -p pingora-core --features rustls -e normalaws-lc-rsandaws-lc-sys, and nothing is addedcargo tree -p pingora-core --features rustls -e normal,devcargo tree -p pingora-core -e normal,dev(no features)cargo fmt --all -- --checkcargo check --workspacecargo check -p pingora-core --all-targets, with and withoutrustlscargo build -p pingora-core --features rustlscargo clippy --all-targets --all -- --allow=unknown-lints --deny=warningscargo test -p pingora-core --lib --no-fail-fast --features rustls192.0.2.1, so the connect-timeout family cannot time out)cargo +1.85.0 check --workspace --exclude pingora-foundationscargo audit/cargo macheteNo source changes; the diff is one manifest.
Note on TLS-backend feature combinations
This only touches the
rustlspath. Theopenssl,boringsslands2nbackends selecttheir own crates and are unaffected —
pingora-rustlsis only compiled when therustlsfeature is on.