Re: [Cose] Comments on draft-schaad-cose-latest (2nd June 2015)

["Jim Schaad" <[e-mail]>]

> -----Original Message-----
> From: Cose [mailto:[e-mail]] On Behalf Of Ilari Liusvaara
> Sent: Thursday, June 04, 2015 9:35 AM
> To: [e-mail]
> Subject: [Cose] Comments on draft-schaad-cose-latest (2nd June 2015)
> 
> I read the spec, and here are some comments (hope I am not too far in the
> weeds):
> 
> 
> Section 1.1:
> 
> "Switch from using a map to using an array at the message level.
> While this change means that it is no longer possible to add new named
> parameters to the top level message, it also means that there is not a need to
> define how older implementations are defined to behave when new fields are
> present. Most of the reasons that a new field would need to be defined are
> adequately addressed by defining a new parameter instead."
> 
> Wasn't top-level message converted back to a map?

Yes it was, I just missed this place.

> 
> 
> Section 1.2:
> 
> "The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL NOT”,
> “SHOULD”, “SHOULD NOT”, “RECOMMENDED”, “MAY”, and “OPTIONAL” in
> this document are to be interpreted as described in [RFC2119]."
> 
> There's also "NOT RECOMMENDED" (synonym of "SHOULD NOT")? :-)

I have never made sure the list was exhaustive.  I have just copied this text forward.  I can add it without problems.

> 
> Section 1.3:
> 
> "There currently is no standard CBOR grammar available for use by
> specifications. In this document, we use the grammar defined in the CBOR data
> definition language (CDDL) [I-D.greevenbosch-appsawg-cbor-cddl]."
> 
> If anything like milestones are to be met, relying on this for final document not
> likely to be a good idea. But it is fine for discussion.
> 
> Some options for later:
> - Defining what is needed.
> - Lots of english prose. :-)

Yes, this is likely to become informational at some point in the future.  

> 
> 
> Section 2:
> 
> "Implementations SHOULD be prepared to find an integer in the location which
> does not correspond to the values 0 to 2. If this is found then the client MUST
> stop attempting to parse the structure and fail.
> Clients need to recognize that the set of values could be extended at a later
> date, but should not provide a security service based on guesses of what is
> there."
> 
> Does this refer to values of key msg_type? If so, aren't the values 1-3, not 0-2?

Good catch - updated.

> 
> Section 3:
> 
> "Both buckets are implemented as CBOR maps. The maps can be keyed by
> signed integers, unsigned integers and strings. The signed and unsigned integers
> are used for compactness of encoding."
> 
> Nit: The term used by CBOR itself seems to "negative integer".

Make sense as +9 is a "signed" number.

> 
> Section 3.1:
> 
> "TODO: Do we need to repeat definitions for all or just for some and refer to the
> JOSE documents?"
> 
> At least noting the rules for determining type of element each thing should have,
> similarly as those are defined in section 7 for keys.

Yes - I could just reference the table in the appendix (or move that table forward) and that would take care of it.  However  there has been some push to not rely on the JOSE documents for this type of detail.

> 
> Section 4:
> 
> "At least one of protected and unprotected MUST be present."
> 
> Why is this requirement there (there may be good reasons, seems to come from
> JOSE)?

This goes back to the missing text/changed text in section 3.  The algorithm identification header field is going to be required.  It this needs to have someplace to be placed and that is one of these two fields.

> 
> "For body_protected and sign_protected, if the fields are not present in their
> corresponding maps, an bstr of length zero should be used."
> 
> Should? The encoding of this is interop-critical. Also, why not use NULL for
> absent field here?

This is a lower case should and does have the same 2119 language meaning (see the text in 1.1 about natural language).  However 'is' will work just as well.

I could just as easily use NULL, but there is no size or conception difference.  The change from NULL to an empty bstr came about from the idea at one point during using arrays that it might be easier to do the parsing if the empty binary string was used instead.  It might also make checking of types easier.  This change got copied down into the structure to be signed as it made the code easier (just copy the item).  As I said, I am not wedded to either way.

> 
> Section 5:
> 
> "One of the byproducts of using the same technique for encrypting and encoding
> both the content and the keys using the various key management techniques, is
> a requirement that all of the key management techniques use an Authenticated
> Encryption (AE) algorithm. (For the purpose of this document we use a slightly
> loose definition of AE
> algorithms.) When encrypting the plain text, it is normal to use an Authenticated
> Encryption with Additional Data (AEAD) algorithm. For key management, either
> AE or AEAD algorithms can be used. See Appendix A for more details about the
> different types of algorithms."
> 
> I can't parse this.
> 
> What this seems to be saying that if performing is:
> - If encrypting plaintext, encryption has to be AEAD
> - If encrypting keys, the encryption can either be AE or AEAD?



> 
> "protected contains the information about the plain text or encryption process
> that is to be integrity protected. The field is encoded in CBOR as a ‘bstr’ if
> present and the value ‘nil’ if there is no data."
> 
> Is that 'nil' leftover from using arrays? Should it be that the member is absent if
> empty?

Yes it is.  I did not make a hard pass over the document looking for all of the changes that needed to be made for killing arrays.

> 
> Also nit: The CBOR value seems to be named "null".
> 
> "iv contains the initialization vector (IV), or it’s equivalent, if one is needed by
> the encryption algorithm. If there is no IV, then the value ‘nil’ is used."
> 
> Should be absent if IV is not needed?

Again - missed this in the array kill pass.  However, I am thinking that IV may migrate to be a header parameter instead.

> 
> "aad contains additional authenticated data (aad) supplied by the application.
> This field contains information about the plain text data that is authenticated,
> but not encrypted. If the application does not provide this data, the value ‘nil’ is
> used."
> 
> Should be absent if no aad?

Again - missed in array kill pass

> 
> "cipherText contains the encrypted plain text. If the cipherText is to be
> transported independently of the control information about the encryption
> process (i.e. detached content) then the value ‘nil’
> is encoded here."
> 
> Should be absent if no ciphertext?
> 
> "A ‘nil’ value if there are no recipients."
> 
> Should be absent if no recipients?
> 
> Section 5.1.1:
> 
> "The ‘protected’, ‘iv’, ‘aad’, ‘ciphertext’ and ‘recipients’ fields MUST be nil."
> 
> Should be absent instead?
> 
> Section 5.1.2:
> 
> "The ‘protected’, ‘aad’, and ‘recipients’ fields MUST be nil."
> 
> Should be absent instead?
> 
> Section 5.1.3:
> 
> "The ‘protected’, ‘aad’, and ‘iv’ fields all use the ‘nil’ value."
> 
> Sould be absent instead?
> 
> Section 5.1.4:
> 
> "The ‘protected’, ‘aad’, and ‘iv’ fields all use the ‘nil’ value."
> 
> Should be absent instead?
> 
> Also, no note of ciphertext field. Should be absent? It seems like presence or
> absence of ciphertext tells apart DKA (5.1.4) and KAwKW (5.1.5).

Currently the difference is based on different algorithm identifiers.  ECDH-ES vs. ECDH-ES+A128KW

> 
> Section 5.1.5:
> 
> "The ‘protected’, ‘aad’, and ‘iv’ fields all use the ‘nil’ value."
> 
> Should be absent instead?
> 
> "At a minimum, the ‘unprotected’ field SHOULD contain the ‘alg’
> parameter, a parameter identifying the recipient asymmetric key, and a
> parameter with the sender’s asymmetric public key."
> 
> Parameter with sender’s asymmetric public key? Is that epk? Or something else?
> Or epk or something else depending on situation?

That is going to depend on if we only support ephemeral-static or also support static-static.  In the latter case it may be any method of identifying a key such as a 'kid'.

> 
> Section 5.2:
> 
> "1. If there is protected data, CBOR encode the map to a byte string and place in
> the protected field of the Enc_structure and the COSE_Encrypt structure."
> 
> Shouldn't this be copy from message, since the field is already byte string there?

It is there for received messages, but no necessary created yet for sending.  But yes that would work as text as well.  At some point in time I will need to do a pass for textual consistency of how I write this type of step.

> 
> Also, should this be null if no protected field?
> 
> "2. Copy the ‘aad’ field from the COSE_Encrypt structure to the Enc_Structure."
> 
> Should this be null if no aad field?
> 
> Section 5.3:
> 
> "1. Verify that the ‘protected’ field is empty.
> 2. Verify that the ‘aad’ field is empty."
> 
> Empty means absent?

Yes

> 
> Section 6:
> 
> "At least one of protected and unprotected MUST be present."
> 
> Why is this requirement (might be good reasons, seems to come from JOSE)?
> 
> "1. Create a MAC_structure and copy the protected and payload elements from
> the COSE_mac structure."
> 
> The protected is written as null if absent?


> 
> Section 7:
> 
> "The same fields defined in [RFC7517] are used here with the following changes
> in rules:"
> 
> The formatting of block below this looks screwed.

One day I will learn correct markdown for RFCs.  I am more used to doing xml and have not figured everything out yet.

> 
> Section 9.2:
> 
> This section looks empty (no text, no subsections)?

Yes - I have not filled this section in yet.

> 
> Section 9.3:
> 
> "NOTE: Need to review the range assignments. It does not necessarily make
> sense as specification required uses 1 byte positive integers and
> 2 byte strings. ## COSE Header Algorithm Key Table"
> 
> There should be section break here?

Yes -mark down error.

> 
> Section C:
> 
> The examples could use some indentation to make easier to read?
> 
> Section C.6:
> 
> There are some null values in the dump (key 6). Are those supposed to be
> signatures?

My examples generation program was broken.  I think this will be fixed in the next version

> 
> Section E:
> 
> "jwk" and "kid" might need integers?

'kid' now has an integer.  Do we believe that jwk is something that makes lots of sense to keep?  How used do we believe that it will be?  i.e. should it be a one byte or a two byte integer value.

> 
> Section G:
> 
> Some of these could use integers?

Need to get some ideas of what algorithms should have one byte integer encoding and which should be two byte integer encoding.  Which are important algorithms?

> 
> Section H:
> 
> - Since kty is mandatory, it should have integer?
> - use and key_ops could have integers as values?
> - important key parameters could have integer values?
> - what are 'r' and 't' for RSA? And is 'dp' missing for RSA?

I have just started a pass working on the key structure.  

There was a typo with an extra 1 being put into the table for dp, it is there but shifted over by one column.  This has been fixed.

Jim

> 
> 
> 
> -Ilari
> 
> _______________________________________________
> Cose mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/cose