[core] AD review of draft-ietf-core-oscore-edhoc-08

[Paul Wouters <[e-mail]>]

Hi,

Overall the document looks good. I have a few comments/nits below and one
issue.

Issue:

         The request payload consists of the EDHOC connection identifier
         C_R encoded as per Section 3.3 of [I-D.ietf-lake-edhoc],
         concatenated with EDHOC message_3.

A last minute change in EDHOC was to encrypt C_R. It is unfortunate that
this now seems to send C_R in the clear again. Can this be avoided?

Comments:

        This optimization is desirable, since the number of protocol
        round trips influences the minimum number of flights,

I don't fully understand this sentence. I think it is trying to say that
doing two protocols fully sequencially one after the other causes more
roundtrips and thus more latency? Perhaps that can be formulated without
the use if "number of flights" ? (which to me is confusing, also because
actual flights talk about legs :)

        That is, the EDHOC data (referred to as "EDHOC messages")

Why not also just call it "EDHOC messages" in this document?

        Finally, the client sends a POST request to the same EDHOC
        resource used earlier to send EDHOC message_1.

This really is confusing to read. I initially read it as "send EDHOC
message_1".
How about:

        Finally, the client sends a POST request to the same EDHOC
        resource used earlier when it sent EDHOC message_1.

Section 3.2: Perhaps change the sentence in "Step 1" to:

        Compose EDHOC message_3 into EDHOC_MSG_3 as per Section 5.4.2 of
[I-D.ietf-lake-edhoc].

so it properly introduces EDHOC_MSG_3 before its first use?

        the server discontinues the protocol [...]

        the Initiator MUST discontinue the protocol [...]

We changed "discontinues the protocol" to "aborts the session" in the EDHOC
document.
You don't mean discontinue to use the protocol defined in this RFC :)

        The server MUST NOT establish a new OSCORE Security Context from
        the present EDHOC session with the client, hence the CoAP response
        conveying the EDHOC error message is not protected with OSCORE.

I don't understand this sentence? Did you mean "as" instead of "hence" ?

        The CoAP response conveying the EDHOC error message MUST have
        Content-Format set to application/edhoc+cbor-seq defined in
        Section 9.9 of [I-D.ietf-lake-edhoc].

There is no section 9.9. I think you mean Section 10.9 (just as Appendix
A.2.3
does that reference)

       As per Section 3.3.2 of [I-D.ietf-lake-edhoc], when using the
        purely-sequential flow shown in Figure 1, the same C_R with
        value 0x01 would be encoded on the wire as the CBOR integer 1
        (0x01 in CBOR encoding), and prepended to EDHOC message_3 in
        the payload of the second EDHOC request.

I'm a bit confused by "on the wire" of C_R because EDHOC message_3
contains: AEAD( ID_CRED_I, Signature_or_MAC_3, EAD_3 )
Do you mean as part of CoAP?

        The EDHOC Connection Identifier C_I is equal to the EDHOC
        Connection Identifier C_R specified in EDHOC message_2 (i.e.,
        after its decoding as per Section 3.3 of [I-D.ietf-lake-edhoc]).

Since C_R is now encrypted in EDHOC message_2, I guess "decoding" is no
longer
the right word. After decrypting ?

        Section 9.10 of [I-D.ietf-lake-edhoc] registers the resource type
        "core.edhoc"

This should be Section 10.10.

        which is taken from the 'Label' column of the "EDHOC External
        Authorization Data" registry defined in Section 9.5 of
        [I-D.ietf-lake-edhoc].

Should be section 10.5. (Please check all section 9 references in the doc
for update)

        by expanding their semantics and specifying admitted values.

What does "admitted" here mean? Maybe "specifying additional values" ?


        the Change Controller is IESG,

I believe the IESG preference is to list IETF as change controller ?




nits:

Personal pet peeve: I dislike using
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
and prefer +-------+---------+---+-------------+-----+ style :)
I have also never seen the usage of open ended boxes, so I would change:

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|Ver| T |  TKL  |      Code     |          Message ID           |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Token (if any, TKL bytes) ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Observe Option| OSCORE Option ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| EDHOC Option  | Other Options (if any) ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|1 1 1 1 1 1 1 1| Payload ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

to:

+---+---+-------+---------------+-------------------------------+
|Ver| T |  TKL  |      Code     |          Message ID           |
+---+---+-------+---------------+-------------------------------+
| Token (if any, TKL bytes)                                     ~
~                                                               ~
+---------------+-----------------------------------------------+
| Observe Option| OSCORE Option                                 ~
+---------------+                                               ~
~                                                               ~
+---------------+-----------------------------------------------+
| EDHOC Option  | Other Options (if any)                        ~
+---------------+                                               ~
~                                                               ~
+---------------+-----------------------------------------------+
|1 1 1 1 1 1 1 1| Payload                                       ~
+---------------+
~                                                               ~
+---------------------------------------------------------------+


Paul