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(insecure-tls): NoCertificateVerification implementation #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleonardolima
Copy link
Collaborator

@oleonardolima oleonardolima commented Sep 27, 2024

fixes #149 bitcoindevkit/bdk#1598

Description

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.

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

  • Updates the NoCertificateVerification implementation for the
    rustls::client::danger::ServerCertVerifier to use the rustls::SignatureScheme from CryptoProvider in use.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • 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

@oleonardolima oleonardolima force-pushed the fix/handling-insecure-certificates branch 2 times, most recently from 17a4152 to a445252 Compare September 27, 2024 17:11
@oleonardolima oleonardolima changed the title fix!: NoCertificateVerification implementation fix(insecure-tls): NoCertificateVerification implementation Sep 27, 2024
@oleonardolima
Copy link
Collaborator Author

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

@oleonardolima oleonardolima self-assigned this Sep 27, 2024
@oleonardolima oleonardolima added the bug Something isn't working label Sep 27, 2024
@oleonardolima oleonardolima marked this pull request as ready for review September 27, 2024 17:35
It updates the `NoCertificateVerification` implementation of
`rustls::client::danger::ServerCertVerifier` trait, it keeps the usage
of both `ServerCertVerified::assertion()` and
`HandshakeSignatureValid::assertion()` usage, but now instead of having
an empty vector vector of supported `SignatureScheme`, it uses the ones
supported by the used `CryptoProvider`.
@LLFourn
Copy link

LLFourn commented Sep 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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