search

bitcoindevkit / rust-electrum-client

Bitcoin Electrum client library. Supports plaintext, TLS and Onion servers.
MIT License
80 stars 62 forks source link

fix(insecure-tls): `NoCertificateVerification` implementation #150

Closed oleonardolima closed 1 month ago

oleonardolima commented 2 months ago

fixes #149 https://github.com/bitcoindevkit/bdk/issues/1598

Description

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.

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 ?

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

oleonardolima commented 2 months ago

I also think we should update validate_domain to something like insecure instead, but would like to know what others think about it.

LLFourn commented 2 months ago

I didn't really understand the nuance here but we should validate domains by default and then have an opt out I think.

oleonardolima commented 1 month ago

I didn't really understand the nuance here but we should validate domains by default and then have an opt out I think.

In the previous state, when using rustls for TLS in an insecure manner (explicitly called/opted out by the user through the validate_domain configuration) it wasn't properly setting up the implementation for ServerCertVerifier, more specifically on the validation for the supported schemes.

This looks to be an improvement over what was there before and it fixes an issue that prevented connecting to an electrum server with a self-signed cert using rustls. I tested setting validate_domain = false and was able to get a response from each of blockstream's esplora, the server at "ssl://testnet.aranguren.org:51002" (fulcrum), and an electrs instance on my local network.

Ideally, we SHOULD properly implement the verification on both verify_tls12_signature and verify_tls12_signature, it should still work fine with self-signed certificates, in an insecure manner, but that doesn't work with all self-signed certificates, such as ssl://testnet.aranguren.org:51002, as this one and some other uses a certificate version other than 3.

Do you think it's fine to have this tradeoff? And, any thoughts on changing the parameter to insecure too?

LLFourn commented 1 month ago

ACK 05771a81d7d0383a089fc20f1e1e228202709f01

I think we should change how this works soon but ACK this for now. People should not be using validate_domain = false except for testnet. We should have an API to add certificates. Better yet add a TOFU API to give you a certificate to persist when you connect to a server for the first time. I'm not sure we should even use webpki here. There aren't that many public electrum servers. We can keep a list updated in the client itself.