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 protected]] On Behalf Of Samuel Erdtman
Sent: Sunday, July 31, 2016 12:42 AM
To: cose <[email protected]>; Jim Schaad <[email protected]>
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.
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 >
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.
_______________________________________________
COSE mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/cose
