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

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

Open
oleonardolima opened this issue Sep 25, 2024 · 4 comments · May be fixed by #150
Labels
documentation Improvements or additions to documentation new feature New feature or request

Comments

@oleonardolima
Copy link
Collaborator

Describe the enhancement

It has been noticed both in CI tests, which currently rely on blockstream's electrum client, and by other users in bitcoindevkit/bdk#1598 and wizardsardine/liana#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

rustls/rustls#124
lightningnetwork/lnd#5450
rigelminer/rigel#130

@notmandatory
Copy link
Member

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
Copy link

jurvis commented Sep 25, 2024

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

@notmandatory notmandatory added documentation Improvements or additions to documentation new feature New feature or request labels Sep 25, 2024
@oleonardolima
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@oleonardolima oleonardolima linked a pull request Sep 27, 2024 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation new feature New feature or request
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants