Ballot for draft-ietf-mls-protocol
Discuss
Yes
No Objection
No Record
Summary: Has a DISCUSS. Needs 8 more YES or NO OBJECTION positions to pass.
# 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".
## 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
# 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"