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

Secret storage support in the main crate #2621

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Secret storage support in the main crate #2621

merged 3 commits into from
Nov 6, 2023

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Sep 26, 2023

This PR utilizes the cryptographic primitives for secret storage support introduced in #2591, and adds a convenient and easy to use API on top of it. As such, this PR depends on #2591.

We're now able to import the private cross-signing keys and verify our own device by entering the secret storage key or passphrase.

The PR should be reviewed commit by commit. An example, in case somebody wants to play around with this as part of the review, is provided as well.

  • Public API changes documented in changelogs (optional)

@poljar poljar requested a review from a team as a code owner September 26, 2023 11:07
@poljar poljar requested review from Hywan and removed request for a team September 26, 2023 11:07
@poljar poljar changed the title Secret storage support in the main crate. Secret storage support in the main crate Sep 26, 2023
@poljar poljar self-assigned this Sep 26, 2023
@poljar poljar requested a review from dkasak September 26, 2023 11:08
@poljar poljar force-pushed the poljar/main-crate-4s branch from dc7ea73 to 7ac24eb Compare September 26, 2023 12:40
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

I like this PR. The code is clean, there is plenty of doc', really good job.

I believe that it may be useful to be able to remove a secret from the store though. What do you think?

crates/matrix-sdk-crypto/src/store/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/secret_storage.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/secret_storage.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/secret_storage.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/secret_storage.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/secret_storage.rs Outdated Show resolved Hide resolved
@poljar poljar force-pushed the poljar/main-crate-4s branch from cd84f74 to 11342f7 Compare September 28, 2023 14:53
@poljar poljar force-pushed the poljar/svuda-seremo-samo-slaninu branch from cda9ed1 to ae5b3a2 Compare October 3, 2023 11:25
@poljar poljar force-pushed the poljar/main-crate-4s branch from 11342f7 to a2c6954 Compare October 3, 2023 11:25
@poljar poljar mentioned this pull request Oct 4, 2023
1 task
@dkasak dkasak mentioned this pull request Oct 10, 2023
1 task
@poljar poljar force-pushed the poljar/svuda-seremo-samo-slaninu branch from e351222 to 9aaba44 Compare October 10, 2023 15:43
Base automatically changed from poljar/svuda-seremo-samo-slaninu to main October 10, 2023 16:15
@poljar poljar force-pushed the poljar/main-crate-4s branch from a2c6954 to 2dd385a Compare October 11, 2023 07:47
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (61eb9ce) 81.35% compared to head (38e10db) 81.36%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2621    +/-   ##
========================================
  Coverage   81.35%   81.36%            
========================================
  Files         204      207     +3     
  Lines       21051    21172   +121     
========================================
+ Hits        17126    17226   +100     
- Misses       3925     3946    +21     
Files Coverage Δ
crates/matrix-sdk/src/client/mod.rs 87.91% <ø> (ø)
crates/matrix-sdk/src/encryption/mod.rs 71.42% <100.00%> (+0.21%) ⬆️
...es/matrix-sdk/src/encryption/secret_storage/mod.rs 100.00% <100.00%> (ø)
...atrix-sdk/src/encryption/secret_storage/futures.rs 73.68% <73.68%> (ø)
...-sdk/src/encryption/secret_storage/secret_store.rs 78.37% <78.37%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poljar poljar force-pushed the poljar/main-crate-4s branch 3 times, most recently from 680f0ee to 821b140 Compare October 20, 2023 15:33
@poljar poljar force-pushed the poljar/main-crate-4s branch 2 times, most recently from f81fb48 to 7ff4592 Compare October 23, 2023 18:55
@poljar poljar changed the base branch from main to poljar/misc-e2ee-helpers October 23, 2023 18:55
@poljar
Copy link
Contributor Author

poljar commented Oct 23, 2023

I rebased this now and changed the base branch to #2758.

@poljar poljar force-pushed the poljar/misc-e2ee-helpers branch from f96590e to 107bdc7 Compare October 24, 2023 09:07
Base automatically changed from poljar/misc-e2ee-helpers to main October 24, 2023 09:23
@poljar poljar force-pushed the poljar/main-crate-4s branch from 7ff4592 to a45f575 Compare October 24, 2023 09:25
@poljar poljar force-pushed the poljar/main-crate-4s branch from a45f575 to 0bfd13e Compare October 25, 2023 07:15
Copy link
Member

@dkasak dkasak left a comment

Choose a reason for hiding this comment

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

Discussed on a call with poljar out-of-band. Left some minor comments and suggestions but this otherwise looks really good. There is one important suggestion, which is a zeroization fix.

After the suggestions are resolved, LGTM.

@poljar poljar force-pushed the poljar/main-crate-4s branch from acf4b02 to 9c60e32 Compare October 26, 2023 08:39
@dkasak dkasak self-requested a review October 26, 2023 10:40
Copy link
Member

@dkasak dkasak left a comment

Choose a reason for hiding this comment

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

Some suggestions.

EDIT: It seems that Github is bugged and has eaten my suggestions alive :(

@poljar poljar force-pushed the poljar/main-crate-4s branch from 9c60e32 to 371c546 Compare October 30, 2023 15:16
@poljar poljar force-pushed the poljar/main-crate-4s branch from 371c546 to 38e10db Compare October 31, 2023 11:21
@poljar
Copy link
Contributor Author

poljar commented Nov 6, 2023

Sorry this is taking too long. If you still remember the suggestions feel free to provide them as a patch.

@poljar poljar merged commit 2e20f00 into main Nov 6, 2023
35 checks passed
@poljar poljar deleted the poljar/main-crate-4s branch November 6, 2023 09:41
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