-
Notifications
You must be signed in to change notification settings - Fork 329
feat: Encrypted state events (MSC3414) #5456
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
feat: Encrypted state events (MSC3414) #5456
Conversation
This is part of #5397 |
79697a3
to
93feb08
Compare
CodSpeed Performance ReportMerging #5456 will not alter performanceComparing Summary
|
9dddf23
to
3dec91a
Compare
For information, #5473 upgrades Ruma and brings in the encrypted state events changes. |
3dec91a
to
ee6a293
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5456 +/- ##
==========================================
- Coverage 88.57% 88.56% -0.01%
==========================================
Files 339 339
Lines 93603 93800 +197
Branches 93603 93800 +197
==========================================
+ Hits 82910 83078 +168
- Misses 6560 6577 +17
- Partials 4133 4145 +12 ☔ View full report in Codecov by Sentry. |
4882f4e
to
5cb4dd9
Compare
testing/matrix-sdk-integration-testing/src/tests/e2ee/state_events.rs
Outdated
Show resolved
Hide resolved
2c93f9f
to
6c49a11
Compare
00c3e99
to
b060f96
Compare
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
…macro Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
Signed-off-by: Skye Elliot <[email protected]>
b060f96
to
61d40bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments:
-
Yay! excites! It generally looks very sensible.
-
I wonder if all this functionality should be behind compile-time cfg guards, since it's entirely unspecced. @poljar: any thoughts on this?
-
For future reference: your commit comments could do with a little more detail. Try and explain how the bit of functionality that bit introduces fits in with the whole.
-
Before this is ready to land in the rust SDK, I think we need some more (or, indeed, any) unit tests, which exercise individual parts of the functionality without rolling it into a big integration test.
For sure an integration test is nice to make sure it all works as a whole, but integration tests are slooow and make it painful to figure out which bit of the code is misbehaving. Plus, unit tests make it easier to exercise the non-happy paths since you can mock out the homeserver. Codecov is having one of its "special" days, but when it recovers I'd expect it to complain about lack of code coverage. There's a concept called the Test Pyramid which says you should have lots of low-level tests and a few high-level tests.
(Generally, I'd expect to see code, and unit tests that exercise that code, landing in the same commit.)
-
We should be able to break this PR into smaller chunks, especially if we have more tests. Smaller PRs are generally a good thing since it means that author and reviewer have less state to keep in their head as the PR goes through multiple rounds of review. (Some guy called richvdh wrote a blog post mentioning this.)
/// | ||
/// # Panics | ||
/// | ||
/// Panics if no such session exists for the given room ID, or the session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Panics if no such session exists for the given room ID, or the session | |
/// Panics if no session exists for the given room ID, or the session |
/// * `room_id` - The id of the room for which the message should be | ||
/// encrypted. | ||
/// | ||
/// * `content` - The plaintext content of the event that should be | ||
/// encrypted as a raw JSON value. | ||
/// | ||
/// * `state_key` - The associated state key of the event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event_type
is missing here, and the order is inconsistent with the actual arguments
@@ -319,3 +321,83 @@ impl<'a> IntoFuture for SendAttachment<'a> { | |||
Box::pin(fut.instrument(tracing_span)) | |||
} | |||
} | |||
|
|||
/// TODO: Future returned by `Room::send_state_event_raw`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this TODO mean?
|
||
/// TODO: Future returned by `Room::send_state_event_raw`. | ||
#[allow(missing_debug_implementations)] | ||
pub struct SendStateEventRaw<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I wonder if this should be called sendRawStateLikeEvent
for consistency with SendRawMessageLikeEvent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going for consistency with Ruma - it uses StateEvent
and MessageLikeEvent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and Ruma uses "message-like" to disambiguate from m.room.message
/ m.message
)
let content = | ||
deserialized_event.original_content().expect("The event should not have been redacted"); | ||
|
||
assert_matches2::assert_let!($pat = content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you've forgotten to use $msg
here.
/// Given a [`TimelineEvent`], assert that the event is a decrypted state | ||
/// event, and that its content matches the given pattern via a let binding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some examples might help me understand how to use this.
/// event, and that its content matches the given pattern via a let binding. | ||
#[macro_export] | ||
macro_rules! assert_let_decrypted_state_event_content { | ||
($pat:pat = $event:expr, $($msg:tt)*) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation could do with mentioning that the macro uses the second and subsequent arguments, if provided, as an error message if the content does not match.
// HACK: wait for sync | ||
let _ = tokio::time::sleep(Duration::from_secs(3)).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find something to wait for, rather than just sleeping for 3 seconds. There's a wait_until_some
helper which I imagine you could combine with is_state_encrypted
to do what you need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that said... didn't we agree this was a bug in enable_encryption[_with_state]
which should be fixed in a separate PR?
/// Whether state event encryption is enabled. | ||
pub encrypt_state_events: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RoomSettings is serialized and persisted in clients' local stores. We'll need to make sure it behaves sensibly when the serialized version omits the field. (In other words, needs #[serde(default)]
or something)
The commit comment is a bit weird too: it says "WASM SDK" but ... this isn't the wasm bindings project.
TBH I'd suggest pulling this RoomSettings stuff out to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included WASM SDK in the commit message since this change (afaik) will only affect it and not the rest of the Rust SDK, since it uses the m.room.encryption
event to decide whether to encrypt rather than this - I can change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably just falls into the more general bracket of "your commit comments could do with a little more detail". You're allowed to use more than one line ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pulled this out to #5511 - I'll drop the commit that introduces this after our meeting.
…crypt_state to ::encrypt_inner
Unable to generate the performance reportThere was an internal error while processing the run's data. We're working on fixing the issue. Feel free to contact us on Discord or at [email protected] if the issue persists. |
Closing in favour of split pull requests: |
Implements support for encrypted state events under
m.room.encrypted
, as outlined in MSC3414.state_events::sync::dispatch
to requireE2EE
when end-to-end encryption is enabled, using it to decryptm.room.encrypted
state events when received.m.room.encrypted
as required state by default.encrypt_state_event
andencrypt_state_event_raw
toOlmMachine
andOutboundGroupSession
;Room::send_state_event
andRoom::send_state_event_Raw
to returnSendStateEvent
andSendStateEventRaw
futures respectifely, that each function similarly to their message-like counterparts.