Add use-rustls-ring feature #135

thunderbiscuit · 2024-06-26T14:24:26Z

This PR adds the ability to build the client using the ring dependency for rustls instead of the new default aws-lc-rs.

As of the 0.23.0 release, rustls changed its default cryptography provider to aws-lc-rs. This new library is actually a set of bindings to a C library maintained by AWS, and they provide prebuilt bindings for some platforms but not all. On these other platforms, the compilation step will attempt to build the bindings, requiring extra dependencies (CMake, libclang and others depending on the platform). This compilation step is what is currently breaking our Android and Swift builds for bdk-ffi. It is certainly possible to build the bindings (and the AWS docs on it are very nice), but for some reason I have not been able to make it work everywhere yet (local, CI, Windows).

This PR enables us to use the previous default ring library for rustls. I basically have to turn off the default features on rustls and re-enable all of them except for the aws_lc_rs. We also have a few feature-gated constructs in the library, for which I needed to add the new proposed use-rustls-ring feature in order to make all of this work for us. Let me know if there are maybe better ways to achieve this!

oleonardolima

Concept ACK c7a96f7

oleonardolima

I think the documentation here could be slightly updated to mention both options use-rustls and use-rusts-ring, wdyt ?

rust-electrum-client/src/lib.rs

Lines 6 to 8 in c7a96f7

    
           //! By default this library is compiled with support for SSL servers using [`rustls`](https://docs.rs/rustls) and support for 
        
           //! plaintext connections over a socks proxy, useful for Onion servers. Using different features, 
        
           //! the SSL implementation can be removed or replaced with [`openssl`](https://docs.rs/openssl).

oleonardolima · 2024-07-08T14:05:20Z

Cargo.toml

+use-rustls = ["webpki-roots", "rustls/default"]
+use-rustls-ring = ["proxy", "webpki-roots", "rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]


@thunderbiscuit Do we need the proxy feature to be set when using use-rustls-ring ?
Is this just to keep the standard like the default ones ?
edit: related to the comment I left at bitcoindevkit/bdk#1491 (comment)

Yep you're right. Fixed in my latest push.

LLFourn · 2024-07-23T03:29:12Z

Can we just expose the rustls provider API here: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html so I can pass in my own provider?
We can have a rustls-aws-lc feature that adds a constructor that uses that. Can do the same for rustls-ring. But I'd like to able to fix this kind of issue in the future without having to wait for a merge.

oleonardolima · 2024-07-25T04:10:46Z

Can we just expose the rustls provider API here: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html so I can pass in my own provider? We can have a rustls-aws-lc feature that adds a constructor that uses that. Can do the same for rustls-ring. But I'd like to able to fix this kind of issue in the future without having to wait for a merge.

That's a good question! I'm not sure and would need to take a look at the docs and give it a shot, but if that's feasible, I agree that could be a better approach.

oleonardolima · 2024-07-27T00:49:44Z

Can we just expose the rustls provider API here: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html so I can pass in my own provider? We can have a rustls-aws-lc feature that adds a constructor that uses that. Can do the same for rustls-ring. But I'd like to able to fix this kind of issue in the future without having to wait for a merge.

That's a good question! I'm not sure and would need to take a look at the docs and give it a shot, but if that's feasible, I agree that could be a better approach.

@thunderbiscuit Taking a look at the documentation it looks like really simple to follow Lloyd's suggestion.

You'd need to use the proper .builder_with_provider() on ClientConfig, probably on the lines:

rust-electrum-client/src/raw_client.rs

Lines 387 to 410 in 64c77ee

    
           let builder = ClientConfig::builder(); 
        
           let config = if validate_domain { 
        
               socket_addr.domain().ok_or(Error::MissingDomain)?; 
        
               let store = webpki_roots::TLS_SERVER_ROOTS 
        
                   .into_iter() 
        
                   .map(|t| TrustAnchor { 
        
                       subject: Der::from_slice(t.subject), 
        
                       subject_public_key_info: Der::from_slice(t.spki), 
        
                       name_constraints: t.name_constraints.map(|nc| Der::from_slice(nc)), 
        
                   }) 
        
                   .collect::<RootCertStore>(); 
        
               // TODO: cert pinning 
        
               builder.with_root_certificates(store).with_no_client_auth() 
        
           } else { 
        
               builder 
        
                   .dangerous() 
        
                   .with_custom_certificate_verifier(std::sync::Arc::new( 
        
                       danger::NoCertificateVerification {}, 
        
                   )) 
        
                   .with_no_client_auth() 
        
           };

thunderbiscuit · 2024-07-27T13:41:35Z

@LLFourn @oleonardolima

Can we just expose the rustls provider API here: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html so I can pass in my own provider? We can have a rustls-aws-lc feature that adds a constructor that uses that. Can do the same for rustls-ring. But I'd like to able to fix this kind of issue in the future without having to wait for a merge.

This is interesting and might be a good way to go long term, but is outside the scope of this PR IMO, which is just a flag fix for rustls changing their default provider. I'll open an issue so we can tackle this approach in a different PR which should get its own review.

The builds are currently breaking for 3 known projects, and I want to make sure we help them move forward. If this non-intrusive flag allows users to fix their build in the short run, let's get it in.

Unless there are oppositions to this, consider merging @notmandatory.

Interested parties:
@oleonardolima
@ValuedMammal
@LLFourn
@notmandatory
@praveenperera

oleonardolima

ACK b415b5c
I do agree that's a good approach, and we can address the further improvement in another PR.

It adds a new field to the client `Config, expecting the `CryptoProvider` from the user. It uses aws-lc-rs or ring providers by default if any of these features are enabled. It's based on the suggestion comment at bitcoindevkit#135, reference: bitcoindevkit#135 (comment)

LLFourn · 2024-07-29T00:17:59Z

ACK b415b5c

praveenperera · 2024-07-29T19:27:06Z

Cargo.toml

@@ -41,5 +41,6 @@ default = ["proxy", "use-rustls"]
 minimal = []
 debug-calls = []
 proxy = ["byteorder", "winapi", "libc"]
-use-rustls = ["webpki-roots", "rustls"]
+use-rustls = ["webpki-roots", "rustls/default"]
+use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]


Suggested change

use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]

use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12", "webpki-roots"]

Ah thanks for catching this 👍

praveenperera · 2024-07-29T19:28:10Z

@thunderbiscuit got error.

44 | extern crate webpki_roots;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

use-rustls-ring does not enable the webpki_roots feature

use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]

But it is imported here

#[cfg(any(
    feature = "default",
    feature = "use-rustls",
    feature = "use-rustls-ring"
))]
extern crate webpki_roots;

its working and compiling after adding the webpki_roots to the deps for the new feature: bitcoinppl@e3314aa

oleonardolima · 2024-07-29T19:54:50Z

@thunderbiscuit got error.
44 | extern crate webpki_roots;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate
use-rustls-ring does not enable the webpki_roots feature
use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]
But it is imported here
#[cfg(any(
    feature = "default",
    feature = "use-rustls",
    feature = "use-rustls-ring"
))]
extern crate webpki_roots;
its working and compiling after adding the webpki_roots to the deps for the new feature: bitcoinppl@e3314aa

Yes, you're right it's missing as it's being used here:

rust-electrum-client/src/raw_client.rs

Lines 392 to 399 in 64c77ee

    
           let store = webpki_roots::TLS_SERVER_ROOTS 
        
               .into_iter() 
        
               .map(|t| TrustAnchor { 
        
                   subject: Der::from_slice(t.subject), 
        
                   subject_public_key_info: Der::from_slice(t.spki), 
        
                   name_constraints: t.name_constraints.map(|nc| Der::from_slice(nc)), 
        
               }) 
        
               .collect::<RootCertStore>();

It got my attention now, that our CI is not properly covering these tests, which is some sort of complicated because of TLS stuff 🤔

thunderbiscuit · 2024-07-31T20:07:34Z

Note that I updated the CI to run the use-rustls-ring feature. Thanks @oleonardolima!

oleonardolima

ACK 8d71f95

notmandatory

ACK 8d71f95

It adds a new field to the client `Config, expecting the `CryptoProvider` from the user. It uses aws-lc-rs or ring providers by default if any of these features are enabled. It's based on the suggestion comment at bitcoindevkit#135, reference: bitcoindevkit#135 (comment)

4dd7e21 Bump version to 0.21.0 and update CHANGELOG.md (Steve Myers) Pull request description: Bumped crate version to 0.21.0 and added below to changelog: ## 0.21.0 - Add use-rustls-ring feature #135 - refactor: make validate_merkle_proof more efficient #134 - chore: set rust edition to 2021, fix clippy, add ci fmt and clippy checks #139 ## 0.20.0 - Upgrade rustls to 0.23 #132 - chore(deps): upgrade rust-bitcoin to 0.32.0 #133 - ci: add test with MSRV 1.63.0 #128 ACKs for top commit: oleonardolima: ACK 4dd7e21 ValuedMammal: ACK 4dd7e21 Tree-SHA512: 3fcec2fb437733eac235bccb1b9c8f6b706e7a713c71de85016adc93f7db128ca6eadb5e9d1d44df27f1b49cce139b222aa9c21343afcf25befdf80a47442e51

It adds a new field to the client `Config, expecting the `CryptoProvider` from the user. It uses aws-lc-rs or ring providers by default if any of these features are enabled. It's based on the suggestion comment at bitcoindevkit#135.

f965f95 feat: enable selecting use-rustls-ring feature on electrum client (thunderbiscuit) Pull request description: This PR is a companion to bitcoindevkit/rust-electrum-client#135. It enables choosing the `ring` dependency on rustls instead of the new default (as of 0.23.0) `aws-lc-rs`. The AWS dependency breaks the Android and Swift builds. I wrote a more detailed explanation on [#135](bitcoindevkit/rust-electrum-client#135). ### Notes to the reviewers Do not merge before: - [x] [#135](bitcoindevkit/rust-electrum-client#135) is merged - [x] A new version of rust-electrum-client is released (will be 0.21.0) - [x] The dependency points to the new version of the client rather than my fork of it. ### Changelog notice ```md Added - bdk_electrum now enables choosing either the `use-rustls` or `use-rustls-ring` feature ``` ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: LLFourn: ACK f965f95 notmandatory: utACK f965f95 oleonardolima: ACK f965f95 Tree-SHA512: c82afa82ef8603bc8e6d024ee5030fa1ec6ab71fbce090182ce3a297ce1a788c1db48f593f05331b1de1931a731a5fc03f804cb1b17f8c7832286fda6c09aa4b

thunderbiscuit force-pushed the feature/rustls-ring branch from feb2374 to c7a96f7 Compare June 26, 2024 14:26

thunderbiscuit mentioned this pull request Jun 26, 2024

Enable selecting use-rustls-ring feature on electrum client bitcoindevkit/bdk#1491

Merged

6 tasks

oleonardolima approved these changes Jun 26, 2024

View reviewed changes

reez mentioned this pull request Jul 1, 2024

Use bdk_electrum upstream bitcoindevkit/bdk-ffi#567

Closed

oleonardolima reviewed Jul 8, 2024

View reviewed changes

thunderbiscuit force-pushed the feature/rustls-ring branch from c7a96f7 to b415b5c Compare July 27, 2024 13:27

thunderbiscuit mentioned this pull request Jul 27, 2024

Allow passing custom crypto provider #137

Open

oleonardolima approved these changes Jul 27, 2024

View reviewed changes

LLFourn approved these changes Jul 29, 2024

View reviewed changes

praveenperera reviewed Jul 29, 2024

View reviewed changes

thunderbiscuit force-pushed the feature/rustls-ring branch from b415b5c to dbf4cca Compare July 31, 2024 19:23

feat: add use-rustls-ring feature

8d71f95

thunderbiscuit force-pushed the feature/rustls-ring branch from dbf4cca to 8d71f95 Compare July 31, 2024 20:06

oleonardolima approved these changes Jul 31, 2024

View reviewed changes

notmandatory approved these changes Jul 31, 2024

View reviewed changes

notmandatory merged commit 54797a0 into bitcoindevkit:master Jul 31, 2024
2 checks passed

thunderbiscuit deleted the feature/rustls-ring branch August 1, 2024 04:37

oleonardolima mentioned this pull request Aug 6, 2024

Allow users to pass their own custom rustls::crypto::CryptoProvider #140

Closed

notmandatory mentioned this pull request Aug 6, 2024

Bump version to 0.21.0 and update CHANGELOG.md #141

Merged

This was referenced Sep 8, 2024

Make ring the default again rustls/rustls#2115

Closed

Make ring the default again rustls/rustls#2116

Closed

Rustls w/ aws-lc-rs on Windows requires NASM rustls/rustls#1913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add use-rustls-ring feature #135

Add use-rustls-ring feature #135

thunderbiscuit commented Jun 26, 2024 •

edited

Loading

oleonardolima left a comment

oleonardolima left a comment

oleonardolima Jul 8, 2024 •

edited

Loading

thunderbiscuit Jul 27, 2024

LLFourn commented Jul 23, 2024

oleonardolima commented Jul 25, 2024

oleonardolima commented Jul 27, 2024

thunderbiscuit commented Jul 27, 2024

oleonardolima left a comment

LLFourn commented Jul 29, 2024

praveenperera Jul 29, 2024

thunderbiscuit Jul 31, 2024

praveenperera commented Jul 29, 2024 •

edited

Loading

oleonardolima commented Jul 29, 2024 •

edited

Loading

thunderbiscuit commented Jul 31, 2024

oleonardolima left a comment

notmandatory left a comment

	//! By default this library is compiled with support for SSL servers using [`rustls`](https://docs.rs/rustls) and support for
	//! plaintext connections over a socks proxy, useful for Onion servers. Using different features,
	//! the SSL implementation can be removed or replaced with [`openssl`](https://docs.rs/openssl).

		use-rustls = ["webpki-roots", "rustls/default"]
		use-rustls-ring = ["proxy", "webpki-roots", "rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]

	use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12"]
	use-rustls-ring = ["rustls/ring", "rustls/logging", "rustls/std", "rustls/tls12", "webpki-roots"]

Add use-rustls-ring feature #135

Add use-rustls-ring feature #135

Conversation

thunderbiscuit commented Jun 26, 2024 • edited Loading

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

thunderbiscuit Jul 27, 2024

Choose a reason for hiding this comment

LLFourn commented Jul 23, 2024

oleonardolima commented Jul 25, 2024

oleonardolima commented Jul 27, 2024

thunderbiscuit commented Jul 27, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

LLFourn commented Jul 29, 2024

praveenperera Jul 29, 2024

Choose a reason for hiding this comment

thunderbiscuit Jul 31, 2024

Choose a reason for hiding this comment

praveenperera commented Jul 29, 2024 • edited Loading

oleonardolima commented Jul 29, 2024 • edited Loading

thunderbiscuit commented Jul 31, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

thunderbiscuit commented Jun 26, 2024 •

edited

Loading

oleonardolima Jul 8, 2024 •

edited

Loading

praveenperera commented Jul 29, 2024 •

edited

Loading

oleonardolima commented Jul 29, 2024 •

edited

Loading