[MLS] Lars Eggert's Discuss on draft-ietf-mls-protocol-17: (with DISCUSS and COMMENT)

[Lars Eggert via Datatracker <[e-mail]>]

Lars Eggert has entered the following ballot position for
draft-ietf-mls-protocol-17: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ 
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-mls-protocol/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

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

CC @larseggert

## Discuss

### Section 17.1, paragraph 11
```
     *  Recommended: Whether support for this ciphersuite is recommended
        by the IETF MLS WG.  Valid values are "Y", "N", and "D", as
        described below.  The default value of the "Recommended" column is
        "N".  Setting the Recommended item to "Y" or "D", or changing a
        item whose current value is "Y" or "D", requires Standards Action
        [RFC8126].
```
The IETF MLS WG may (should) close at some future point. I think the text should
talk about the IETF recommending a ciphersuite, not the MLS WG.

### Unclear RFC status

Intended RFC status in datatracker is "Proposed Standard", but document says
"Informational".


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

## 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