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

MSC3946: Dynamic room predecessor #3946

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Dec 9, 2022

A proposal for specifying a room's predecessor after the fact.

Rendered

Client implementations:

@MadLittleMods MadLittleMods changed the title MSC0000: Dynamic room predecessor MSC3946: Dynamic room predecessor Dec 9, 2022

key | type | value | description | required
--- | --- | --- | --- | ---
`predecessor_room_id` | string | Room ID | A reference to the room that came before and was replaced by this room | yes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

predecessor_room_id is a bit wordy but plainly describes what it is and what the value should be 🤷

@MadLittleMods MadLittleMods marked this pull request as ready for review December 9, 2022 07:03
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Dec 12, 2022
type.


## Security considerations
Copy link
Member

@turt2live turt2live Jan 5, 2023

Choose a reason for hiding this comment

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

I'm a bit worried this MSC introduces higher risk of security issues in clients regarding room takeovers. Currently, the relationship between the previous and current room can be verified (barring state resets), giving the client confidence that room B is in fact the most recent edition of room A. With the relationship being mutable, clients are likely to not check the predecessor correctly and therefore allow someone to hide/take over non-tombstoned rooms.

The current relationship of rooms is very linear, and though this MSC says the mutable event takes precedence, not all clients will have updated to consider that point. This can lead to inconsistencies in what users see, or abusive scenarios where rooms are hidden from some people and not others.

Copy link
Member

Choose a reason for hiding this comment

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

(I think we race conditioned a bit and share more-or-less the same concern, just worded a bit differently.)

Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly more concerned about clients getting the implementation wrong, despite warnings in the spec, though yes: the root issue is "mutable is scary", I think.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 10, 2023

Choose a reason for hiding this comment

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

Currently, the relationship between the previous and current room can be verified (baring state resets), giving the client confidence that room B is in fact the most recent edition of room A.

There is nothing "verifiable" about the m.room.create predecessor value besides that it came from someone with proper power levels. A Matrix room can be created with whatever predecessor desired (even multiple rooms with the same predecessor). And same for the tombstone from some old room pointing to a new room. And as a note in case onlookers aren't as familiar, tombstones are already mutable.

To me, the chain of tombstone and predecessor are mere suggestions of where you can continue looking at history in the room. I'm not quite following the line of thought to get the severity with danger in any of this.

All of these problems like selectively sharing the state only to certain servers or state resets seem completely separate to this MSC. If we can't trust the state in a room, then everything falls apart.

Copy link
Member

Choose a reason for hiding this comment

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

The presence of a tombstone pointing to the new room and a predecessor pointing to the tombstoned room forms a verifiable relationship - this was specifically designed in for room upgrades.

With this MSC, we'd be breaking that relationship we previously went out of our way to make verifiable - this is where the concern is coming from. If we're breaking the verifiable nature of the relationship, it needs to be for justifiable/secure reasons.

We already acknowledge that state resets and similar can cause the relationship to be broken, however the expectation in those cases is that clients actually split the room into two logical rooms again, even if half the relationship still exists. With both halves of the relationship, the client can merge the rooms together safely.

Copy link
Member

Choose a reason for hiding this comment

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

How is this a takeover? Above all, this is just a client display thing. And second, it looks like one room needs the tombstone and other one needs a predecessor for things to look replaced. Both sides acknowledged each other for this to happen. Anything other than this seems like a bad assumption on the clients part.

The case I'm concerned about isn't when both rooms point at each other, but rather when a client developer inevitably makes a mistake in the implementation of this MSC. There's already significant risk of clients getting the implementation wrong, and making the predecessor dynamic opens the door wider for mistakes (despite warnings in the spec, which often go unheeded). This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 10, 2023

Choose a reason for hiding this comment

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

This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

This seems like it would be a client doing something very wrong. We can't help a client that doesn't look at both tombstone and predecessor to see that they match. That's a bare minimum even with the current state of tombstone/predecessor if you're making rooms look replaced. A client can show the pointer forward/backward without this verification though.

Even a naive client implementation that only looks at the create event for the predecessor is safe in the new world that this MSC creates.

If a client implementation updates to look at m.room.predecessor, then that is also pretty fool-proof regardless of which era of state they look at. The only tricky thing I could think of is if we also lived in a MSC3779 world where someone doesn't check for the canoncial version with state_key: "" but that's not a concern yet (and is a general problem).

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

I don't think this MSC increases the risk level. We have the exact same risk with the current tombstone/predecessor state of things.

And to address that one aspect, it sounds we should consider another MSC to verify this server-side so clients can more easily rely on eating whatever we feed it.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

This would be interesting to find. We should start a new thread if we come up with anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

One datapoint around verifiability: I found a bug in matrix-js-sdk's code for verifying links: matrix-org/matrix-js-sdk#3089 . The fact that this has remained undiscovered for a long time may imply that people are not paying a great deal of attention to verifiability.


key | type | value | description | required
--- | --- | --- | --- | ---
`predecessor_room_id` | string | Room ID | A reference to the room that came before and was replaced by this room | yes
Copy link
Contributor

Choose a reason for hiding this comment

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

@MadLittleMods did you consider including event_id like in the create event? Some code in matrix-react-sdk expects to be able to have that. I'm not sure yet what the implications would be of not providing it when looking for a room's predecessor. (I imagine we can work around it, but I haven't looked into it yet)

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 30, 2023

Choose a reason for hiding this comment

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

did you consider including event_id like in the create event?

Didn't know about it so no consideration was given to it so far. For my own reference: https://spec.matrix.org/v1.5/client-server-api/#mroomcreate

event_id (Required): The event ID of the last known event in the old room.
room_id (Required): The ID of the old room.

Adding an optional last_known_event_id field seems fine 👍

I assume it's not related to the upgrade verification part at all since we should be looking at the latest state in the room. But this sort of info could be nice as one option for a continuation point to jump to in the old room.

It terms of workaround fallbacks, we can just go to where the latest tombstone in the old room is or just the latest in the old room. One more complicated option is to use the /timestamp_to_event endpoint to find the latest event from when the predecessor was sent but doesn't seem necessary or the right thing to do exactly.

This comment was marked as off-topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I understand the use cases, are my assumptions above correct? Are there other use cases? Is this useful?

@justjanne

This comment was marked as duplicate.

key | type | value | description | required
--- | --- | --- | --- | ---
`predecessor_room_id` | string | Room ID | A reference to the room that came before and was replaced by this room | yes
`via_servers` | [string] | List of server names | The servers to attempt to join the room through. These servers should be participating in the room to be useful. This option is necessary since room ID's are not routable on their own.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be via for consistency with already specced shapes, e.g. m.space.child. https://spec.matrix.org/v1.5/client-server-api/#mspacechild

Copy link
Contributor Author

@MadLittleMods MadLittleMods Feb 9, 2023

Choose a reason for hiding this comment

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

There is also /join/{roomIdOrAlias}?server_name=foo.bar.

I can update this to be whatever name we want to start being consistent about. Thanks for that link, https://spec.matrix.org/v1.5/appendices/#routing is also an interesting reference.

I chose via_servers because it seemed like the best plainly readable thing and pretty much self-explains what the content should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec 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.

6 participants