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
Feature Request: Avoid panicking when ring and aws_lc_rs are both specified #1877
Comments
I agree that the current setup (from #1766, I think) is not ideal for libraries. I think we should have gone with something like this: diff --git a/rustls/src/crypto/mod.rs b/rustls/src/crypto/mod.rs
index 74b19ea6..9f029aed 100644
--- a/rustls/src/crypto/mod.rs
+++ b/rustls/src/crypto/mod.rs
@@ -215,6 +215,29 @@ pub struct CryptoProvider {
}
impl CryptoProvider {
+ /// Returns the default `CryptoProvider` for this process.
+ ///
+ /// If a crypto provider has been installed using [`install_default()`], return that.
+ /// Otherwise, return the default provider for the crate features in use. If both
+ /// first-party providers are enabled, this will return the aws-lc-rs provider.
+ #[cfg(any(feature = "aws_lc_rs", feature = "ring"))]
+ pub fn installed_or_default() -> Arc<Self> {
+ match PROCESS_DEFAULT_PROVIDER.get() {
+ Some(provider) => provider.clone(),
+ None => {
+ #[cfg(feature = "aws_lc_rs")]
+ {
+ return Arc::new(aws_lc_rs::default_provider());
+ }
+
+ #[cfg(all(feature = "ring", not(feature = "aws_lc_rs")))]
+ {
+ return Arc::new(ring::default_provider());
+ }
+ }
+ }
+ }
+
/// Sets this `CryptoProvider` as the default for this process.
///
/// This can be called successfully at most once in any process execution. And used this in the builders. This would never panic, because it is only available when one of the first-party providers is enabled, but it would require restricting the builder constructors to only be available when at least one of the first-party providers is enabled. For now, we could still provider this as a helper for libraries. We could also make a change to avoid panicking in the case where both first-party providers are available, and pick aws-lc-rs in that case, which seems reasonable to me. The current setup breaks feature additivity which is a bit of a pain. I also wonder whether it is the right choice to implicitly install a crypto provider in |
Even if it would still panic (or somehow propagating a result) if no feature is specified it would be still reasonable. It is less accidental to go for 1-2 features enabled to 0, compared to enabling both features accidentally. I fear that maybe removing this failure will have negative effect of making libraries enable at least 1 feature to use this builder. Instead libraries could use the builder, and expect the user to enable at least 1 feature or set the provider dynamically. Libraries which want to make sure of no failure can enable the feature they want.
I think that would better. The user can later setup a default provider which would be then used for new instances. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
That proposal also breaks feature additivity: if you carefully arrange to enable only the The purpose of the panic is to signal clearly: "you have failed to unambiguously select a provider via crate features." I think my first preference is to see if any progress is made with the "mutually-exclusive, global features" proposal. If not, I think we could look into making the default provider thing fail at link time, perhaps. That would look something like: (in the rustls crate)
The
But linker errors are exceedingly user-unfriendly, and reading |
What about making this opt-in for the application developer? I feel that few care about specific crypto backend and those how care can opt-in. Imagine suddenly having your code not work in production because you enabled a feature by mistake. (E.g: someone using rustls and rustls is not big part of the application so they won't notice immediately) Example for possible opt-in:
|
I agree that it's not ideal, but I think "additivity" is the specific feature that your code will still compile independent of what features you add. So my proposal doesn't break what I would call additivity, although it does have non-local effects that might also be surprising.
I don't see movement on this happening organically, that is, without someone specifically funding this work. Having thought about it a bit, I agree that panics are probably worse than having the wrong crypto provider be used, especially since a clear solution (have the application install a default) is available for the latter problem. |
Would it be more reasonable to have |
@NobodyXu that would make it impossible to first check if a process-wide default has been installed already. |
That's true, but having runtime panic still sounds pretty bad |
The caller can bypass this code path by using |
I see, thanks, that does look alright if you can override it. |
I don't think this issue surfaced a workable alternative we could implement that doesn't have its own new set of downsides. I'm personally leaning towards saying we should close the issue for now. WDYT? |
Personally, I prefer what I suggested (using aws-lc-rs override ring) instead of panicking but it is up to you. |
I think that proposal is unworkable based on Ctz's point:
We've seen very passionate arguments in favour of being able to use ring and only ring and this approach would harm the wishes of those users. |
I think it's important to address this issue, as it is a barrier to ecosystem adoption in other libraries. What if rustls had separate Cargo features to enable each provider and select each provider as the default? The documentation could tell end users to enable one default selection feature, and tell libraries to only use the features that enable providers. This would still leave one case where initialization could panic (if neither or both default selection features were enabled, and there wasn't a runtime default provider installed) but this could now be solved either by using |
I am going to restate what I have said before. You can close the issue if you aren't comfortable with what I suggested.
I feel that CryptoProvider::install_default should be suitable for them. In addition, the current approach is not very effective for these users as they can only get only runtime panic and only in few cases, as any authors could directly ClientConfig::builder_with_provider() to use whatever due to their preference without the user noticing. I think the current approach will encourage the library authors (after they are accidentally bitten by the panic) to use ClientConfig::builder_with_provider like in rocket and maybe reqwest. Due to this the library author would construct CryptoProvider based on their opinion. So instead of having specified behavior (if two features are enabled this feature will take over). Each library author would choose the feature to prefer. For example as shown in Rocket. The default will be ring if no CryptoProvider was installed as default.
This way, we ended with the worst of both worlds. Users cannot ensure using what feature they want, libraries are bitten by random panics while in development, the behavior is not specified and is up to each library author. What would be effective is link-time-error as suggested in this thread (which I personally don't prefer as there are users who want both at the same time) or having a function or macro that these users can add to get compile error if they want to:
Playground link. |
Personally I find @Alvenix's line of reasoning pretty compelling. As a downstream library maintainer I haven't wanted to adopt the installed default if that means I can incur a panic, and if downstream maintainers don't adopt it it's pretty pointless to have the option at all. |
Could it be that many of people who like using ring, like that it have easier build requirements? I am in that category and would ditch aws-lc-rs if ring supported more stuff. If so, if the two features are accidentally enabled and the application compile correctly they would be happy and satisfied. If it doesn't compile due to strange aws-lc-rs errors they would know that the aws-lc-rs feature was accidentally enabled somewhere and they may have to patch some dependency to avoid aws-lc-rs. In any case, strange compilation errors due to missing build dependencies are preferable to panics for me. |
Checklist
I know the current design for crypto providers is probably well discussed but I wanted to add one data point against the current behavior for reference. This issue is mostly rephrasing my discussion at 1876:
Is your feature request related to a problem? Please describe.
When I accidentally enabled these two features (directly or indirectly through a crate) I had few panics that I had to debug. I consider myself somewhat familiar with Rust and (and cargo features) so debugging this was easy although annoying. However, this could be a little road blocker for people less familiar with Rust.
Describe the solution you'd like
Changing the internal function from_crate_features() to choose either
ring
oraws_lc_rs
CryptoProvider when both features are specified, instead of returning None. In addition, removing the related panic. In addition, specify the behavior so it is no longer ambiguous.Describe alternatives you've considered
Setup the default crypto provider on main. However, sometimes when developing a library I probably shouldn't do that. I would have to document to the users, that they need to setup the crypto providers if they don't want to have a panic accidentally.
The text was updated successfully, but these errors were encountered: