[Lake] pre-last-call comments on edhoc draft-18

[Stephen Farrell <[e-mail]>]

Hiya,

While warming up to hit the pub-req button for edhoc I gave
draft-18 a full read-through. I have a few non-nit comments
(some just questions) and a bunch of nits, all of which
are fine to process as part of IETF last call. (IOW, I don't
see a need to hold up hitting the pub-req button to handle
any of these.)

To be clear; these are comments made whilst wearing no hat
at all - I'll chat with my co-chair shortly about hitting
that pub-req button:-)

Lastly - for a 100 page document, this seems entirely
credible for implementations that interoperate. That's
really quite an achievement, so well done all!

Cheers,
S.

non-nits:

1) 3.5.2, 2nd para: on what basis is rfc5280 a normative ref
whilst [I-D.ietf-cose-cbor-encoded-cert] is informative?
They seem to be equally required for implementation based
on this text.  Given [I-D.ietf-cose-x509] is normative,
perhaps it wouldn't add delay if both were?

2) 3.5.2: I forget if use of the full DER-encoded X.509 cert
was discussed on the list or not? If not, the alternative
would have been to use the SPKI which has some slightly
different properties that can sometimes be beneficial
(potentially easier use of different CAs on each side). I'm
not asking for a change here btw, just to check was this
discussed on the list. (I see appendix D.2 talks a bit
about this.)

3) 5.2.3: what happens if the decode of message_1 fails?
(Say due to an unknown suite.) Does EAD_1 get provided to
the application?  I hope not! Maybe the text after the
bullets needs to specify this explicitly? Same for other
sections wrt providing EAD to the application (esp EAD_1
and EAD_2).

4) 9.4: As the method field is how we handle breaking
changes, do we really want that registry to be
specification required? The alternative might be standards
action, if we prefer the IESG to be involved in such
changes.

nits:

- section 1.3: this only mentions appendix A - is there a
   good reason for that or is it just text that's not
changed in ages? Probably omitting or including mention of
all appendices is better.

- section 2, p8 bullet says "Verification of the selected
   cipher suite." - that puzzled me a bit as I'd have
assumed that the transcript hashes provide/cover that
validation? If that's the case maybe this bullet could be
deleted, if not, be interested in what's meant.

- reference to [I-D.ietf-lake-traces]: not really a comment
   on edhoc, but we should probably ask the WG if we want to
hold publication of edhoc as an RFC until the traces work
is also done (i.e. create a small cluster for those two in
the RFC editor queue).

- 3.4.1, 2nd last para: the "MUST" statements here seem a
   bit bogus in 2119 interop terms. If those are only for
emphasis, then s/MUST/must/might be better to avoid other
people commenting on this later.

- 3.5.3: the two example definitions for ID_CRED_X seem to
   be introducing a notation "{ 4: kid_x }..." before that
notation is defined/referenced. Maybe add at least a
forward pointer to 1.3 about such notations? Or, if 1.4 is
considered to cover this, then that's fine. (I didn't read
all the referred things from 1.4:-)

- 3.8: I bet a beer EAD item criticality won't work as
   planned:-)

- 3.8: since zero is used for padding it's not true to say
   "non-negative" means non-critical.

- 3.8: Am I getting this right? If I define a new EAD item
   "foo" with a codepoint of 17 then sending 17 means
non-critical but sending -17 means "treat as critical"? The
text isn't quite clear (here) as to whether +x and -x mean
the same EAD type, just with different criticality, or,
whether +x and -x correspond to independent EAD item types.

- 3.8.1: This leaves the reader wondering what is an
   "appropriate length" of padding? I'm also left wondering
what is the point of padding in EAD_1 which is not
encrypted? (Is it only useful if there's also some lower
layer confidentiality?)

- 3.9: "incompliance" - that's a very uncommon word, maybe
   that'd be better rephrased e.g. s/incompliance/lack of
compliance/?

- 3.9: it correctly says here >1 transport may be used, but
   earlier (last para of 3.4 before 3.4.1) it said I and R
"need to have agreed on a transport" (note the singular).
I'd change or delete the text in 3.4 rather than that in
3.9 probably.

- 4.1.1 refers to Figure 8 but that's a few pages ahead and
   the names of the keys (e.g. PRK_3e2m) aren't intuitive.
It'd be good to somehow introduce those names earlier,
maybe just by moving Figure 8 earlier, but at least by
pointing out the naming scheme (that PRK_2 refers to
message 2 and how etc.). In any case, it'd be good to do an
editorial pass with the goal of making understanding the
key derivation easier for a new reader. (Requiring readers
to reverse engineer that seems sub-optimal:-)

- 4.1.1/4.1.2 maybe "Extract()" and "Expand()" aren't the
   best function names? Those probably exist already in lots
of libraries.  Perhaps "EDHOC_Expand()" etc. would make it
easier to map implementations to the spec? Or whatever
names are used in current implementations. (I'd also
s/EDHOC-KDF/EDHOC_KDF/g to get a name that works for
compilers and same for EDHOC-Exporter.)

- Figure 8 looks more like a table than a figure:-)

- 4.1.3: TEE is used without expansion.

- 4.2.1: just checking - there's a lowercase "must" there -
   is that intentional? (It could be correct as it says
"must be unique" which is always tricky:-)

- section 5: As a non-implementer, I didn't understand the
   "MAY contain parameters" sentence there.

- 5.1: What's a "session"? I think that's a nit, but wonder
   a bit if someone's tested/analysed in case there's any
odd state that can be reached if a node has forgotten state
but receives a message assuming the state is known/stored.
It's probably ok though.

- 5.1: is "SHALL be processed" right? Maybe just "are
   processed" is ok? (Just trying to avoid others asking
this again, I don't really care myself:-)

- 5.3.2: "the length of the protocol" seems to mean "for
   that session" but neither is that well defined - could
this be made more precise?

- 5.3.2: The "<<...>>" notation wasn't described so far.
   (See earlier comment wrt 1.4)

- 6.3.2, figure 11: Shouldn't G_X in the 2nd message_1 be
   G_X' or something to indicate a different DH public
value? Is the same C_I meant to be sent in both message_1's
or not?

- Appendix A.2: This has an RFC2119 SHOULD. If the intent
   was that none of the appendices had such terms, that'd be
wrong, but I'm not sure if that was the intent of the WG.
(There are more examples of 2119 terms in appendices.)
Might be good to say (e.g. in 1.4) whether or not such 2119
terms are important for implementers who might otherwise
ignore 'em.

- Appendix C.1 ends with a table (with no label/caption)
   that doesn't seem to have text describing it anywhere?

- Appendix I: This seems to describe an optional to
   implement thing that has two internal options. I don't
see that the text tells me which I ought implement, if I
choose to try support long plaintexts.  The problem is
indicated by the paras beginning "A potential
work-around..." and the following one starting "Another
solution..." I read those as two different ways to handle
long plaintexts.)

- Appendix J: If you do s/EDHOC-KDF/EDHOC_KDF/ then I guess
   you'll also do s/EDHOC-KeyUpdate/EDHOC_KeyUpdate/ maybe.

- Acks: Sad to say it, and I'm not sure which is best, but
   maybe consider s/Jim Schaad/the late Jim Schaad/? Hard to
know how to phrase that well though:-(