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

Jump to bottom

MSC2870: Protect server ACLs from redaction #2870

Merged
merged 3 commits into from Apr 15, 2024
Merged

MSC2870: Protect server ACLs from redaction #2870

merged 3 commits into from Apr 15, 2024

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Nov 20, 2020 •
edited

@turt2live turt2live changed the title Protect server ACLs from redaction MSC2870: Protect server ACLs from redaction Nov 20, 2020
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Nov 20, 2020
@anoadragon453
Copy link
Member

This MSC looks pretty solid, though we'd need an implementation before proposing FCP.

@turt2live
Copy link
Member Author

Do we? This seems like the sort of MSC where we don't.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@ShadowJonathan ShadowJonathan mentioned this pull request Apr 19, 2022
Merged
@turt2live turt2live added requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned. maybe v12? candidate for room version 12 labels Jun 1, 2023
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Feb 21, 2024
@turt2live
Copy link
Member Author

Looks like Ruma added support for this: ruma/ruma#1080

With that implementation:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Feb 21, 2024 •
edited by erikjohnston

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

Once at least 75% of reviewers approve (and there are no outstanding concerns), 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 information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Feb 21, 2024
@turt2live turt2live added this to Needs idea feedback / initial review in Spec Core Team Backlog via automation Feb 21, 2024
@turt2live turt2live moved this from Needs idea feedback / initial review to Ready for FCP ticks in Spec Core Team Backlog Feb 22, 2024
@mscbot
Copy link
Collaborator

mscbot commented Apr 9, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 9, 2024
Gnuxie
Gnuxie reviewed Apr 9, 2024
View reviewed changes
proposals/2870-protect-acls-from-redaction.md
## Proposal

In a future room version, the `allow`, `deny`, and `allow_ip_literals` fields of the `m.room.server_acl`
state event are protected from redaction. Typically this measure is only taken when a field is critical
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are proposing to protect the fields from redaction, then why not go a step further and validate the ACL event in authorization rules against its schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the benefit of doing so?

Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent foot gunning in the same spirit as protecting fields from redaction, by also validating these protected fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already a stretch to include ACLs in the redaction algorithm, as mentioned in other threads. I'm not sure that adding more event validation to the auth rules will meaningfully help the room model operate any better. It's a lot harder to send invalid data in a "production" room than it is to redact, for example. Test/development rooms might still be at risk, but their very presence puts them at risk of breaking, imo.

In any case, this is probably something worth discussing in a future MSC.

proposals/2870-protect-acls-from-redaction.md Show resolved Hide resolved
proposals/2870-protect-acls-from-redaction.md Show resolved Hide resolved
@turt2live turt2live moved this from Ready for FCP ticks to In FCP in Spec Core Team Backlog Apr 10, 2024
@turt2live
Copy link
Member Author

SCT: fyi this has been updated to incorporate feedback from before/during FCP. I don't believe the changes materially alter the MSC to cause FCP to be restarted, but folks are free to disagree with that assement.

@mscbot
Copy link
Collaborator

mscbot commented Apr 14, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Apr 14, 2024
@turt2live turt2live merged commit ecf9963 into old_master Apr 15, 2024
@turt2live turt2live deleted the travis/msc/gives-protection-from-redactions branch April 15, 2024 17:24
@turt2live turt2live moved this from In FCP to Requires spec writing in Spec Core Team Backlog Apr 15, 2024
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Apr 15, 2024
turt2live added a commit that referenced this pull request Apr 15, 2024
@turt2live
MSC2870: Protect server ACLs from redaction (#2870)
781ab5d 
* Protect server ACLs from redaction

* Update proposals/2870-protect-acls-from-redaction.md

* Incorporate clarifying review
@turt2live turt2live moved this from Requires spec writing to BLOCKED, requires spec writing in Spec Core Team Backlog Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auscompgeek auscompgeek auscompgeek left review comments

@richvdh richvdh richvdh left review comments

@Gnuxie Gnuxie Gnuxie left review comments

@anoadragon453 anoadragon453 anoadragon453 approved these changes

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees
No one assigned
Labels
kind:maintenance MSC which clarifies/updates existing spec maybe v12? candidate for room version 12 proposal A matrix spec change proposal requires-room-version An idea which will require a bump in room version spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec unassigned-room-version Remove this label when things get versioned.
Projects
Spec Core Team Backlog
  
BLOCKED, requires spec writing
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@turt2live @anoadragon453 @mscbot @auscompgeek @richvdh @erikjohnston @Gnuxie