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

Open
wants to merge 6 commits into
base: master
from

Conversation

@richvdh
Copy link
Member

commented Jul 14, 2019

@richvdh richvdh added the proposal label Jul 14, 2019

@ara4n

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

looks plausible to me. sounds like we should get it out there in a room v6 sooner than later

@turt2live turt2live self-requested a review Jul 14, 2019

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

We probably need to get agreement on #1996 and #1601 as a prerequisite here. If people have opinions on them, perhaps they could weigh in on the relevant bugs.

@bwindels

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

For completeness, there is also some ongoing discussion as part of MSC1849/2154 about preserving content/m.relates_to/rel_type on redaction for at least edits, if not all relations.

proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

left a comment

lgtm - I'm more torn on whether notifications on power levels should be kept though.

proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved

richvdh and others added some commits Jul 16, 2019

Update proposals/2176-update-redaction-rules.md
fix typo

Co-Authored-By: Kitsune Ral <Kitsune-Ral@users.sf.net>
@richvdh

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

A quick note on some of my thinking here.

As I see it, there is a scale between properties which we absolutely must preserve on redaction, for fear of breaking the protocol (mostly those involved in event auth), and those which have very little effect on the protocol at all so can safely be redacted. Somewhere in the middle we have things like predecessor, which probably isn't really a thing you want to redact, but actually the protocol will mostly keep working.

I'm trying quite hard to limit the list of things that get preserved to things which completely break the protocol, rather than just things that are a bit annoying/surprising when you redact them, mostly to keep the server-server protocol as simple as possible. The fact that we have to bump the room version to change the redaction rules also means that it's really not great to have to keep adding new rules. Rather, I see it as the job of either clients (or, possibly, the C-S API) to protect you from foot-guns.

All that said, there is room for debate on where different keys sit on that scale, and where we draw the line, but I'd like each candidate to be retained to be argued on its own merits.

* the `notifications` key. Rationale: symmetry with the other `power_levels`
settings. (Maybe? See
https://github.com/matrix-org/matrix-doc/issues/1601#issuecomment-511237744.)

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 17, 2019

Member

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

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 17, 2019

Author Member

I disagree. It doesn't change the behaviour of the room at the server-server level.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 17, 2019

Member

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

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 17, 2019

Member

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.

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 22, 2019

Author Member

@erikjohnston: yeeesss... I'm not quite clear what your point is though?

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 26, 2019

Member

I still don't quite understand what a client should do if it loads a room (having no previous state) and receives m.room.encryption with empty algorithm (right away, again, no previous state). Should it consider the room encrypted at all? If yes, which algorithm should it assume?

This comment has been minimized.

Copy link
@richvdh

richvdh Jul 30, 2019

Author Member

@KitsuneRal this isn't a question that is limited to redactions, but: I would suggest that a client warns a user that the room state is invalid.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 12, 2019

Author Member

I'm going to resolve this thread; if people feel strongly that I'm making the wrong call on this, then reopen/start a new one.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Aug 20, 2019

Member

I thought that I sent this comment but actually it got stuck behind the unpressed "Submit review" button (and then GitHub tucked it back where I drafted it). Hope it's placed properly now.

I think I understand where the disconnect is: I'm afraid I don't quite buy the argument that for the sake of reducing the change surface we're restricting redaction exclusions to server interaction pieces. It's the clients ultimately sending and using those events, so redaction should at least somewhat protect basic room integrity from the client perspective as well.

Now the state can be broken in a lot of different ways and (I totally agree here) clients ought to warn users about things that may break their experience; but when it comes to m.room.encryption, there's no good way for a client to handle any readable content of the room once that state is broken.

To reiterate, the case in point is not about repeated m.room.encryption events; it's about a client joining/loading a room where m.room.encryption is already redacted. From the client perspective, this room is irreparably broken along with all of its encrypted messages already in DAG.

Going as far as imagining a m.room.encryption event with obscene/illegal contents for "algorithm" sent to the previously unencrypted room (again - repeated encryption events are not proper and I don't consider them here), in that case the room also gets broken for everyone but no user content is lost - because the room was unencrypted and now it is "encrypted" with nonsensical algorithm. In such case it would be great to have algorithm redacted even though the room has to be thrown out, in order to eliminate improper content from DAG. Ideally such events should not be accepted to DAG in the first place - then the case for the breakage and redaction wouldn't even emerge.

Updates
* preserve *all* of `create`
* don't preserve `notifications` or `algorithm`, and add some justifcation.

@turt2live turt2live self-requested a review Jul 22, 2019

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

I've updated the MSC to (1) change the create event so that all keys are preserved, and (2) clarify that I don't think the notifications property should be preserved, and (2b) to record why I think this is important.

Further input welcome...

proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved

* 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

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 23, 2019

Member

It's true the event graph doesn't care, however the historical context of rooms in clients will be affected by this decision. Currently if one pings the room and redacts the power levels, the client has no choice but to de-highlight the notification. This is less relevant for push notifications as they are already executed prior to the power level event being redacted (it'd be weird and unlikely that someone pings the room then immediately redacts the power levels), however when the user is scrolling back through the room or searching they will have lost highlighting on the event.

tldr: we should preserve it for the purposes of historically accurate highlighting of the room.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 26, 2019

Member

Is there any difference to redacting, say, a room topic? It also affects user interface, and clients should explicitly process that, and few do it correctly.

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 26, 2019

Member

The slight difference is that the topic should absolutely be redactable for when someone abuses the text field as a means for sending abuse. The redaction algorithm is also meant to give people the ability to remove harmful content/messages, however whether a message is highlighted or not doesn't really change that.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 12, 2019

Author Member

I'm not sure this is a strong enough argument for me to want to include it in the list. I'll accept lobbying though...

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 19, 2019

Member

My lobbying budget seems to have run out for the quarter :(

If the arguments above aren't convincing enough, there's nothing new I can provide.

@@ -0,0 +1,96 @@
# MSC2176: Update the redaction rules

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 23, 2019

Member

Should m.room.guest_access preserve guest_access? If it doesn't, what does it mean to redact a guest access policy of can_join? Ignoring the fact that redacting most state events violates the schemas (name for instance is required on the room name event), the guest access event defaults to forbidden: are all guests kicked from the room now? How does a remote server even validate that a guest account is a guest account?

I believe this is the only state event which raises questions about authorization: the remainder are already covered by the proposal or the existing algorithm.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 12, 2019

Author Member

remote servers can't distinguish between a guest account and a regular user account, so m.room.guest_access isn't involved in the auth checks done on events received over federation (see also: https://matrix.org/docs/spec/rooms/v1#authorization-rules). I'd therefore argue that it's less important to preserve its value than it is for power_levels and friends.

To answer your other questions:

If it doesn't, what does it mean to redact a guest access policy of can_join.... the guest access event defaults to forbidden: are all guests kicked from the room now?

according to https://matrix.org/docs/spec/client_server/r0.5.0#id159: yes, although that certainly isn't implemented in synapse currently. (It's also not implemented when the guest access changes due to state resolution rather than due to a new event).

Ignoring the fact that redacting most state events violates the schemas

yeah; this is a general problem. It begins to interact with validation of events in general (see also: #1646): the current situation is that there is no verification that state events received over federation match the schema, so we have to be prepared to handle events that do not match the schema anyway.

@KitsuneRal
Copy link
Member

left a comment

Typo fixing - however, might be entirely unnecessary if we keep algorithm around :)

proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved

richvdh and others added some commits Jul 30, 2019

Apply suggestions from code review
Co-Authored-By: Travis Ralston <travpc@gmail.com>
Co-Authored-By: Kitsune Ral <Kitsune-Ral@users.sf.net>
@richvdh

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Nothing else seems to be happening here, so I'd like to propose FCP:

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.