Subject: [COSE] Review of draft-ietf-cose-rfc8152bis-struct-03

From: Ludwig Seitz <[email-masked]>
Date: Fri, 16 Aug 2019 15:05:32 +0200

Hello WG, Jim,

here is my review of draft-ietf-cose-rfc8152bis-struct-03. Note that it 
consists mostly of nits.

Regards,

Ludwig

Nit: Section 1:

"
    o  The procedures used to compute build the inputs to the
       cryptographic functions required for each of the structures.
"

either "compute" or "build", but both sounds wrong to me.

==
Nit: section 1:

"Applications may additionally want to defined
    additional data fields as part of the stucture."

should be "... want to define ... structure"

==
Nit: Section 1.2

"standalong" should be "standalone"

==
Nit: Section 1.4

"Since that time CBOR Data Definition
    Language (CDDL) [I-D.ietf-cbor-cddl] has been published as an RFC."

This (and other places in the draft) should refer to RFC 8610 instead of 
the I-D.

==
Nit: Section 1.6

"Authenticated Encryption with Authenticated Data (AEAD) [RFC5116]"

RFC 5116 expands AEAD to 'Authenticated Encryption with Associated Data'

==
Nit: Section 3

"The structure of COSE has been designed to have two buckets ..."

and then after the second paragraph:
"Two buckets are provided for each layer"

This essentially repeats the same information, and feels unnecessary.
Perhaps:
"The two buckets provided for each layer are:"

==
Section 3

"empty_or_serialized_map = bstr .cbor header_map / bstr .size 0"

This CDDL uses the operators '.cbor' and '.size', which are not defined 
in section 1.4 although the text in section 1.4 makes it appear such as 
if it was presenting all the CDDL operators that are used in this document.

I suggest to provide a short definition of these operators in section 1.4

==
Section 3.1

"Parameters defined in this document do not need to be
       included as they should be understood by all implementations."

I wonder if this 'should' should be a MUST?

==
Nit: Section 3.1
"Partial IV: [...] The IV can be placed in the unprotected header ..."

This should read "The Partial IV can be placed in the ..."

==
Section 3.1
"The message IV is generated by the following steps:

   1.  Left-pad the Partial IV with zeros to the length of IV.

   2.  XOR the padded Partial IV with the context IV.
"

There seems to be a part missing here, since "context IV" is not 
mentioned anywhere else in this document.

==

Nit: 4.3

"these fields use a TLV structure so
       they can be concatenated without any problems."

This is the first use of the abbreviation TLV, so it should be expanded.

==
Nit: 4.4

  " 4.  Place the resulting signature value in the 'signature' field of
        the array."

Although it is clear from the context, one might whish to specify which 
array to avoid confusion.

==
Nits: Section 5.

"That is the countersignature makes a statement about the
    existance of a signature and"

Should be "existence" (in several places).

AND

" as oppose to attesting"

should be "as opposed to attesting"

AND

"A tagged COSE_Countersign steruture is identified byt the CBOR tag TBD0."

should be "structure" and "by"

==
Nit: 5.2

"The object was to keep the countersignature as small as possible ..."

should be "The objective ..."

==
Nit: Section 15

"An example of a profile can be found in
    [I-D.ietf-core-object-security] where a profiles was developed for
    carrying content in combination with CoAP headers."

"profiles" should be "profile" or the whole sentence rephrased to avoid 
the second occurrence of 'profile*'

==
Nit Section 18.

"... at the time of posting of this Internet-Draft"

This sentence will be wrong as soon as the RFC is published.

==

Appendix A page 59

"In many cases, applications that make the algorithm identifier
       implicit will also want to make the context identifier implicit
       for the same reason."

'context identifier' is not defined as a concept anywhere. Perhaps it 
would be
good to elaborate on what is meant to eliminate possible confusion with 
e.g. the 'context' used in 13.5. or 7.3. or 6.3.

==

Note: I did not check the correctness of the examples in Appendix C.

-- 
Ludwig Seitz, PhD
Security Lab, RISE
Phone +46(0)70-349 92 51

---

From: Ludwig Seitz <[email-masked]>
Date: Fri, 16 Aug 2019 15:07:59 +0200

> the second occurrence of 'profile*'

I forgot to add: the reference should be updated to RFC 8613.

/Ludwig

-- 
Ludwig Seitz, PhD
Security Lab, RISE
Phone +46(0)70-349 92 51

---

From: Jim Schaad <[email-masked]>
Date: Fri, 16 Aug 2019 17:07:16 -0700

-----Original Message-----
From: Ludwig Seitz <[email-masked]> 
Sent: Friday, August 16, 2019 6:06 AM
To: [email-masked]; Jim Schaad <[email-masked]>
Subject: Review of draft-ietf-cose-rfc8152bis-struct-03

Hello WG, Jim,

Regards,

Ludwig

Nit: Section 1:

"

[JLS] Me too. - fixed

==
Nit: section 1:

[JLS] Fixed

==
Nit: Section 1.2

[JLS] Already fixed.

==
Nit: Section 1.4

This (and other places in the draft) should refer to RFC 8610 instead of 
the I-D.
[JLS] Already fixed, but fixed it in the algs draft as well.

==
Nit: Section 1.6

[JLS] Fixed here and in the algs draft.

==
Nit: Section 3

[JLS] I looked at this and trimmed it down even more to "The two buckets are:"

==
Section 3

[JLS] I agree, I should have done then when Carsten suggested using them originally.

==
Section 3.1

[JLS] No I don't think so.  As an example, many implementations in the IoT world are going understand only one of IV and PartialIV.  There is no real reason for them to understand both if they are only using OSCORE as an example.  (An OSCORE implementation would not need to understand any of these headers.)  Specific applications can be designed so that this would not be true.

==
Nit: Section 3.1

[JLS] Fixed

==
Section 3.1

[JLS] Fixed - I re-wrote the second sentence to define both the context and partial IV values.

==

Nit: 4.3

[JLS]  This is not required as it is on the RFC Editor list of well known abbreviations. https://www.rfc-editor.org/materials/abbrev.expansion.txt

==
Nit: 4.4

[JLS] It is a bit problematical to say which array it is going into because this is one of three different arrays.  However, it would go into a non-array for CounterSignature0.  Hmmmmmm.

==
Nits: Section 5.

AND

AND

[JLS] FIxed

==
Nit: 5.2

[JLS] Fixed

==
Nit: Section 15

[JLS] Fixed

==
Nit Section 18.

[JLS] And is academic since the section is to be removed prior to publish as an RFC.

==

Appendix A page 59

[JLS] I put in a reference to 'key context' in the terminology section so it is now defined (albeit indirectly).
==

[JLS] You get the same answer as Matt - these have not changed and were auto-generated for RFC 8152.  They are also in the examples project on github.

Everything is in a pull request https://github.com/cose-wg/cose-rfc8152bis/pull/13 along with the changes from Matt's review.

Jim

-- 
Ludwig Seitz, PhD
Security Lab, RISE
Phone +46(0)70-349 92 51