Commit 6fe96fd
committed
Merge #150: fix(insecure-tls):
05771a8 fix: `NoCertificateVerification` implementation (Leonardo Lima)
Pull request description:
fixes #149 bitcoindevkit/bdk#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, 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](https://github.com/rustls/webpki/blob/1a0d1646d0bb1b7851bf81c6244cab09c352d8ef/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 05771a8
ValuedMammal:
ACK 05771a8
notmandatory:
ACK 05771a8
Tree-SHA512: f74dedf458853fb19cd21dedb5b92158acd865ee0ab0fd6bbb2b3e267bac22edc7cf004d2dc0068f66d2665d87e6dd419231710a02317e3b2bfaa9f408b30759NoCertificateVerification implementation1 file changed
+27
-18
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
299 | 299 | | |
300 | 300 | | |
301 | 301 | | |
302 | | - | |
303 | | - | |
304 | | - | |
305 | | - | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
306 | 306 | | |
307 | 307 | | |
308 | | - | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
309 | 315 | | |
310 | 316 | | |
311 | 317 | | |
312 | 318 | | |
313 | | - | |
314 | | - | |
315 | | - | |
316 | | - | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
317 | 323 | | |
318 | | - | |
| 324 | + | |
319 | 325 | | |
320 | 326 | | |
321 | 327 | | |
322 | 328 | | |
323 | 329 | | |
324 | 330 | | |
325 | 331 | | |
326 | | - | |
327 | | - | |
328 | | - | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
329 | 335 | | |
330 | 336 | | |
331 | 337 | | |
332 | 338 | | |
333 | 339 | | |
334 | 340 | | |
335 | | - | |
336 | | - | |
337 | | - | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
338 | 344 | | |
339 | 345 | | |
340 | 346 | | |
341 | | - | |
| 347 | + | |
342 | 348 | | |
343 | 349 | | |
344 | 350 | | |
| |||
420 | 426 | | |
421 | 427 | | |
422 | 428 | | |
423 | | - | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
424 | 433 | | |
425 | 434 | | |
426 | 435 | | |
| |||
0 commit comments