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

Switch to vsss-rs for share generation and reconstruction #491

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

r-n-o
Copy link
Contributor

@r-n-o r-n-o commented Nov 11, 2024

Summary & Motivation (Problem vs. Solution)

This branch is a subset of the changes included in #457. It only deals with switching the shamir secret sharing library from a custom implementation to the open source crate vsss-rs: https://crates.io/crates/vsss-rs

The main limitation of the new crate is the lack of 1/1 support. This is actually a good thing because it'll force us to use a "true" shamir split in preprod. We're going to use a 2/2 setting.

How I Tested These Changes

Unit tests / lints only for now.

Pre merge check list

  • Add script to reshard preprod keys
  • Test this in mono
  • Successful boot standard in preprod
  • key forward in preprod -- was unsuccessful because of QOS manifest mismatch. Expected since we go from 1/1 to 2/2 with different public keys!

@r-n-o r-n-o requested a review from a team as a code owner November 11, 2024 21:40
@r-n-o r-n-o force-pushed the rno/switch-to-vsss-rs-crate branch 2 times, most recently from 0d31941 to c3f2260 Compare November 11, 2024 23:06
@r-n-o r-n-o force-pushed the rno/switch-to-vsss-rs-crate branch from ce446e5 to 569e2ce Compare November 12, 2024 00:08
@r-n-o r-n-o force-pushed the rno/switch-to-vsss-rs-crate branch from 8098a48 to faf77e1 Compare November 14, 2024 15:29
@cr-tk
Copy link
Collaborator

cr-tk commented Nov 14, 2024

Quick experimental review tool change summary on the dependencies:

"Cratename","Old Versions (all)","External Versions (some)","New Versions","Diff Review Options","Recommendation","Metadata1","Metadata2"
"thiserror-impl-no-std","-","-","2.0.2","2.0.2","review full","",""
"thiserror-no-std","-","-","2.0.2","2.0.2","review full","",""
"vsss-rs","-","-","4.3.8","4.3.8","review full","",""
""
"generic-array","0.14.7","0.14.7","1.1.0","0.14.7->1.1.0","review diff","",""
"syn","1.0.109 | 2.0.66 | 2.0.68 | 2.0.70","2.0.86","2.0.87","2.0.86->2.0.87","review diff","",""
"thiserror","1.0.61","1.0.66","1.0.69","1.0.66->1.0.69","review diff","",""
"thiserror-impl","1.0.61","1.0.66","1.0.69","1.0.66->1.0.69","review diff","",""
""
"keccak","-","0.1.5","0.1.5","0.1.5","trust external","",""
"sha3","-","0.10.8","0.10.8","0.10.8","trust external","",""
""
"base16ct","0.1.1","0.2.0","0.2.0","0.2.0","trust external","",""
"crypto-bigint","0.4.9","0.5.5","0.5.5","0.5.5","trust external","",""
"elliptic-curve","0.12.3","0.13.8","0.13.8","0.13.8","trust external","",""
"ff","0.12.1","0.13.0","0.13.0","0.13.0","trust external","",""
"group","0.12.1","0.13.0","0.13.0","0.13.0","trust external","",""
""
"generated diff between main@37da3ac714b0d5800f5cd04f644ea161aff1f5c2 and rno/switch-to-vsss-rs-crate@5b344df79f40f0dd42a09dd3a2ae00026e7e15f3 at 2024-11-14 17:54:31.440898+00:00"

I've previously looked at this via the old PR, and we're lucky in that half of the "new" changes are already covered via trust from upstream Rust packages that use it ("trust external"), so this should be doable to go over again next week once @r-n-o gives the go-ahead.

Copy link
Contributor

@jack-kearney jack-kearney left a comment

Choose a reason for hiding this comment

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

Incredible

@r-n-o
Copy link
Contributor Author

r-n-o commented Nov 15, 2024

Based on @cr-tk's message above we still need to review the following dependency updates:

@mark-nesbitt
Copy link

Notes:

skimmed this error, vsss-rs, and generic-array and noticed no obfuscated code. Release notes for generic-array showed mostly bug fixes.

syn diff was quite large (149 files, 1301 commits). There's definitely one author that makes a lot of changes, but there were several active committers. Not sure where this leaves us, tbh

@mark-nesbitt
Copy link

I will look through syn later today and come back with a +1 if all looks good

@mark-nesbitt
Copy link

Ok I skimmed the diff on syn. That was a lot of code -- the only thing I think it accomplished was the ability to notice large chunks of obfuscated compiled code... there was too much code for anything else to be accomplished within a reasonable amount of time.

That said, we have put the bare minimum of eyes on the dep.

@jack-kearney jack-kearney merged commit 12d4113 into main Nov 15, 2024
5 checks passed
@jack-kearney jack-kearney deleted the rno/switch-to-vsss-rs-crate branch November 15, 2024 22:04
@cr-tk
Copy link
Collaborator

cr-tk commented Nov 18, 2024

@r-n-o @mark-nesbitt for posterity, quick comment re

update: syn from 1.0.109 to 2.0.87 if we want to cover the widest gap.

Usually we would want to review the smallest gap. Since our code already uses (= we already trust) the four versions 1.0.109 | 2.0.66 | 2.0.68 | 2.0.70, heuristically the smallest gap between that and the new 2.0.87 would be to pick the biggest version number variant of the crate that we already use. In this specific case, one of the upstream Rust projects we trust already uses 2.0.86 however, so the gap is even smaller. The diff to review would only have been "2.0.86->2.0.87", as indicated in the relevant line of the tooling output 🙂

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