Subject: [COSE] Feedback on draft-ietf-cose-rfc8152bis-struct-05 and draft-ietf-cose-rc8152bis-algs-04

From: Gregory Guthe <[email-masked]>
Date: Wed, 28 Aug 2019 14:26:32 -0400

These look ready to me, some nits below (probably some duplicates with
other feedback). I didn't check the examples.

struct:

If layers are for different levels of nesting, it might be helpful to
front-load some context from Appendix B.

p. 25 typo "sginature"

p. 26 "CBOR tag TBO0" waiting on IANA registration?

p. 34 "COSE_MAC is used for all other cases. These include a requirement
for multiple recipients, the key being unknown, and a recipient algorithm
of other than direct." rephrase? sounds like one case requiring three
properties instead any case requiring one of the three properties

p. 45 "an unsupported algorthms"

p. 55 "the use of a single key for multiple algorithms has been
demonstrated in some cases to leak information about a key ..." Maybe "a
key" to "the key" if we're talking about that specific key.  It'd be good
to have links to references going over the cases too.

p. 65 maybe "the second context contains a key for dealing with encrypted
messages" -> "the second context contains another key for dealing with
encrypted messages"

p. 67 maybe "the output of the example presented in both a hex and a CBOR
diagnostic ... format" -> "the output of the example presented in both hex
and CBOR diagnostic .. formats"

algs:

p. 3 Introduction paragraph 2 "This document provides for an initial set of
algorithms that are then use with those structures": "use" to "used" or
"provides an initial set of algorithms for use with ..."

p. 17 4.3.1 "nounce" typo

p. 18 maybe "any positive salt length" instead of "any amount of salt"

p. 23 AlgorithmID: "The field exists ... for each of those algorithms"
maybe rephrase

p. 23 PartyUInfo: "are encoded in the order presented" possibly add "below"
or "following order" though it's pretty clear from the context too

Looking forward to updating the test cases for go-cose, and checking with
Franziskus about updating cose-rust.

Cheers,
Greg Guthe

---

From: Jim Schaad <[email-masked]>
Date: Wed, 4 Sep 2019 12:30:08 -0700

See inline comments.

Jim

From: COSE <[email-masked]> On Behalf Of Gregory Guthe
Sent: Wednesday, August 28, 2019 11:27 AM
To: [email-masked]
Subject: [COSE] Feedback on draft-ietf-cose-rfc8152bis-struct-05 and draft-ietf-cose-rc8152bis-algs-04

These look ready to me, some nits below (probably some duplicates with other feedback). I didn't check the examples.

struct:

If layers are for different levels of nesting, it might be helpful to front-load some context from Appendix B.

p. 25 typo "sginature" -- fixed

p. 26 "CBOR tag TBO0" waiting on IANA registration? – Yes – this is a new registration with this document

p. 34 "COSE_MAC is used for all other cases. These include a requirement for multiple recipients, the key being unknown, and a recipient algorithm of other than direct." rephrase? sounds like one case requiring three properties instead any case requiring one of the three properties --  

[JLS] If we switch the /and a/or a/ do you think that clears this up?

p. 45 "an unsupported algorthms" -- fixed

p. 55 "the use of a single key for multiple algorithms has been demonstrated in some cases to leak information about a key ..." Maybe "a key" to "the key" if we're talking about that specific key.  It'd be good to have links to references going over the cases too.  – changed ‘a’ to ‘that’.  

[JLS] Are you looking for links to each of those three instances or something else?  It may be that this should be removed from the document as the “mentioned before” is no longer relevant in the structure document only in the algorithm document – 

ALL: Should this just be killed?

p. 65 maybe "the second context contains a key for dealing with encrypted messages" -> "the second context contains another key for dealing with encrypted messages"

[JLS] Switched to “the second context contains a different key”

p. 67 maybe "the output of the example presented in both a hex and a CBOR diagnostic ... format" -> "the output of the example presented in both hex and CBOR diagnostic .. formats"

[JLS] Changed to “both as a hex dump and in CBOR diagnostic notation format.”

algs:

p. 3 Introduction paragraph 2 "This document provides for an initial set of algorithms that are then use with those structures": "use" to "used" or "provides an initial set of algorithms for use with ..."

[JLS] fixed --         This document provides an initial set of algorithms for use with those structures.

p. 17 4.3.1 "nounce" typo -- fixed

[JLS] fixed – that makes sense to me.

p. 23 AlgorithmID: "The field exists ... for each of those algorithms" maybe rephrase

[JLS]  I have changed the sentences, does this read better?

            The field exists in the context information so that a different key is generated for each algorithm even of all of the other context information is the same.

            In practice, this means if algorithm A is broken and thus finding the key is relatively easy, the key derived for algorithm B will not be the same as the key derived for algorithm A.

p. 23 PartyUInfo: "are encoded in the order presented" possibly add "below" or "following order" though it's pretty clear from the context too

[JLS] Doesn’t cost anything - done

Looking forward to updating the test cases for go-cose, and checking with Franziskus about updating cose-rust.

[JLS] If we did this right, then there are no changes in any of the test cases.  Although somebody just pointed out that my test case for RSA-PSS was totally messed up.

Cheers,

Greg Guthe