Stephen's pre-last-call comments #377
Comments
|
nits: Included all sections before security considerations. change to "negotiation of ciphersuite", with added clarification OK, let's keep that in mind :-) Didn't find any MUSTs in 3.4.1, did you mean 3.5.0?
The interaction between EDHOC and application is kinda critical on this point, but perhaps is not a compliance requirement? Check updated text and see if it reads better. So I obviously had to rewrite stuff to not lose the beer :-) Check updated text and see if it reads better. Check updated text and see if it reads better. I added reasons for padding. Check updated text and see if it reads better. Done. Check updated text and see if it reads better. I did an editorial pass. Check updated text and see if it reads better. Done. This was discussed before, and changed in the opposite direction, but I think your argument makes sense. Noted. That applies to many of the "figures". Not done :-) Done. Yes, that is intentional. The MUST comes in the following sentence. Good question. It is a correct statement in terms of COSE formats, but I'm not convinced it is relevant. I make a separate issue of that. EDHOC session is used throughout the document. Check updated text and see if it reads better. This is actually an important statement. If e.g. message_2 is received it shall not be processed unless the protocol state indicates that it is waiting for message_2 as it might e.g. be a replay. Can we state this more clearly? Changed to "EDHOC session". Check updated text and see if it reads better. Good catch, thanks! Check updated text in 1.4 and see if it reads better. Check updated text and see if it reads better. Check updated text and see if it reads better. Done. Check updated text and see if it reads better. |
Yes, that seems like a good thing to ask the WG. |
|
Nits merged. Question sent to WG. Closing now. |
Stephen's pre-last-call comments:
https://mailarchive.ietf.org/arch/msg/lake/_5eVv4p8epYbA1yls471jqETyTU/
non-nits:
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?
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.)
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).
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:-(
The text was updated successfully, but these errors were encountered: