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

RustCrypto | Implement keybackup loop #3652

Merged
merged 21 commits into from
Aug 17, 2023

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Aug 7, 2023

Implements backup loop for rust crypto.

Fixes https://github.com/vector-im/crypto-internal/issues/113

Depends on:

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@richvdh richvdh removed their request for review August 9, 2023 09:58
Base automatically changed from rav/element-r/keybackup_02_check_backup to develop August 9, 2023 10:13
src/rust-crypto/backup.ts Outdated Show resolved Hide resolved
src/rust-crypto/backup.ts Outdated Show resolved Hide resolved
src/rust-crypto/backup.ts Outdated Show resolved Hide resolved
spec/test-utils/test-data/static-test-data.ts Outdated Show resolved Hide resolved
@germain-gg germain-gg removed their request for review August 14, 2023 11:10
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane

@richvdh richvdh self-requested a review August 16, 2023 14:41
spec/test-utils/test-data/generate-test-data.py Outdated Show resolved Hide resolved
spec/test-utils/test-data/generate-test-data.py Outdated Show resolved Hide resolved
spec/integ/crypto/megolm-backup.spec.ts Show resolved Hide resolved
spec/integ/crypto/megolm-backup.spec.ts Outdated Show resolved Hide resolved
src/crypto/backup.ts Outdated Show resolved Hide resolved
src/crypto/backup.ts Outdated Show resolved Hide resolved
src/rust-crypto/backup.ts Show resolved Hide resolved
Comment on lines +286 to +287
# Just use radom bytes for the ratchet parts
ratchet = randbytes(32 * 4)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid randomness in this script, so that when we re-run it, only important things change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a static seed to have stable random at each generation

Copy link
Member

Choose a reason for hiding this comment

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

can't we just hardcode the strings, rather than having to seed the random generator and call randbytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks to me that it has best of the two words, it's stable and later on I can add more keys easilly test key upgrade and more

spec/test-utils/test-data/generate-test-data.py Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

spec/test-utils/test-data/generate-test-data.py Outdated Show resolved Hide resolved
spec/test-utils/test-data/generate-test-data.py Outdated Show resolved Hide resolved
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Aug 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Aug 17, 2023
@BillCarsonFr BillCarsonFr removed this pull request from the merge queue due to a manual request Aug 17, 2023
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Aug 17, 2023
Merged via the queue into develop with commit c18d691 Aug 17, 2023
20 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/element-r/keybackup_backup_loop branch August 17, 2023 14:23
@richvdh
Copy link
Member

richvdh commented Aug 29, 2023

There was some confusion about why the cypress tests didn't run on this PR. So: for the record:

  1. The cypress tests only run in matrix-js-sdk as part of the merge queue (ie, after you press "Merge when ready"). This isn't terribly helpful, but the logic goes that (1) running them for every commit would be quite expensive in terms of Github Actions workers, and (2) the most likely tests to fail under Rust Crypto will be crypto-related(*), and we'll be running crypto-related cypress tests locally as part of development.

    *: recent experience shows that it's not actually the crypto test that fail, mostly because we are testing that carefully. It's all the other tests which are inherently flaky and subject to minor timing variations.

  2. The cypress tests did run, and fail, as pert of the merge queue on this PR. However, they are not currently a mandatory part of the merge queue, making them effectively useless. This is probably my fault, but in any case we need to get them back to a state where they are passing more often than not before we can change it.

    Github does a terrible job of exposing the failed test run via the UI, but you can see it by clicking on the commit sha (c18d691) and then the red cross:
    image
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants