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

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jul 14, 2019

@richvdh richvdh added the proposal A matrix spec change proposal label Jul 14, 2019
@ara4n
Copy link
Member

ara4n 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 July 14, 2019 22:02
@richvdh
Copy link
Member Author

richvdh commented Jul 15, 2019

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

@bwindels
Copy link
Contributor

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
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

richvdh 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.)

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.

* preserve *all* of `create`
* don't preserve `notifications` or `algorithm`, and add some justifcation.
@turt2live turt2live self-requested a review July 22, 2019 16:55
@richvdh
Copy link
Member Author

richvdh 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...

@turt2live turt2live added the unassigned-room-version Remove this label when things get versioned. label Jul 22, 2019
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
proposals/2176-update-redaction-rules.md Show resolved Hide resolved
proposals/2176-update-redaction-rules.md Show resolved Hide resolved
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

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 2 commits July 30, 2019 08:00
Co-Authored-By: Travis Ralston <[email protected]>
Co-Authored-By: Kitsune Ral <[email protected]>
@richvdh
Copy link
Member Author

richvdh commented Aug 19, 2019

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

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot 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 at least 75% 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.

@mscbot mscbot added the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Aug 19, 2019
@turt2live turt2live removed the unassigned-room-version Remove this label when things get versioned. label May 4, 2022
@turt2live
Copy link
Member

This MSC is currently assigned to v10, though apparently we need to unblock MSC2174 too. #3604

@turt2live
Copy link
Member

This went back to not having an assigned room version (pulled out of v10).

@turt2live turt2live added the requires-room-version An idea which will require a bump in room version label May 20, 2022
@turt2live turt2live mentioned this pull request May 24, 2023
5 tasks
@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Jun 19, 2023
@turt2live turt2live removed blocked Something needs to be done before action can be taken on this PR/issue. unassigned-room-version Remove this label when things get versioned. labels Jun 26, 2023
@turt2live
Copy link
Member

This is now formally assigned to room version 11: #3820

@turt2live
Copy link
Member

Spec PR: matrix-org/matrix-spec#1604

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Aug 1, 2023
@turt2live
Copy link
Member

Merged 🎉

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Aug 15, 2023
@zecakeh zecakeh mentioned this pull request Aug 24, 2023
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal requires-room-version An idea which will require a bump in room version
Projects
Status: Done to some definition
Status: Done for now
Development

Successfully merging this pull request may close these issues.