Skip to content

feat(rooms): a room can renew without anybody joining or leaving - #1389

Open
stormer78 wants to merge 1 commit into
mainfrom
feat/rooms-self-update
Open

feat(rooms): a room can renew without anybody joining or leaving#1389
stormer78 wants to merge 1 commit into
mainfrom
feat/rooms-self-update

Conversation

@stormer78

Copy link
Copy Markdown
Contributor

add_member and remove_member both produce a commit, and until now they were the only things that did. So "a room renews" meant "a room adds or removes somebody" — which is not what renewal means, and §9's lifecycle and every epoch anchor ride renewals. A room with a stable membership could not renew at all.

RoomGroup::self_update is MLS's post-compromise security taken deliberately rather than as a side effect of somebody joining: the committer replaces its own leaf key, so an attacker holding the old one is locked out from the next epoch. A group that never commits never heals.

Two things the doc comment says, because a caller would otherwise find them out

It does not preserve readability. Advancing the epoch makes everything sealed below it unreadable to anyone who has not derived those keys — which is exactly what RetentionPolicy::Chained exists to prevent. The rung that prevents it is minted by SealedRoom, not here, because a rung is bound to the room and RoomGroup deliberately does not know which room it is for. A caller that advances without minting one severs the history.

welcome is None. There is nobody new to welcome, and a caller looking for one would be looking for a party that does not exist.

The test worth keeping

assert_ne!(before, after,
    "if the storage key survived a commit, post-compromise security would be a \
     claim this type does not keep");

The other two pin that a renewal moves the epoch by exactly one and changes nobody, and that renewals compose — a commit that did not advance would be a renewal that healed nothing, which is the whole point of taking one.

A note for whoever runs these

mls is off by default on vti-rooms — a host that only stores ciphertext needs none of it, and OpenMLS is a substantial dependency to make it carry. So cargo test -p vti-rooms compiles none of this and reports a cheerful pass over 84 unrelated tests. It needs --features mls, which is how the workspace's union feature set picks it up.

I mention it because it caught me: a cargo check -p vti-rooms on this change succeeded without compiling the function it added.

Closes X4.1 on the data-rooms todo. What it unblocks — a renewal task and the commit relay that delivers the commit without the owner fanning out O(n) — is X4.2 and still open.

`add_member` and `remove_member` both produce a commit, and until now they were
the **only** things that did. So "a room renews" meant "a room adds or removes
somebody", which is not what renewal means — and §9's lifecycle, and every epoch
anchor, ride renewals. A room with a stable membership could not renew at all.

`RoomGroup::self_update` is MLS's post-compromise security taken deliberately
rather than as a side effect of somebody joining: the committer replaces its own
leaf key, so an attacker holding the old one is locked out from the next epoch.
A group that never commits never heals.

Two things the doc comment says because a caller would otherwise find them out:

- It does **not** preserve readability. Advancing the epoch makes everything
  sealed below it unreadable to anyone who has not derived those keys, which is
  what `RetentionPolicy::Chained` exists to prevent — and the rung that prevents
  it is minted by `SealedRoom`, not here, because a rung is bound to the room and
  this type deliberately does not know which room it is for. A caller that
  advances without minting one severs the history.
- `welcome` is `None`. There is nobody new to welcome, and a caller looking for
  one would be looking for a party that does not exist.

`a_renewal_changes_the_key_records_are_sealed_under` is the test worth keeping:
if the storage key survived a commit, post-compromise security would be a claim
this type does not keep.

Note for whoever runs these: `mls` is **off by default** on `vti-rooms` — a host
that only stores ciphertext needs none of it — so `cargo test -p vti-rooms`
compiles none of this. It needs `--features mls`, which is how the workspace's
union feature set picks it up.
@stormer78
stormer78 requested a review from a team as a code owner September 9, 2026 23:10
@affinidi-appsecurity-bot

Copy link
Copy Markdown

🛡️ AI Agentic Security Code Review

🔎 A manual security review is recommended before merging. Please contact the Security team for specifics and remediation guidance.

ℹ️ Detailed findings are not published on public repositories; the Security team holds the complete report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants