Feature Request: Avoid panicking when ring and aws_lc_rs are both specified

[Alvenix]

Checklist

• I've searched the issue tracker for similar requests

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 or aws_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.

[djc]

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 get_default_or_install_from_crate_features() -- instead, it could just return a provider if no default has been installed.

[Alvenix]

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.

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 also wonder whether it is the right choice to implicitly install a crypto provider in get_default_or_install_from_crate_features() -- instead, it could just return a provider if no default has been installed.

I think that would better. The user can later setup a default provider which would be then used for new instances.

[jbr]

Not sure if this is the right place for this feedback, but I accidentally enabled both features while switching over some code from previous default (ring) to new default (aws-ls-rs) and almost didn't catch this because I was running cargo check instead of executing the code. If having both features enabled is a show-stopping error (panic in likely-unrecoverable code) and the information is available at compile time (cargo features), it would be more ergonomic to emit a compile_error instead of panicking at runtime in get_default_or_install_from_crate_features.

[Alvenix]

the information is available at compile time

It isn't. Because the user could have installed a default crypto provider.

[ctz]

The current setup breaks feature additivity which is a bit of a pain.

That proposal also breaks feature additivity: if you carefully arrange to enable only the ring feature because you want to use ring, but later something in your distant dependency enables aws-lc-rs too, you silently end up using aws-lc-rs against your wishes.

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)

#[cfg(feature = "aws-lc-rs")]
specify_default_provider! ( &aws_lc_rs::PROVIDER );

#[cfg(feature = "ring")]
specify_default_provider! ( &ring::PROVIDER );

extern "C" {
  pub static RUSTLS_DEFAULT_PROVIDER: &'static CryptoProvider;
}

The specify_default_provider!() macro would expand to emitting a definition of extern "C" RUSTLS_DEFAULT_PROVIDER. The final link would fail if:

• anything refers to RUSTLS_DEFAULT_PROVIDER (ie, you have used ClientConfig::builder() and co), but nothing provides it -- via a missing symbol error
• multiple specify_default_provider! are in play, with a multiple definition error

But linker errors are exceedingly user-unfriendly, and reading RUSTLS_DEFAULT_PROVIDER would introduce unsafe code into the crate.

[Alvenix]

That proposal also breaks feature additivity: if you carefully arrange to enable only the ring feature because you want to use ring, but later something in your distant dependency enables aws-lc-rs too, you silently end up using aws-lc-rs against your wishes.

The purpose of the panic is to signal clearly: "you have failed to unambiguously select a provider via crate features."

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:

fn main() {
  // Some static function or macro that panic or fail to compile if both features are enabled
  rustls::assert_only_single_provider();
  // Or simply specfying the provider at runtime using the crypto provider
}

[djc]

That proposal also breaks feature additivity: if you carefully arrange to enable only the ring feature because you want to use ring, but later something in your distant dependency enables aws-lc-rs too, you silently end up using aws-lc-rs against your wishes.

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.

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.

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.

[NobodyXu]

Would it be more reasonable to have compile_error!, instead of a runtime panic when both are specified?

[djc]

@NobodyXu that would make it impossible to first check if a process-wide default has been installed already.

[NobodyXu]

@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

[djc]

The caller can bypass this code path by using builder_with_provider() instead.

[NobodyXu]

I see, thanks, that does look alright if you can override it.