Review

[Bren2010]

These are the missing/incorrect validations:

• If there are any non-default proposal types, verify that all receivers support that proposal type.
• If a commit is from a new_member_commit sender:
  • Verify at most one Remove proposal.
  • If Remove is present, verify new and old credentials represent the same client.
  • If Remove is not present, verify credential does not represent a client already in the group.
• If commit is from a member:
  • Verify no Update or Remove proposal applies to the same leaf as another.
  • Verify Add proposals represent distinct clients.
  • Verify no Add proposal represents a client already in the group, unless there's a corresponding Remove proposal.
  • If a ReInit is present, verify it is the only proposal.
  • Verify there's no ExternalInit proposal.
• If path is populated, verify that none of the public keys of the UpdatePath appear in any node of the ratchet tree.
• For Add proposals, verifying the expiration should be optional.
• For Update proposals, verify new and old credentials represent the same client.
• For GroupContextExtension proposals:
  • Verify all members of the new group indicate support for all extensions.
  • If there is a RequiredCapabilities extension, verify all members of the new group indicate support for required capabilities.
• If there's no GroupContextExtension proposal:
  • Verify all members of the new group indicate support for all extensions in GroupContext.
  • If there is a RequiredCapabilities extension in the GroupContext, verify all members of the new group indicate support for required capabilities.
• Verify uniqueness in the new group of all members' signature_key and encryption_key.
• Verify that each LeafNode's credential type is supported by all members of the new group.

Where a validation has been implemented incorrectly, I tried to leave a note in the code.

[keks]

Thank you, that makes things a lot easier! I am starting to go through these. I will create the PRs upstream and then we can merge them here.

[raphaelrobert]

@Bren2010 thanks for the overview. Validation is not yet complete in OpenMLS, but some of the checks are tracked here. Note that this is a moving target and we are working towards a more holistic view.

At first sight, it looks like quite a few checks you mention should already be checked. If you think something is incorrect, we'd be interested in knowing what exactly it is.

[keks]

I have added most of the checks to the (still incomplete) validation dashboard we recently set up. There are a few things I am not sure about, though:

For Add proposals, verifying the expiration should be optional.

Can you elaborate what you mean by this? I understand the RFC allows being more lenient in the validation, but my understanding is that this is up to the implementation.

For Update proposals, verify new and old credentials represent the same client.

I believe that is based on this paragraph, is that correct? the responsibility of the AS, and not OpenMLS. That said, we definitely still have some work to do to help the AS to do all the checks.

Verify all members of the new group indicate support for all extensions.

as well as

Verify all members of the new group indicate support for all extensions in GroupContext.

I don't think so. My understanding of the RFC is that we only have to check that these are in the required capabilities.

[Bren2010]

Can you elaborate what you mean by this? I understand the RFC allows being more lenient in the validation, but my understanding is that this is up to the implementation.

The problem occurs when a client is syncing with a group after a period of being offline. Many of the KeyPackages that the client reads may have expired, leading the client to reject a commit as invalid, even though the KeyPackages were valid when they were sent. The best solution here is probably to keep some estimate of when data was sent, and verify expirations against that.

I believe that is based on this paragraph, is that correct? the responsibility of the AS, and not OpenMLS. That said, we definitely still have some work to do to help the AS to do all the checks.

Yes. I didn't see a provision to check this but I did see an attempt at checking this when processing ReInits, so I wasn't sure about the API separation you had in mind.

I don't think so. My understanding of the RFC is that we only have to check that these are in the required capabilities.

Here is the MUST: https://www.rfc-editor.org/rfc/rfc9420.html#section-13.4-6