mempool/rust-electrum-client
mempool/rust-electrum-client
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge bitcoindevkit/rust-electrum-client#150: fix(insecure-tls): NoCertificateVerification implementation

Browse Source 
05771a81d7 fix: `NoCertificateVerification` implementation (Leonardo Lima)

Pull request description:

  fixes #149 https://github.com/bitcoindevkit/bdk/issues/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, https://github.com/bitcoindevkit/bdk/issues/1598 and https://github.com/wizardsardine/liana/issues/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 https://github.com/bitcoindevkit/bdk/issues/1598 which uses X.509 certificates with any version other than 3 (it uses version 1), see [here](1a0d1646d0/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 05771a81d7
  ValuedMammal:
    ACK 05771a81d7
  notmandatory:
    ACK 05771a81d7

Tree-SHA512: f74dedf458853fb19cd21dedb5b92158acd865ee0ab0fd6bbb2b3e267bac22edc7cf004d2dc0068f66d2665d87e6dd419231710a02317e3b2bfaa9f408b30759
This commit is contained in:
valued mammal 2024-10-23 12:51:45 -04:00
commit 6fe96fddae
No known key found for this signature in database
1 changed files with 27 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
src/raw_client.rs
View File

@ -299,23 +299,29 @@ impl RawClient<ElectrumSslStream> {
))]
mod danger {
use crate::raw_client::ServerName;
use rustls::client::danger::ServerCertVerified;
use rustls::pki_types::CertificateDer;
use rustls::pki_types::UnixTime;
use rustls::Error;
use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified};
use rustls::crypto::CryptoProvider;
use rustls::pki_types::{CertificateDer, UnixTime};
use rustls::DigitallySignedStruct;
#[derive(Debug)]
pub struct NoCertificateVerification {}
pub struct NoCertificateVerification(CryptoProvider);
impl NoCertificateVerification {
pub fn new(provider: CryptoProvider) -> Self {
Self(provider)
}
}
impl rustls::client::danger::ServerCertVerifier for NoCertificateVerification {
fn verify_server_cert(
&self,
_end_entity: &CertificateDer,
_intermediates: &[CertificateDer],
_server_name: &ServerName,
_ocsp_response: &[u8],
_end_entity: &CertificateDer<'_>,
_intermediates: &[CertificateDer<'_>],
_server_name: &ServerName<'_>,
_ocsp: &[u8],
_now: UnixTime,
) -> Result<ServerCertVerified, Error> {
) -> Result<ServerCertVerified, rustls::Error> {
Ok(ServerCertVerified::assertion())
}
@ -323,22 +329,22 @@ mod danger {
&self,
_message: &[u8],
_cert: &CertificateDer<'_>,
_dss: &rustls::DigitallySignedStruct,
) -> Result<rustls::client::danger::HandshakeSignatureValid, Error> {
Ok(rustls::client::danger::HandshakeSignatureValid::assertion())
_dss: &DigitallySignedStruct,
) -> Result<HandshakeSignatureValid, rustls::Error> {
Ok(HandshakeSignatureValid::assertion())
}
fn verify_tls13_signature(
&self,
_message: &[u8],
_cert: &CertificateDer<'_>,
_dss: &rustls::DigitallySignedStruct,
) -> Result<rustls::client::danger::HandshakeSignatureValid, Error> {
Ok(rustls::client::danger::HandshakeSignatureValid::assertion())
_dss: &DigitallySignedStruct,
) -> Result<HandshakeSignatureValid, rustls::Error> {
Ok(HandshakeSignatureValid::assertion())
}
fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> {
vec![]
self.0.signature_verification_algorithms.supported_schemes()
}
}
}
@ -420,7 +426,10 @@ impl RawClient<ElectrumSslStream> {
builder
.dangerous()
.with_custom_certificate_verifier(std::sync::Arc::new(
danger::NoCertificateVerification {},
#[cfg(feature = "use-rustls")]
danger::NoCertificateVerification::new(rustls::crypto::aws_lc_rs::default_provider()),
#[cfg(feature = "use-rustls-ring")]
danger::NoCertificateVerification::new(rustls::crypto::ring::default_provider()),
))
.with_no_client_auth()
};