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

MSC2176: Update the redaction rules #2176

Merged
merged 6 commits into from
Oct 7, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions proposals/2176-update-redaction-rules.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
# MSC2176: Update the redaction rules
richvdh marked this conversation as resolved.
Show resolved Hide resolved
uhoreg marked this conversation as resolved.
Show resolved Hide resolved

The current [redaction
algorithm](https://matrix.org/docs/spec/client_server/r0.5.0#redactions) is now
somewhat dated. This MSC proposes a number of changes to the rules which will
improve the security and reliability of the Matrix protocol.

## Proposal

The following changes will require a new room version, since changes to the
redaction algorithm also change the way that [event
hashes](https://matrix.org/docs/spec/server_server/r0.1.2#calculating-the-reference-hash-for-an-event)
(and hence event IDs) are calculated.

The following *event* keys are to be *removed* from the list of those to be
preserved by a redaction:

* `membership`
* `prev_state`

(Note this refers to the *event-level* `membership` property, rather than the
richvdh marked this conversation as resolved.
Show resolved Hide resolved
similarly-named sub-property under the `content` key.)

Rationale: neither of the above properties have defined meanings any more in the Matrix
protocol, so there is no reason for them to be special-cased in this way.

The following are to be added to the list of subkeys of the content property
which are preserved:

* `m.room.create` preserves *all* content. Rationale: the values in a
`create` event are deliberately intended to last the lifetime of the room,
and if values are redacted, there is no way to add correct settings
afterwards. It therefore seems non-sensical to allow redaction of a `create`
event.

* `m.room.redaction` should allow the `redacts` key (assuming
[MSC2174](https://github.com/matrix-org/matrix-doc/pull/2174) is merged).
Rationale: currently, redacting a redaction can lead to inconsistent results
among homservers, depending on whether they receive the `m.room.redaction`
result before or after it is redacted (and therefore may or may not redact
the original event).

* `m.room.power_levels` should allow (in addition to the keys already listed
in the spec):

* the `invite` key. Rationale: this is required to authenticate
`m.room.member` events with the `invite` membership. Currently, redacting
a `power_levels` event will mean that such events cannot be authenticated,
potentially leading to a split-brain room.

## Other properties considered for preservation

Currently it is *not* proposed to add these to the list of properties which are
proposed for a redaction:

* The `notifications` key of `m.room.power_levels`. Unlike the other
properties in `power_levels`, `notifications` does not play a part in
authorising the events in the room graph. Once the `power_levels` are
richvdh marked this conversation as resolved.
Show resolved Hide resolved
replaced, historical values of the `notifications` property are
irrelevant. There is therefore no need for it to be protected from
redactions.

* The `algorithm` key of `m.room.encryption`. Again, historical values of
`m.room.encryption` have no effect, and servers do not use the value of the
property to authenticate events.
Comment on lines +63 to +65
Copy link

Choose a reason for hiding this comment

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

This might be useful for backfilling into E2EE rooms.


The effect of redacting an `m.room.encryption` event is much the same as that
of sending a new `m.room.encryption` event with no `algorithm` key. It's
unlikely to be what was intended, but adding rules to the redaction
algorithm will not help this.

### Background to things not included in the proposal

The approach taken here has been to minimise the list of properties preserved
by redaction; in general, the list is limited to those which are required by
servers to authenticate events in the room. One reason for this is to simplify
the implementation of servers and clients, but a more important philosophical
reason is as follows.

Changing the redaction algorithm requires changes to both servers and clients,
so changes are difficult and will happen rarely. Adding additional keys now
sets an awkward precedent.

It is likely that in the future more properties will be defined which might be
convenient to preserve under redaction. One of the two scenarios would then
happen:

Copy link
Member

Choose a reason for hiding this comment

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

m.room.encryption's algorithm should be preserved.

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 disagree. It doesn't change the behaviour of the room at the server-server level.

Copy link
Member

@KitsuneRal KitsuneRal Jul 17, 2019

Choose a reason for hiding this comment

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

But if we eventually have two algorithms rather than one how should a client treat m.room.encryption with redacted algorithm?

Copy link
Member Author

Choose a reason for hiding this comment

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

But if we eventually have two algorithms rather than one

I don't think there is a way to end up with two encryption algorithms in the room state?

how should a client treat m.room.encryption with redacted algorithm?

The same way that they should treat a new m.room.encryption event which disables encryption: with great suspicion.

Copy link
Member

Choose a reason for hiding this comment

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

How are new clients to the room meant to do encryption if they don't know the algorithm?

Can we please relax the restrictions on what goes into the algorithm? It affects client authors too, and it's ridiculous to exclude something because it doesn't affect the core protocol operation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KitsuneRal: I think there might be a misunderstanding here.

there's no good way for a client to handle any readable content of the room once that state is broken
...
this room is irreparably broken along with all of its encrypted messages already in DAG.

m.room.encryption is not supposed to define how you interpret received events, so I don't agree that the existing messages are invalidated once m.room.encryption is redacted.

again - repeated encryption events are not proper

there is nothing wrong with repeated m.room.encryption events?

Copy link
Member

Choose a reason for hiding this comment

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

m.room.encryption is not supposed to define how you interpret received events, so I don't agree that the existing messages are invalidated once m.room.encryption is redacted.

That's a pretty strong statement. Then what's the purpose of algorithm there?

there is nothing wrong with repeated m.room.encryption events?

Except they should be treated with great suspicion because there's no defined semantics for any m.room.encryption event except the first one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then what's the purpose of algorithm there?

It indicates to clients how they should encrypt messages in this room.

there's no defined semantics for any m.room.encryption event except the first one?

I don't think that's true. I can't find anything in the spec about this, but my mental model is that clients should keep a record of rooms which are encrypted (along with the encryption algorithm), and give the user a warning if they send a message in a room which was previously encrypted and is no longer (or if the encryption is 'weaker', for some handwavey definition of weaker).

But certainly it's valid for there to be multiple encryption events in a room, changing minor settings.

Copy link
Member

Choose a reason for hiding this comment

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

There's also the problem of clients joining a room which has a redacted encryption event: They won't have the prior knowledge that it was encrypted and will assume it's not.

ftr riot-web does exactly as you describe: it ignores changes to the encryption event it wasn't expecting.

Copy link
Member

Choose a reason for hiding this comment

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

I've created https://github.com/matrix-org/matrix-doc/issues/2272 for clarifying the semantics of m.room.encryption events. I'd suggest ignoring m.room.encryption for the purposes of this MSC, and discussing in that issue for a future MSC, as I think that it's a larger issue that shouldn't hold this one up.

* We would be forced to issue yet more updates to the redaction algorithm,
with a new room versions and mandatory updates to all servers and clients, or:

* We would end up with an awkward asymmetry between properties which were
preserved under this MSC, and those which were introduced later so were not
preserved.

In short, I consider it important for the elegance of the Matrix protocol that
we do not add unnecessary properties to the list of those to be preserved by
redaction.