feat: introduce rustls-no-provider feature flag

[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.

We add our own rustls-no-provider feature flag which allows users to opt out of aws-lc-rs being the default crypto provider of reqwest in a backwards-compatible way.

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 🙇

[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.

[szokeasaurusrex]

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.

Got it. Also, just wondering, was this a regression from a previous version of the SDK, or has the SDK never pulled in aws-lc-rs before? If it was a regression, which version still worked for you?

[thomaseizinger]

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.

Got it. Also, just wondering, was this a regression from a previous version of the SDK, or has the SDK never pulled in aws-lc-rs before? If it was a regression, which version still worked for you?

It was a regression. I believe 0.46 was the last one that worked without any patching from our side. (We currently depend on a fork)

It came in with the bump of reqwest to 0.13: https://github.com/getsentry/sentry-rust/pull/998/changes#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR317-R338

[lcian]

reqwest 0.13 switched from defaulting native-tls to rustls and also the default backend from ring to aws-lc-rs: https://github.com/seanmonstar/reqwest/releases/tag/v0.13.0

[szokeasaurusrex]

Circling back here @thomaseizinger, I like @lcian's idea of introducing a separate rustls-no-provider feature flag, which only pulls in rustls without a crypto provider. That way, this fix can remain backwards compatible.

Would you be willing to update the PR accordingly, or shall we take this over from here?

[thomaseizinger]

Would you be willing to update the PR accordingly, or shall we take this over from here?

Oh, I didn't realise you wanted me to continue working on this PR. I am okay with the idea, happy for you to take over and ship whichever solution you are comfortable with.

[szokeasaurusrex]

@thomaseizinger to clarify, I am also happy to take it over; however, if you wanted to contribute the fix, I didn't want to steal that opportunity away from you 😅 so just wanted to follow up and see what your plan is

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.36%. Comparing base (a57b91c) to head (7a78d79).
⚠️ Report is 83 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
+ Coverage   73.81%   74.36%   +0.55%     
==========================================
  Files          64       67       +3     
  Lines        7538     7942     +404     
==========================================
+ Hits         5564     5906     +342     
- Misses       1974     2036      +62     

[szokeasaurusrex]

Nice, thanks again for the contribution!