Feature Request: Avoid panicking when ring and aws_lc_rs are both specified about rustls HOT 19 OPEN

I am going to restate what I have said before. You can close the issue if you aren't comfortable with what I suggested.

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

        CryptoProvider::get_default()
            .map(|arc| (**arc).clone())
            .unwrap_or_else(ring::default_provider)

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:

enum EnabledCryptoBackend {
    AwsLc,
    Ring,
    Both,
    None,
}

// This should be available in rustls 
const fn get_enabled_crypto_backend() -> EnabledCryptoBackend {
    if cfg!(all(feature = "ring", feature = "aws_lc_rs")) {
        EnabledCryptoBackend::Both
    } else if cfg!(feature = "ring") {
        EnabledCryptoBackend::Ring
    } else if cfg!(feature = "awc_ls_rs") {
        EnabledCryptoBackend::AwsLc
    } else {
        EnabledCryptoBackend::None
    }
}

fn main() {
    // Currently in beta
    const {
        // This fails without any features
        // assert!(matches!(get_enabled_crypto_backend(), EnabledCryptoBackend::Ring));
        assert!(matches!(get_enabled_crypto_backend(), EnabledCryptoBackend::None));
    }
}

Playground link.

from rustls.

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.

from rustls.

the information is available at compile time

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

from rustls.

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.

from rustls.

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

from rustls.

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.

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.

from rustls.

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.

from rustls.

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.

from rustls.

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.

from rustls.

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
}

from rustls.

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.

from rustls.

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

from rustls.

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

from rustls.

@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

from rustls.

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

from rustls.

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?

from rustls.

Personally, I prefer what I suggested (using aws-lc-rs override ring) instead of panicking but it is up to you.

from rustls.

I think that proposal is unworkable based on Ctz's point:

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.

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.

from rustls.

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 install_default() or changing the end user-only feature flags. (In particular, adding a dev-dependency that picks a default provider feature would be more ergonomic for unit tests than calling install_default() in each.) Separating the dual purposes of the feature flags this way would mean the flags that enable providers would be truly additive. If rustls still included one of the "default provider selection" features in its default feature, we would still have the problem we have today of a few library crates forgetting to set no-default-features and causing problems. If not, then there would no longer be a default choice of a default, and end users would need to read the documentation and add a feature as an additional barrier to entry, but there would be better adherence by library crates to the guidance to not select a default provider.

from rustls.