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
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ COSE mailing list [email protected] https://www.ietf.org/mailman/listinfo/cose
