electrum_client: disrepancies w/ certificate verification between use-rustls & use-openssl
#1598
Labels
bug
Something isn't working
Milestone
Comments
|
Thanks for debugging and collecting the related info. Although I didn't delve into this yet and reproduced the problem, I think it's somewhat related to a problem I faced in the past with LND, reference: lightningnetwork/lnd#5450. It requires further investigation, but if it's not a problem downstream with how There's also the issue that we don't expose the @notmandatory @thunderbiscuit, have there been any previous discussions on why it's not exposed ? |
looks have the same root cause
Yes, it looks to me the intended logic is to NOT check the certificate if but ends up not as intended for rustls
right this make it uneasy to fallback to openssl
|
Merged
8 tasks
notmandatory
added a commit
that referenced
this issue
Oct 2, 2024
f602d1b feat(bdk_electrum): add `use-openssl` as a feature (Leonardo Lima) Pull request description: partially addresses #1598 <!-- You can erase any parts of this template not applicable to your Pull Request. --> ### Description It's a simple PR to expose the `use-openssl` from `electrum-client` to `bdk_electrum`. It partially addresses #1598, allowing the user to use `openssl` as an alternative to `rustls`, as there are some problems with it when handling some types of TLS certificates. Do we need to add some sort of `bdk_electrum` tests using the new exposed `use-openssl` feature ? <!-- Describe the purpose of this PR, what's being adding and/or fixed --> ### Notes to the reviewers <!-- 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 - Adds `use-openssl` as feature to `bdk_electrum`, exposing `electrum-client` `use-openssl` feature. <!-- 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 Top commit has no ACKs. Tree-SHA512: 86e0fdeaa0b6a48e0c7ddde6e3890ce86510b86ad727adf05cde5e8c311a8c6b7614ae57bae0cff9e7b8443478a324e5ce9e5180023d501453a5c1e9e45920e1
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
trying to connect to an electrum server w/ a self-signed certificate using
use-rustlsfails but succeed w/use-opensslTo Reproduce
code snippet:
Expected behavior
should succeed w/ both features,
use-rusttlsfails w/:trying w/ a local electrs + nginx + self-sign certificate i got this in nginx error log w/
use-rustls:and this w/
use-openssl:Build environment
The text was updated successfully, but these errors were encountered: