-----Original Message-----
From: Ludwig Seitz <[email protected]>
Sent: Friday, August 16, 2019 6:06 AM
To: [email protected]; Jim Schaad <[email protected]>
Subject: Review of draft-ietf-cose-rfc8152bis-struct-03
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.
[JLS] Me too. - fixed
==
Nit: section 1:
"Applications may additionally want to defined
additional data fields as part of the stucture."
should be "... want to define ... structure"
[JLS] Fixed
==
Nit: Section 1.2
"standalong" should be "standalone"
[JLS] Already fixed.
==
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.
[JLS] Already fixed, but fixed it in the algs draft as well.
==
Nit: Section 1.6
"Authenticated Encryption with Authenticated Data (AEAD) [RFC5116]"
RFC 5116 expands AEAD to 'Authenticated Encryption with Associated Data'
[JLS] Fixed here and in the algs draft.
==
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:"
[JLS] I looked at this and trimmed it down even more to "The two buckets 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
[JLS] I agree, I should have done then when Carsten suggested using them
originally.
==
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?
[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
"Partial IV: [...] The IV can be placed in the unprotected header ..."
This should read "The Partial IV can be placed in the ..."
[JLS] Fixed
==
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.
[JLS] Fixed - I re-wrote the second sentence to define both the context and
partial IV values.
==
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.
[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
" 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.
[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.
"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"
[JLS] FIxed
==
Nit: 5.2
"The object was to keep the countersignature as small as possible ..."
should be "The objective ..."
[JLS] Fixed
==
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*'
[JLS] Fixed
==
Nit Section 18.
"... at the time of posting of this Internet-Draft"
This sentence will be wrong as soon as the RFC is published.
[JLS] And is academic since the section is to be removed prior to publish as an
RFC.
==
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.
[JLS] I put in a reference to 'key context' in the terminology section so it is
now defined (albeit indirectly).
==
Note: I did not check the correctness of the examples in Appendix C.
[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
_______________________________________________
COSE mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/cose
