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

Share Megolm session keys when inviting a new user #5853

Merged

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Apr 27, 2022

Closes #4153 and relates to MSC3061

This PR aims to improve users experience when they are first invited to a room. Users will be able to decrypt and view previous messages

PR includes:

  • Share megolm sessions extracted from the latest messages. We decided not to share the whole history for performance reasons
  • Share inbound sessions to all user devices
  • Will store an extra boolean flag sharedHistory in inbound Megolm sessions. That flag will determine whether or not the inbound session will be shared. That flag respects the room history visibility settings at the time:
    world_readable -> sharedHistory = true
    shared -> sharedHistory = true
    invited -> sharedHistory = false
    joined -> sharedHistory = false
  • Will share keys appropriately even if the user is unknown (will download the appropriate keys for that user)
  • Rotate session with respect to room history visibility
  • Integration Tests

Update

Added MSC3061 as a lab flag (off by default).
image

Enabling it will trigger an initial sync.

Crypto database have been migrated to stop using frozen serializable java objects for session metadata (using json now)

@ariskotsomitopoulos ariskotsomitopoulos requested review from Anderas, a team and ouchadam and removed request for a team April 27, 2022 10:04
@github-actions
Copy link

github-actions bot commented Apr 27, 2022

Unit Test Results

146 files  +  4  146 suites  +4   2m 34s ⏱️ +4s
236 tests +  7  236 ✔️ +  7  0 💤 ±0  0 ±0 
788 runs  +28  788 ✔️ +28  0 💤 ±0  0 ±0 

Results for commit 0340719. ± Comparison against base commit 483b1ab.

♻️ This comment has been updated with latest results.

}.mapCatching {
val userDevices = cryptoStore.getUserDevices(userId)
userDevices?.forEach {
// Lets share the provided inbound sessions for every user device
Copy link
Contributor

Choose a reason for hiding this comment

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

general comment for the comments, they look like they could be replaced with functions

// Get inbound session from sessionId and sessionKey
sessionInfoSet?.inboundSessions()

// Filter only sessions with sharedHistory enabled
?.onlySessionsWithSharedHistory()

// Share the session to userId with deviceId
fun shareSessionWithUserDevice(session, roomId, userId, deviceId)

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Thx for the PR 👏
Please see my comments. Also it would be good to add a bit more log (.debug level to keep track on what hapenned in RS, and .verbose to help debugging).
I think the part in MXMegolmDecryption would need some rework because I refactored part of it in my latest PR, we could check that together when it will be merged.

@BillCarsonFr BillCarsonFr force-pushed the feature/aris/crypto_share_room_keys_past_messages branch from 464ad55 to 607c959 Compare May 6, 2022 17:36
@BillCarsonFr
Copy link
Member

This PR #5559 should be merged first

@BillCarsonFr BillCarsonFr force-pushed the feature/aris/crypto_share_room_keys_past_messages branch from 607c959 to d9f64ad Compare May 12, 2022 10:28
@BillCarsonFr
Copy link
Member

Pushed an update with changed needed to have polyjuice test passing.
org.matrix.msc3061.shared_history was not properly transmitted and use from/to m.room_key and m.forwarded_room_key

Migrated the OlmInboundGroupSessionWrapper2serializable class (frozen) to a MXInboundMegolmSessionWrapper composed of the actual session (olm) and a data class serialized in json.

Moved things around a bit and rebased after latest SDK PR #5492
Updated the test to use less internal access (crypto store)

@ariskotsomitopoulos I think we should review the way we get the last messages of the timeline. We could add a new API in timeline service. This could be usefull for other features, like redact the n last message of someone (when moderating)

@BillCarsonFr
Copy link
Member

@ariskotsomitopoulos One last thing to add is the export/import to backup and file:

the session_data field in key backups of this key has a shared_history property set to true in the decrypted JSON structure, and the SessionData type used in key exports has a shared_history property that is set to true for this key.

@BillCarsonFr
Copy link
Member

Oh also @ariskotsomitopoulos , please add a configuration flag in MXCryptoConfig to enable/disable history sahring.
If disabled always set the flag to false when sending keys, and do not send on invite.

@BillCarsonFr BillCarsonFr force-pushed the feature/aris/crypto_share_room_keys_past_messages branch from a196a52 to fb5f0cb Compare July 1, 2022 08:05
@sonarcloud
Copy link

sonarcloud bot commented Jul 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

0.2% 0.2% Coverage
0.0% 0.0% Duplication

@BillCarsonFr BillCarsonFr merged commit 8dc57fe into develop Jul 1, 2022
@BillCarsonFr BillCarsonFr deleted the feature/aris/crypto_share_room_keys_past_messages branch July 1, 2022 15:33
@rubo77
Copy link

rubo77 commented Jul 17, 2022

  • We decided not to share the whole history for performance

Is there a way to enable sharing the whole history? On my server this performance is not so important

@BillCarsonFr BillCarsonFr mentioned this pull request Jul 18, 2022
12 tasks
@filipesmedeiros
Copy link

Just to make sure, any updates on this? The Labs feature is disabled for be, not sure exactly why. Are there plans to move this forward? Are there any replacements in the meantime?

Thanks so much for your work! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sharing room keys for past messages
6 participants