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

MSC4245: Immutable encryption algorithm #4245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Dec 21, 2024

Rendered


Disclosure: I am Director of Standards Development at The Matrix.org Foundation C.I.C., Matrix Spec Core Team (SCT) member, employed by Element, and operate the t2bot.io service. This proposal is written and published under my Director of Standards Development hat, with a focus on interoperability between existing systems.


Dependend on by:

@turt2live turt2live added e2e proposal A matrix spec change proposal client-server Client-Server API kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Dec 21, 2024
@turt2live turt2live marked this pull request as ready for review December 21, 2024 00:35
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation requirements:

  • Client
  • Server (to prevent clients making mistakes)
  • A rollout plan

Comment on lines +7 to +9
state event being sent. To avoid this situation, this proposal suggests that the encryption algorithm
can *optionally* be specified in the [`m.room.create`](https://spec.matrix.org/v1.13/client-server-api/#mroomcreate)
event instead, nullifying the `m.room.encryption` event's `algorithm` field.
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see immutable encryption in all rooms, not just MLS rooms, and potentially even make it non-optional in future room versions. Changing the encryption protocol for a room from a server-controlled setting is inherently problematic and almost never useful.

Indeed, clients today mostly ignore any changes in the room encryption algorithm after they've observed the first m.room.encryption event. This means the existence of the algorithm field only risks client splitbrains without actually serving any purpose.

Placing the algorithm in the create event would ensure uniqueness because to change it, you'd have to be in a different room. This also hints at the natural way to change the algorithm: room upgrades.

Copy link
Member Author

Choose a reason for hiding this comment

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

this proposal isn't MLS-specific, fwiw. It's just using MLS as a driver. The encryption_algorithm could also be a megolm string, making that room stuck on megolm.

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted, while setting a standard for future rooms to use. As we see uptake (and critically, problems/cheers as a result of the immutability), we could remove m.room.encryption's algorithm field.

Copy link
Member

@dkasak dkasak Dec 21, 2024

Choose a reason for hiding this comment

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

this proposal isn't MLS-specific, fwiw. It's just using MLS as a driver. The encryption_algorithm could also be a megolm string, making that room stuck on megolm.

Great!

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted

I'm not sure that the benefits outweigh the costs, so I'm raising this as an early request to look into whether we might want to be more proactive on this front. The encryption algorithm is very much something that should be baked into the room / group, and there should be no opportunities for participants to disagree on whether the room is encrypted or not and using which algorithm.

The modern Matrix approach is to rely on room upgrades more heavily, so it's really hard to come up with a convincing reason why encryption, of all things, should be left malleable, with all the downsides that carries.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rationale for optionality is to allow existing rooms to continue "upgrading" from unencrypted to encrypted

... which is fine when all participants are behaving and have accepted the m.room.encryption event, but due to the eventually consistent nature of Matrix, that's not guaranteed to happen immediately or even within any reasonable timeframe. Worse is that someone who enables encryption on a room has no visibility whatsoever into whether other participant servers are consistent or not until one of their users sends a message, by which time a leak could have happened in plain-text.

I can see value in just deprecating m.room.encryption altogether in favour of an approach like this.

## Potential issues

From a security perspective, it's entirely possible that the `m.room.encryption` event's `algorithm`
is different from the `m.room.create`'s `encryption_algorithm`. Clients may (rightly) get confused
Copy link
Contributor

Choose a reason for hiding this comment

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

One possibility is to extend the auth rules so that an m.room.encryption event will only be accepted if either the create event doesn't specify an algorithm or if the algorithm matches the one specified in the create event exactly.

periods.

Note that an empty string, or unrecognized value, may be present in `encryption_algorithm`: this is
legal, and causes `m.room.encryption`'s `algorithm` to be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

What if encryption_algorithm is null? It would be good to be explicit about this, even if the answer is that it's the same as any other unrecognised value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API e2e kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants