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

Add a cryptoConfig to limit room key requests #4333

Closed
wants to merge 1 commit into from

Conversation

yostyle
Copy link
Contributor

@yostyle yostyle commented Oct 25, 2021

In the case a fork wants to disable cross signing feature and to support legacy mode for the room key requests, it is best not to request the sender device.

@github-actions
Copy link

Unit Test Results

0 files   -   48  0 suites   - 48   0s ⏱️ -52s
0 tests  -   91  0 ✔️  -   91  0 💤 ±0  0 ±0 
0 runs   - 244  0 ✔️  - 244  0 💤 ±0  0 ±0 

Results for commit f343f84. ± Comparison against base commit f2c22c1.

This pull request removes 91 tests.
im.vector.app.features.call.conference.JitsiWidgetDataFactoryTest ‑ jitsiWidget_V1_failure
im.vector.app.features.call.conference.JitsiWidgetDataFactoryTest ‑ jitsiWidget_V1_success
im.vector.app.features.call.conference.JitsiWidgetDataFactoryTest ‑ jitsiWidget_V2_success
im.vector.app.features.crypto.keys.KeysExporterTest ‑ given different file size returned for export when exporting then throws UnexpectedExportKeysFileSizeException
im.vector.app.features.crypto.keys.KeysExporterTest ‑ given exported file is missing after export when exporting then throws IllegalStateException
im.vector.app.features.crypto.keys.KeysExporterTest ‑ given output stream is unavailable for exporting to when exporting then throws IllegalStateException
im.vector.app.features.crypto.keys.KeysExporterTest ‑ when exporting then writes exported keys to context output stream
im.vector.app.features.crypto.quads.SharedSecureStorageViewModelTest ‑ given a key info with passphrase and on EnterKey step when going back then step is EnterPassphrase
im.vector.app.features.crypto.quads.SharedSecureStorageViewModelTest ‑ given a key info with passphrase when initialising then step is EnterPassphrase
im.vector.app.features.crypto.quads.SharedSecureStorageViewModelTest ‑ given a key info without passphrase when initialising then step is EnterKey
…

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

I let @BillCarsonFr approve the PR if it's OK for him. I just have one question.

@@ -68,7 +71,7 @@ internal class MXMegolmDecryption(private val userId: String,
override fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult {
// If cross signing is enabled, we don't send request until the keys are trusted
// There could be a race effect here when xsigning is enabled, we should ensure that keys was downloaded once
val requestOnFail = cryptoStore.getMyCrossSigningInfo()?.isTrusted() == true
val requestOnFail = cryptoStore.getMyCrossSigningInfo()?.isTrusted().orTrue()
Copy link
Member

Choose a reason for hiding this comment

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

The code is not strictly equivalent to the previous version. Is it intended?

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit strange yes, and unwanted I think (or should be guarded by a flag)

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.

I think that we should go with something more complete and configurable.
Probably a KeyForwardingStrategy:
Level 0 -> No Forwarding
Level 1 -> Forward if was intented to be shared initialy
Level 2 -> Level 1 + To current user verified sessions
Level 3 -> Level 2 + Key was initialy shared with another device belonging to a cross signed & verified user

Would Level 2 be what you want

@BillCarsonFr
Copy link
Member

This PR #5559 is adding a new boolean in MXCryptoConfig to limit room key requests to own devices

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