Skip to content

Install rustls's CryptoProvider based on features #171

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jul 8, 2025

Previously, we'd already assume use-rustls to use the default aws-lc-rs provider and use-rustls-ring to use the ring CryptoProvider, e.g., for NoCertificateVerification.

However, we wouldn't actually install the respective provider based on the features, leading to a reachable panic at runtime when user tried to access ssl:// Electrum servers.

Here, we fix this omission and install the default provider according to the configured features.

(cc @oleonardolima @thunderbiscuit)

Previously, we'd already assume `use-rustls` to use the default
`aws-lc-rs` provider and `use-rustls-ring` to use the `ring`
`CryptoProvider`, e.g., for `NoCertificateVerification`.

However, we **wouldn't** actually install the respective provider based
on the features, leading to a **reachable** panic at runtime when user
tried to access `ssl://` Electrum servers.

Here, we fix this omission and install the default provider according to
the configured features.
@tnull
Copy link
Contributor Author

tnull commented Jul 8, 2025

FWIW, an alternative would be to introduce an use-rustls-aws-lc-rs feature and also switch the NoCertificateVerification over to use this. But as it stands currently, the current behavior is inconsistent and unsafe.
Let me know what you prefer. If we'd add the new feature and switch to it by default, it would also fix #137 it seems.

@oleonardolima oleonardolima added the bug Something isn't working label Jul 9, 2025
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK 41c5228

I wasn't able to reproduce the panic locally. Do you have a specific scenario/impl that you could share?

I assume you're referring to the following panic: no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point

That's produced by rustls::ClientConfig::get_default_or_install_from_crate_features, is that right ?

It tries to install the default CryptoProvider for the given process, and I wonder why it's not able to do it in your scenario, are you using ambiguous features ?

I don't see much problem with installing it explicitly based on the features, as it's also recommended by rustls docs.

@tnull
Copy link
Contributor Author

tnull commented Jul 10, 2025

I assume you're referring to the following panic:

Yes:

[2025-07-10T07:42:11Z INFO  electrum_client::raw_client] Trying to connect to 34.8.62.41:60002 (attempt 1/1) with timeout 10s

thread 'main' panicked at /home/tnull/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustls-0.23.28/src/crypto/mod.rs:249:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point

It tries to install the default CryptoProvider for the given process, and I wonder why it's not able to do it in your scenario, are you using ambiguous features ?

In terms of rustls-related crates, we currently set

bdk_electrum = { version = "0.23.0", default-features = false, features = ["use-rustls"]}
electrum-client = { version = "0.23.1", default-features = true, features = ["use-rustls"] }
bdk_esplora = { version = "0.22.0", default-features = false, features = ["async-https-rustls", "tokio"]}
esplora-client = { version = "0.12", default-features = false, features = ["tokio", "async-https-rustls"] }
esplora-client_0_11 = { package = "esplora-client", version = "0.11", default-features = false, features = ["tokio", "async-https-rustls"] }
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
lightning-transaction-sync = { version = "0.1.0", features = ["esplora-async-https", "time", "electrum"] }

Indeed it seems we have both ring and aws-lc-rs present in the current tree, possibly connected to us currently having electrum-client 0.21 and 0.23, as well as two versions of esplora_client for compatibility reasons in the tree. In any case, I don't think that having some crate set an additional feature in your workspace should ever result in a panic at runtime.

I don't see much problem with installing it explicitly based on the features, as it's also recommended by rustls docs.

Well, there is a slight problem in that it's recommended for binaries, while they suggest libraries to keep it open. However, as mentioned in the OP we already infringe on that for the NoCertificateVerification, so might as well do it for the cert-verifying path, at least until we find a better solution.

@notmandatory notmandatory moved this to Needs Review in BDK Chain Jul 10, 2025
@oleonardolima
Copy link
Contributor

I don't see much problem with installing it explicitly based on the features, as it's also recommended by rustls docs.

Well, there is a slight problem in that it's recommended for binaries, while they suggest libraries to keep it open. However, as mentioned in the OP we already infringe on that for the NoCertificateVerification, so might as well do it for the cert-verifying path, at least until we find a better solution.

Indeed, it seems like an okay fix for now, but ideally, we should move forward towards #137 and expose CryptoProvider api.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 41c5228.

Seems like a decent fix until we actually do #137 (or we migrate to the new Electrum client? Not sure where that's at).

In any case I see no reason to hold this one up.

Copy link
Collaborator

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 41c5228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

4 participants