fix(reqwest): do not pull in aws-lc-rs by default

[thomaseizinger]

Description

Activating the rustls feature of reqwest pulls in aws-lc-rs by default as the crypto provider. To allow people to make their own decision about which crypto provider to use with rustls, we should depend on the rustls-no-provider feature flag instead.

Issues

Resolves: #1102

Reminders

• Add GH Issue ID & Linear ID (if applicable)
• Add an entry to CHANGELOG.md, following the format of existing entries
• The PR title should use Conventional Commits style (feat:, fix:, ref:, meta:, etc.)
• Useful links for external contributors: Sentry SDK development docs, Discord community

[sdk-maintainer-bot]

This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer.

To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR.

Please review our contributing guidelines for more details.

[thomaseizinger]

This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer.

Oh my god. That is one of the most contribution hostile things I've experienced in a while. A PR is a perfectly suited place for a discussion as to whether something should be contributed to the repository. This whole dance of making an issue first, discussing it and only then being allowed to open a PR is very discouraging for small fixes like this.

Perhaps at least make a rule to only apply this to first-time contributors and don't punish people that have a good track record of contributions.

[stephanie-anderson]

Hey, thanks for opening the PR and also thanks for your honest feedback. You're definitely right 👍 We will overhaul this workflow and how it behaves, and roll out a better version early next week. In the meantime I hope we didn't discourage you too much, because we do value your (and everyone's) contributions to this codebase 🙇

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit d0ea483. Configure here.}

[cursor]

Rustls feature breaks reqwest transport without crypto provider

High Severity

Changing reqwest?/rustls to reqwest?/rustls-no-provider means no crypto provider (ring or aws-lc-rs) is bundled for the reqwest transport. When a user enables rustls + reqwest features (the documented way to use rustls), rustls::ClientConfig::builder() will panic at runtime with "no process-level CryptoProvider available" because no compiled-in provider exists. The .expect() in reqwest.rs compounds this with a misleading message suggesting to "Enable either the native-tls or the rustls feature" even though rustls is already enabled. Meanwhile, ureq?/rustls (unchanged) still bundles ring, creating an inconsistency where ureq works but reqwest doesn't.

 

^{Reviewed by Cursor Bugbot for commit d0ea483. Configure here.}

[szokeasaurusrex]

Hey @thomaseizinger, thanks again for the contribution, and sorry about the bot.

I am a bit concerned about making this change, however. Specifically, it can cause a runtime panic for anyone compiling with default-features = false.

I reproduced with a minimal app using:

sentry = { version = "0.48.0", default-features = false, features = ["reqwest", "rustls"] }

With this PR’s change:

rustls = ["dep:rustls", "reqwest?/rustls-no-provider", "ureq?/rustls"]

the app compiles, but panics when Sentry initializes the reqwest transport:

thread 'main' panicked at .../reqwest-0.13.3/src/async_impl/client.rs:2461:5:
No provider set

Reverting only this feature mapping back to reqwest?/rustls makes the same app run successfully, since reqwest/rustls pulls in and installs a crypto provider.

This is masked with Sentry default features because transport also enables native-tls, but users with default-features = false + reqwest + rustls lose a working TLS backend unless they install a provider themselves.

Do you see any way to implement this change in a non-breaking, backwards compatible way? If not, I would want to see a strong argument for why the benefits of this change outweigh the costs of potentially breaking existing users.

[thomaseizinger]

Thank you for re-opening the PR! I think the entire point of this change is to break the default behaviour. The problem with pulling in aws-lc-rs is that it breaks our iOS build because aws-lc-rs requires a C-compiler toolchain and some other stuff to be set up correctly, otherwise it will not build. I don't really want to set all of that up for a dependency that we are not actively using.

The way I see this is: sentry is a library and you shouldn't make any choices for your users as to which crypto provider they set up at runtime. Yes, not setting anything up will panic at runtime but it is an unconditional panic that gets discovered by any smoke test and anything beyond a hello-world will already configure their crypto-provider anyway in their fn main.

The strong argument is the same argument that applies to cargo feature flags in general: They are additive and cannot be turned off, therefore every crate should only ever activate what it really needs. Sentry the library doesn't need a working crypto provider. The binaries that use it need one because they actually run code and they should be in charge of selecting which provider they want.

[lcian]

Another approach would be to make this a new rustls-no-provider feature flag instead.
That would give users the possibility to bring their own provider by choosing the new flag, while avoiding the introduction of a panic for existing users of the rustls feature.

[thomaseizinger]

Another approach would be to make this a new rustls-no-provider feature flag instead. That would give users the possibility to bring their own provider by choosing the new flag, while avoiding the introduction of a panic for existing users of the rustls feature.

Yeah that works too. I guess Sentry is unlikely to be a library that is buried deep in the dependency tree so this is fine. Otherwise, the two label approach can bring with it the pain of it getting accidentally activated because some other dependency you are pulling in activated the wrong one.