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 deletion specification - server side #7609

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

AureliaDolo
Copy link
Contributor

@AureliaDolo AureliaDolo commented Jun 17, 2024

Related #7364

Copy link
Contributor

@vxgmichel vxgmichel left a comment

Choose a reason for hiding this comment

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

A couple of comments but LGTM otherwise 👍

@touilleMan should review and validate this before it's merged

docs/rfcs/1000-shamir-based-recovery.md Outdated Show resolved Hide resolved
docs/rfcs/1000-shamir-based-recovery.md Show resolved Hide resolved
docs/rfcs/1000-shamir-based-recovery.md Outdated Show resolved Hide resolved
docs/rfcs/1000-shamir-based-recovery.md Show resolved Hide resolved
"type": "DateTime"
},
{
"name": "previous_timestamp",
Copy link
Member

Choose a reason for hiding this comment

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

I find previous_timestamp not clear enough, as it may refer to the previous timestamp in the shamir topic (which is itself different for each user), for instance:

  1. Alice creates shamir for herself with Bob as recipient
  2. Bob creates shamir for himself with Mallory as recipient
  3. Alice delete her shamir

In this case, Alice's shamir topic contains certificates from steps 1 and 3, while Bob's shamir topic contains certificates from steps 1, 2 and 3 (hence it is weird for Bob that certificate from step 3's previous_timestamp field refers to certificate from step 1...)

Copy link
Member

Choose a reason for hiding this comment

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

On top of that, I think a user_id field is missing: this field has been added to shamir brief/share certificates now that author field doesn't contains the user ID (as DeviceID and UserID are now both UUID).

In theory author's user ID can be retrieved from fetching the author's device certificate, however this is cumbersome given in the certificate storage we want to index the certificate by the author user ID (given it is the discriminant in the shamir topic) but the code requires that all indexed data are within the certificate (and changing this code is just not worth the complexity)

Copy link
Member

Choose a reason for hiding this comment

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

So, to summarize both previous points, I think we should have:

{
    // User here must be the one owning the device used as author
    // (i.e. a user can only remove its own Shamir recovery)
    "name": "to_delete_setup_user_id",
    "type": "UserID"
},
{
    // User ID + timestamp is enough to uniquely identify a Shamir recovery
    "name": "to_delete_setup_timestamp",
    "type": "DateTime"
},

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.e. a user can only remove its own Shamir recovery)

And that means if we want a configuration where only admins can delete shamir certificates, it should be quite easy

docs/rfcs/1000-shamir-based-recovery.md Outdated Show resolved Hide resolved
4
],
"req": {
"cmd": "shamir_recovery_deletion",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"cmd": "shamir_recovery_deletion",
"cmd": "shamir_recovery_delete_setup",

The command name should contain a verb

(btw I guess shamir_recovery_setup is a bit weird on this regard given setup is both the verb and the noun, since after that we refer to the configuration set by this command as the shamir setup)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe shamir_recovery_setup_setup ?

docs/rfcs/1000-shamir-based-recovery.md Outdated Show resolved Hide resolved
docs/rfcs/1000-shamir-based-recovery.md Outdated Show resolved Hide resolved
},
{
// Share recipients lists are incoherent
"status": "incoherent_date"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Share recipients are the only not identifying data that we have in the deletion certificate to check. Maybe we could include more data from the brief to ensure coherence. I'm not sure how much it's needed though

@AureliaDolo AureliaDolo changed the title Shamir deletion - server side Shamir deletion specification - server side Jun 25, 2024
@AureliaDolo AureliaDolo marked this pull request as ready for review June 25, 2024 07:33
@AureliaDolo AureliaDolo added this pull request to the merge queue Jun 25, 2024
Merged via the queue into master with commit df87d24 Jun 25, 2024
12 checks passed
@AureliaDolo AureliaDolo deleted the aurelia/shamir_deletion branch June 25, 2024 07:36
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.

3 participants