-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Support encrypted state events MSC4362 #30877
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
base: develop
Are you sure you want to change the base?
Conversation
|
Two potential considerations:
|
47ec3fd to
f568949
Compare
IMO it would make sense that clients with this flag enabled can create / enable rooms to have encrypted state, but all supporting clients regardless of features should be able to interact in these rooms. So, they should probably attempt to send encrypted state if the room requires it. |
I agree! The last commit I made locks |
|
It appears that the state events sent immediately after room creation are not encrypted - investigating... Although this may be preferable behaviour - clients could fall back to this if room key sharing ever fails? |
|
Latest changes enabling room name encryption might belong in the JS SDK? Thoughts appreciated! |
|
It will be quite challenging to improve coverage on |
florianduros
left a comment
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.
First quick review :)
src/createRoom.ts
Outdated
| const timeout = setTimeout(() => { | ||
| logger.warn("Timed out while waiting for room to enable encryption"); | ||
| roomState.off(RoomStateEvent.Update, onRoomStateUpdate); | ||
| resolve(); | ||
| }, 3000); |
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.
If we have to wait for this long, we need a spinner or some feedback to the user
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.
There is an existing spinner, I could force it to be shown If state event encryption is enabled?
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.
@florianduros this code (as linked by Skye above) has existing logic for whether or not we want to show a spinner, so I am guessing that this already covers what we need - if it's a "background" operation then we should not show a spinner, and if it's a "foreground" operation then we should. All this PR does is introduce a potential additional delay, but because there is already a possibility of a spinner I think we can assume that this is already potentially slow, so nothing has materially changed here.
What do you think?
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.
Okay make sense. We can add later a spinner if we think the ux lacks feedback
src/createRoom.ts
Outdated
|
|
||
| const roomState = resolvedRoom.getLiveTimeline().getState(Direction.Forward)!; | ||
|
|
||
| // Soft fail, since the room will still be functional if the initial state is not encrypted. |
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.
Also if it fails, how do we inform the user?
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 about to write that we don't care, but I've changed my mind: I think we should fail if this doesn't work: otherwise we will send potentially-confidential information in the clear.
richvdh
left a comment
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.
a few drive-by comments; sorry, haven't done a full review
src/createRoom.ts
Outdated
| // Set room avatar | ||
| if (opts.avatar) { | ||
| let url: string; | ||
| if (opts.avatar instanceof File) { | ||
| ({ content_uri: url } = await client.uploadContent(opts.avatar)); | ||
| } else { | ||
| url = opts.avatar; | ||
| } | ||
| await client.sendStateEvent(roomId, EventType.RoomAvatar, { url }, ""); | ||
| } | ||
| } |
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 could do with a test that hits this codepath
src/createRoom.ts
Outdated
| await new Promise<void>((resolve, reject) => { | ||
| if (resolvedRoom.hasEncryptionStateEvent()) { | ||
| return resolve(); | ||
| } | ||
|
|
||
| const roomState = resolvedRoom.getLiveTimeline().getState(Direction.Forward)!; | ||
|
|
||
| // Soft fail, since the room will still be functional if the initial state is not encrypted. | ||
| const timeout = setTimeout(() => { | ||
| logger.warn("Timed out while waiting for room to enable encryption"); | ||
| roomState.off(RoomStateEvent.Update, onRoomStateUpdate); | ||
| resolve(); | ||
| }, 3000); | ||
|
|
||
| const onRoomStateUpdate = (state: RoomState): void => { | ||
| if (state.getStateEvents(EventType.RoomEncryption, "")) { | ||
| roomState.off(RoomStateEvent.Update, onRoomStateUpdate); | ||
| clearTimeout(timeout); | ||
| resolve(); | ||
| } | ||
| }; | ||
|
|
||
| roomState.on(RoomStateEvent.Update, onRoomStateUpdate); | ||
| }); |
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 createRoom function is way too long and complicated. Factor this bit (at least) out to a separate function?
Signed-off-by: Skye Elliot <[email protected]>
330ec69 to
9d06c66
Compare
andybalaam
left a comment
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.
Adding a review, primarily to remind myself what needs doing when I get back to this.
|
|
||
| let e2eeStateSection: JSX.Element | undefined; | ||
| if ( | ||
| SettingsStore.getValue("feature_msc3414_encrypted_state_events", null, false) && |
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.
Should we replace 3414 with 4362 everywhere?
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.
Hmm, no because MSC4362 says to use prefix io.element.msc3414.encrypt_state_events
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.
But we could rename the feature flag, though that might get confusing.
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.
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.
Hmm, no because MSC4362 says to use prefix
io.element.msc3414.encrypt_state_events
Perhaps MSC4362 should be adjusted to use just encrypt_state_events, as in MSC3414, which we can move to if it ever gets merged into the spec? I agree with Rich that this should probably be io.element.msc4362.encrypt_state_events. I wrote up MSC4362 primarily to document what I had implemented. Still, now that the proposal exists, I think we should start working more in the other direction, where the proposal informs the implementation?
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 updated MSC4362 to use io.element.msc4362.encrypt_state_events and I will update this PR to reflect that.
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.
Work on this is in:
- events: Rename unstable-msc3413 feature to unstable-msc4362 ruma/ruma#2287
- crypto: Refer to MSC4362 when we are talking about encrypted state matrix-org/matrix-rust-sdk#5868
- base: Bump ruma to 91424b1fc matrix-org/matrix-rust-sdk#5873
- Bump matrix-rust-sdk to 1c22d0b25bd matrix-org/matrix-sdk-crypto-wasm#280
- Update encrypted state to say MSC4362 everywhere matrix-org/matrix-js-sdk#5079
- element-web
Since what we have implemented is described by that MSC.
…tically Update tests for wording changes
When the labs flag ("Encrypted state events") is enabled, a new option ("Encrypt state events") appears when creating a room.
Whether or not the labs flag is enabled, in a room created with this option, state events are encrypted and decrypted as specified in MSC4362.
People invited to the room later (without MSC4268 enabled) will not be able to decrypt state (e.g. room name) that was sent before they joined.
Checklist
public/exportedsymbols have accurate TSDoc documentation.