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

[Shamir port] Implement greeting procedure #8995

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

vxgmichel
Copy link
Contributor

@vxgmichel vxgmichel commented Nov 25, 2024

Close #8841

TODO:

  • more tests
  • proper error handling
  • greeter detects when the device is not recoverable due to too many recipients revoked
  • claimer detects when the device is not recoverable due to too many recipients being revoked

@vxgmichel vxgmichel force-pushed the issue-8841 branch 2 times, most recently from 08dde62 to d8d9e29 Compare November 29, 2024 12:35
@vxgmichel vxgmichel marked this pull request as ready for review November 29, 2024 12:47
@vxgmichel vxgmichel requested review from a team as code owners November 29, 2024 12:47
@@ -64,6 +64,9 @@ pub async fn main(args: Args) -> anyhow::Result<()> {
let ctx = step4_device(ctx).await?;
save_device(ctx, save_mode).await
}
UserOrDeviceClaimInitialCtx::ShamirRecovery(_) => {
panic!("Shamir recovery invitation is not supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Return an error instead of panicing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's part of the todo list

Copy link
Contributor

Choose a reason for hiding this comment

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

I mark the PR as a draft then

Comment on lines +446 to +447
if cfg!(test)
&& recovery_device
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if cfg!(test)
&& recovery_device
#[cfg!(test)]
if recovery_device

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion would complain about recovery_device being mut

};

#[parsec_test(testbed = "shamir", with_server)]
async fn shamir(tmp_path: TmpPath, env: &TestbedEnv) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is long, I would split it using sub-function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really the point in splitting a single test scenario into subfunction.

There could be multiple smaller tests but I like to have at least one test for testing the full scenario. My plan is to add more smaller tests around it for edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having sub-function simplify reading the test since you wont need to read its implementation first

do_a();
do_b();
check_c();

) -> Result<ShamirRecoveryShareData, CertifListShamirRecoveryError> {
ops.store
.for_read(|store| async move {
// TODO: check that the shamir is actually recoverable
Copy link
Contributor

Choose a reason for hiding this comment

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

Meaning that enough recipient are still here / not revoked ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update rfc accordingly

@FirelightFlagboy FirelightFlagboy marked this pull request as draft December 2, 2024 15:29
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.

[Shamir port] Implement greeting procedure
3 participants