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

AureliaDolo merged 1 commit into master from aurelia/shamir

Jun 14, 2024

Contributor

AureliaDolo commented May 16, 2024 •

edited

Loading

Related #6090

AureliaDolo requested review from a team as code owners

May 16, 2024 15:43

AureliaDolo force-pushed the aurelia/shamir branch from bee2c50 to ea581bf Compare

May 16, 2024 15:44

AureliaDolo marked this pull request as draft

May 16, 2024 15:46

AureliaDolo marked this pull request as draft

May 16, 2024 15:46

AureliaDolo self-assigned this

AureliaDolo requested review from vxgmichel

May 16, 2024 15:46

AureliaDolo force-pushed the aurelia/shamir branch 4 times, most recently from b34bdcb to e5ed694 Compare

May 16, 2024 16:32

AureliaDolo marked this pull request as ready for review

May 17, 2024 15:01

Contributor Author

AureliaDolo commented May 17, 2024

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 commented

View reviewed changes

libparsec/crates/protocol/schema/authenticated_cmds/shamir_create_shared_recovery_device.json5 Outdated Show resolved Hide resolved

AureliaDolo commented

View reviewed changes

server/parsec/components/shamir.py Outdated Show resolved Hide resolved

AureliaDolo commented

View reviewed changes

server/tests/api_v4/authenticated/test_shamir_recovery_setup.py Outdated Show resolved Hide resolved

AureliaDolo commented

View reviewed changes

libparsec/crates/testbed/src/template/crc_hash.rs Outdated Show resolved Hide resolved

FirelightFlagboy requested changes

View reviewed changes

libparsec/crates/types/src/id.rs Outdated Show resolved Hide resolved

AureliaDolo force-pushed the aurelia/shamir branch 4 times, most recently from e7b72a9 to 3047103 Compare

May 23, 2024 09:52

FirelightFlagboy reviewed

View reviewed changes

libparsec/crates/types/src/id.rs Outdated Show resolved Hide resolved

libparsec/crates/testbed/src/template/crc_hash.rs 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 Outdated Show resolved Hide resolved

server/parsec/components/postgresql/shamir.py Outdated Show resolved Hide resolved

AureliaDolo commented

View reviewed changes

server/parsec/_parsec_pyi/ids.pyi Show resolved Hide resolved

FirelightFlagboy mentioned this pull request

Define python token types in specific file #7388

Open

AureliaDolo force-pushed the aurelia/shamir branch 3 times, most recently from a92c18a to 4687a91 Compare

May 27, 2024 11:19

AureliaDolo commented

View reviewed changes

server/tests/api_v4/authenticated/test_shamir_recovery_setup.py Outdated Show resolved Hide resolved

vxgmichel requested changes

View reviewed changes

Contributor

vxgmichel left a comment

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

AureliaDolo requested review from FirelightFlagboy and vxgmichel

June 4, 2024 15:34

vxgmichel requested changes

View reviewed changes

Contributor

vxgmichel left a comment

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

AureliaDolo force-pushed the aurelia/shamir branch from 87058bd to f2f59e9 Compare

June 6, 2024 15:17

AureliaDolo requested a review from vxgmichel

June 6, 2024 15:19

AureliaDolo commented

View reviewed changes

libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved

mmmarcos approved these changes

View reviewed changes

Contributor

mmmarcos left a comment

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 reviewed

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 approved these changes

View reviewed changes

Contributor

vxgmichel left a comment

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 mentioned this pull request

[rust] Factorize de/serialization tests #7549

Open

FirelightFlagboy reviewed

View reviewed changes

libparsec/crates/protocol/tests/authenticated_cmds/v4/shamir_recovery_setup.rs Outdated Show resolved Hide resolved

AureliaDolo force-pushed the aurelia/shamir branch from d6f1e61 to d54ecfb Compare

June 7, 2024 14:01

AureliaDolo requested a review from FirelightFlagboy

June 7, 2024 14:02

vxgmichel approved these changes

View reviewed changes

Contributor

vxgmichel left a comment

LGTM 👍

FirelightFlagboy approved these changes

View reviewed changes

AureliaDolo force-pushed the aurelia/shamir branch from d54ecfb to 09e1023 Compare

June 7, 2024 14:23

touilleMan requested changes

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"

Member

touilleMan Jun 7, 2024

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 mentioned this pull request

Fix data&protocol tests involving payload dump #7510

Closed

2 tasks

AureliaDolo force-pushed the aurelia/shamir branch from 09e1023 to b6dca97 Compare

June 10, 2024 08:30

AureliaDolo requested a review from touilleMan

June 10, 2024 08:30

touilleMan approved these changes

View reviewed changes

Member

touilleMan left a comment

👍

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 force-pushed the aurelia/shamir branch from b6dca97 to 67e0024 Compare

June 14, 2024 14:21


          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 force-pushed the aurelia/shamir branch from 67e0024 to 55b5045 Compare

June 14, 2024 14:24

AureliaDolo enabled auto-merge

June 14, 2024 14:24

AureliaDolo added this pull request to the merge queue

Merged via the queue into master with commit 95111c9

14 checks passed

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

Labels

None yet