Thanks for the quick reply Jim, I have fond another thing I want to ask
about and then I have added comments inline.
I added you Göran in direct message because I had a question on a
formulation that you had requested, search for your name in the response
form Jim and you´ll find the question.
I might just be lacking in CBOR skills but to see it seems like an
inconsistency
3. Header Parameters
“Senders SHOULD encode an empty protected map as a
zero length binary object (i.e., the byte string h'a0'). This
encoding is used because it is both shorter and the version used
in the serialization structures for cryptographic computation.”
Here it says that I should use the “zero length binary object” since it is
the form used in “the serialization structures for cryptographic
computation.”, but in section
“4.4. Signing and Verification Process” and
“6.3. How to compute and verify a MAC” it says when creating the
MAC_structure or Sig_structure:
“The protected attributes from the COSE_MAC structure. If there are no
protected attributes, a zero length bstr is used.”
and
“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.”
//Samuel
On Sun, Jul 31, 2016 at 10:14 PM, Jim Schaad <[email protected]> wrote:
> 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 protected]]
> Sent: Sunday, July 31, 2016 12:42 AM
> To: cose <[email protected]>; Jim Schaad <[email protected]>
> Subject: 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.
>
> 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.
>
> [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.
>
> * 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.
>
> [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.
>
I´ll reread and think about it.
> * 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.
>
> [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.
>
I understand that the cost of creating is low. I was thinking in lines of
not having to duplicate maintenance work and synergies of getting registry
updates from two directions since the content is so similar.
>
> * 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..
>
> [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.
>
I can see your point, I might personally had selected another path, but I´m
fine with your choice if no one else thinks it is a problem..
>
> == 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).
>
> [JLS] See comment above.
>
>
> === 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?
>
> [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.
>
My bad
>
> === 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.”
>
> [JLS] that is reasonable.
>
> “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 >
> 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.
>
> [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.
>
Okay, if that is the case then it makes sense to keep it.
>
>
> === 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.
>
> [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.
>
Okay
>
> “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.
>
> [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.
>
Thanks
>
> === 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.
> [JLS] Fine.
>
> “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?
> [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.
>
Okay
>
> === 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.
>
> [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.
>
Would it not make sense then to have no shortened names. If I understand
things correctly all labels have descriptions in the specification, there
is no labels where we just refer to the JOSE description.
I don´t really need the shortening, what i need/want is names I can use
directly in variable/constant names. In the implementation I´m working on I
try to keep the user from knowing about the integer labels, letting the
library do the translation to and from names and then it would be more
convenient if the names where possible to user directly as
variables/constants i.e. no spaces in the names.
>
> “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?
>
> [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.
>
Okay
>
> “(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
> [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.
>
That is a challenge, when I read it again I see two other problems what
does "more optional" meed and where is the breaking point between more and
less optional items (similar to 1024 for TCP and UDP ports). Maybe it would
be okay to remove the content of the parentheses.
>
> “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.
>
> “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?
>
> [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".
>
Now I get it, thanks
>
> === 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?
>
> [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.
>
Of course.
>
> === 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.
>
Seems reasonable I thought about the added code complexity but not about
the detached solution.
>
> * 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?
>
> [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.
>
Not sure, I think I noted it when implementing (or thinking about) and had
to understand how to take the decrypted CEK and input it to the underlying
crypto engine. When implementing both sides i would know the format i.e.
application context but if I would like to be more flexible and support to
handle different key formats (e.g. in a server) then I would need to know
the format of the key, right?
>
> === 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
>
> [JLS] should be upper case
>
> * 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.
>
> [JLS] That makes sense
>
> === 7. Key Objects
> * The CDDL example looks different, in my opinion worse, compared to the
> other earlier examples.
>
> [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.
>
Okay, so if I understand it correctly there kty, kid etc. has other numeric
label values in other context so to have the string in the CDDL might
confuse since it is translated to other numeric values. Not sure I agree
with earlier reviewer but okay.
Why is it not possible to use the same numeric lable for kty, kid, etc in
all places?
>
> * Are there any examples, I found no reference here.
>
> [JLS] Added reference
>
>
> === 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.
>
> [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.
>
Okay
>
>
> === 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.
>
> [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.
>
> === 17.2. COSE Testing Library
> I would prefer test vectors in the specification compared to this github
> link.
>
> [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.
>
Okay, and you have created a great number of examples on the github pages,
which is awesome, I have not yet understood exactly how I would use them
but I guess I will by spending some time on it.
>
> Jim
>
>
> _______________________________________________
> COSE mailing list
> [email protected]
> https://www.ietf.org/mailman/listinfo/cose
>
_______________________________________________
COSE mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/cose
