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.
# 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).
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.
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.)
If we don't mind spurious stuff in signer input, we should include
"protected" even for COSE_Sign1 (as an empty bstr), simplifying
implementation.
Why are kids always bstr? It seems natural to allow tstrs here.
key_ops could more naturally be a bit field (CDDL .bits).
And start from 0.
I don't understand the effect of Appendix A. Does this relieve the
statements earlier of the form "The 'alg' parameter MUST be present."?
(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.
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.
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.
10.2.1
o The key and nonce pair MUST be unique for every message
encrypted.
(for a single key, 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.)
# 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.
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.
2., second item list
Maybe introduce local terms "untagged message" and "tagged message" in
item 1 and 2, resp.
3. "Two buckets..."
Maybe add example "(i.e., the byte string h'a0')" to clarify what a
CBOR-serialized empty map is.
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.)
Some of the statements in 3 also are true for key structures, that is
not always clear.
"should perform the same checks that..." -> "should also
check that the same label does not occur in both the protected and
unprotected headers"
4.
"cannot be converted" -- of course they can. They just lose something
then. Can we be more explicit what is that what we lose?
4.2
"COSE Single signature messages" -- what is that?
4.3
"if the same relative numbering is kept" -- huh?
(Of course, you wouldn't.)
5.4
(define AE algorithm)
6.3
How is that "encode it as a binary value" done?
6.3
Maybe we can use names like ToBeMaced at the place where the input is
defined so the reference later is clearer. (Twice.)
10.2.1
"the portions encryption" -- Huh?
There are (fortunately few) cases of text that is repeated without
need. PartyVInfo could really just explained as analogous to
PartyUInfo.
12.1.2
unpredictable for whom?
12.2
The "MUST deal" is still not that 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").
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?
# 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
