From cd5549d483782d9171f3c810974786b7672a9d95 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 14 Jul 2019 22:50:46 +0100 Subject: [PATCH 1/6] Proposal to update the redaction algorithm --- proposals/2176-update-redaction-rules.md | 48 ++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 proposals/2176-update-redaction-rules.md diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md new file mode 100644 index 0000000000..a939178a5e --- /dev/null +++ b/proposals/2176-update-redaction-rules.md @@ -0,0 +1,48 @@ +# MSC2176: Update the redaction rules + +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 +preserved by a redaction: + + * `membership` + * `prev_state` + +(Note this refers to the *event-level* `membership` property, rather than the +similarly-named sub-property under the `content` key.) + +Ratinale: neither of the above properties have defined meanings in the Matrix +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 +which should be preserved: + + * `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. + + * `m.room.power_levels` should allow the `notifications` key. Rationale: + symmetry with the other `power_levels` settings. (Maybe? See + https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.) + + +## 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? From b49a95024580e77bf45e04913f4bab4aded8e932 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 16 Jul 2019 19:26:02 +0100 Subject: [PATCH 2/6] Update proposals/2176-update-redaction-rules.md fix typo Co-Authored-By: Kitsune Ral --- proposals/2176-update-redaction-rules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md index a939178a5e..d32c9cdabf 100644 --- a/proposals/2176-update-redaction-rules.md +++ b/proposals/2176-update-redaction-rules.md @@ -21,7 +21,7 @@ preserved by a redaction: (Note this refers to the *event-level* `membership` property, rather than the similarly-named sub-property under the `content` key.) -Ratinale: neither of the above properties have defined meanings in the Matrix +Rationale: neither of the above properties have defined meanings in the Matrix 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 From d324cac847507f9527ce53cd224fa6bc0f873015 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 16 Jul 2019 19:32:34 +0100 Subject: [PATCH 3/6] preserve powerlevel --- proposals/2176-update-redaction-rules.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md index d32c9cdabf..14c97da0cb 100644 --- a/proposals/2176-update-redaction-rules.md +++ b/proposals/2176-update-redaction-rules.md @@ -37,9 +37,16 @@ which should be preserved: * `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. - * `m.room.power_levels` should allow the `notifications` key. Rationale: - symmetry with the other `power_levels` settings. (Maybe? See - https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.) + * `m.room.power_levels` should allow: + + * 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. + + * the `notifications` key. Rationale: symmetry with the other `power_levels` + settings. (Maybe? See + https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.) ## Potential issues From 9e264fedc964c5ed44f39e7ddc23e68c5fd48486 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 22 Jul 2019 16:47:49 +0100 Subject: [PATCH 4/6] Updates * preserve *all* of `create` * don't preserve `notifications` or `algorithm`, and add some justifcation. --- proposals/2176-update-redaction-rules.md | 59 ++++++++++++++++++++---- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md index 14c97da0cb..dea3015f8c 100644 --- a/proposals/2176-update-redaction-rules.md +++ b/proposals/2176-update-redaction-rules.md @@ -27,6 +27,12 @@ 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 which should be preserved: + * `m.room.create` should preserve *all* content. Rationale: the values in a + `create` event are deliberately intented 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 @@ -34,9 +40,6 @@ which should be preserved: 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. - * `m.room.power_levels` should allow: * the `invite` key. Rationale: this is required to authenticate @@ -44,12 +47,50 @@ which should be preserved: a `power_levels` event will mean that such events cannot be authenticated, potentially leading to a split-brain room. - * the `notifications` key. Rationale: symmetry with the other `power_levels` - settings. (Maybe? See - https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.) +## 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 + 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. + + The effect of redacting an `m.room.redaction` event is much the same as that + of sending a new `m.room.redaction` 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: + * 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: -## Potential issues + * 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. -What if there is spam in sub-properties of the `notifications` property of -power-levels? Should we not be able to redact it? +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. From f1f293678bcf1eaaca05a6748bf85639796e4f2b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 30 Jul 2019 08:00:48 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-Authored-By: Travis Ralston Co-Authored-By: Kitsune Ral --- proposals/2176-update-redaction-rules.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md index dea3015f8c..75bfc9719d 100644 --- a/proposals/2176-update-redaction-rules.md +++ b/proposals/2176-update-redaction-rules.md @@ -12,7 +12,7 @@ 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 +The following *event* keys are to be *removed* from the list of those to be preserved by a redaction: * `membership` @@ -21,14 +21,14 @@ preserved by a redaction: (Note this refers to the *event-level* `membership` property, rather than the similarly-named sub-property under the `content` key.) -Rationale: neither of the above properties have defined meanings in the Matrix +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 should be added to the list of subkeys of the content property -which should be preserved: +The following are to be added to the list of subkeys of the content property +which are preserved: - * `m.room.create` should preserve *all* content. Rationale: the values in a - `create` event are deliberately intented to last the lifetime of the room, + * `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. @@ -63,8 +63,8 @@ proposed for a redaction: `m.room.encryption` have no effect, and servers do not use the value of the property to authenticate events. - The effect of redacting an `m.room.redaction` event is much the same as that - of sending a new `m.room.redaction` event with no `algorithm` key. It's + 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. From 353b6cd198aa4f1cb13fd7d3dcfceba323151f3f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 12 Aug 2019 13:10:22 +0100 Subject: [PATCH 6/6] clarification --- proposals/2176-update-redaction-rules.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/proposals/2176-update-redaction-rules.md b/proposals/2176-update-redaction-rules.md index 75bfc9719d..574028f218 100644 --- a/proposals/2176-update-redaction-rules.md +++ b/proposals/2176-update-redaction-rules.md @@ -40,7 +40,8 @@ which are preserved: result before or after it is redacted (and therefore may or may not redact the original event). - * `m.room.power_levels` should allow: + * `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