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

crypto: Implemented sharing room keys for past messages (MSC3061) #2650

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Michael-Hollister
Copy link
Contributor

@Michael-Hollister Michael-Hollister commented Oct 2, 2023

Fixes #580

Overview

I have created an implementation for MSC 3061 and wanted to share the changes should the team want to merge into the SDK.

Note this follows the implementation suggested in the MSC where keys are forwarded upon invitation of a new user. Based on past discussion in #580 however, this approach was was not preferred due to some flaws. I thought I would still share this implementation however for the following reasons:

  1. MSC 3061 has officially been merged into the spec per my understanding: MSC3061: Sharing room keys for past messages matrix-spec-proposals#3061
  2. The proposed changes discussed in Implement MSC3061: Sharing room keys for past messages  #580 sounded to me that it requires a new MSC proposal for adding a new event type and improving the key sharing process.
  3. This implementation provides a working solution that is equivalent to what is offered currently in the other Matrix SDKs, which can be used until the preferred solution is developed.

Any feedback welcome.

PR Dependencies:

Testing:

Tested key sharing with verified and unverified devices with multiple clients including:

  • FUTO Circles iOS / Android
  • iamb
  • Element / Cinny (interoperability testing since these don't use the crypto FFI)

Todo:

Signed-off-by: Michael Hollister [email protected]

@Michael-Hollister Michael-Hollister requested a review from a team as a code owner October 2, 2023 08:01
@Michael-Hollister Michael-Hollister requested review from Hywan and removed request for a team October 2, 2023 08:01
@poljar poljar requested review from poljar and removed request for Hywan October 2, 2023 08:39
@dkasak dkasak self-requested a review October 2, 2023 08:40
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

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

Comparison is base (a433d51) 78.74% compared to head (ada3e82) 78.51%.
Report is 85 commits behind head on main.

❗ Current head ada3e82 differs from pull request most recent head 68fd193. Consider uploading reports for the commit 68fd193 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2650      +/-   ##
==========================================
- Coverage   78.74%   78.51%   -0.23%     
==========================================
  Files         200      200              
  Lines       20360    20445      +85     
==========================================
+ Hits        16032    16053      +21     
- Misses       4328     4392      +64     
Files Coverage Δ
...trix-sdk-crypto/src/olm/group_sessions/outbound.rs 93.61% <100.00%> (+0.10%) ⬆️
...tes/matrix-sdk-crypto/src/types/events/room_key.rs 63.88% <100.00%> (+1.03%) ⬆️
...es/matrix-sdk-crypto/src/types/events/to_device.rs 61.40% <ø> (ø)
...es/matrix-sdk-crypto/src/olm/group_sessions/mod.rs 74.35% <66.66%> (-0.65%) ⬇️
crates/matrix-sdk/src/room/mod.rs 82.79% <90.00%> (+0.14%) ⬆️
...-sdk-crypto/src/types/events/forwarded_room_key.rs 33.33% <0.00%> (-2.16%) ⬇️
...atrix-sdk-crypto/src/olm/group_sessions/inbound.rs 81.39% <60.00%> (-1.03%) ⬇️
crates/matrix-sdk-crypto/src/gossiping/machine.rs 63.17% <36.36%> (-2.10%) ⬇️
crates/matrix-sdk-crypto/src/machine.rs 78.59% <14.63%> (-4.86%) ⬇️

... and 3 files with indirect coverage changes

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

@poljar
Copy link
Contributor

poljar commented Oct 14, 2023

To manage expectations a bit, I sadly won't have the time to take a proper look at this before the end of the month.

@Michael-Hollister
Copy link
Contributor Author

To manage expectations a bit, I sadly won't have the time to take a proper look at this before the end of the month.

No problem, thanks for letting me know and taking the time to review this when ready.

@Hywan Hywan removed the request for review from poljar December 4, 2023 13:46
@bnjbvr bnjbvr changed the title Implemented sharing room keys for past messages (MSC3061) crypto: Implemented sharing room keys for past messages (MSC3061) May 2, 2024
@Michael-Hollister
Copy link
Contributor Author

If this PR is of interest being reviewed/merged, let me know and I can fix conflicts and push a few fixes to issues that exist in this old version. Otherwise, my impression from the discussion in matrix-org/matrix-spec#1655 is that the PR will be not be merged, so I will be holding off updating until needed.

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.

Implement MSC3061: Sharing room keys for past messages
2 participants