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.

Sign up for GitHub

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

Jump to bottom

Port: Shamir-based shared recovery device #7324

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Port: Shamir-based shared recovery device #7324

merged 1 commit into from
Jun 14, 2024

Conversation

AureliaDolo
Copy link
Contributor

@AureliaDolo AureliaDolo commented May 16, 2024 •
edited
Loading

Related #6090

Fix #7357

@AureliaDolo AureliaDolo requested review from a team as code owners May 16, 2024 15:43
@AureliaDolo AureliaDolo marked this pull request as draft May 16, 2024 15:46
@AureliaDolo AureliaDolo marked this pull request as draft May 16, 2024 15:46
@AureliaDolo AureliaDolo self-assigned this May 16, 2024
@AureliaDolo AureliaDolo force-pushed the aurelia/shamir branch 4 times, most recently from b34bdcb to e5ed694 Compare May 16, 2024 16:32
@AureliaDolo AureliaDolo marked this pull request as ready for review May 17, 2024 15:01
@AureliaDolo
Copy link
Contributor Author

I have to split the checklist into issues and I'll implement the postrgre version once every route has been tested with the memory impl

@AureliaDolo AureliaDolo force-pushed the aurelia/shamir branch 4 times, most recently from e7b72a9 to 3047103 Compare May 23, 2024 09:52
@FirelightFlagboy FirelightFlagboy mentioned this pull request May 23, 2024
Open
@AureliaDolo AureliaDolo force-pushed the aurelia/shamir branch 3 times, most recently from a92c18a to 4687a91 Compare May 27, 2024 11:19
vxgmichel
vxgmichel requested changes Jun 4, 2024
View reviewed changes
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 bit of naming to change, LGTM otherwise 👍

server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
libparsec/crates/protocol/schema/authenticated_cmds/shamir_create_shared_recovery_device.json5 Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
vxgmichel
vxgmichel requested changes Jun 4, 2024
View reviewed changes
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.

Ooops I missed a couple of things about error handling in my first reviews :)

server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
mmmarcos
mmmarcos approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

Great job for this first PR! 💯 👏
I left some comments, but nothing major.

libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
server/parsec/components/memory/datamodel.py Outdated Show resolved Hide resolved
server/parsec/components/memory/shamir.py Outdated Show resolved Hide resolved
server/tests/api_v4/authenticated/test_shamir_recovery_setup.py Outdated Show resolved Hide resolved
server/tests/api_v4/authenticated/test_shamir_recovery_setup.py Show resolved Hide resolved
FirelightFlagboy
FirelightFlagboy reviewed Jun 7, 2024
View reviewed changes
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
vxgmichel
vxgmichel approved these changes Jun 7, 2024
View reviewed changes
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 minor comments, LGTM otherwise 👍

libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Show resolved Hide resolved
@AureliaDolo AureliaDolo mentioned this pull request Jun 7, 2024
Open
vxgmichel
vxgmichel approved these changes Jun 7, 2024
View reviewed changes
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.

LGTM 👍

touilleMan
touilleMan requested changes Jun 7, 2024
View reviewed changes
libparsec/crates/protocol/Cargo.toml Show resolved Hide resolved
libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated
Comment on lines 149 to 152
// "ballpark_client_early_offset": 32.0
// "ballpark_client_late_offset": 32.0
// "client_timestamp": "2009-02-13T23:31:30Z"
// "server_timestamp": "2009-02-13T23:31:30Z"
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
// "ballpark_client_early_offset": 32.0
// "ballpark_client_late_offset": 32.0
// "client_timestamp": "2009-02-13T23:31:30Z"
// "server_timestamp": "2009-02-13T23:31:30Z"
// ballpark_client_early_offset: 32.0
// ballpark_client_late_offset: 32.0
// client_timestamp: ext(1, timestamp(2009-02-13T23:31:30.000000Z))
// server_timestamp: ext(1, timestamp(2009-02-13T23:31:30.000000Z))

2 very minor remarks:

  • Indentation seems off compared with the status field
  • the key part of the fields are displayed without double quotes (there is not much reason for it apart for consistency with the rest of the codebase)

And 1 (a bit) more important:

  • datetime are displayed as ext(1, 946774800.0) (or sometime ExtType(code=1, data=b'\x00\x03]\x01;7\xe0\x00')). The point here is "2009-02-13T23:31:30Z" stands for a string, where in msgpack it is serialized as an extension type (msgpack extension type itself is divided between a code that must be an integer, and an arbitrary long array of bytes).

Anyway the point here is to keep the info this is serialized as an extension with code=1. Also note the data part is currently messy in our codebase, with often a float (we used to represent the datetime as a float) or as bytes (as ExtType(code=1, data=b'\x00\x03]\x01;7\xe0\x00') is output we get when deserializing msgpack with Python, which is pretty convenient to quickly created the representation of payload)
I'm going to link this comment to the issue about re-creating the test payload for data&protocol, I think it's there that we should settle on how we want to represent the extension type (so in the meantime you can do whatever you think is the best on this ^^)

@touilleMan touilleMan mentioned this pull request Jun 7, 2024
Closed
2 tasks
touilleMan
touilleMan approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@touilleMan touilleMan left a comment

Choose a reason for hiding this comment

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

👍

libparsec/crates/protocol/schema/authenticated_cmds/shamir_recovery_setup.json5 Outdated Show resolved Hide resolved
server/parsec/components/shamir.py Outdated Show resolved Hide resolved
@AureliaDolo
Port shamir_recovery_setup from v2.17
55b5045 
- update the RFC
- split invalid_data error into all possible variants
- in rust serialiaztion tests, the macro test_roundtrip_serialization
    could be used for other tests too.
@AureliaDolo AureliaDolo added this pull request to the merge queue Jun 14, 2024
Merged via the queue into master with commit 95111c9 Jun 14, 2024
14 checks passed
@AureliaDolo AureliaDolo deleted the aurelia/shamir branch June 14, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmmarcos mmmarcos mmmarcos approved these changes

@touilleMan touilleMan touilleMan approved these changes

@vxgmichel vxgmichel vxgmichel approved these changes

@FirelightFlagboy FirelightFlagboy FirelightFlagboy approved these changes

Assignees

@AureliaDolo AureliaDolo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shamir port: server side route create_shared_recovery_device
5 participants
@AureliaDolo @mmmarcos @touilleMan @vxgmichel @FirelightFlagboy