Re: [Int-dir] INTDIR Review of draft-ietf-lake-edhoc-18

[Göran Selander <[e-mail]>]

Hi Donald,

Thanks very much for your intdir review. We have tried to address your comments in PR #392
https://github.com/lake-wg/edhoc/pull/392

Inline responses below. Further comments are welcome.

Thanks,
Göran


From: Donald Eastlake <[e-mail]>
Date: Sunday, 1 January 2023 at 20:11
To: [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: INTDIR Review of draft-ietf-lake-edhoc-18
I am an assigned INT directorate reviewer for <draft-ietf-lake-edhoc-18.txt>. These comments were written primarily for the benefit of the Internet Area Directors. Document editors and shepherd(s) should treat these comments just like they would treat comments from any other IETF contributors and resolve them along with any other Last Call comments that have been received. For more details on the INT Directorate, see https://datatracker.ietf.org/group/intdir/about/ <https://datatracker.ietf.org/group/intdir/about/>.

I found this to be a relatively long, detailed, and intricate draft which assumed knowledge of multiple other documents concerning CBOR, COSE, CWT, and CDDL. The document specifies Ephemeral Diffie-Hellman Over COSE (EDHOC), a lightweight authenticated key exchange protocol including forward secrecy, identity protection, and cipher suite negotiation. I do not see any particular INT Area concerns with this document.

Based on my review, if I were on the IESG I would ballot this document as NO OBJECTION

The following are issues that SHOULD be corrected before publication:

Section 3.8, Page 21: In the last paragraph of the section, "must not" -> "MUST NOT"

Done.

Section 3.8.1: I would recommend against the use of random padding and in favor of deterministic padding (perhaps dependent on the padding length, e.g., pad with bytes whose value is the padding length modulo 256). Random padding provides a convenient covert channel and will use up some pseudorandom number generator entropy. As long as it is going to get strongly encrypted, there seems to be no advantage to using random padding material.

Padding in message_1 is not encrypted, and specifically for this case it has been requested to allow random bytes. To avoid different handling of padding in the different messages we have required randomly generated padding for all messages.

Sections 5.2.1/5.2.2, Pages 30/31: I found it quite confusing, on first reading, how the explanation of SUITES_I is split between these two sections and the way "preference" is used in two different ways: as the preference the Initiator has for different cipher suites but also as the suite the Responder prefers among those supported by the Initiator. And the text in 5.2.2 may be inconsistent because it says the Initiator MUST select its most preferred cipher suite based "on what it can assume to be supported by the Responder" but later says this SHOULD change for certain error messages from a Responder. (There is also an extraneous close curly brace ("}") in the last line on page 30.)
(Done)
     The text for SUITES_I in Section 5.2.1 is nice and clear and almost complete. The text on constructing SUITES_I in Section 5.2.2 seems to me to be long, complicated, and confusing. I suggest changing the text for SUITES_I in Section 5.2.1 to something like " - array of cipher suites which the Initiator supports constructed as specified in Section 5.2.2". The change the text in Section 5.2.2 for the SUITES_I bullet item to something like:
* Construct SUITES_I as an array of cipher suites which the Initiator supports in order of preference with the first in network byte order being the most preferred and ending with the one selected by I for this session. If the cipher suite most preferred by I is selected then SUITES_I contains only that cipher suite and is encoded as an int. All cipher suites, if any, preferred by the Initiator over the selected one MUST be included. (See also Section 6.3.)
- The selected suite is based on what the Initiator can assume to be supported by the Responder; however, if the Initiator previously received from the Responder an error message with error code 2 containing SUITES_R (see Section 6.3) indicating cipher suites supported by the Responder, then the Initiator SHOULD select its most preferred supported cipher suite among those (bearing in mind that error messages are not authenticated and may be forged).
- The Initiator MUST NOT change its order of preference for cipher suites and MUST NOT omit a cipher suite preferred to the selected one because of previous error messages received from the Responder."

Great proposal! We made the change, not verbatim, but very close.

Figures 11 and 12, Page 42: It looks like these figures have gigantic captions of 9 or 10 lines. Is that what you intended? I think that Figure captions need to be like Section names, usually one line although occasionally two lines is OK. While I do not think it is required for this document, sometimes relatively long RFCs with many figures have a Table of Figures after the Table of Contents (see RFC 6325 for an example); that wouldn't work with a 9-line caption. I suggest changing these to short captions (maybe "Cipher Suite Negotiation Example 1" and "Cipher Suite Negotiation Example 2" although that isn't very specific or creative). Then move the current caption text to a paragraph referencing the Figure.

As John mentioned in a comment on github issue #388, there are different styles for captions. We reverted to short captions (according to your proposal) to align with the rest of the document.

Section 8.8, Page 51: "a random seed must" -> "a random seed MUST"

Done.

Section 8.8, Page 53: There should be a definition or a reference for "Trusted Execution Environment"

Done.

Section 9: GENERAL IANA CONSIDERATIONS PROBLEMS
     If additional values can be specified in future documents, which is true for these registries you are creating, there must be a "Reference" column saying where that value was assigned and its meaning specified.
     In almost all cases there should be a Reserved value that can be used as some sort of escape if a future extension needs to be defined.
     I notice that all of these requests to create registries and assign code points are written as if they have already been done but a spot check of several seems to indicate that these have not yet been done by IANA. It is true that an Internet Draft should generally be written so that it reads correctly when published as an RFC. However, I think the IANA Considerations are somewhat of an exception. Because code points are frequently assigned in advance of the publication or approval of a protocol, to avoid confusion between those that have and have not yet been done, the usual wording is to say that IANA is "requested" to do something unless it has actually already been done. So I would re-word all of these as requests (except for any that have actually been done).
Done.
     I would also add text between the 9. and 9.1. headers, something like "This Section gives IANA Considerations and, unless otherwise noted, conforms with [RFC8126]." Then you can pull out all the [RFC8126] references in the Section.

Done.

Section 9.1, Page 54:  It is unreasonable to expect IANA to know what a "uint" is or what its range is. It is best to be as explicit and exact as practical in the IANA Considerations so a complete table is better than just a list of a few values. I suggest replacing this Section with something like

IANA is requested to create a new registry under the new registry group "Ephemeral Diffie-Hellman Over COSE (DHOC)" as follows:

Registry Name:  EHOC Exporter Label
Assignment Policy:  Expert Review
Reference:  [this document]

 Label     Description                   Reference
------    ----------------------------   ---------
     0    Derived OSCORE Master Secret   [this document]
     1    Derived OSCORE Master Salt     [this document]
2-65534   unassigned
65535     Reserved                       [this document]

We did something similar. Assignment policy, however, in general depend on label value so we made a separate table for that.

Sections 9.2, 9.3, 9.4, 9.5, and 9.6: All of these should be changed in a way analogous to that indicated above.

We made similar changes to all sections. For some sections, the table would not fit so we kept the list.

Section 9.11, Page 60: In first sentence
OLD
   "Expert Review"
NEW
   "Expert Review" or "Specification Required"

added or "Standards Action with Expert Review".

Section 9.11, Page 60:
OLD
   *  Specifications are recommended.  When specifications are not
      provided,
NEW
   *  Even for "Expert Review" specifications are recommended.  When specifications are not
      provided for a request where Expert Review is the assignment policy,

Done.
Appendix B: I assume "ceil" is the ceiling function but this is not specified anywhere. Suggest adding to the end of the first sentence in which it is used "where ceil(x) is the smallest integer not less than x".

Done.

The following are minor wording issues with the document:

Section 1.1, Page 4:
OLD
   This specification emphasizes the possibility to reference rather
   than to transport credentials in order to reduce message overhead,
   but the latter is also supported.
NEW
   This specification emphasizes the possibility of referencing rather
   than transporting credentials in order to reduce message overhead,
   but the latter is also supported.

Done.

Section 1.3: Claims to talk about the remainder of the document but says nothing about Appendices B through J. (I do not think it needs to mention K. (Actually, when I have a "changes" appendix like K I usually label it as "Appendix Z" to make it clear it is not part of the sequence of any other appendices.) )

Another comment lead us to make a change here. We have now removed the outline of specific content in Appendix A, and only mention that there are appendices, see:
https://lake-wg.github.io/edhoc/draft-ietf-lake-edhoc.html#name-document-structure
We didn’t want to list the content of all appendices – the purpose here is not completeness but overview of the most relevant content. An alternative I’m considering is to delete this section, there is already a table of contents 😊

Section 3.5.3, Pages 16/17, in the two bulleted items:
"for the Initiator to retrieve" -> "the Initiator retrieving"
"for the Responder to retrieve" -> "the Responder retrieving"

Done.

Section 1.2, Page 5, this is just a bit too verbose for my taste:
OLD
   Compared to the DTLS 1.3 handshake [RFC9147] with ECDHE and
   connection ID, the EDHOC message size when transferred in CoAP can
   be less than 1/6 when RPK authentication is used, see
   [I-D.ietf-lwig-security-protocol-comparison].
NEW
   When RPK authentication is used, the EDHOC message size transferred
   in CoAP can be less than 1/6 that of a DTLS 1.3 handshake [RFC9147]
   with ECDHE and connection ID
   [I-D.ietf-lwig-security-protocol-comparison].

We rephrased the paragraph to simplify the text.

Section 3.9, Page 23: "incompliance" -> "noncompliance" (2 occurrences)

Had already been changed to “lack of compliance”.

Section 5.1, Pages 29/30: I found the last two sentences of the last paragraph of Section 5.1 a bit confusing and wordy. Suggest the following:
OLD
   This assumes that message duplication due to re-transmissions is
   handled by the transport protocol, see Section 3.4.  The case when
   the transport does not support message deduplication is addressed in
   Appendix G.
NEW
   Message deduplication MUST be done by the transport protocol (see
   Section 3.4) or as described in Appendix G.

Done.

Section 7, Page 43: In the next to last sentence "supporting one or both of these is no essential difference" does not read well. Suggest replacing it with "supporting one or both of these is not significantly different".

Done.

Section 8.2, Page 48: In the last sentence:
OLD
         it is recommended to
   use a freshly generated authentication key as identity in each
   initial TOFU exchange.
NEW
   using a freshly generated authentication key as identity in each
   initial TOFU exchange is RECOMMENDED.

Done.

Section 8.3, Pages 48/49:
OLD
   it is RECOMMENDED to use the
   same hash algorithm as in the cipher suite but with as much
   truncation as possible, i.e., when the EDHOC hash algorithm is
   SHA-256 it is RECOMMENDED to use SHA-256/64 in x5t and c5t.
NEW
   using the
   same hash algorithm as in the cipher suite, but with as much
   truncation as possible, is RECOMMENDED.  That is, when the EDHOC hash algorithm is
   SHA-256, using SHA-256/64 in x5t and c5t is RECOMMENDED.

Done.

Section 8.8, Page 54:
OLD
      It is RECOMMENDED to discontinue the protocol if
   the received EDHOC message is not deterministic CBOR.
NEW
      Discontinuing the protocol if
   the received EDHOC message is not deterministic CBOR is RECOMMENDED.

Done

Section 9.6: The table in this section should probably have a figure number and caption since other tables in this document do.

Section 9.11, Page 60: "approving point assignment" -> "approving code point assignment"

Appendix C, Page 75: "implementors to get used to" -> "implementors get used to"

Done.