Allow guests to operate in encrypted rooms #7314

awesome-michael · 2020-04-21T15:30:07Z

Guests are not able to operate in encrypted rooms and keep getting lots of errors. These changes enable the guests to write in encrypted rooms. I think one problem is that the guest was not allowed to get the member list of the room and therefore had no list which encryption keys he has to request to encrypt his message properly.
Honestly I'm not sure why I needed the change in the events.py file. I tested this configuration and guests can write and read in encrypted rooms now.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Michael Albert [email protected]

Signed-off-by: Michael Albert <[email protected]>

clokep · 2020-05-15T19:27:45Z

@awesome-michael Looks like this needs a newsfile added (and maybe a merge from develop since it has been a bit)? Were you waiting for feedback on the approach?

awesome-michael · 2020-05-18T06:53:09Z

Is a newsfile the same as the changelog mentioned in the checklist?
Do you know if these endpoints were blocked for guests on purpose?

richvdh · 2020-06-05T17:14:38Z

synapse/rest/client/v1/events.py

@@ -81,7 +81,7 @@ def __init__(self, hs):
        self._event_serializer = hs.get_event_client_serializer()

    async def on_GET(self, request, event_id):
-        requester = await self.auth.get_user_by_req(request)
+        requester = await self.auth.get_user_by_req(request, allow_guest=True)


this endpoint is deprecated, and nothing should be using it. See https://matrix.org/docs/spec/client_server/r0.6.1#deprecated-get-matrix-client-r0-events-eventid.

richvdh · 2020-06-05T17:17:44Z

Is a newsfile the same as the changelog mentioned in the checklist?

yes.

Do you know if these endpoints were blocked for guests on purpose?

The list of endpoints which guests can access is specced at https://matrix.org/docs/spec/client_server/r0.6.1#id112. If we want to open up more endpoints, then that is a spec change, which would require a spec-change proposal.

I'm a bit surprised if /rooms/<room_id>/members is the only thing that needs changing to allow guest users to operate in encrypted rooms, but if that's true then certainly it seems to make sense to change the specification and synapse in this way.

richvdh · 2020-07-02T16:01:39Z

@awesome-manuel just checking in on this. are you still interested in working on it?

awesome-michael · 2020-07-03T09:10:59Z

From your last comment I assume there is a spec change needed before this PR can be used. Unfortunately I'm not sure I can write a propper spec-change proposal for that as I'm not enough into the details of the existing guest concept.

richvdh · 2020-07-03T09:54:27Z

Ok, thanks. My impression is that having guests operate in e2e rooms is a fundamentally flawed concept, since there is no way for them to get the keys to the events in the room (which is probably as it should be: there's no point in encrypting events if anyone can turn up as a guest and get the keys to those events).

So for now I'm going to close this, but if you'd like to discuss this in more detail, we can!

awesome-michael · 2020-07-03T13:03:05Z

I kind of disagree. If you dont want guests to join the room thats what the guest join rules are for. Now you force guests (and users that want to communicate with them) to use unencrypted rooms.
How would the keys to event be retrieved (as the events endpoint is deprecated as you pointed out above)?

ptman · 2020-07-03T13:06:01Z

The chat is probably still encrypted (HTTPS), just not e2e encrypted. The hard problem with encryption is usually key management. How do you manage keys with a guest account?

awesome-manuel · 2020-07-03T13:33:40Z

Maybe we should explain our use case in more detail:

We need user accounts that can be invited (e.g. via a link) into a 1-to-1 chat or into a room
Those users are not allowed to register without an invitation
Those users are not allowed to create new rooms on their own

Especially the last point is tricky to enforce with normal user accounts, so we had the idea to use guest accounts for that. The guest accounts are only for temporary conversations and all content can or must be deleted after that conversation.

richvdh · 2020-07-03T17:00:54Z

Ok, this is fair, and I mis-spoke when I said that this was a fundamentally flawed concept; I'd forgotten that guest users could actually join rooms in the same way as normal users.

Now that I look again at the list of what guest users can and can't do, I can't actually see much reason that e2e shouldn't work for them.

So a couple of things here: firstly, have you actually tested this, and confirmed that it is both necessary and sufficient to get e2e working correctly for guest accounts? If so, I'd very much encourage you to create an MSC for this: it shouldn't need much more than creating a document that explains the proposed change and why it is a useful extension to the protocol.

However, I'd also say this: it's unclear to me what role guest accounts actually have in the matrix protocol and where the lines of what you can and can't do end up. That's a larger topic which might bear discussion in #matrix-spec:matrix.org or similar - I don't think this PR is the best place.

Especially the last point is tricky to enforce with normal user accounts,

Have you looked into third_party_event_rules? https://github.com/matrix-org/synapse/blob/master/docs/sample_config.yaml#L2163. It provides a hook where you can say whether a given user is, or is not, allowed to create a room.

awesome-michael · 2020-07-29T17:43:12Z

Now that the MSC is merged, would you open this PR again or should I file a new one?

clokep · 2020-07-29T18:15:54Z

@awesome-michael I think you'll need to merge in develop to get CI to pass. 👍

anoadragon453 · 2020-07-29T23:19:19Z

@awesome-michael Your changelog must end in either .doc, .bugfix, .misc, .feature, .removal or .docker. Please see the contributing guide on this for more info.

For this change, I would probably mark it as a .misc (internal change) or even a .feature. I don't think I would consider it a bug fix.

awesome-michael · 2020-07-30T09:20:46Z

Thanks for that hint.. the error message didn't help me getting there..

richvdh

thank you for bearing with this while the CI completed!

(As a note for future reference: ideally changes like this to the specified behaviour of the API would be backed up with sytest tests to ensure that the new feature doesn't regress, and so that other homeserver implementations also provide the specced behaviour. But in this case that would probably be more work than making the code change, so I'll not insist on it.)

@lugino-emeritus

Synapse 1.19.0rc1 (2020-08-13) ============================== Removal warning --------------- As outlined in the [previous release](https://github.com/matrix-org/synapse/releases/tag/v1.18.0), we are no longer publishing Docker images with the `-py3` tag suffix. On top of that, we have also removed the `latest-py3` tag. Please see [the announcement in the upgrade notes for 1.18.0](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). Features -------- - Add option to allow server admins to join rooms which fail complexity checks. Contributed by @lugino-emeritus. ([\#7902](#7902)) - Add an option to purge room or not with delete room admin endpoint (`POST /_synapse/admin/v1/rooms/<room_id>/delete`). Contributed by @dklimpel. ([\#7964](#7964)) - Add rate limiting to users joining rooms. ([\#8008](#8008)) - Add a `/health` endpoint to every configured HTTP listener that can be used as a health check endpoint by load balancers. ([\#8048](#8048)) - Allow login to be blocked based on the values of SAML attributes. ([\#8052](#8052)) - Allow guest access to the `GET /_matrix/client/r0/rooms/{room_id}/members` endpoint, according to MSC2689. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#7314](#7314)) Bugfixes -------- - Fix a bug introduced in Synapse v1.7.2 which caused inaccurate membership counts in the room directory. ([\#7977](#7977)) - Fix a long standing bug: 'Duplicate key value violates unique constraint "event_relations_id"' when message retention is configured. ([\#7978](#7978)) - Fix "no create event in auth events" when trying to reject invitation after inviter leaves. Bug introduced in Synapse v1.10.0. ([\#7980](#7980)) - Fix various comments and minor discrepencies in server notices code. ([\#7996](#7996)) - Fix a long standing bug where HTTP HEAD requests resulted in a 400 error. ([\#7999](#7999)) - Fix a long-standing bug which caused two copies of some log lines to be written when synctl was used along with a MemoryHandler logger. ([\#8011](#8011), [\#8012](#8012)) Updates to the Docker image --------------------------- - We no longer publish Docker images with the `-py3` tag suffix, as [announced in the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). ([\#8056](#8056)) Improved Documentation ---------------------- - Document how to set up a client .well-known file and fix several pieces of outdated documentation. ([\#7899](#7899)) - Improve workers docs. ([\#7990](#7990), [\#8000](#8000)) - Fix typo in `docs/workers.md`. ([\#7992](#7992)) - Add documentation for how to undo a room shutdown. ([\#7998](#7998), [\#8010](#8010)) Internal Changes ---------------- - Reduce the amount of whitespace in JSON stored and sent in responses. Contributed by David Vo. ([\#7372](#7372)) - Switch to the JSON implementation from the standard library and bump the minimum version of the canonicaljson library to 1.2.0. ([\#7936](#7936), [\#7979](#7979)) - Convert various parts of the codebase to async/await. ([\#7947](#7947), [\#7948](#7948), [\#7949](#7949), [\#7951](#7951), [\#7963](#7963), [\#7973](#7973), [\#7975](#7975), [\#7976](#7976), [\#7981](#7981), [\#7987](#7987), [\#7989](#7989), [\#8003](#8003), [\#8014](#8014), [\#8016](#8016), [\#8027](#8027), [\#8031](#8031), [\#8032](#8032), [\#8035](#8035), [\#8042](#8042), [\#8044](#8044), [\#8045](#8045), [\#8061](#8061), [\#8062](#8062), [\#8063](#8063), [\#8066](#8066), [\#8069](#8069), [\#8070](#8070)) - Move some database-related log lines from the default logger to the database/transaction loggers. ([\#7952](#7952)) - Add a script to detect source code files using non-unix line terminators. ([\#7965](#7965), [\#7970](#7970)) - Log the SAML session ID during creation. ([\#7971](#7971)) - Implement new experimental push rules for some users. ([\#7997](#7997)) - Remove redundant and unreliable signature check for v1 Identity Service lookup responses. ([\#8001](#8001)) - Improve the performance of the register endpoint. ([\#8009](#8009)) - Reduce less useful output in the newsfragment CI step. Add a link to the changelog section of the contributing guide on error. ([\#8024](#8024)) - Rename storage layer objects to be more sensible. ([\#8033](#8033)) - Change the default log config to reduce disk I/O and storage for new servers. ([\#8040](#8040)) - Add an assertion on `prev_events` in `create_new_client_event`. ([\#8041](#8041)) - Add a comment to `ServerContextFactory` about the use of `SSLv23_METHOD`. ([\#8043](#8043)) - Log `OPTIONS` requests at `DEBUG` rather than `INFO` level to reduce amount logged at `INFO`. ([\#8049](#8049)) - Reduce amount of outbound request logging at `INFO` level. ([\#8050](#8050)) - It is no longer necessary to explicitly define `filters` in the logging configuration. (Continuing to do so is redundant but harmless.) ([\#8051](#8051)) - Add and improve type hints. ([\#8058](#8058), [\#8064](#8064), [\#8060](#8060), [\#8067](#8067))

* commit 'b6c6fb795': Allow guests to operate in encrypted rooms (#7314)

Allow guests to operate in encrypted rooms

9ce2ad1

Signed-off-by: Michael Albert <[email protected]>

richvdh reviewed Jun 5, 2020

View reviewed changes

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 5, 2020

richvdh closed this Jul 3, 2020

uhoreg mentioned this pull request Jul 15, 2020

MSC2689: Fix E2EE for guests matrix-org/matrix-spec-proposals#2689

Merged

Remove access to deprecated endpoint

1776b00

richvdh reopened this Jul 29, 2020

richvdh requested a review from a team July 29, 2020 17:47

Add changelog

5d323c2

awesome-michael force-pushed the guest_access branch from 9c1f9b9 to 5d323c2 Compare July 29, 2020 18:08

Merge develop into guest_access

be8b233

Rename changelog

6fa4d0e

clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 30, 2020

This was referenced Jul 30, 2020

Link to the contributing guide if newsfragment check fails #8002

Closed

Synapse: Link to the contributing guide if newsfragment check fails matrix-org/pipelines#82

Closed

richvdh approved these changes Aug 3, 2020

View reviewed changes

richvdh merged commit b6c6fb7 into matrix-org:develop Aug 3, 2020

babolivier pushed a commit that referenced this pull request Sep 1, 2021

Merge commit 'b6c6fb795' into anoa/dinsic_release_1_21_x

d670be7

* commit 'b6c6fb795': Allow guests to operate in encrypted rooms (#7314)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow guests to operate in encrypted rooms #7314

Allow guests to operate in encrypted rooms #7314

awesome-michael commented Apr 21, 2020 •

edited

Loading

clokep commented May 15, 2020

awesome-michael commented May 18, 2020

richvdh Jun 5, 2020

richvdh commented Jun 5, 2020

richvdh commented Jul 2, 2020

awesome-michael commented Jul 3, 2020

richvdh commented Jul 3, 2020

awesome-michael commented Jul 3, 2020

ptman commented Jul 3, 2020

awesome-manuel commented Jul 3, 2020

richvdh commented Jul 3, 2020

awesome-michael commented Jul 29, 2020

clokep commented Jul 29, 2020

anoadragon453 commented Jul 29, 2020

awesome-michael commented Jul 30, 2020

richvdh left a comment

Allow guests to operate in encrypted rooms #7314

Allow guests to operate in encrypted rooms #7314

Conversation

awesome-michael commented Apr 21, 2020 • edited Loading

Pull Request Checklist

clokep commented May 15, 2020

awesome-michael commented May 18, 2020

richvdh Jun 5, 2020

Choose a reason for hiding this comment

richvdh commented Jun 5, 2020

richvdh commented Jul 2, 2020

awesome-michael commented Jul 3, 2020

richvdh commented Jul 3, 2020

awesome-michael commented Jul 3, 2020

ptman commented Jul 3, 2020

awesome-manuel commented Jul 3, 2020

richvdh commented Jul 3, 2020

awesome-michael commented Jul 29, 2020

clokep commented Jul 29, 2020

anoadragon453 commented Jul 29, 2020

awesome-michael commented Jul 30, 2020

richvdh left a comment

Choose a reason for hiding this comment

awesome-michael commented Apr 21, 2020 •

edited

Loading