Skip to content
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

build(esplora): Add async-https-rustls flag to esplora client #1179

Merged

Conversation

thunderbiscuit
Copy link
Contributor

Description

The bdk_esplora crate currently doesn't expose the async-https-rustls flag offered by the rust-esplora-client crate and instead requires users to build using the default-tls flag on reqwest, which uses the platform-specific openssl library when compiling. This creates complications for cross-compilation, notably for our Android builds that currently support 3 architectures (arm64-v8a, armeabi-v7a, and x86_64). In order to solve this we can either compile the openssl libraries for each of the platforms we want to support, or use the rustls-tls version of reqwest. The second options is much easier and requires less fiddling with the internals of the Android native development kit and cross-compilation rabbit holes.

Before we merge this I want to make sure I understand the tradeoffs between the native-tls and the rustls-tls and confirm that there are not potential issues there, but from what I understand they should provide the same functionality/security, and because these are already available/exposed in reqwest and rust-esplora-client, I think this should be a fairly straightforward additional feature we offer.

Changelog notice

Added:
  - New async-https-rustls feature flag for the bdk_esplora crate, allowing to compile rust-esplora-client using rustls-tls instead of the default native-tls.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Copy link
Contributor

@realeinherjar realeinherjar left a comment

Choose a reason for hiding this comment

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

ACK 7330c7f

@realeinherjar
Copy link
Contributor

realeinherjar commented Oct 16, 2023

Also in my up-to-date CargoMSRV.lock in #1165 I had to use rustls versions 0.21.1 and 0.20.8.
I see that MSRV CI is failing.

@tnull
Copy link
Contributor

tnull commented Oct 17, 2023

Before we merge this I want to make sure I understand the tradeoffs between the native-tls and the rustls-tls and confirm that there are not potential issues there, but from what I understand they should provide the same functionality/security, and because these are already available/exposed in reqwest and rust-esplora-client, I think this should be a fairly straightforward additional feature we offer.

Really interested to hear about your findings. I think generally using the platform's TLS implementation (i.e., native-tls) would be preferable as it would allow users to receive critical security fixes via OS upgrades, instead of having to wait and eventually upgrade once rustls, esplora-client, and BDK ship new versions/builds. It would also be preferable from an MSRV perspective as shipping newer rustls versions might conflict with the MSRV requirements, potentially leaving the choice between not shipping fixes and breaking MSRV.

I however heard before that making native-tls work on Android isn't as trivial as rustls, however, I haven't looked too closely into it yet. So, again, would be really interested to hear about your learnings.

@nondiremanuel
Copy link

Lib Team Call comment: useful for android binding, possibly to be included in the next bdk_esplora release. Since it should be quick adding a flag, we added to alpha.3

@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0-alpha.3 Nov 20, 2023
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 6817ca9

Copy link
Contributor

@realeinherjar realeinherjar left a comment

Choose a reason for hiding this comment

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

ACK 6817ca9

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

I am not yet sure if it's better to use rustls-tls or native-tls, however, I think it's important to offer the choice to our users (since, as you said, we already offer it in rust-esplora-client).

I found this post on the rust forum: https://users.rust-lang.org/t/any-reasons-to-prefer-native-tls-over-rustls/37626/6. It seems that the only drawback is what @tnull said, about not getting security patches shipped with the OS, and possible MSRV issues. Someone mentioned that rustls wasn't audited, but that changed in Jun 2020.

ACK 6817ca9

@notmandatory notmandatory merged commit 959b4f8 into bitcoindevkit:master Dec 5, 2023
12 checks passed
@notmandatory notmandatory mentioned this pull request Jan 3, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants