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

electrum: fail to connect to a server w/ a self-signed certificate #1300

Open
pythcoiner opened this issue Sep 9, 2024 · 5 comments · May be fixed by #1500
Open

electrum: fail to connect to a server w/ a self-signed certificate #1300

pythcoiner opened this issue Sep 9, 2024 · 5 comments · May be fixed by #1500
Assignees
Labels

Comments

@pythcoiner
Copy link
Collaborator

pythcoiner commented Sep 9, 2024

we actually fails to connect (in ssl) to an electrum server that have a self-signed certificate, in order to allow it we should have a look to few points:

  • add an option to installer/settings to allow user to disable the certificate check, we should keep the default behaviour to check the certificate.
  • certificate check should be disabled if we set electrum_client::Config validate_domain to false but not works as intended if electrum_client is used w/ default use-rusttls feauture. It works fine w/ use-openssl feature btw.
    ( i've opened an issue about this disrepancy in bdk repo)
  • bdk_electrum crate does not offer use-openssl feature:
    Updating crates.io index
      Adding bdk_electrum v0.17.0 to dependencies
             Features:
             + use-rustls
             - use-rustls-ring

bdk_electrum Cargo.toml:

[dependencies]
bdk_core = { path = "../core", version = "0.1" }
electrum-client = { version = "0.21", features = [ "proxy" ], default-features = false }

[dev-dependencies]
bdk_testenv = { path = "../testenv", default-features = false }
bdk_chain = { path = "../chain", version = "0.18.0" }

[features]
default = ["use-rustls"]
use-rustls = ["electrum-client/use-rustls"]
use-rustls-ring = ["electrum-client/use-rustls-ring"]
@pythcoiner pythcoiner changed the title electrum: fail to connect to a server w/ a self sign certificate electrum: fail to connect to a server w/ a self-signed certificate Sep 9, 2024
@nondiremanuel nondiremanuel moved this to Todo in Liana General Sep 9, 2024
@nondiremanuel nondiremanuel added this to the v8 - Liana milestone Sep 16, 2024
@nondiremanuel
Copy link
Collaborator

As per what we were saying in the chat, we should be able to do it after the bump of BDK version (https://github.com/wizardsardine/liana-backend/pull/233).

Let's also remember to edit back the error message we are going to insert with #1342

@pythcoiner
Copy link
Collaborator Author

i'm really not sure upgrading bdk solve our issue, the issue seems related to rust-tls, and the bdk client does not reexport the ssl feature that seems the only actual way to make it work w/ self-signed certificate

@evanlinjin
Copy link

evanlinjin commented Sep 20, 2024

You don't need bdk_electrum to reexport features. You can just add another dependency to liana's Cargo.toml file.

electrum-client = { version = "0.21.0", features = ["use-openssl", "proxy"] }

@oleonardolima
Copy link

@pythcoiner I have a candidate fix in bitcoindevkit/rust-electrum-client#150 in case you'd like to take a look.

@pythcoiner
Copy link
Collaborator Author

i'll review it

ValuedMammal added a commit to bitcoindevkit/rust-electrum-client that referenced this issue Oct 23, 2024
…tion

05771a8 fix: `NoCertificateVerification` implementation (Leonardo Lima)

Pull request description:

  fixes #149 bitcoindevkit/bdk#1598
  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  It has been noticed some issues by both users and developers, as reported in #149, bitcoindevkit/bdk#1598 and wizardsardine/liana#1300, when using the library with `use-rustls-{ring}` feature to connect to electrum servers that use self-signed certificates, there are even some issues when trying to connect to `ssl://electrum.blockstream.info:50002` server.

  To connect in an insecure manner either with `rustls` or `openssl` features, the user can set the `validate_domain` field in the `Config` to false, this will either set the `SslVerifyMode::NONE` when using `openssl`, or use the custom `NoCertificateVerification` for the
  `rustls::client::danger::ServerCertVerifier` trait when using `rustls`, that said it should ignore the certificate verification when used.

  At the current library state, it's failing because we didn't set up the supported `rustls::SignatureScheme` properly, returning an empty vector at the moment. This PR focuses on fixing this issue by relying on the `CryptoProvider` in usage to get the correct and supported signature schemes.

  As part of the research to understand the problem, I've noticed that ideally, we should still use both the `rustls::webpki::verify_tls12_signature` and `rustls::webpki::verify_tls12_signature` and only rely on `rustls::client::danger::ServerCertVerified::assertion()` to ignore the certificate verification, however, it would still fail in scenarios such as bitcoindevkit/bdk#1598 which uses X.509 certificates with any version other than 3 (it uses version 1), see [here](https://github.com/rustls/webpki/blob/1a0d1646d0bb1b7851bf81c6244cab09c352d8ef/src/cert.rs#L202-L218).

  I kept the current behavior to also ignore the TLS signature, but I still would like to bring this to the discussion, should we validate it properly and update the documentation to mention the `webpki` limitation instead ?

  ### Notes to the reviewers

  I kept the current behavior to also ignore the TLS signature, but I still would like to bring this to the discussion, should we validate it properly and update the documentation to mention the `webpki` limitation instead ?

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice

  - Updates the `NoCertificateVerification` implementation for the
  `rustls::client::danger::ServerCertVerifier` to use the `rustls::SignatureScheme` from `CryptoProvider` in use.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

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

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  LLFourn:
    ACK 05771a8
  ValuedMammal:
    ACK 05771a8
  notmandatory:
    ACK 05771a8

Tree-SHA512: f74dedf458853fb19cd21dedb5b92158acd865ee0ab0fd6bbb2b3e267bac22edc7cf004d2dc0068f66d2665d87e6dd419231710a02317e3b2bfaa9f408b30759
@nondiremanuel nondiremanuel moved this from Todo to In Review in Liana General Dec 16, 2024
@nondiremanuel nondiremanuel moved this from In Review to In Progress in Liana General Dec 16, 2024
@pythcoiner pythcoiner moved this from In Progress to In Review in Liana General Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

4 participants