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.

Sign up for GitHub

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

MSC2176: Update the redaction rules #2176

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd5549d
Proposal to update the redaction algorithm
richvdh Jul 14, 2019
b49a950
Update proposals/2176-update-redaction-rules.md
richvdh Jul 16, 2019
d324cac
preserve powerlevel
richvdh Jul 16, 2019
9e264fe
Updates
richvdh Jul 22, 2019
f1f2936
Apply suggestions from code review
richvdh Jul 30, 2019
353b6cd
clarification
richvdh Aug 12, 2019
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
48 changes: 48 additions & 0 deletions proposals/2176-update-redaction-rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# 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 should be *removed* from the list of those to be
richvdh marked this conversation as resolved.
Show resolved Hide resolved
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.)

Ratinale: neither of the above properties have defined meanings in the Matrix
richvdh marked this conversation as resolved.
Show resolved Hide resolved
richvdh marked this conversation as resolved.
Show resolved Hide resolved
protocol, so there is no reason for them to be special-cased in this way.

The following should be added to the list of subkeys of the content property
richvdh marked this conversation as resolved.
Show resolved Hide resolved
which should be preserved:
richvdh marked this conversation as resolved.
Show resolved Hide resolved

* `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.create` should allow the `room_version` key. Currently, redacting an
`m.room.create` event will make the room revert to a v1 room.
richvdh marked this conversation as resolved.
Show resolved Hide resolved

* `m.room.power_levels` should allow the `notifications` key. Rationale:
richvdh marked this conversation as resolved.
Show resolved Hide resolved
symmetry with the other `power_levels` settings. (Maybe? See
https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.)

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 •
edited
Loading

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.


## Potential issues

What if there is spam in sub-properties of the `notifications` property of
power-levels? Should we not be able to redact it?
richvdh marked this conversation as resolved.
Show resolved Hide resolved