The Messaging Layer Security (MLS) Protocol
draft-ietf-mls-protocol-20

# Internet AD comments for draft-ietf-mls-protocol-17
CC @ekline

## Comments

* Thank you very much for all of the explicitly worked examples.

### S7.3

* What is the reason for MUST vs RECOMMENDED difference in the two different
  circumstances when validating the `lifetime` field?

* Consider adding a platitude about communicating nodes SHOULD use some
  method of time synchronization...

## Nits

### S3.2

* "top of these basic mechanism" -> "top of these basic mechanisms"
  (or maybe "top of this basic mechanism")

### S6.3.1

* "in an PrivateContentTBE structure" -> "in a PrivateContentTBE structure"

Thank you for the work on this document.

Many thanks to Rich Salz for his early ART ART review: https://mailarchive.ietf.org/arch/msg/art/cPgtxv10gxxU8_MXCai5iGHG8WE/ and to the authors for addressing Rich's comments.

I only have had time to scan the document, and I haven't identified any ART issues.

** Section 2.  
   Handshake Message:  A PublicMessage or PrivateMessage carrying an MLS
      Proposal or Commit object, as opposed to application data.

This definition of a handshake message is being defined in terms of a Proposal or Commit object, neither of which are defined in this session.   Furthermore, PublicMessage or PrivateMessage aren’t defined either as this point in the text.  To improve clarity, consider defining these key terms.

** Section 2.  Editorial.
   The PublicMessage and PrivateMessage formats are defined in
   Section 6; they represent integrity-protected and confidentiality-
   protected messages, respectively.  

The use of “respectively” in the text is suggesting that PrivateMessages don’t have integrity protection which is not accurate.  

** Section 3.1.  Typo. s/the the/the/

** Section 5.1.1.
(see the Cryptographic Dependencies section of
   the HPKE specification for more information).

Is this Section 4 of RFC9180?  If so, why not say that?

** Section 5.3.3.

   However, applications SHOULD NOT rely on the data in an
   application_id extension as if it were authenticated by the
   Authentication Service,

What would be the circumstance where the application_id extension would be treated with equal standing as something from the AS?  Why can’t this be a “MUST”?

** Section 12.2.

   A group member creating a commit and a group member processing a
   commit MUST verify that the list of committed proposals is valid
   using one of the following procedures, depending on whether the
   commit is external or not.

Unsaid is what to do if the proposal list is not valid.  Please clarify.

** Section 12.2

   *  It contains multiple Update and/or Remove proposals that apply to
      the same leaf.  If the committer has received multiple such
      proposals they SHOULD prefer any Remove received, or the most
      recent Update if there are no Removes.

Is this normative behavior required, or could it be left up to application?  The mixing of updates/removes on the same node and the SHOULD guidance already means that this “recovery from an invalid node” may not be consistent across clients/DS-es.

Same observation on recovering from ReInit with any other proposals.

** Section 12.4.

   Due to the asynchronous nature of proposals, receivers of a Commit
   SHOULD NOT enforce that all valid proposals sent within the current
   epoch are referenced by the next Commit.  In the event that a valid
   proposal is omitted from the next Commit, and that proposal is still
   valid in the current epoch, the sender of the proposal MAY resend it
   after updating it to reflect the current epoch.

Is there any additional retry mechanism?  Let’s say the sender of the proposal doesn’t see it reflect in the epoch after it sent proposal.  And then not in the next epoch.  Does it try resending indefinitely?

** Section 15.1.
   If Alice asks
   Bob "When are we going to the movie?", then the answer "Wednesday"
   could be leaked to an adversary solely by the ciphertext length.

This setup seems a little simplistic.  If the application data is encrypted to begin with, how does the adversary know the question being posed by Alice?

Thanks for working on this interesting topic. Thanks for Gorry fairhost for this TSVART review.

I have few comments, which I believe if addressed will improve the document. 

- There is a mismatch between intended status of this spec (PS) and what in the doc header (Informational). I see Lars already have a discuss on it, so supporting it.

- The specification says the UPDATE messages SHOULD be periodically sent and expects the applications to decided on the period between UPDATE messages. As I can see there are multiple implementations, can't we also recommend some guidelines and considerations on how to select the period? In some discussions I saw we were talking about hours and months..

- it says - 
     " In general, however, applications should take care that they do not send MLS messages at a rate that overwhelms the transport over which messages are being sent."
  
   I am not sure I understand what "overwhelms the transport" means. Are we talking about the congestion control or bandwidth estimation part of transport protocols? Usually the application have no or very vague information about the internal protocol states, in that case how the applications are expected to take care of this? are we suggesting some interaction application rate control?

# Éric Vyncke, INT AD, comments for draft-ietf-mls-protocol-17
CC @evyncke

Thank you for the work put into this document. It is obviously a very much needed protocol. Please note that my work schedule was hectic for my day job, and I was unable to review in depth this document, hence I am trusting the SEC ADs for the security aspects (notably sections 5, 7-10, 16), else I would have balloted a YES. SVG graphics are also very nice to read (except for figure 9 and others ;-) ). Section 13 on extensibility is also a nice and important idea.

The document is really easy to read, in my opinion, it is even a better architecture document than draft-ietf-mls-architecture-10 and *I would even suggest that the IETF does not publish the latter* as the former contains a better introduction to MLS.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Sean Turner for the shepherd's detailed write-up including the WG consensus ***and*** the justification of the intended status. 

Other thanks to Suresh Krishnan, the Internet directorate reviewer (at my request), please consider this int-dir review:
https://datatracker.ietf.org/doc/review-ietf-mls-protocol-17-intdir-telechat-krishnan-2023-01-30/ and I read that Martin & Richard have already replied to Suresh.

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS

### Intended Status

I second Lars' point about the intended status. As the IETF Last Call clearly indicated 'proposed standard', a revised I-D will be enough IMHO.

### Implementation of complex protocol

Based on my affiliation, I obviously know about one implementation (unsure whether it is a project or it is deployed). It would be nice to know whether there are other implementations of this protocol, which looks quite complex to implement.

### Ratchet

As the whole concept of MLS relies on "ratchet trees", an early short introduction to them would have made reading easier. The reader has to wait until section 4 to get some explanations.

### Section 1.1

Suggestion: swap the introduction of AD and DS as it sounds more logical timewise.

For DS, at this point in the document, readers can only guess what `KeyPackage` and `Proposal` are. Suggest not to use the capitalised words but simply their lowercase equivalent.

### Section 2.1.2

In `encoded on the remaining bits, in network byte order` I find weird to use *byte* order for *bits*. Should it rather be *MSB first* ? Or even suppress it as all other bit encoding in IETF document implicitly used this notation ?

Does this compressed encoding add value in a world with long crypto material ? (just curious here)

`up to 2^30 bytes` is about 1 Gbytes, this is huge and for sure will not often be used (written by the guy supporting 128-bit addresses). Feel free to ignore this comment of course ;-)

### Section 3.2

Where was "leaf secret" in `Updating the leaf secret of a member` defined ?

Which is the relationship between the directory/service provider and AS/DS ? 

As it seems that the group channel is part of DS, then let's make it clear in the figure ?

`Once A has updated its state, the new member has processed the Welcome, and any other group members have processed the Commit,` should this rather be `Once A has updated its state and the new member has processed the Welcome and any other group members have processed the Commit,` i.e., no Oxford comma as the "once" applies to all three conditions ? Bear with me as I am not an English speaker.

`a rate that overwhelms the transport` is transport really the only bottleneck ? I would assume that crypto processing could be another one (but could be wrong). While is seems really smart and nice on the 3 parties example, I wonder how it could scale to thousands of nodes (per -architecture) with frequent join/leave by members.

### Section 3.3

Suggest to explicitly refer to figure numbers and not only by "next figure" (or even nothing in the text pointing to the figure).

### Section 4

Some justification of the log(N) would be nice for the reader (or an external reference), does it come from the binary tree ? Does the amount of subgroups influence the number of crypto operations ? 

### Section 4.1.1

To be honest, I failed to understand the example because "unmerged" is explained later in the text. No suggestion though...

### Section 4.2

s/all the intermediate nodes they're below/all the intermediate nodes that are below/ ?

### Section 5.1.3

Probably due to my lack of knowledge about "TLS syntax", but I find imposing a syntax `label = "MLS 1.0 " + Label;` on an *opaque* field a little weird...

### Section 11

`The creator ... verify that the chosen version and ciphersuite is the best option supported by all members.`, but how can the creator do that when creating the group ?

### Section 16

This section basically confirms my point of view of the -architecture I-D, i.e., it is about security considerations of the -protocol.

## NITS

### Figure numbers

There are a lot of nice and useful figures in the I-D, may I suggest to add a label/reference to all of them ?

### Section 2

s/shared cryptographic state/shared cryptographic states/ ?

### Section 4.1

Suggest either remove the last paragraph (about implementations) or move it after the 1st paragraph of 4.1.1 (where the *index* is introduced).

### Section 8.4

s/Groups which already have an out-of-band mechanism/Groups that already have an out-of-band mechanism/ ?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

# GEN AD review of draft-ietf-mls-protocol-17

CC @larseggert

## Comments

### Section 2.1, paragraph 0
```
     We use the TLS presentation language [RFC8446] to describe the
     structure of protocol messages.  In addition to the base syntax, we
     add two additional features, the ability for fields to be optional
     and the ability for vectors to have variable-size length headers.
```
It might be worthwhile to extract the TLS syntax and its additions into a
separate RFC at some point.

### Section 17.1, paragraph 24
```
     The mandatory-to-implement ciphersuite for MLS 1.0 is
     MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519 which uses Curve25519
     for key exchange, AES-128-GCM for HPKE, HKDF over SHA2-256, and
     Ed25519 for signatures.
```
If it's "mandatory-to-implement", please use RFC2119 terms?

### "Appendix D.", paragraph 9
```
             L = self.root
             R = Node.blank_subtree(self.depth)
             self.root = Node(None, left=self.root, right=R)
             self.depth += 1
```
L is never used after assignment?

### Too many authors

The document has six authors, which exceeds the recommended author limit. Has
the sponsoring AD agreed that this is appropriate?

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditional`; alternatives might be `classic`, `classical`, `common`,
   `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Paragraph 0

The mix of SVG and ASCII art in the figures is a bit odd. I guess the SVG
tools aren't capable enough for some of the images?

### Outdated references

Document references `draft-irtf-cfrg-aead-limits-05`, but `-06` is the latest
available revision.

Reference `[RFC7540]` to `RFC7540`, which was obsoleted by `RFC9113` (this may
be on purpose).

### Grammar/style

#### Section 3, paragraph 9
```
ts thus maintain the property that the the epoch secret is confidential to th
                                   ^^^^^^^
```
Possible typo: you repeated a word.

#### "A", paragraph 4
```
rs are removed from the group in a similar way, as shown in Figure 5. Any me
                              ^^^^^^^^^^^^^^^^
```
Consider replacing this phrase with the adverb "similarly" to avoid wordiness.

#### "A", paragraph 4
```
orce access control policies on top of these basic mechanism. Group A B ...
                                       ^^^^^^^^^^^^^^^^^^^^^
```
The plural demonstrative "these" does not agree with the singular noun
"mechanism".

#### Section 7.3, paragraph 6
```
sulting node secrets and key pairs. Thus A would have the private keys to nod
                                    ^^^^
```
A comma may be missing after the conjunctive/linking adverb "Thus".

#### Section 7.3, paragraph 8
```
n use it to derive the node secret and and key pair for the node Y'. As requ
                                   ^^^^^^^
```
Possible typo: you repeated a word.

#### Section 7.9, paragraph 15
```
he previous epoch. This is done when an new member is joining via an external
                                     ^^
```
Use "a" instead of "an" if the following word doesn't start with a vowel sound,
e.g. "a sentence", "a university".

#### Section 8.3, paragraph 3
```
bel KeyPackageTBS and a content comprising of all of the fields except for th
                                ^^^^^^^^^^^^^
```
Did you mean "comprising" or "consisting of"?

#### Section 11, paragraph 17
```
roposal type describe how each individual proposals is applied. When creatin
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
```
Use a singular noun after the quantifier "each", or change it to "all".

#### Section 12.1.7, paragraph 6
```
Content message as described in Section Section 6.1. * Verify that the propos
                                ^^^^^^^^^^^^^^^
```
Possible typo: you repeated a word.

#### Section 12.4.3.1, paragraph 33
```
 also set a new encryption_secret. Hence this change MUST be applied before
                                   ^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Hence".

#### Section 12.4.3.2, paragraph 15
```
 a variant of the HN1 construction analyzed in [NAN]. A sample of the ciphert
                                   ^^^^^^^^
```
Do not mix variants of the same word ("analyze" and "analyse") within a single
text.

#### Section 14, paragraph 3
```
ended item to "Y" or "D", or changing a item whose current value is "Y" or "D
                                      ^
```
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

#### Section 14, paragraph 5
```
ETF might have consensus to leave an items marked as "N" on the basis of it
                                  ^^^^^^^^
```
The plural noun "items" cannot be used with the article "an". Did you mean "an
item" or "items"?

#### Section 16.7, paragraph 3
```
presentation language [RFC8446]. Therefore MLS messages need to be treated a
                                 ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Therefore".

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool