-
Notifications
You must be signed in to change notification settings - Fork 389
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
MSC2870: Protect server ACLs from redaction #2870
MSC2870: Protect server ACLs from redaction #2870
Conversation
This MSC looks pretty solid, though we'd need an implementation before proposing FCP. |
Do we? This seems like the sort of MSC where we don't. |
Looks like Ruma added support for this: ruma/ruma#1080 With that implementation: @mscbot fcp merge |
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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
## 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
* Protect server ACLs from redaction * Update proposals/2870-protect-acls-from-redaction.md * Incorporate clarifying review
Rendered
Implementation: ruma/ruma#1080
FCP tickyboxes