Use vsss-rs for share generation and reconstruction #457

emostov · 2024-06-18T17:39:40Z

Summary & Motivation (Problem vs. Solution)

Closes ENG-1687

Note, there is some extra diff in here from running a newer version of clippy. I can work on reverting if this causes issues

How I Tested These Changes

Pre merge check list

Update CHANGELOG.MD

wip wip get to compile Initial reshard provision and unit test code clean up fix n choose k refactor n choose k Finish tests for reshard add test for boot reshard Add generate reshard input wip wip wip wip Get reshard-renencrypt working Get post share working lint wip Build get reshard output Add new secrets for file key get full thing working e2e refactor wip get qos core to compile Get all of qos core working refactor to not use quorumpubkey wrapper wip Get e2e tests working with new input Add logic for checking that e2e share recombination works finish integration test lint stuff Improve human verifiactions clean up

lthibault · 2024-09-13T16:03:04Z

Upgraded to vsss-rs v4.3.8 64ce106

lthibault · 2024-09-16T16:01:54Z

Per discussion in Google Meet, I am removing the legacy (i.e. unfairly-named "insecure") code. This removes backwards compatibility for 1-of-1 shards, and will require us to reshard dev/preprod keys. Prod keys have already been resharded.

cr-tk · 2024-09-24T13:47:13Z

@lthibault some comments and questions.

The removal of the legacy code paths has the effect that this PR cannot land in actively-used contexts of dev/preprod until the resharding there has been done, right?

The reason we've labeled the legacy code functions with insecure in their name is that they have several implementation problems, including likely security side channel problems during generation, which can be a problem depending on their usage. Given that we now have a better alternative that avoids those problems, we wanted to make it clear to developers that the use of the old functions is discouraged. More context: ENG-1720, ENG-1384.

In c264d9c , you've changed the unit tests by directly replacing the use of the legacy calls with the modern alternative. Unfortunately, that doesn't really make sense here. If you look at the unit test structure, the modern functions are already called directly one line above - the old_* variants which called the legacy functions were included in combination with assert_eq!() to ensure the results between old and new are identical and therefore consistent.
Instead, I suggest removing the variable definitions and asserts with the old_* lines since they're redundant now.

…res.

lthibault · 2024-09-24T20:51:54Z

The removal of the legacy code paths has the effect that this PR cannot land in actively-used contexts of dev/preprod until the resharding there has been done, right?

Correct. I currently have a resharded dev key (2-of-2), which is now supported as of d87496a.
Resharding remains to be performed in preprod and prod environments.

[...] Unfortunately, that doesn't really make sense here.

Oh nice catch. I must have been on autopilot.

[...] Instead, I suggest removing the variable definitions and asserts with the old_* lines since they're redundant now.

Done: d6db3e7

If there's nothing else (and CI passes), I intend to proceed with resharding preprod.

cr-tk · 2024-10-30T14:33:02Z

@lthibault what's the status on this PR?

From what I can see, https://github.com/tkhq/mono/pull/3323 exists as a springboard to do testing on preprod, but isn't actually meant to be merged - instead, this PR (#457) would need to land in QOS main, and then mono could bump the QOS submodule.

https://github.com/tkhq/namespaces/pull/65 paves the road for mono to actually work with this new PR state that no longer handles 1-of-1 SSS situations, and that's out of the way now.

@Ulexus 04b0c11 was a necessary Cargo configuration fix to make this work, right? What about 5ba801e , can/should this stay in as-is, given the DANGER commit warning?

https://github.com/tkhq/mono/pull/3397 seems to be related since it aims to un-break the qos handling, but there's probably a followup there that needs some attention.
Given what I've found on background information, this flag directly propagates to the aws cli crate, where this code is responsible for it.

/// Connects to the enclave console and prints it continuously.

Can you outline where the danger in this comes from? The host learning sensitive information via printed output on the console that is normally inaccessible to the host, for example, if the debug_mode is left on accidentally or maliciously in production?

PR 3397 in mono seems to related to un-break QOS via a stagex/Linux kernel relevant revert, but there this followup probably still needs some attention.

r-n-o

Posting my partial review from months ago! I'll be taking over this PR; my strategy is going to be: let's break it up into multiple chunks. Some updates made here are very minor cleanup-style items (README updates, stale files); others are very critical (actual vsss-rs libarary).

r-n-o · 2024-09-16T14:38:52Z

CHANGELOG.MD

+- BREAKING CHANGE: qos_core: quorum key resharding service, new state machine transitions, and new `ProtocolMsg` variants (#428)
+- qos_client: commands to run quorum key resharding and high level documentation (#428)


428 is a PR that seems closed. Why this mentioned in the context of this PR which is about VSSS?

r-n-o · 2024-09-16T14:41:12Z

src/images/common/Containerfile

@@ -50,4 +50,4 @@ COPY --from=file . /
 COPY --from=gcc . /
 COPY --from=linux-nitro /bzImage .
 COPY --from=linux-nitro /nsm.ko .
-COPY --from=linux-nitro /linux.config .
+COPY --from=linux-nitro /linux.config .


I'm not sure that's a good change? Typically we want a newline at the end of a file, that's how POSIX works: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

r-n-o · 2024-09-16T14:42:29Z

src/integration/mock/keys/new-share-set/quorum_threshold

@@ -0,0 +1 @@
+3


counter-example here I suppose: this file + the pub keys are fine as-is without a newline because this is more like a binary blob and less like a text file? Depends on what loads this file exactly.

(but if it's all the same, I'd rather end with a newline character)

r-n-o · 2024-09-16T14:44:52Z

src/integration/tests/reshard.rs

@@ -0,0 +1,338 @@
+use std::{


Why are we picking this up here? This test is related to a new reshard functionality which isn't related to VSSS afaict?

r-n-o · 2024-11-11T23:11:33Z

This change is now split into the following PRs:

Change workflow name to StageX build for consistency with mono #487 (trivial)
Remove stale files: .gitmodules, .lfsconfig #488 (trivial)
README cleanup #489 (trivial)
Cleanup: remove unused RSA key #490 (trivial)
Switch to vsss-rs for share generation and reconstruction #491 (main change -- still contains a few drive-by improvements unfortunately; lint/fmt-related mostly!)

cr-tk · 2024-11-14T17:49:04Z

@r-n-o good work on splitting up this PR. Can we close #457 now that the others have replaced it, or do you want to leave it open for now for visibility / discussion followup?

r-n-o · 2024-11-14T20:48:20Z

@cr-tk thank you! I think we can close this PR for clarity. I have no issue with further discussion if needed but given the intent isn't to merge this PR, better to be explicit and close it. Thanks for flagging!

emostov added 30 commits June 18, 2024 12:43

Update changelog for #428

712d75b

Update gitignore

ff89a91

wip

955f350

wip enclave reshard service

64e73c2

try getting enclave_reshard

ec3c54f

make sure no toolchain

0f28dd8

small fix

add57ba

enclave_reshard builds

9987767

add enclave_reshard target

21046c7

try fix perms issue

c9fd19d

remove enclave_reshard image (pivoting to mono)

54f56e3

revert ci changes

7b8925f

fix some comments

e08bb9d

another small comment fix

dfc8996

Update reshard service

8d1eca2

small adjustment to default dev port

69469a9

ensure deterministic reshard input

3c0cfad

sort share set members

3ab39ca

ensure reshard input is deterministic when reading in

17b44ff

update ci

a335417

address comments; fix tests

5342ff3

review comments; try fix CI

b887810

lint

1608d8c

update common image tags

7427f33

remove fail fast

e51d281

make sure to pull only hash pinned

13a7aed

just run make; no updload

3063310

respond to some feedback

553043c

remove lfs and release section

72b6de2

Fix length of test data.

4e8cd4c

lthibault self-assigned this Sep 16, 2024

lthibault added 2 commits September 17, 2024 12:58

Remove deprecated_insecure_shares_generate.

bb3150b

Remove deprecated_insecure_shares_reconstruct.

605447b

lthibault force-pushed the zeke-reshard-6-vsss-rs branch from 50ca13f to 605447b Compare September 17, 2024 20:48

lthibault added 3 commits September 17, 2024 16:49

Fix unit tests.

c264d9c

Eliminate dead code.

4b08bdb

make lint

d1d1130

lthibault added 4 commits September 24, 2024 14:48

Fix typo in docstring.

3727f35

Expect 2-of-2 sharing in dangerous_dev_boot.

d87496a

lintfix

6764a72

Remove redundant calls to shares_generate in make_and_reconstruct_sha…

d6db3e7

…res.

Ulexus added 2 commits October 21, 2024 17:45

qos_net: fix missing vm feature

04b0c11

DANGER: enable attach_console on debug mode

5ba801e

jack-kearney unassigned lthibault Nov 8, 2024

r-n-o reviewed Nov 11, 2024

View reviewed changes

r-n-o self-assigned this Nov 11, 2024

This was referenced Nov 11, 2024

Change workflow name to StageX build for consistency with mono #487

Merged

Remove stale files: .gitmodules, .lfsconfig #488

Merged

README cleanup #489

Merged

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

Merged

r-n-o closed this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use vsss-rs for share generation and reconstruction #457

Use vsss-rs for share generation and reconstruction #457

emostov commented Jun 18, 2024 •

edited

Loading

lthibault commented Sep 13, 2024 •

edited

Loading

lthibault commented Sep 16, 2024

cr-tk commented Sep 24, 2024 •

edited

Loading

lthibault commented Sep 24, 2024 •

edited by r-n-o

Loading

cr-tk commented Oct 30, 2024 •

edited

Loading

r-n-o left a comment

r-n-o Sep 16, 2024

r-n-o Sep 16, 2024

r-n-o Sep 16, 2024

r-n-o Sep 16, 2024

r-n-o commented Nov 11, 2024

cr-tk commented Nov 14, 2024 •

edited

Loading

r-n-o commented Nov 14, 2024

		- BREAKING CHANGE: qos_core: quorum key resharding service, new state machine transitions, and new `ProtocolMsg` variants (#428)
		- qos_client: commands to run quorum key resharding and high level documentation (#428)

Use vsss-rs for share generation and reconstruction #457

Use vsss-rs for share generation and reconstruction #457

Conversation

emostov commented Jun 18, 2024 • edited Loading

Summary & Motivation (Problem vs. Solution)

How I Tested These Changes

Pre merge check list

lthibault commented Sep 13, 2024 • edited Loading

lthibault commented Sep 16, 2024

cr-tk commented Sep 24, 2024 • edited Loading

lthibault commented Sep 24, 2024 • edited by r-n-o Loading

cr-tk commented Oct 30, 2024 • edited Loading

r-n-o left a comment

Choose a reason for hiding this comment

r-n-o Sep 16, 2024

Choose a reason for hiding this comment

r-n-o Sep 16, 2024

Choose a reason for hiding this comment

r-n-o Sep 16, 2024

Choose a reason for hiding this comment

r-n-o Sep 16, 2024

Choose a reason for hiding this comment

r-n-o commented Nov 11, 2024

cr-tk commented Nov 14, 2024 • edited Loading

r-n-o commented Nov 14, 2024

emostov commented Jun 18, 2024 •

edited

Loading

lthibault commented Sep 13, 2024 •

edited

Loading

cr-tk commented Sep 24, 2024 •

edited

Loading

lthibault commented Sep 24, 2024 •

edited by r-n-o

Loading

cr-tk commented Oct 30, 2024 •

edited

Loading

cr-tk commented Nov 14, 2024 •

edited

Loading