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

Update cryptography dependencies #1345

Merged
merged 15 commits into from
Mar 9, 2022
Merged

Update cryptography dependencies #1345

merged 15 commits into from
Mar 9, 2022

Conversation

remoun
Copy link
Contributor

@remoun remoun commented Jan 25, 2022 •
edited
Loading

Motivation

There are some breaking changes in digest, which have cascaded to other crates so we have to update all the crates that depend on it transitively.

Builds on:
mobilecoinfoundation/KDFs#1
mobilecoinfoundation/bulletproofs#2
mobilecoinfoundation/curve25519-dalek#2
mobilecoinfoundation/ed25519-dalek#2
mobilecoinfoundation/x25519-dalek#2
mobilecoinfoundation/schnorrkel#2

In this PR

  • Update the forked versions we use with their respective dependency updates.
  • Some mechanical updates around breaking changes.

Fixes #1089.

Future Work

  • Update other dependencies to latest.
  • Dedupe deps.

@remoun remoun requested review from jcape and cbeck88 January 25, 2022 07:00
@remoun remoun mentioned this pull request Jan 25, 2022
Closed
cbeck88
cbeck88 reviewed Jan 25, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
cbeck88
cbeck88 reviewed Jan 25, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
cbeck88
cbeck88 suggested changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

I did not review the changes in forks so in that sense my review is not complete. The code changes here all looked fine, just the changes in cargo toml looked problematic

  • Don't use personal forks
  • Pin to revisions not branches or else repeatable builds are undermined

jcape
jcape suggested changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

Mostly looks good, I agree with Chris' comments as well, and there are a couple show-stoppers that have crept in.

Cargo.lock Outdated Show resolved Hide resolved
Cargo.lock Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
attest/ake/Cargo.toml Outdated Show resolved Hide resolved
crypto/ake/enclave/Cargo.toml Outdated Show resolved Hide resolved
peers/test-utils/src/lib.rs Outdated Show resolved Hide resolved
transaction/core/Cargo.toml Outdated Show resolved Hide resolved
transaction/core/src/membership_proofs/mod.rs Show resolved Hide resolved
transaction/core/src/tx.rs Outdated Show resolved Hide resolved
transaction/std/Cargo.toml Outdated Show resolved Hide resolved
@remoun remoun requested review from cbeck88 and jcape January 25, 2022 23:19
jcape
jcape reviewed Jan 27, 2022
View reviewed changes
Copy link
Contributor

@jcape jcape 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, will give formal approval once the other PRs have been merged and we've branched for 1.2.

@jcape jcape self-requested a review January 27, 2022 00:24
@remoun remoun mentioned this pull request Jan 27, 2022
Open
cbeck88
cbeck88 approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

approved based on reading all the dependent PRs

This was referenced Jan 28, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Feb 28, 2022
Closed
Closed
Remoun Metyas and others added 13 commits February 28, 2022 18:08
Clean up usage of PhantomData
5c35409
@dependabot
Bump blake2 from 0.9.2 to 0.10.2
1b126bd 
Bumps [blake2](https://github.com/RustCrypto/hashes) from 0.9.2 to 0.10.2.
- [Release notes](https://github.com/RustCrypto/hashes/releases)
- [Commits](RustCrypto/hashes@blake2-v0.9.2...blake2-v0.10.2)

---
updated-dependencies:
- dependency-name: blake2
  dependency-type: direct:production
  update-type: version-update:semver-minor
@remoun
Update {curve,ed,x}25519 dependencies.
06953de
@remoun
Regroup mods and use canonical pub use style.
c034900
Update simpler Digest references.
5e7c0ef
@remoun
Update hkdf, hmac, sha2, sha3 and signature
4f5dfe0
@remoun
Update schnorrkel dependency.
b1dc0ab
@remoun
Update bulletproofs dependency
360abc0
Pin ed25519 version to fix a "failed to select a version for the requ…
bbd6de2 
…irement" error
Remove a couple of unnecessary dependencies on ed25519
0b7f5dd
Generalize NoiseCipher impl to any type implementing AeadMute+NewAead…
9a048e8 
…+Sized, and introduce a similar NoiseDigest type
Replace some direct deps on blake2 with mc_crypto_hashes
311979f
Remove some unnecessary target.cfg blocks
782ffcd
@remoun remoun requested review from cbeck88 and a team March 2, 2022 01:13
cbeck88
cbeck88 previously approved these changes Mar 2, 2022
View reviewed changes
jcape
jcape suggested changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

This mostly LGTM, the main thing is making sure we're explictly requiring hkdf = "0.12.3", because we need at least 0.12.1, but there were various bugs fixed going into 0.12.3 that we need.

connection/Cargo.toml Outdated Show resolved Hide resolved
crypto/box/Cargo.toml Outdated Show resolved Hide resolved
account-keys/Cargo.toml Outdated Show resolved Hide resolved
crypto/noise/Cargo.toml Outdated Show resolved Hide resolved
transaction/core/Cargo.toml Show resolved Hide resolved
@remoun remoun requested a review from jcape March 8, 2022 23:31
@jcape jcape requested a review from a team March 9, 2022 00:12
@remoun remoun requested a review from cbeck88 March 9, 2022 01:46
cbeck88
cbeck88 approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

LGTM thank you

@remoun remoun merged commit 94651ea into mobilecoinfoundation:master Mar 9, 2022
@remoun remoun deleted the chore-update-crypto-deps branch March 9, 2022 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcape jcape jcape approved these changes

@cbeck88 cbeck88 cbeck88 approved these changes

Assignees

@remoun remoun

Labels
dependencies Pull requests that update a dependency file
Projects
Blockchain Core
Archived in project
Milestone
(2022-Q1) Reduce supply chain attack ...
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@remoun @jcape @cbeck88