Text changes can be found at https://github.com/cose-wg/cose-spec/pull/160
More comments inline Jim > -----Original Message----- > From: COSE [mailto:[email protected]] On Behalf Of Carsten Bormann > Sent: Sunday, June 12, 2016 12:59 PM > To: Justin Richer <[email protected]> > Cc: cose <[email protected]> > Subject: Re: [COSE] WGLC > > Carsten Bormann wrote: > > expect my full review > > Below. > This was one of the better drafts I have reviewed in my IETF life. > > Grüße, Carsten > > > The COSE message syntax draft is mostly ready to implement. > Some further editorial work could improve the document. > (Pull request #158 contains some typos and other trivial editorial > improvements.) > > # Process > > 8.2 > Are we creating a dependency here on a document that might take forever to > complete? > Maybe mark this section with a "Sollbruchstelle" (~ toilet paper > perforation), so > it can be detached into a separate document if that happens. While it might be that it takes forever to finish, this is not the impression that I have of where the RG chairs want to be. At worst this might stall the final publication of the RFC but I would be surprised if it was by far. Also I would note that this is an Informative reference this means that it is possible to publish with a reference to the ID just like we are going to do with the CDDL document. > > # Technical > > 2. second item list, item 3/4. > The media type system REST adopted from MIME is represented by the Content- > Type header field in HTTP and the Content-Format (!) Option in CoAP. (The > latter also covers what is Content-Encoding in HTTP.) > Editorial: The text here is about media types, and only then about how these > are > represented in the REST protocols. > Technical: CoAP Content-Formats are represented by unsigned integers, not > integers in general (fix header parameter 3, content type). I think you covered this in your pull request. If not, what are you looking for. > > Related in 3.1: > Technical: HTTP's Content-Type header fields and CoAP Content-Formats can > contain (media type) parameters (like COSE's cose-type), the definition of > header parameter 3 seems to allow this only on the > (unsigned) integer side (Content-Format), not for a tstr. Your right it makes sense to allow this. It is a loss of functionality from the JOSE documents that I missed. > > While discriminator strings such as "Signature", "Signature1", and > "CounterSignature" are cute, maybe a numeric label would be more appropriate > for processing by a constrained implementation. > (I know that these are just processed, not exchanged. Still.) This was the hardest one for me to think about and respond to. I can understand why you are saying this. I wish that could whole heartedly agree but I can't. I can agree with shortening these strings, but going to integers just seems to me to be asking for potential problems in the future if we start having other structures created, or even the current structures being re-purposed for a different specific purpose. I also think that strings are easier for people to recognize problems in general. Not sure yet what to do about this. > > If we don't mind spurious stuff in signer input, we should include "protected" > even for COSE_Sign1 (as an empty bstr), simplifying implementation. If we allow this, I would not want to allow it as an empty bstr but as something that would be totally different. That being the case I don't think that absent is any harder than nil would be. Making this an absent field might make it slightly harder to implement, but given that you need to deal with the fact that the sources of information is different I don't think that there is a real problem here. > > Why are kids always bstr? It seems natural to allow tstrs here. I don't see any real benefit to that. Allowing for two types now means that you need to worry about what the type is if you are going to display, you need to keep the type for doing comparisons and so forth. This would seem to make things complicated. If one wishes to do a textual display one can see if it follows a text constrained format and then do the display if one wishes to. > > key_ops could more naturally be a bit field (CDDL .bits). > And start from 0. That would be true for the set of key ops that we have defined. However, it would mean that future applications would potentially have a more difficult time creating a key op for their specific purpose. I would worry that applications would only expect a small value for the bit field and have problems with larger values. This is not a problem with the current method. > > I don't understand the effect of Appendix A. Does this relieve the statements > earlier of the form "The 'alg' parameter MUST be present."? That would be for a specific application to state. But in the generic case it does not. > > (I'll add my ceterum censeo about private-use spaces and the X-Dash > effect.) > > # Editorial/technical > > While the changes discussed here do not change the technical substance of the > protocol, they may facilitate implementation. > > The definition of the "protected" header map fields should use more of what > CDDL can do to define them, see #154. > > Please use consistent spacing, "label: value" (preferred) or consistently > "label : > value"; that makes searching so much simpler. Done - although I disagree on the preferred version. > > Hmm, Sign1, Encrypt1, and Mac0. Sign1 contains 1 signature. Encrypt1 > contains > 0 recipient structures. Mac0 contains 0 recipient structures. Find the odd > one > out :-) I think it would be less confusing to have Sign1, Encrypt0, and Mac0. Yes you are right > > The counter signature parameter (parameter 7) contains one or more counter > signatures, not just one. > > One of the nice things about JOSE is that the various labels have a defined > name, > which is then re-used as a variable or field name in an implementation. We > are > not really providing the same level of service. > (Also cf. the previous paragraph -- this is talking about a parameter called > "counter signature", but that doesn't stand out in that sentence, so I also > referenced the parameter number 7.) A related problem are sentences such as > o The 'kty' field MUST be present and it MUST be 'Symmetric'. > Of course, we mean it must be 4 (and not the string 'Symmetric' in bstr or > tstr > form), but it would not help to state this numerically. Not sure how to address this comment. I can be more proactive about making sure that the single quotes are used everywhere that it is the name of a parameter field rather than a concept and mark that in the terminology section up top. Is that going to be enough or do you have a more solid proposal? > > 10.2.1 > o The key and nonce pair MUST be unique for every message > encrypted. > (for a single key, that is) I am not sure what you are asking for here. It would be equally true to have the note "(for a single nonce, that is)". > > 12.2 > o At a minimum, the 'unprotected' field MUST contain the 'alg' > parameter and SHOULD contain a parameter identifying the shared > secret. > Why is the first a MUST if we can have the second? > (The 'unprotected' field could be attacker-controlled; information attached to > the shared secret cannot be.) The first falls out from the requirement that an 'alg' parameter always be present in a message. One could also use this in the absence of a 'kid' to filter out the set of keys to be used. The second would be insufficient if the same secret could be used for multiple algorithms. I.e. it could be used for both a key wrap algorithm in a recipient structure and as a CEK for an Encrypt0 message with a different algorithm. In both cases you are going to end up in a situation of having a denial of service attack if either value was changed by an attacker. > > # Editorial > > 1.1 and the rest of the document don't agree whether it is "MAC messages" or > "MACed messages". Since the messages do not only contain a MAC, the second > phrasing is more accurate. That makes sense > > 1.2 and 1.5 could be combined, as someone searching for terminology might > find one of them and then not the other. > > 1.4 could possibly be read as completely ruling out non int/tstr map keys in > any > CBOR contained in COSE messages; maybe it could be more explicit that this > constraint is on the message structures defined by this specification. I think you are probably being overly paranoid, however I made some changes for this > > 2., second item list > Maybe introduce local terms "untagged message" and "tagged message" in item > 1 and 2, resp. I have identified both of these terms in the 2nd bullet. This makes me wonder if I should swap the order of bullets 1 and 2. > > 3. "Two buckets..." > Maybe add example "(i.e., the byte string h'a0')" to clarify what a CBOR- > serialized empty map is. That makes sense > > I think I understand what the sentence with "finesses" is trying to say, but > it may > be confusing. (Same with "vindicated" in 9.1 -- we probably can agree on the > gist > of this statement, but at least the dictionary sense "clear (someone) of > blame or > suspicion" is not appropriate here.) The dictionary also has "to show that (someone or something that has been criticized or doubted) is correct, true, or reasonable" for this as well. But I can see the point. > > Some of the statements in 3 also are true for key structures, that is not > always > clear. If this is true, then those statements should be echoed in section 7 as well. Which statements do you think are missing there. > > "should perform the same checks that..." -> "should also check that the same > label does not occur in both the protected and unprotected headers" Yes, that works better > > 4. > "cannot be converted" -- of course they can. They just lose something then. > Can we be more explicit what is that what we lose? Done > > 4.2 > "COSE Single signature messages" -- what is that? Done > > 4.3 > "if the same relative numbering is kept" -- huh? > (Of course, you wouldn't.) On a strictly naïve implementer basis, I am willing to bet that this might be one of the most common bugs that is going to pop up when testing. If you copy over the correct set of options in the sender and then do the same in the validator 90+ percent of the time it is going to be just fine and work just great. Only if the set of options actually gets changed during the testing process is it going to be a problem. > > 5.4 > (define AE algorithm) Done > > 6.3 > How is that "encode it as a binary value" done? Missed that one when the other two were re-written > > 6.3 > Maybe we can use names like ToBeMaced at the place where the input is > defined so the reference later is clearer. (Twice.) done > > 10.2.1 > "the portions encryption" -- Huh? Cleaned up > Section 11.2 > There are (fortunately few) cases of text that is repeated without need. > PartyVInfo could really just explained as analogous to PartyUInfo. Given that I pulled out the PartyInfo structure, it makes sense to cut this down even farther. > > 12.1.2 > unpredictable for whom? I don't think it matters who it is unpredictable for, but I would consider there to be a distinction between random and a "predictable" but random method such as encrypting a counter which is incremented. > > 12.2 > The "MUST deal" is still not that clear. The following text seems to be to be clear what this means. Can you elaborate on what you think is not clear? > > 12.4 second para, second sentence. > Hmm. COSE is designed to be usable in that environment, but that doesn't mean > it never can be used in an "on-line" environment (we actually plan to do this > in > CoAP "object security"). I think of the way you are using it more as a store-and-forward without necessarily having a separate store entity. After all, the protocol that you are outlining would work just as well in a true store-and-forward environment as well. However, I can also think of it as being an 'on-line' protocol as well. These are the terms that I have normally used for distinguishing between message based protocols and something like TLS/DTLS which is more session based. Do you have a suggestion for a better pair of contrasting terms for this? > > 16.9 > The cose-key media types are sometimes called cose-key+cbor and sometimes > cose-key. If we don't have +cbor on application/cose itself, why should it > be on > the key types? I had a reason for doing it this way, but I cannot seem to find a historical note that I left myself about why I removed the +cbor for one of them and not for the other. It might have some vague reasoning that I was thinking of the possibility of a different encoding being used for the key structures but not for the message structures but I can't think of why that would have been. > > # Editorial, covered in #158 > > 1. > I prefer the abbreviation "IoT" > (IOT is interoperability testing to some of us) > > 4.4 > s/consistent/well-defined/ > > 5.4 > s/absent/empty/g > > # Other > > Where the evolution from RFC 2633 to RFC 5751 is mentioned, maybe there > could be a brief mention that there was another stage (RFC 3851). > > _______________________________________________ > COSE mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/cose _______________________________________________ COSE mailing list [email protected] https://www.ietf.org/mailman/listinfo/cose
