Switch to `vsss-rs` for share generation and reconstruction

r-n-o commented 2 weeks ago

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

[x] Add script to reshard preprod keys
[x] Test this in mono
[x] Successful boot standard in preprod
[x] 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!

cr-tk commented 2 weeks ago

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.

r-n-o commented 2 weeks ago

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

new dependency: thiserror: https://github.com/dtolnay/thiserror, version 2.0.2 and 1.0.69. This crate is tiny and widely used.
new dependency: vsss-rs: https://github.com/mikelodder7/vsss-rs-std, version 4.3.8. Written by our own cryptography consultant.
update: generic-array from 0.14.7 to 1.1.0. Tiny crate: https://github.com/fizyk20/generic-array
update: syn from 1.0.109 to 2.0.87 if we want to cover the widest gap. Bigger crate, but heavily used: https://github.com/dtolnay/syn

mark-nesbitt commented 2 weeks ago

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 commented 2 weeks ago

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

mark-nesbitt commented 2 weeks ago

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.

cr-tk commented 1 week ago

@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 :slightly_smiling_face:

tkhq / qos