Ensure we always startup with a `rustls` `CryptoProvider` #600

tnull · 2025-08-01T12:28:55Z

The rustls library recently introduced this weird behavior where they expect users to, apart from configuring the respective feature, also explictly call CryptoProvider::install_default. Otherwise they'd simply panic at runtime whenever the first network call requiring TLS would be made. While we already made a change upstream at rust-electrum-client, we also make sure here that we definitely, always, absolutley are sure that we have a CryptoProvider set on startup.

ldk-reviews-bot · 2025-08-01T12:28:58Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

src/builder.rs

joostjager · 2025-08-01T12:50:10Z

src/builder.rs

@@ -1525,6 +1527,24 @@ fn build_with_store_internal(
 	})
 }

+fn optionally_install_rustls_cryptoprovider() {


Add doc when this can be removed again (when a newer version of rust-electrum-client is used)?

Tbh, given how messy this is, we might keep it in for a while longer just to be sure no user ever again panics at runtime because of this.

If it is in the rust electrum client, is this still possible? Assuming rustls is messy, then still if we makes sure to install either in ldk-node or in rust-electrum-client, than we are equally good?

src/builder.rs

tnull · 2025-08-01T13:50:13Z

Putting in draft until we're clear what's going on (give we just saw a CI job where a single test case hit the introduced assert). What a mess.

tnull · 2025-08-12T13:03:31Z

CI failure of CLN job is preexisting (has been fixed on main since)

src/builder.rs

TheBlueMatt · 2025-08-12T13:12:00Z

src/builder.rs

+
+	// Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
+	if rustls::crypto::CryptoProvider::get_default().is_none() {
+		let res = rustls::crypto::aws_lc_rs::default_provider()


docs say this is only available with the aws_lc_rs feature and rustls::crypto::ring is only available with the ring feature. Doesn't that imply that we're somehow building with both as dependencies? Can we fix that?

docs say this is only available with the aws_lc_rs feature and rustls::crypto::ring is only available with the ring feature. Doesn't that imply that we're somehow building with both as dependencies? Can we fix that?

Yes, we have both in our tree. Preferably, we get to an aws-lc-rs-only tree, however a) we need the changes of lightningdevkit/rust-lightning#3587 (which we get as part of #462) b) we still need to switch to only use rustls-ring for Swift and possibly Kotlin bindings as aws-lc-rs fails to link properly for Uniffi Swift bindings and nobody could make it work yet.

So currently I even consider going back to a ring-only tree, even though the fact that it recently was marked unmaintained (see rustsec/advisory-db#2227 and rustsec/advisory-db#2230) isn't exactly confidence inspiring. Plus, aws-lc-rs requires cargo-bindgen to build, which is a bit annoying.

we need the changes of lightningdevkit/rust-lightning#3587

I'm not quite sure about this? cargo tree tells me bdk_electrum v23 (direct dependency) pulls in electrum-client v24 which depends on rustls (v23) with ring. Also, the direct dependency on esplora-client v11 pulls in reqwest which depends on hyper-rustls and rustls (v21) with ring. Looks like there's quite a few dependencies on it left.

Btw, note that we eventually might want to also find a fix that allows us to drop these explicit installs again, especially since our upstream libraries do not expose the particular rustls crate they're using, we might end up in a stale state where we install the crypto provider for some rustls version, even if electrum-client moved on to use more recent versions of the crate.

I mean that seems like the reason rustls docs say "libraries should use ClientConfig::builder()/ServerConfig::builder() or otherwise rely on the CryptoProvider::get_default() provider."

They want everyone to just depend on rustls without any configured crypto provider at all, and let the actual application configure it, that way you dont end up with ring and aws-lc-rs in your tree. I get what they're going for and obviously its annoying to deal with for downstream users, but it does kinda feel like maybe we should be going that way?

I mean first of all we should open an issue upstream and ask^H^H^Hbeg them to just use features for this.

I don't think this is worth. There are a bunch of issues / prior discussion around this already on their github. I would cite them, if Github wouldn't be broken right now.

The current logic of "always compile both then set aws-lc-rs if the user didn't override it" is fine (its similar in practice to what rustls should be doing), but obviously totally nuts from the PoV of compiling both always. We should be focusing on not compiling either unless the downstream application opts into one, and then we can set a default like reqwest does.

The issue with the present approach is that now the user won't be able to choose their provider if LDK Node is somewhere in their tree.

Ah, hmm, went and looked and rustls actually doens't require you to call install_default as long as rustls is built without both ring and aws_lc_rs, which should be where we get to (see https://docs.rs/rustls/latest/src/rustls/crypto/mod.rs.html#265-284). So once we clean up the dep tree to not have both versions of rustls we'll be able to remove this code. This makes their stance a good bit less nonsensical (though its still, indeed, a bit weird).

The issue with the present approach is that now the user won't be able to choose their provider if LDK Node is somewhere in their tree.

Hmm? We can't override the users' pick, the API doesn't actually let us, so we can just ignore that case? I wasn't imagining we'd panic on failure.

Ah, hmm, went and looked and rustls actually doens't require you to call install_default as long as rustls is built without both ring and aws_lc_rs, which should be where we get to (see https://docs.rs/rustls/latest/src/rustls/crypto/mod.rs.html#265-284). So once we clean up the dep tree to not have both versions of rustls we'll be able to remove this code. This makes their stance a good bit less nonsensical (though its still, indeed, a bit weird).

Right, the issue is that even if we get to a single-feature tree (say ring), we can't really lean on that, because we cannot control if another crate in the tree would enable aws-lc-rs without knowing about the consequences elsewhere, no?

Hmm? We can't override the users' pick, the API doesn't actually let us, so we can just ignore that case? I wasn't imagining we'd panic on failure.

Hmm, alright. I'll drop the debug_assert.

Right, the issue is that even if we get to a single-feature tree (say ring), we can't really lean on that, because we cannot control if another crate in the tree would enable aws-lc-rs without knowing about the consequences elsewhere, no?

Sure, but that's a downstream issue, not our issue?

Well, yes, but it's super weird to change our behavior to the degree of suddenly panicking, just because a downstream user adds or removes another dependency. Again, they might not even know anything about rustls. Heck, they might even be two hops or more away and might not know anything about LDK Node, Electrum, etc.

tnull · 2025-08-12T13:26:15Z

Force-pushed with the following changes:

diff --git a/src/builder.rs b/src/builder.rs
index 28cf2f7d..080d6568 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -75,5 +75,5 @@ use std::fs;
 use std::path::PathBuf;
 use std::sync::atomic::AtomicBool;
-use std::sync::{Arc, Mutex, RwLock};
+use std::sync::{Arc, Mutex, Once, RwLock};
 use std::time::SystemTime;
 use vss_client::headers::{FixedHeaders, LnurlAuthToJwtProvider, VssHeaderProvider};
@@ -1531,19 +1531,21 @@ fn optionally_install_rustls_cryptoprovider() {
        // Acquire a global Mutex, ensuring that only one process at a time install the provider. This
        // is mostly required for running tests concurrently.
-       static _MUTEX: Mutex<()> = Mutex::new(());
-
-       // Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
-       if rustls::crypto::CryptoProvider::get_default().is_none() {
-               let res = rustls::crypto::aws_lc_rs::default_provider()
-                       .install_default()
-                       .or(rustls::crypto::ring::default_provider().install_default());
-               debug_assert!(res.is_ok(), "We need to install a CryptoProvider");
-       }
+       static INIT_CRYPTO: Once = Once::new();
+
+       INIT_CRYPTO.call_once(|| {
+               // Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
+               if rustls::crypto::CryptoProvider::get_default().is_none() {
+                       let res = rustls::crypto::aws_lc_rs::default_provider()
+                               .install_default()
+                               .or(rustls::crypto::ring::default_provider().install_default());
+                       debug_assert!(res.is_ok(), "We need to install a CryptoProvider");
+               }

-       // Refuse to startup without TLS support. Better to catch it now than even later at runtime.
-       assert!(
-               rustls::crypto::CryptoProvider::get_default().is_some(),
-               "We need to have a CryptoProvider"
-       );
+               // Refuse to startup without TLS support. Better to catch it now than even later at runtime.
+               assert!(
+                       rustls::crypto::CryptoProvider::get_default().is_some(),
+                       "We need to have a CryptoProvider"
+               );
+       });
 }

TheBlueMatt · 2025-08-12T14:33:53Z

src/builder.rs

+		// Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
+		if rustls::crypto::CryptoProvider::get_default().is_none() {
+			let res = rustls::crypto::aws_lc_rs::default_provider()
+				.install_default()


This is not fallible. The docs should write this (can we contribute upstream here) but this fails only when there's already a default provider, so we should just swallow the error.

Yes, but we don't know if the user previously installed another provider. This is why we debug_assert here, although it could be argued we should just log and continue.

I meant the fallback to try to install ring is unnecessary because the error indicates we already have a provider, not that we failed to install aws_lc_rs.

TheBlueMatt · 2025-08-12T14:39:50Z

src/builder.rs

+
+	// Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
+	if rustls::crypto::CryptoProvider::get_default().is_none() {
+		let res = rustls::crypto::aws_lc_rs::default_provider()


I mean first of all we should open an issue upstream and ask^H^H^Hbeg them to just use features for this. They have a "default" in their docs, they should use it unless the application configures something different as long as the feature is set and thus aws-lc-rs is available (or some other default).

The current logic of "always compile both then set aws-lc-rs if the user didn't override it" is fine (its similar in practice to what rustls should be doing), but obviously totally nuts from the PoV of compiling both always. We should be focusing on not compiling either unless the downstream application opts into one, and then we can set a default like reqwest does.

tnull · 2025-08-13T11:10:28Z

Force-pushed once more with:

diff --git a/src/builder.rs b/src/builder.rs
index 080d6568..d3cded4a 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -1536,8 +1536,7 @@ fn optionally_install_rustls_cryptoprovider() {
                // Ensure we always install a `CryptoProvider` for `rustls` if it was somehow not previously installed by now.
                if rustls::crypto::CryptoProvider::get_default().is_none() {
-                       let res = rustls::crypto::aws_lc_rs::default_provider()
+                       let _ = rustls::crypto::aws_lc_rs::default_provider()
                                .install_default()
                                .or(rustls::crypto::ring::default_provider().install_default());
-                       debug_assert!(res.is_ok(), "We need to install a CryptoProvider");
                }

src/builder.rs

tnull · 2025-08-14T08:12:42Z

Force-pushed the following changes:

diff --git a/src/builder.rs b/src/builder.rs
index d3cded4a..97a02b2e 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -1537,6 +1537,5 @@ fn optionally_install_rustls_cryptoprovider() {
                if rustls::crypto::CryptoProvider::get_default().is_none() {
                        let _ = rustls::crypto::aws_lc_rs::default_provider()
-                               .install_default()
-                               .or(rustls::crypto::ring::default_provider().install_default());
+                               .install_default();
                }

tnull · 2025-08-14T08:21:05Z

The more I think about the issue, the more it seems like there's only bad choices in all direction. My current take-away is:

we land this PR and Ensure we always startup with a rustls CryptoProvider (main) #601 as short-term fixes for the issue at hand
As part of/post Upgrade to LDK 0.2 #462 we try to revert to a purely rustls-ring dependency tree, keeping some safeguard in place in case downstream users would enable both features.
We contribute upstream to rust-electrum-client to fallback to rustls-ring by default whenever the user hasn't set a specific CryptoProvider, mirroring reqwest's behavior. I.e., we'd (partially) revert Install rustls's CryptoProvider based on features bitcoindevkit/rust-electrum-client#171 in favor of a more relaxed approach that still tries 'best effort' to pick a crypto provider.

@TheBlueMatt WDYT?

The `rustls` library recently introduced this weird behavior where they expect users to, apart from configuring the respective feature, also explictly call `CryptoProvider::install_default`. Otherwise they'd simply panic at runtime whenever the first network call requiring TLS would be made. While we already made a change upstream at `rust-electrum-client`, we also make sure here that we definitely, always, absolutley are sure that we have a `CryptoProvider` set on startup.

TheBlueMatt · 2025-08-14T12:20:34Z

It does seem to me like rustls has basically forced everyone to do the reqwest dance - expose features to ensure a default is set (why ring vs aws-lc-rs?) but allow the downstream app to default-features = false to pick their own. It's completely bonkers cause it pollutes basically the entire rust ecosystem with features specifically for rustls but it is where we are today (I went and read upstream issues a bit, imo we could make a stronger argument here, they haven't really grappled with this issue I don't think).

With that in mind, I agree that we should land this as long as our tree is polluted with both, but we really need to get features in place to let the downstream user pick.

tnull requested a review from joostjager August 1, 2025 12:29

joostjager reviewed Aug 1, 2025

View reviewed changes

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch 2 times, most recently from d3441ab to 90c67ca Compare August 1, 2025 13:15

tnull marked this pull request as draft August 1, 2025 13:50

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch from 90c67ca to 7c08563 Compare August 12, 2025 12:50

tnull requested a review from TheBlueMatt August 12, 2025 13:03

tnull marked this pull request as ready for review August 12, 2025 13:03

TheBlueMatt reviewed Aug 12, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Aug 12, 2025

Ensure we always startup with a rustls CryptoProvider (main) #601

Merged

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch from 7c08563 to 33afe91 Compare August 12, 2025 13:25

TheBlueMatt reviewed Aug 12, 2025

View reviewed changes

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch from 33afe91 to 3450259 Compare August 13, 2025 11:09

tnull requested a review from TheBlueMatt August 13, 2025 11:10

TheBlueMatt reviewed Aug 13, 2025

View reviewed changes

src/builder.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch from 3450259 to fba4fe8 Compare August 14, 2025 08:12

tnull force-pushed the 2025-08-fix-rustls-crypto-provider branch from fba4fe8 to 9fa5686 Compare August 14, 2025 08:22

TheBlueMatt approved these changes Aug 14, 2025

View reviewed changes

tnull merged commit 201217f into lightningdevkit:release/0.6 Aug 14, 2025
15 checks passed

Ensure we always startup with a rustls CryptoProvider #600

Ensure we always startup with a rustls CryptoProvider #600

Uh oh!

Conversation

tnull commented Aug 1, 2025

Uh oh!

ldk-reviews-bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Aug 1, 2025

Uh oh!

tnull commented Aug 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Aug 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Aug 13, 2025

Uh oh!

Uh oh!

tnull commented Aug 14, 2025

Uh oh!

tnull commented Aug 14, 2025

Uh oh!

TheBlueMatt commented Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

Ensure we always startup with a `rustls` `CryptoProvider` #600

Ensure we always startup with a `rustls` `CryptoProvider` #600

ldk-reviews-bot commented Aug 1, 2025 •

edited

Loading

tnull Aug 14, 2025 •

edited

Loading