Last Call Review of draft-ietf-core-oscore-edhoc-09
review-ietf-core-oscore-edhoc-09-secdir-lc-hardaker-2023-11-16-00

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

The summary of the review is: almost ready

Note: this is an early review, per request

In general, the document reads quiet well but I have a number of
comments and suggestions below that may help if you choose to use them.

Please note that I'm not an expert in CoAP and OSCORE and EDHOC, which
makes anything I say questionable from the start :-)

Questions/comments:

- Section 2, P4: you might mention that the reverse is only usable in
  the classic case.  I know you're describing the classic/generic case
  here, but it would be helpful to pre-seed the reader that this mode
  won't work with this draft.

- Section 2, P7: you state that the server replies with 2.04 (Changed)
  -- is this *always* the case?  Aren't there cases where an error or
  other response might occur?  (reminder: I'm not a CoAP expert)

- Section 3, "Since EDHOC message_3 may be too large..." ... "EDHOC
  message_3 has instead to be transported in tho CoAP payload... ".
  It's unclear to me reading if this is *always* the case, or only if
  it's too large do you do that.  [note for implementation simplicity:
  doing it the same way either way requires less decisions to be made]

- I admit confusion after reading the whole thing about where C_R, kid
  and the responder identifier comes from and when they're equal, etc
  ("is both the server's Receiptient ID (IE.e the client's sender ID)
  and the EDHOC Connection Identifier C_R of the server", but later "The
  Responder MUST choose a C_R" seem in conflict).  I thought I
  understood it at one point (and with another read I'm sure I'd get
  more clarity), but it caused my secdir focused brain to think "well,
  if the client specifies the value and the server has to use it to
  distinguish between different clients, then what happens if 2 clients
  end up deriving/using the same value?"  I think clarity about this,
  maybe with a solid example, would help.  I was very happy to read
  later in the security considerations that "Even if a client did not
  fulfill this requirement, it would not have any impact in terms of
  security.", but in the end without a better understanding I remain
  skeptical (note: mostly that's my job as a reviewer here).

- Section 3.2.1, last P: can you add a recommendation about what to do
  when the performance advantage might be sub-optimal, or how to detect
  algorithmically when this is the case? 

- section 3.3 P4 (not bulleted): "the server has to make sure" -- why
  isn't this a MUST?

- section 7 bullets "This is the sum of the 64-bit MACs": IMHO, this
  wording doesn't match the real world math operations with the
  probability of attacking two MACs.  I recognize that
  draft-ietf-lake-edhoc also says this, but I argue it's not correctly
  worded either.  When trying to break something with two independent
  MACs, they're not "added" (summed) -- they're actually multiplied.

- section 7: additionally, the 128-bit minimum is only assured with the
  current defined algorithms.  I doubt future algorithms will be
  standardized for use that will be weaker, but the wording could be
  improved by saying "given the currently defined algorithms" or
  something to explain this "feature" is actually dependent on the IANA
  registry contents, eg.

- section 8.3: the registry is defined with ranges from -65536 to 65535,
  which I note two issues with:  A) no where does the section actually
  state this is the full range (even though the text assignment ranges
  implies it), and B) this space is actually 2^17 in size which seems
  odd.  If you're going to use negative values and want space that fits
  into 16 bits, then the space should be -32768 to 32767.

Nits/suggestions:

- Abstract: it would be helpful to start the abstract with the purpose
  before the rest; simply move the third sentence to the first (with a
  bit of rewording) and it will add clarity (IMHO).

- introduction, paragraph 2 (P2), sentence 1 (S1): add "with CoAP" after
  "the EDHOC protocol"
  
- For figure 1 and 2, the messages contain some things that are actualy
  headers ("Content-Format:") and others that are labels ("Header:",
  "Payload:"), which is kind of confusing to read.  A naive reader might
  actually expect "Payload:" to be in the sent message.

- Figure 1, EDHOC Response: C_I is missing here.  In general, please
  review all the messages of both Figure 1 and 2 for whether and where
  C_I and C_R should be included.  It's a bit confusing because it might
  be contained within a sub-element but is listed anyway for clarity,
  etc, but IMHO it should be consistently put in or omitted in each
  message based on some formula (included or explicit).  EG, C_R is
  technically inside EDHOC message_3 (if I read the draft right), but
  yet is explicitly listed as if it's separate in the payload.  This may
  be intentional but if so then I think C_I should also be listed in
  the message 2 flow (EDHOC response).  Same with figure 2's EDHOC
  response and EDHOC + OSCORE request.

- Figure 2, EDHOC + OSCORE request: mention the option(s) in the
  payload?  It might be helpful to indicate to the reader that this is
  where the various options go since you're talking about them.

- Section 3.2 bullet 3: I note that COMB_PAYLOAD is not yet mentioned
  and thus could use a brief definition or maybe just a reference to the
  normative specification.

- section 3.2: I'd be tempted to put the entire beginning of 3.2 into a
  subsection 3.2.1, which would make referencing it in 3.2.1 (which
  would become 3.2.2) less confusing and circular.  You'd likely need a
  short intro before both sections though, but that should be easy
  enough.  Same comment for Section 3.3 and 3.3.1.

- section 3.2, bullet 5: it seems odd that this is near the last step
  instead of the first (IE, you're adding the signal long after doing
  everything to support it).  Similarly, the last bullet in section 6
  seems like it should really be the first bullet.

- section 3.4 note to the editor: I don't understand why this note is
  here or why you'd want to delete the last bullet.

- section 3.4, description for figure 4: it would be helpful to specify
  where a few of the other values come from (CBOR encoding), such as
  the 93 and ff values.  TL;DR: some elements of your example get
  encoded in ways that don't match the description (eg, EDHOC option
  value) and could use an explanation.

- The separation between 3.1 and 4.1 confused me a bit, I would have put
  them together but this stylistic. 

- 4.1.3: It seems weird that this is worded as a MUST with a bullet as
  opposed to a MUST NOT.  I'd change the bullet into a MUST NOT and put
  it as the first sentence maybe.

-- 
Wes Hardaker
USC/ISI