Subject: [COSE] draft-ietf-cose-msg-15 review

From: Samuel Erdtman <[email-masked]>
Date: Sun, 31 Jul 2016 09:41:54 +0200

Hi

I after reading the specification a second time and started on a JavaScript
implementation things are starting to make sense and I like it. Maybe a bit
long (feels like a mountain to climb) before starting to work with it.
However well done Jim and all others that has contributed. I like the
combination of prose and CDDL, it is great to have the same thing described
in two ways.

If it´s not to late I would like to contribute with a set of comments.

== General comments
* Is it possible to link to tables it would be great not having to search
form them e.g. in section 3.1. where Common COSE Headers Parameters
referencing Table 2.

* Encryption section, 5, was harder to read then Signing, section 4, and
Mac, section 6. It felt a more mixed up, but I don’t have any concert
suggestions that would make it much better.

* Is this specification registering a whole new set of algorithms? and
registries? for CWT we only add mappings but keep the attributes, this is
also much of the work done in the ACE framework, but when I read here it
looks like you have new registries for everything. would it not be better
to just have mappings here.

* Without going into a deeper analysis it looks like section 7 to 9, and to
some extent 10 and 11 are doing the same thing as JWA (RFC7518). Would it
be possible to not duplicate things from RFC7518. I think this reflects in
the new IANA registries setup too. I think it would be good if we could try
to reuse more of the work gone into JOSE e.g. maybe only defining mappings
for the algorithm identifiers to CBOR labels/keys..

== Section specific comments and questions

=== 16.4.  COSE Algorithms Registry

“The initial contents of the registry can be found in Table 10,
   Table 9, Table 11, Table 5, Table 7, Table 8, Table 15, Table 16,
   Table 17, and Table 18.  The specification column for all rows in
   that table should be this document.”
* It would be great with links to the different sections (see general
comment about table links).

=== 1.1.  Design changes from JOSE
“MACed messages have the ability to use the same set of recipient
      algorithms as enveloped messages for obtaining the MAC
      authentication key.”
* Why is this called out for MACed messages, is this not true for signed
too?

=== 1.3.  CBOR Grammar
“We therefore describe the CBOR structures in prose.”
* I would prefer not to use ‘we’ and write something like “The CBOR
structures are therefore describe in prose.”

“The collected CDDL can be extracted from the XML version of this
   document via the following XPath expression below.  (Depending on the
   XPath evaluator one is using, it may be necessary to deal with &gt;
   as an entity.)”
* Is the XML available for ready RFCs? I have only found it for drafts, if
not this comment might not be very helpful.

=== 1.4.  CBOR Related Terminology
“In COSE, we use strings, negative integers and unsigned
   integers as map keys.”
* Feels like mix of terms with unsigned and negative integers I think it
should be signed and unsigned or positive and negative integers.

“The integers are used for compactness of
   encoding and easy comparison.”
* A motivation for strings would be good too since the existence of the
option requires implementors to handle both (not crash if a string comes).
If I remember correctly the reason was was of development/debugging.

=== 3.  Header Parameters
“Two buckets are provided for each layer:”
* This is the first time I see the term layer used, maybe an explanation in
the terms section

“(With the exception of the COSE_Sign structure, the
   only data that needs to cross layers is the cryptographic key.)”
* Is the parentheses really required, I would just drop them.

“Labels in each of the maps MUST be unique.  When processing messages,
   if a label appears multiple times, the message MUST be rejected as
   malformed.  Applications SHOULD perform the same checks that the same
   label does not occur in both the protected and unprotected headers.
   If the message is not rejected as malformed, attributes MUST be
   obtained from the protected bucket before they are obtained from the
   unprotected bucket.”
* Does this mean that I can have algorithm in both protected and
unprotected bucket but that the protected one takes presence? Could we not
just forbid duplication over both buckets?

=== 3.1.  Common COSE Headers Parameters
* Why is some of the headers shortened (alg, crit) but not all (content
type)? I would prefer if we could map as close to JOSE as possible and I
would also like to avoid spaces in the names because then I can very easily
do JSON input to my implementation and have it do the transformation of
labels from strings to integers. When reading more I see that this applies
to more sections.

“This parameter MUST be present in the
      COSE_Signature, COSE_Sign1, COSE_Encrypt, COSE_Encrypt0, COSE_Mac,
      and COSE_Mac0 structures”
* Should it not be COSE_Sign and not COSE_Signature?

“(The algorithm range is -1 to -65536; the higher
         end is for more optional algorithm specific items.)”
* How does this relate to “Integer labels in the range -1 to -255 can be
omitted”?
* With the “higher end” are you referring to -1? it seem strange to have
the more optional parameters in that end to me

“As the IV is authenticated by the
      encryption process, it SHOULD be placed in the unprotected header
      bucket.”
* Is there a good reason for this SHOULD? why is it better to put it in the
unprotected header? if I could I would put all my headers in the protected
and not have to bother with the unprotected part. I would prefer the
phrasing under Partial IV to be “As the IV is authenticated by the
encryption process, this value can be placed in the unprotected header
bucket”

“COSE_Sign, COSE_Sign1, COSE_Signature, COSE_Encrypt,
      COSE_recipient, COSE_Encrypt0, COSE_Mac and COSE_Mac0”
* In table 2 it says that the counter signature is one or many
COSE_Signature, so it does not seem to match this statement?
* When is a counter signature an encryption?

=== 4.4.  Signing and Verification Process
“The protected attributes from the body structure encoded in a
       bstr type.  If there are no protected attributes, a bstr of
       length zero is used.”
* Or a empty map CBOR encoded, right?

=== 5.1.  Enveloped COSE Structure
* This might have been up before, but has it been considered to swap place
of ciphertext and recipients so that not the full message has to be
received before the CEK can be retrieved and maybe the ciphertext can be
decrypted as the message is received.
* I cannot se any language on the key format in the recipients structure is
is always a COSE key? should the content type header be used to describe
the key format? or is it implicitly known by the application context?

=== 5.3.  Encryption Algorithm for AEAD algorithms
=== 5.4.  Encryption algorithm for AE algorithms
* Capital ‘A’ on algorithm or a small ‘a’ in section 5.3 headline

* Heading “5.4.  Encryption algorithm for AE algorithms” and “5.3.
Encryption Algorithm for AEAD algorithms” is confusing compared to the
equivalent sections in mac and signing. I would like theses sections to
call out that it is how to encrypt and decrypt.

=== 7.  Key Objects
* The CDDL example looks different, in my opinion worse, compared to the
other earlier examples.

* Are there any examples, I found no reference here.

=== 7.1.  COSE Key Common Parameters
“If this parameter is present in the key structure,
      the application MUST verify that this algorithm matches the
      algorithm for which the key is being used.”
* This is not reflected in Signing, Encryption and Mac section, maybe it
would be good to add something there too, if the relation to COSE_Key
should be close.

=== 16.  IANA Considerations
Why is the major type included in the registry? Is it not enough with the
specification reference and having the definition there. In my opinion the
main goal of the registration is to avoid label collisions and and not to
provide more then basic information.

=== 17.2.  COSE Testing Library
I would prefer test vectors in the specification compared to this github
link.

---

From: Mike Jones <[email-masked]>
Date: Sun, 31 Jul 2016 18:26:30 +0000

These seem like really useful review comments.  I hope they can be incorporated in a new draft, possibly as part of resolving IETF last call comments.

                                                       -- Mike

From: COSE [mailto:[email-masked]] On Behalf Of Samuel Erdtman
Sent: Sunday, July 31, 2016 12:42 AM
To: cose <[email-masked]>; Jim Schaad <[email-masked]>
Subject: [COSE] draft-ietf-cose-msg-15 review

Hi

I after reading the specification a second time and started on a JavaScript implementation things are starting to make sense and I like it. Maybe a bit long (feels like a mountain to climb) before starting to work with it. However well done Jim and all others that has contributed. I like the combination of prose and CDDL, it is great to have the same thing described in two ways.

== General comments
* Is it possible to link to tables it would be great not having to search form them e.g. in section 3.1. where Common COSE Headers Parameters referencing Table 2.

* Encryption section, 5, was harder to read then Signing, section 4, and Mac, section 6. It felt a more mixed up, but I don’t have any concert suggestions that would make it much better.

* Is this specification registering a whole new set of algorithms? and registries? for CWT we only add mappings but keep the attributes, this is also much of the work done in the ACE framework, but when I read here it looks like you have new registries for everything. would it not be better to just have mappings here.

* Without going into a deeper analysis it looks like section 7 to 9, and to some extent 10 and 11 are doing the same thing as JWA (RFC7518). Would it be possible to not duplicate things from RFC7518. I think this reflects in the new IANA registries setup too. I think it would be good if we could try to reuse more of the work gone into JOSE e.g. maybe only defining mappings for the algorithm identifiers to CBOR labels/keys..

* It would be great with links to the different sections (see general comment about table links).

=== 1.1.  Design changes from JOSE
“MACed messages have the ability to use the same set of recipient
      algorithms as enveloped messages for obtaining the MAC
      authentication key.”
* Why is this called out for MACed messages, is this not true for signed too?

=== 1.3.  CBOR Grammar
“We therefore describe the CBOR structures in prose.”
* I would prefer not to use ‘we’ and write something like “The CBOR structures are therefore describe in prose.”

“The collected CDDL can be extracted from the XML version of this
   document via the following XPath expression below.  (Depending on the
   XPath evaluator one is using, it may be necessary to deal with &gt;
   as an entity.)”
* Is the XML available for ready RFCs? I have only found it for drafts, if not this comment might not be very helpful.

=== 1.4.  CBOR Related Terminology
“In COSE, we use strings, negative integers and unsigned
   integers as map keys.”
* Feels like mix of terms with unsigned and negative integers I think it should be signed and unsigned or positive and negative integers.

“The integers are used for compactness of
   encoding and easy comparison.”
* A motivation for strings would be good too since the existence of the option requires implementors to handle both (not crash if a string comes). If I remember correctly the reason was was of development/debugging.

=== 3.  Header Parameters
“Two buckets are provided for each layer:”
* This is the first time I see the term layer used, maybe an explanation in the terms section

* Does this mean that I can have algorithm in both protected and unprotected bucket but that the protected one takes presence? Could we not just forbid duplication over both buckets?

=== 3.1.  Common COSE Headers Parameters
* Why is some of the headers shortened (alg, crit) but not all (content type)? I would prefer if we could map as close to JOSE as possible and I would also like to avoid spaces in the names because then I can very easily do JSON input to my implementation and have it do the transformation of labels from strings to integers. When reading more I see that this applies to more sections.

“(The algorithm range is -1 to -65536; the higher
         end is for more optional algorithm specific items.)”
* How does this relate to “Integer labels in the range -1 to -255 can be omitted”?
* With the “higher end” are you referring to -1? it seem strange to have the more optional parameters in that end to me

“As the IV is authenticated by the
      encryption process, it SHOULD be placed in the unprotected header
      bucket.”
* Is there a good reason for this SHOULD? why is it better to put it in the unprotected header? if I could I would put all my headers in the protected and not have to bother with the unprotected part. I would prefer the phrasing under Partial IV to be “As the IV is authenticated by the encryption process, this value can be placed in the unprotected header bucket”

“COSE_Sign, COSE_Sign1, COSE_Signature, COSE_Encrypt,
      COSE_recipient, COSE_Encrypt0, COSE_Mac and COSE_Mac0”
* In table 2 it says that the counter signature is one or many COSE_Signature, so it does not seem to match this statement?
* When is a counter signature an encryption?

=== 5.1.  Enveloped COSE Structure
* This might have been up before, but has it been considered to swap place of ciphertext and recipients so that not the full message has to be received before the CEK can be retrieved and maybe the ciphertext can be decrypted as the message is received.
* I cannot se any language on the key format in the recipients structure is is always a COSE key? should the content type header be used to describe the key format? or is it implicitly known by the application context?

* Heading “5.4.  Encryption algorithm for AE algorithms” and “5.3.  Encryption Algorithm for AEAD algorithms” is confusing compared to the equivalent sections in mac and signing. I would like theses sections to call out that it is how to encrypt and decrypt.

=== 7.  Key Objects
* The CDDL example looks different, in my opinion worse, compared to the other earlier examples.

* This is not reflected in Signing, Encryption and Mac section, maybe it would be good to add something there too, if the relation to COSE_Key should be close.

=== 16.  IANA Considerations
Why is the major type included in the registry? Is it not enough with the specification reference and having the definition there. In my opinion the main goal of the registration is to avoid label collisions and and not to provide more then basic information.

=== 17.2.  COSE Testing Library
I would prefer test vectors in the specification compared to this github link.

---

From: Jim Schaad <[email-masked]>
Date: Sun, 31 Jul 2016 13:14:25 -0700

First, thanks for the review comments.  It is never too late until the RFC is actually published and even then a new version can be published to address future issues.

Individual comments inline

Jim

From: Samuel Erdtman [mailto:[email-masked]] 
Sent: Sunday, July 31, 2016 12:42 AM
To: cose <[email-masked]>; Jim Schaad <[email-masked]>
Subject: draft-ietf-cose-msg-15 review

Hi

== General comments 

[JLS]  I am going to assume you are looking at the HTMLized version of the document on the IETF web site. The links that are inserted are a function of the tool that converts the document from text to HTML rather than a function of the source document (which does have the links in the XML).  If you look at the HTML version on the github site, then you can get the links but will not have table numbers until a new version of xml2rfc is released (assuming it has my bug fixes in it).  Sorry, cannot help on this issue.

[JLS] I have a feeling that this is because of the recursive nature of how encryption is done.  Mac does not suffer from that because all of the recipient processing is covered in encryption.  Without a specific suggestion it is hard to figure out what to do.  It will be interesting to see if others have suggestions.

[JLS] First, registries are cheap so the mere fact that we are creating a large number of new registries is not, in and of itself, a problem.  

[JLS] I did consider re-using the JWA registries for the registries that I created, in the end I did not for a couple of reasons.  The primary reason is that the there are some algorithms defined in the COSE document which are not the same as are defined in JWA.   The method of going from a ECDH key agreement operation to a symmetric key is completely different both in terms of the context structure and KDF algorithm used to do the derivation.  This would mean that one would start getting multiple algorithms that appear to be the same but aren't in many details.  There are a couple of registries such as the Curve Table which are going to have the same contents, but it just seemed easier to continue making new registries rather than try and figure out which should be duplicated and which should be separate.  Going in from the IANA web page, it is going to be easier to just find all of the COSE registries by name if the start with the same string.

[JLS] See comment above.

[JLS] No this is not true for signed messages.  For MACed messages, one needs to convey the secret symmetric key from the sender to the recipient in the same way that one does this for encrypted messages.  For signed messages one needs to identify a public key, there is no need to keep that field secret.

=== 1.3.  CBOR Grammar

[JLS] that is reasonable.

[JLS] This depends on where one gets the RFC and how far into the future one looks.  It is true that currently only the text is available for an RFC on the RFC Editor web site, however if one goes through the data tracker the XML is available. In the, hopefully not too distant future, the RFC Editor web site is going to start having XML for RFCs as that is becoming the official version of an RFC with HTML and other versions as just instantiations of the XML.

[JLS] Yeah, I had the same problem with this as well.  These are the terms used by CBOR so blame them.  I think the issue is that normally unsigned integers includes zero, but positive integers do not include zero.  I don't know why they did not use signed and unsigned.

[JLS] While that would be one motivation, it is not really the motivation that I had.  I was looking for a cheap way to increase the number of two byte labels.  The requirement not to crash is no different if the label is a string or a floating point value.  This requirement is in the next paragraph.   The code to ignore string labels until we have defined one is not really a big issue.  I have added some justification text on the github version.

=== 3.  Header Parameters

[JLS] Fine.

[JLS] Yes that is what the text means.  The reason that this is a should rather than must is that for some people might find the checks that all of the headers are unique to be burdensome on small devices.  Providing the alternate method of getting the same result, that is the the protected headers win, means that we will have a consistent set of implementations.  Removal of fields from the protected headers will cause failures to occur while adding things by middle boxes to the unprotected headers will not cause a cryptographic failure.

[JLS] I have kept the same names where it seemed reasonable, but used new names where it seemed more appropriate.  For 'content type', what is defined here does not have an exact match in the JWS world.  It ends up combining some of both the 'typ' and 'ctyp' fields of JOSE.  If you have a JWT, then I have been told you put that into the 'typ' field of JWS, but it goes into the 'content type' field of COSE.    The other two which have spaces in them do not have a counterpart in JOSE.  However, given that the string names are just for discussion purposes I am not sure that using really short names is generally a good way to go.

[JLS] No COSE_Signature is correct.  There is no algorithm field for a COSE_Sign as the signature algorithm needs to be different for each signer when multiple signers are on a message.

[JLS] The intention is to say greater in absolute magnitude.  If you have a better term then please share it.  Smaller and lower seem to have the same problem as higher does.

“As the IV is authenticated by the
      encryption process, it SHOULD be placed in the unprotected header
      bucket.”
* Is there a good reason for this SHOULD? why is it better to put it in the unprotected header? if I could I would put all my headers in the protected and not have to bother with the unprotected part. I would prefer the phrasing under Partial IV to be “As the IV is authenticated by the encryption process, this value can be placed in the unprotected header bucket”
[JLS] The strengthening of this statement was made at the request of Göran so he should probably respond.  

[JLS] You missed the preceding sentence "The counter signature can occur as an unprotected attribute in any of the following structures" 
It should have a "parameter" between "signature" and "can".

[JLS] No, this is a case where the fact that there are two equivalent encodings means we must choose one of them and only use it.  The same byte stream must always be obtained regardless of which of those two encodings are used.

=== 5.1.  Enveloped COSE Structure
* This might have been up before, but has it been considered to swap place of ciphertext and recipients so that not the full message has to be received before the CEK can be retrieved and maybe the ciphertext can be decrypted as the message is received.
[JLS] No that was never discussed.  Given that we are focused on small messages I don't think that it would be a big win given that it would make the processing of the encryption structures more complicated.  If that is something that is desired, I would recommend that such an application uses a detached content and send the COSE structure followed by the ciphertext stream.

[JLS] There is no format for a key, it is a binary value.  You are the second person who seem to think it should be structured content.  Please let me know what in the text made you think this as it is clearly not sufficiently explicit.

[JLS] should be upper case

[JLS] That makes sense

=== 7.  Key Objects

[JLS] This was done in response to an earlier review where it was noted that the strings in the CDDL and the strings in the text did not match.  Since the numbers are not the same, even though the strings are it was deemed better to remove the strings from the CDDL than to rewrite all of the text which would have had other problems.

[JLS] Added reference

[JLS] No I don't think it should be in those places as well.  There is no requirement that you need to be using COSE_Key objects to move keys around.  While it is easy for somethings, in other cases there are going to be completely different methods of moving keys.  Since the place one draws keys from may not always be a COSE_Key object, I don't think that putting this text in everywhere is the best use of space.

[JLS] Your opinion is noted.  I would prefer to have the type information due to the fact that not all specifications are going to be public.  Having the type information will still allow for middle boxes to check that the type is correct even if the content of the field is not understood.

[JLS] You were complaining earlier about the length of the document already.  If you want a printed RFC which contains the broken out examples then argue for that on the list, but I prefer having it outside of the document as it makes things easier to correct if they are wrong.  Also if you look at the github link you will see that there are lot of examples which are not in this document as it can be much more complete.

Jim