More clarity around validating proposals

[mulmarta]

The text in the RFC around validating proposals, choosing which proposals go into a commit and how to validate a received commit could do with a bit more work.

The root issue is that Section 13.2 states “The sender of a Commit MUST include all valid proposals that it has received during the current epoch. Invalid proposals include, for example, proposals with an invalid signature or proposals that are semantically invalid, such as an Add when the sender does not have the application-level permission to add new users.” There are two intertwined issues with this text:

1. The qualifier “MUST”
2. The text is too vague about what a “valid Proposal” is. E.g. it could be interpreted to mean a Proposal is “valid” relative to the current group state: Can it be applied to the group as it stands now? Then it’s “valid”.

Put simply, it doesn’t make sense to only validate proposals individually. Instead one should validate if the full list of proposals can be committed. Indeed, other parts of the RFC enforce validation steps that apply to the full list. E.g.

• A ReInit proposal MUST be the only one in the commit, and other existing proposals SHOULD be preferred (13.2.1).
• Out of multiple proposals affecting the same leaf, the committer MUST choose an arbitrary one (13.2, “If there are multiple (…)”).
• Out of multiple PSK proposals with the same ID, the committer MUST choose an arbitrary one (13.2).

In fact, once extensions come into play there are likely many more similar situations, even ones affecting lists of otherwise benign proposals (e.g. Add/Remove). Suppose a chat app is built on MLS with a custom room moderator roles extension. The app wants MLS to enforce two rules: Only mods can Add/Remove. Rooms should always have at least 1 Mod. Here’s how the RFC’s current text can become a problem: A and B are the only mods in a room. A makes proposal P1 that removes B. Meanwhile, B makes prop P2 that removes A. C gets both proposals. What should C do?

• Option 1: validate & cache both proposals. → Problem: C can no longer issue a commit as it “MUST include all valid proposals”. (Alternatively, it does commit to both as instructed by the RFC which leaves the room without a mod.)
• Option 2: validate & cache the first prop received (say, P1), but reject P2 as validating both means C can’t commit. → Problem: If someone else commits to P2 by ref. C can’t process the commit as it doesn’t “know” P2.

So, to clean all this up a bit more, this PR proposes the following main changes:

1. Add a section "Validating Proposals" that defines a procedure for validating a list of proposals. The procedure starts with checking that each individual proposal is valid, as described in the "Proposals" section. It also has a note that applications may add more rules.
2. Clean up a bit the validation rules in the "Proposals" section and remove redundancies. Now for each proposal, we precisely define when it is valid.
3. In the "Commit" section, add a requirement that both a committer and a receiver of a commit MUST verify that the committed proposal list is valid according to section "Validating Proposals".
4. Downgrade the requirement that “The sender of a Commit MUST include all valid proposals that it has received“ to "The sender of a Commit SHOULD include all valid proposals that it has received, as long as this does not make the proposal list invalid."

Note that the validation in section "Proposals" can be done when a commit is received. The final check in "Validating Proposals" is done when a commit is created and when it is received.

This also fixes #657.

[seanturner]

Discussed 19 May. Will leave it open for further review from @bifurcation @raphaelrobert review.

[bifurcation]

Virtual interim 2022-05-19:

• SHOULD include all valid proposals could use explanation of why it's not a MUST
  • Proposals can contradict each other
  • Application policy might declare some policy invalid
• Agreement in principle, @raphaelrobert and @bifurcation to review and merge when ready

[bifurcation]

I'm not clear why this says "regardless of its use". Maybe you mean "In all these cases...", but I would prefer leaving it unadorned to not imply that there's a restriction to those cases (in case another one comes up).

If you're going to say "starts with..." you need to have more steps that follow.

[mulmarta]

Thanks for pointing this out! (We used to have a earlier version with more cases and more steps.)

[bifurcation]

Suggest using a more interesting example here, such as including an Update and Remove for the same member.

[bifurcation]

It's slightly confusing that the above is a negative check, and this is a positive check. But that might be unavoidable. I might rephrase as something like:

... the list is valid if it contains only the following proposals (not necessarily in this order):

• Exactly one ExternalInit proposal
• At most one Remove proposal...
• Zero or more PSK proposals...
• No other proposals

(The last bullet is a little redundant, but it bears repeating.)

[bifurcation]

(And maybe we should require order here!)

[Bren2010]

I opened a PR here that resolves the merge conflicts and makes some additional suggested changes: mulmarta#1

[Bren2010]

@mulmarta Please update to resolve merge conflicts.

[bifurcation]

Couple of nits, otherwise lgtm.

[bifurcation]

I'm not sure why this is here. It seems like it follows from the Add/Update LeafNode validity requirements.

[bifurcation]

@mulmarta I proactively merged my editorial nits (with review from @Bren2010) in the interest of getting this merged. If I've screwed anything up, obviously feel free to file a follow up to fix.