Skip to content

fix: Update reqwest and disable OpenSSL to address CVEs#254

Merged
jbencin merged 1 commit intohirosystems:masterfrom
jbencin:fix/update-reqwest
Apr 18, 2023
Merged

fix: Update reqwest and disable OpenSSL to address CVEs#254
jbencin merged 1 commit intohirosystems:masterfrom
jbencin:fix/update-reqwest

Conversation

@jbencin
Copy link
Copy Markdown
Contributor

@jbencin jbencin commented Apr 11, 2023

Description

Update reqwest dependency and remove OpenSSL support

Applicable issues

  • fixes a few CVEs found by GitHub's automated scanner

Additional info (benefits, drawbacks, caveats)

There are a few reasons to prefer Rustls over OpenSSL:

  • Avoid security risks associated with using FFI and interfacing with C code
  • Simplifies builds (especially cross-compiles) by not requiring external libraries
  • Has been formally audited by Cure53 (see here)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 11, 2023

Codecov Report

Merging #254 (80cdf0e) into master (a762f8f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #254   +/-   ##
=======================================
  Coverage   93.47%   93.47%           
=======================================
  Files           6        6           
  Lines         337      337           
=======================================
  Hits          315      315           
  Misses         22       22           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jbencin jbencin marked this pull request as ready for review April 11, 2023 20:04
@jbencin jbencin requested a review from fpbgg April 18, 2023 15:48
Copy link
Copy Markdown
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Looks good to me. Have you tried spinning up a devnet with this change to make sure everything still works?

@jbencin
Copy link
Copy Markdown
Contributor Author

jbencin commented Apr 18, 2023

Have you tried spinning up a devnet with this change to make sure everything still works?

I can do that. Do we actually use TLS anywhere in the node and do you know of any way to specifically test it? Otherwise I'll just start it up with clarinet integrate and see what happens

@obycode
Copy link
Copy Markdown
Contributor

obycode commented Apr 18, 2023

Do we actually use TLS anywhere in the node and do you know of any way to specifically test it?

I'm not sure, maybe @kantai can help answer that. I think using clarinet integrate would be a good test regardless.

@kantai
Copy link
Copy Markdown
Contributor

kantai commented Apr 18, 2023

Yep -- I think the only place that TLS is used in the node is (optionally) when making L1 RPC requests. This is configured with the burnchain.rpc_ssl config option, which would allow the L1 RPC to use, e.g., https://stacks-node-api.testnet.stacks.co/v2/info

I think just testing this out with clarinet integrate should be sufficient, though -- I am pretty confident these changes are not going to cause any issues.

@jbencin
Copy link
Copy Markdown
Contributor Author

jbencin commented Apr 18, 2023

Just tested everything with clarinet integrate, everything looks good and both v1 and v2 endpoints are reachable. I'm not sure if any of this is testing TLS, but it at least confirms that the update to reqwest didn't break regular HTTP traffic.

If anyone else wants to try, it's the fix-update-reqwest tag on DockerHub. I'm going to merge now...

@jbencin jbencin merged commit b0c93bc into hirosystems:master Apr 18, 2023
@jbencin jbencin deleted the fix/update-reqwest branch April 18, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants