search

bitcoindevkit / rust-electrum-client

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

fix+refactor(tls): it should properly handle self-signed certificates, return a clear error otherwise #149

Closed oleonardolima closed 1 month ago

oleonardolima commented 2 months ago

Describe the enhancement

It has been noticed both in CI tests, which currently rely on blockstream's electrum client, and by other users in https://github.com/bitcoindevkit/bdk/issues/1598 and https://github.com/wizardsardine/liana/issues/1300 that the electrum-client does not properly connect to electrum servers with self-signed certificates while using the validate_domain: false settings, and neither returns a proper and clear regarding the problem. There is some issue when using the rustls crate that it fails for self-signed certificates, on other hand openssl works just fine.

Therefore, I'm creating this issue mainly for two purposes:

  1. Improve the documentation regarding the usage of validate_domain: false, when using either openssl and rustls with it's expected behavior.
  2. Improve the error handling and propagation, reporting proper TLS certificate validation errors to the user.
  3. Investigate and fix the inner issue with rustls custom certificate validation.

Use case

Allow users to properly use and connect electrum servers with self-signed certificates, either with `openssl` or `rustls`. **Additional context** https://github.com/rustls/rustls/issues/124 https://github.com/lightningnetwork/lnd/issues/5450 https://github.com/rigelminer/rigel/issues/130
notmandatory commented 2 months ago

Good research on this issue. Even if we only get 1 and 2 above done that's a good start. From what I can gather from the other issues you listed rustls is somehow more strict than openssl on how self signed certs need to be created.

jurvis commented 2 months ago

wanted to come here to 👍 this issue as well because we've had a few customers run into this.

oleonardolima commented 2 months ago

I think I've partially found a solution for the issue, at least to work with blockstream's electrum. We're using the custom implementation of rustls::client::danger::ServerCertVerifier trait in a weird way, using a better way as shown in their examples and documentation it works with blockstream's electrum.

However, it still does not work with the custom electrum server mentioned by pythcoiner in the issue, but it gives now a better error w.r.t to the certificates, but I think it's solvable. I'll open the candidate PR soon (after BitDevs).

oleonardolima commented 2 months ago

I think I've partially found a solution for the issue, at least to work with blockstream's electrum. We're using the custom implementation of rustls::client::danger::ServerCertVerifier trait in a weird way, using a better way as shown in their examples and documentation it works with blockstream's electrum.

However, it still does not work with the custom electrum server mentioned by pythcoiner in the issue, but it gives now a better error w.r.t to the certificates, but I think it's solvable. I'll open the candidate PR soon (after BitDevs).

If users want to use a self signed certificates they would need to use it with Version 3, which is not the case for the one from pythcoiner.