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

MSC3061: Sharing room keys for past messages #3061

Open
wants to merge 4 commits into
base: old_master
Choose a base branch
from

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Mar 12, 2021

@uhoreg uhoreg changed the title MSCxxxx: Sharing room keys for past messages MSC3061: Sharing room keys for past messages Mar 12, 2021
@uhoreg uhoreg added e2e proposal A matrix spec change proposal proposal-in-review labels Mar 12, 2021
@uhoreg
Copy link
Member Author

uhoreg commented Mar 16, 2021

Implemented by matrix-org/matrix-js-sdk#1640

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 8, 2021
jryans added a commit to matrix-org/matrix-react-sdk that referenced this pull request Apr 9, 2021
After discussion with Product, we're ready to enable this key sharing work from
#5763 (based on
matrix-org/matrix-spec-proposals#3061).
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@cvwright
Copy link

cvwright commented Aug 4, 2021

Implemented by matrix-org/matrix-js-sdk#1640

Is anyone working on this for matrix-ios-sdk?

## Security considerations

Clients should still ensure that keys are only shared with authorized users and
devices.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? How does the client know, that a user should be in the room and wasn't maliciously added by the homeserver? Even if you set a room to shared history, you probably only want members to read the history, that you accepted as having been allowed into the room. Considering the end aspect in e2ee, we want the client to approve of a member having joined the room, before sharing history. We don't want to automatically share keys with anyone, who joins the room, because the server could for a short time "fake" a user in the room and as such get access to all the history and then make the user disappear again. Then encrypting the room would be useless. Should we require prompting the user once before sharing past history with a new user in the room?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is sort of a generic "make sure that you don't accidentally over-share when you implement this". In this case, clients only share the keys with users that the client invites, so it's decided that the user should be authorized. For determining which devices are authorized, in general, the client should probably just do what it normally does for determining which devices to send keys to.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a really scary trap, because a client dev might forget, that the servers device list and member list isn't really that trustworthy. We somewhat get around that by warning for unverified devices, when sending a message, but that will never work for sharing past message keys.

Copy link
Member

Choose a reason for hiding this comment

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

The proper solution here is to base the decision whether to share on the trust state of the user we're inviting.

If the invited user is untrusted (in cross-signing terms), the client should at least display a warning along with a list of devices that would get the keys. This would allow the inviter to at least review the list and consciously proceed with the action if they want to do so.

If the invited user is trusted, but some of their devices are not verified yet, the client should again display a warning with a list of those devices, again asking whether to proceed.

If the user is trusted and all of their devices are verified, then no warning is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

So only the inviter will ever share keys?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't have to be just the inviter who shares keys; others can share keys as well, but it's up to them to ensure that they do so securely. Spec-wise, we try to avoid telling clients exactly how to handle keys, as different clients may wish to make different tradeoffs. But I will add a comment about checking device trust before sharing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some more words here. Hopefully it's clearer now.

@richvdh
Copy link
Member

richvdh commented Nov 16, 2022

Given that this is now implemented in a bunch of clients, can we land it?

@uhoreg
Copy link
Member Author

uhoreg commented Dec 21, 2022

Given that this is now implemented in a bunch of clients, can we land it?

I think that I've addressed all the comments, so it should be pretty much OK to land now. But given that some of the comments were non-trivial, I don't think it's fair to try to push for FCP when many people are on holiday. So I'll probably give it a few weeks into the New Year, and if it still looks fine, I'll make the FCP call then.

The suggested behaviour in this MSC is to only share additional keys when
inviting another user. This does not allow users who join the room but were
not invited (for example, if membership is restricted to another space, or if
the room is publicly joinable) to receive the keys. Also, if the inviter does
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps worth noting that a quick workaround for this would be knocking: if rather than making rooms restricted-join, instead we had a restricted-knock, then users in the permitted group would be allowed to knock... and whoever answers the knock would then have the responsibility to share all the keys with them.

Copy link
Member

Choose a reason for hiding this comment

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

MSC3787 (in room version 10) added a knock_restricted join rule which lets users knock or join via the join rule restrictions. I think you're suggesting allowing them to knock if they meet the restrictions though?

@kristovatlas
Copy link

kristovatlas commented Jan 21, 2023

I see client implementations but not server ones. Does this require making server changes as well or can it be implemented purely client-side? Context: I work on a fork of dendrite and matrix-js-sdk and want this feature.

@uhoreg
Copy link
Member Author

uhoreg commented Jan 24, 2023

It's entirely client-side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants