[E2E] Encrypted rooms support #235
Conversation
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Looking good in general. :)
| elif etype == "m.room.guest_access": | ||
| self.guest_access = econtent["guest_access"] == "can_join" | ||
| elif etype == "m.room.encryption": | ||
| if econtent.get("algorithm") == "m.megolm.v1.aes-sha2": |
There was a problem hiding this comment.
According to the e2e implementation guide, we also need to handle events with algorithm = m.olm.v1.curve25519-aes-sha2. Not sure if that's true though, since the section right above about enabling e2e says that only m.megolm.v1.aes-sha2 is permitted. Could you clarify this please?
There was a problem hiding this comment.
Hm, i'm surprised the guide says you need to implement both. I'm not aware of any legitimate use for pure olm encryption within a room currently, other than compatibility with ~2016 vintage Matrix E2E. Pure olm is of course used for toDevice messages however.
There was a problem hiding this comment.
@non-Jedi I can't find where this would be written in the guide, maybe you confused m.room.encryption and m.room.encrypted?
Olm encrypted events are indeed handled as toDevice events in later parts.
There was a problem hiding this comment.
Yep. @Zil0 got it. Exactly what happened. (wish github had a way to mark specific comment chains as resolved rather than just whole reviews...)
matrix_client/api.py
Outdated
| content={"messages": messages} | ||
| ) | ||
|
|
||
| def get_encryption_algorithm(self, room_id): |
There was a problem hiding this comment.
Can this be a generic get_state_event method instead? where URL is just built from event_type?
matrix_client/client.py
Outdated
| room = Room(self, room_id) | ||
| if self.encryption: | ||
| try: | ||
| self.api.get_encryption_algorithm(room_id) |
There was a problem hiding this comment.
This technically needs to check whether the room was ever encrypted and whether the algorithm was valid, right?
There was a problem hiding this comment.
Absolutely, this was really bad.
|
Done |
| if self._encryption: | ||
| try: | ||
| event = self.api.get_state_event(room_id, "m.room.encryption") | ||
| if event["content"]["algorithm"] == "m.megolm.v1.aes-sha2": |
There was a problem hiding this comment.
Do you need to check that event has a content property, and that event["property"] has an algorithm property (or else handle a KeyError exception), or is there some Python magic going on here?
There was a problem hiding this comment.
Mmmmh, there are no such checks because this field is required as per https://matrix.org/speculator/spec/drafts%2Fe2e/client_server/unstable.html#id326. The SDK is generally not robust against malformed events, though your concern is still valid, especially considering how weird it is for such a method to potentially raise a KeyError. Maybe @non-Jedi has some thoughts?
There was a problem hiding this comment.
If the rest of the python-sdk does the same thing as this, maybe it's fine to merge as-is.
There was a problem hiding this comment.
Yep. In absence of a strong typing system, we assume JSON follows matrix spec. Think this is fine unless the "content" or "algorithm" fields are optional.
4 participants
We need to know with maximum reliability if a room is encrypted. No crypto stuff in this one.
Room.__init__, and add acheck_encryptionparameter? In_mkroomwe could then writeRoom(self, room_id, check_encryption=self.encryption). Not implementing this right away because I'm not sure this is the perfect approach either.)Some of those things won't get triggered on low
cache_level. Maybe we should add a warning somewhere?Also, if encryption is not enabled when instantiating MatrixClient, unencrypted messages are sent to encrypted rooms just like before. Once the encryption has fully landed, maybe this stops being an expected behaviour and should be disabled by default, with some kind of explicit
insecure=Trueflag to enable it.Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>