Rohan's review #23
Rohan's review #23
Conversation
laurencelundblade
left a comment
laurencelundblade
left a comment
There was a problem hiding this comment.
Hi Rohan,
Thank you very much for the detailed review. I really appreciate the help getting the wording right.
Note that some of this text is rearranged in PR #27 -- there will be merge conflicts between this PR and that one. That's OK. We'll fix them when we get to them.
| For example, CBOR Web Token, {{-CWT}} does not specify serialization; therefore, a full and proper CWT decoder must be able to handle variable-length CBOR argments plus indefinite-length strings, arrays and maps. | ||
| For example, CBOR Web Token (CWT), {{-CWT}} does not specify serialization; therefore, a full and proper CWT decoder must be able to handle variable-length CBOR arguments plus indefinite-length strings, arrays and maps. | ||
|
|
||
| > Note that CBOR Object Signing and Encryption (COSE) {{-COSE}} uses effectively ordinary encoding (see {{OrdinarySerialization}}) for its internal structures, such as the `Sig_structure`. |
There was a problem hiding this comment.
Section 9 of RFC 9052 references section 4.2.1 of RFC 8949 which is about deterministic serialization. In principle COSE, absolutely requires determinism for the three structures.
Typically, there are no maps in these structures. When there are no maps ordinary == deterministic. However there can be maps in the values of parameters. For example the 'crit' header parameter is a map.
I don't think COSE should be mentioned in the context of ordinary serialization at all.
There was a problem hiding this comment.
"The [serialization] restriction applies to the encoding of the Sig_structure, the Enc_structure, and the MAC_structure."
There are no maps in these three COSE structures. Each one is an array. The definition referencesempty_or_serialized_map, which is poorly named, as it is a bstr encoded map, not a map. By my read, COSE allows the payload map inside the payload bstr, the protected header parameters (including crit) inside the protected bstr, and the unprotected map (which is not included in any COSE signature, encryption, or MAC operation) to use general encoding.
The three COSE Internal types however are ordinary serialized (since they contain no maps). The array and bstrs use the shortest length representation and only definite lengths.
|
|
||
| This document defines two CBOR serializations: "ordinary serialization" and "deterministic serialization." | ||
| It also introduces the term "general serialization" to name the full, variable set of serialization options defined in {{-cbor}}. | ||
| It also introduces the term "general serialization" to name the full, variable set of serialization options defined in RFC 8949. |
There was a problem hiding this comment.
Accidental change?
There was a problem hiding this comment.
No, as mentioned, neither the Abstract nor Section headings are allowed to contain references
| * Big numbers described in {{Section 3.4.3 of -cbor}} MUST be accepted. | ||
| * Leading zeros SHOULD be ignored. | ||
| * An empty string SHOULD be accepted and treated as the value zero. | ||
| * An empty bstr SHOULD be accepted and treated as the value zero. |
There was a problem hiding this comment.
Should be "byte string". RFC 8949 doesn't mention "bstr"
There was a problem hiding this comment.
it mentions bstr in the CDDL, but you're right that "byte string" would be clearer.
| * An empty bstr SHOULD be accepted and treated as the value zero. | |
| * An empty byte string SHOULD be accepted and treated as the value zero. |
| To some degree it is a de facto standard for common CBOR protocols. | ||
|
|
||
| However, it is not suitable if determinism is needed because the order of items in a map is allowed to vary. | ||
| However, it is not suitable if determinism is needed and the CBOR contains maps, because the order of items in a map is allowed to vary. |
There was a problem hiding this comment.
I prefer not to go down the path of qualifying use of determinism with "if it contains maps".
Note that this text is moved in PR #27
There was a problem hiding this comment.
I think that is just ignoring reality. The example with COSE clearly shows this. It's also the case for most of MIMI Content. Anything else designed with structs (arrays) instead of maps will also have this consideration. I don't think you can say "because the order of items in a map is allowed to vary" and not acknowledge that if there are no maps there is no such variation.
| That is, deterministic encoding imposes no requirements over and above the requirements for decoding ordinary serialization. | ||
|
|
||
| > Note that a decoder MAY reject out-of-order maps as a consequence of {{SerializationChecking}}. | ||
|
|
There was a problem hiding this comment.
I don't want to add this. I think checking decoders are not that necessary and I want the normative sections to be as simple as possible.
There was a problem hiding this comment.
It is a Note, and it only contains a MAY. I think it is harmless and it will prevent people from saying "but the spec didn't say I was allowed to do this", which happens a LOT.
|
|
||
|
|
||
| ## Divergence from {{-cbor}} {#NaNCompatibility} | ||
| ## Divergence from RFC 8949 {#NaNCompatibility} |
There was a problem hiding this comment.
Spurious change?
There was a problem hiding this comment.
No, as mentioned above, references are not allowed in Section headings (nor in the Abstract).
| General serialization does support them. | ||
|
|
||
| New protocol designs can — and generally should—avoid non — non-trivial NaNs. | ||
| New protocol designs SHOULD avoid non-trivial NaNs. |
There was a problem hiding this comment.
I would like to be "can be avoided" in the text somehow.
There was a problem hiding this comment.
why shouldn't new protocol designs avoid non-trivial NaNs?
| - IEEE 754 doesn't set requirements for their handling. | ||
| - Non-trivial NaNs are not well-supported across CPUs and programming environments. | ||
| - Implementing preferred serialization for non-trivial NaNs is complex and error-prone; many CBOR implementations don't support it or don't support it correctly. | ||
| - Implementing preferred serialization for non-trivial NaNs is complex and error-prone; many CBOR implementations don't encode and/or decode non-trivial NaN, or don't encode or decode them correctly. |
There was a problem hiding this comment.
How about this:
Because preferred serialization of non-trivial NaNs is difficult and error-prone to implement, many CBOR libraries either do not support it or implement it incorrectly.
There was a problem hiding this comment.
I wanted to keep the part about encoding and decoding. Is this better?
| - Implementing preferred serialization for non-trivial NaNs is complex and error-prone; many CBOR implementations don't encode and/or decode non-trivial NaN, or don't encode or decode them correctly. | |
| - Because preferred serialization of non-trivial NaNs is difficult and error-prone to implement, many CBOR implementations don't encode and/or decode non-trivial NaN, or don't encode or decode them correctly. |
small rewordings, typos, and NITS
Note: references are not allowed in the Abstract or Section headings.