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.

Sign up for GitHub

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

Jump to bottom

Upgrade rustls to 0.23 #132

Merged
merged 1 commit into from
May 27, 2024
Merged

Upgrade rustls to 0.23 #132

merged 1 commit into from
May 27, 2024

Conversation

nickfarrow
Copy link
Contributor

@nickfarrow nickfarrow commented Apr 17, 2024 •
edited
Loading

With rustls 0.23 there is no longer a dependency on ring, allowing easier compilation for various targets.

Not super confident with my updates to ServerCertVerifier and Der of certificates (is this being tested?), needs review.

@nickfarrow nickfarrow mentioned this pull request Apr 17, 2024
Merged
@nickfarrow nickfarrow marked this pull request as ready for review April 18, 2024 04:41
@RCasatta
Copy link
Member

Version 0.21 has also other issues...

Crate:     rustls
Version:   0.21.7
Title:     `rustls::ConnectionCommon::complete_io` could fall into an infinite loop based on network input
Date:      2024-04-19
ID:        RUSTSEC-2024-0336
URL:       https://rustsec.org/advisories/RUSTSEC-2024-0336
Severity:  7.5 (high)
Solution:  Upgrade to >=0.23.5 OR >=0.22.4, <0.23.0 OR >=0.21.11, <0.22.0

RCasatta
RCasatta reviewed Apr 22, 2024
View reviewed changes
src/raw_client.rs Outdated
@@ -21,13 +21,12 @@ use bitcoin::{Script, Txid};

#[cfg(feature = "use-openssl")]
use openssl::ssl::{SslConnector, SslMethod, SslStream, SslVerifyMode};
use rustls::pki_types::{Der, TrustAnchor};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen from failing CI jobs, this must be gated

@nickfarrow
upgrade rustls to 0.23
28b1aaa 
* With rustls 0.23 there is no longer a dependency on ring, allowing for
  easier compilation for various targets.
@oleonardolima oleonardolima mentioned this pull request May 22, 2024
Merged
3 tasks
notmandatory
notmandatory approved these changes May 27, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 28b1aaa

@notmandatory notmandatory merged commit 1bbae7d into bitcoindevkit:master May 27, 2024
1 check passed
@nickfarrow
Copy link
Contributor Author

Apologies for the substantial mess this caused!

When making this PR it allowed some android target to build where I was having substantial trouble with ring. It has broken many other targets however and this was not readily apparent.

I think in the future I should take more time to test across other targets?

Big thanks for your work resolving this @thunderbiscuit

@thunderbiscuit
Copy link
Member

Yeah ring has caused me problems in the past too with the Android NDK, so I totally understand.

I don't think we could have predicted this, and certainly testing with all possible architecture would be insane. I'm puzzled by the decision in rustls to straight replace the default provider for something that doesn't build everywhere, but then again their docs (aws-lc-rs and co.) are very good and should in theory allow us to build for all platforms; it's just been harder than advertised IMO 😆.

@notmandatory notmandatory mentioned this pull request Aug 6, 2024
Merged
notmandatory added a commit that referenced this pull request Aug 6, 2024
@notmandatory
Merge #141: Bump version to 0.21.0 and update CHANGELOG.md
0b97659 
4dd7e21 Bump version to 0.21.0 and update CHANGELOG.md (Steve Myers)

Pull request description:

  Bumped crate version to 0.21.0 and added below to changelog:

  ## 0.21.0

   - Add use-rustls-ring feature #135
   - refactor: make validate_merkle_proof more efficient #134
   - chore: set rust edition to 2021, fix clippy, add ci fmt and clippy checks #139

  ## 0.20.0

  - Upgrade rustls to 0.23 #132
  - chore(deps): upgrade rust-bitcoin to 0.32.0 #133
  - ci: add test with MSRV 1.63.0 #128

ACKs for top commit:
  oleonardolima:
    ACK 4dd7e21
  ValuedMammal:
    ACK 4dd7e21

Tree-SHA512: 3fcec2fb437733eac235bccb1b9c8f6b706e7a713c71de85016adc93f7db128ca6eadb5e9d1d44df27f1b49cce139b222aa9c21343afcf25befdf80a47442e51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCasatta RCasatta RCasatta left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nickfarrow @RCasatta @thunderbiscuit @notmandatory