Hi Elwyn, Thank you very much for the in depth review! We have incorporated your changes in the newly submitted v-18 https://datatracker.ietf.org/doc/html/draft-ietf-ace-oscore-profile-18 , but you can also see the specific changes in the github: https://github.com/ace-wg/ace-oscore-profile/commit/c98ea9c53994aaa85add5c4a3436a14935fa0471 Answers inline. Thanks again, Francesca On 23/03/2021, 23:29, "Elwyn Davies via Datatracker" <noreply@xxxxxxxx> wrote: Reviewer: Elwyn Davies Review result: Ready with Nits I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please wait for direction from your document shepherd or AD before posting a new version of the draft. For more information, please see the FAQ at <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. Document: draft-ietf-ace-oscore-profile-17 Reviewer: Elwyn Davies Review Date: 2021-03-23 IETF LC End Date: 2020-07-20 IESG Telechat date: 2021-03-25 Summary: Ready with nits. A very great improvement on the previously reviewed version. Thanks. FP: Thank you! Major issues: None Minor issues: Would it be useful to provide some advice on the length of salts and IDs to go with the advice on length of nonces? There is some in s3.3 of RFC 8613 but some other reference might be helpful, maybe placed in s3.2.1. and/or s4. FP: This draft does not have any other requirements on salts and IDs than RFC 8613. The IDs need not be long, but unique. The salt need not even be unique. According to RFC 5869 Section 3.1 “HKDF is defined to operate with or without salt”. However, the use of salt adds to the strength of HKDF. “Ideally, the salt value is a random (or pseudorandom) string of the length HashLen.” But the salt going into HKDF is composed of the master salt and the nonces. It therefore suffices with nonces of length HashLen/2 generated by the peers as described in this draft, independent of salt. Nits/editorial comments: General: The RFC Editor conforms rigorously to American practice and allows only the use of double quote marks (") in the text when marking strings as quotations and such like. The document makes extensive but not totally consistent, use of single quotes to flag up field names and such like (e.g., 'nonce1'). In practice these are unnecessary, but may be replaced by the RFC Editor if left in place. Personally. I think most of them can be removed. NB this does not affect CBOR items such as h'1645. FP: We have now removed all single quotes "'" and replaced them with the XML2RFC v3 supported <tt></tt>. We will make sure to follow RFC Editor recommendations about this. General: There are lots of usages of 'CBOR diagnostic notation without the tag and value abbreviations'. An abbreviation would reduce the verbiage. FP: Fair point, we have added this to the terminology. General: It is slightly confusing to have Nonce 1/N1/nonce1 and Nonce 2/N2/nonce2 used in the document. Am I right in thinking Nonce 1 and N1 are the same with nonce1 being the name of the JSON/CBOR parameter used to carry the value? A few words of clarrification would help. FP: There is a difference between the three: Nonce 1 is the name of the field, N1 is the value, nonce1 is the parameter's label. If you have suggestion on where to add this, that would be helpful. Abstract/s1: It would be useful to introduce the name of the profile (coap_oscore) up front. It rather sneaks out in s3. FP: Good point, added. s1, para 2: Need to expand CBOR on first use. FP: Usually yes, but because here it is used as the expansion of COSE, we used the RFC Editor abbreviation expansion, which keeps CBOR non-expanded. s2, end of para 3: s/as well/instead/? or s/as well/alternatively/. FP: Ok, replaced with alternatively. s2, para 7 and s6, bullet 2: s/e.g. expiration./for example, expiration./ FP: Ok, fixed. s3.1, para 3 and last para: s/reported/shown/ FP: Ok, fixed. s3.1, Figure 2 and Figure 3: Appendix F.3 of draft-ietf-ace-oauth-authz reports that req_aud was replaced by audence at version 19 of the document. FP: Good catch! Fixed. s3.2, second set of bullets: Need to expand HMAC and HKDF on first use (not well-known in RFC Editor list). It would also be useful to put a pointer to section 11.1 of RFC 8152 here to indicate the allowed HKDF algorithms. FP: Agreed. We have added it to the terminology section (to expand it) and added a reference to it. Also added the pointer to COSE. s3.2, 2nd para after 2nd set of bullets: s/The applications needs/The application needs/ FP: Ok, fixed. s3.2, 3rd para after 2nd set of bullets: s/parameeter/parameter/ FP: Fixed. s3.2, 4th para after 2nd set of bullets: s/the use of CBOR web token/the use of a CBOR web token/ FP: Fixed s3.2.1: OLD: IANA "OSCORE Security Context Parameters" registry (Section 9.4), defined for extensibility, and is specified below. NEW: IANA "OSCORE Security Context Parameters" registry (Section 9.4), defined for extensibility, and the initial set of parameters defined in this document is specified below. END FP: Ok, fixed. s3.2.1, below Figure 9: Expand CDDL. FP: Why? This is not the first time CDDL appears. s4.1, para 1 and s4.2, para 2: s/RECOMMENDS to use/RECOMMENDS using/ FP: Ok, fixed. s4.1, para 1 and s4.2. para 2: s/as nonce's value/as the nonce's value/ FP: Fixed. s4.1, para 7: s/renew/update/ [renew implies the same identifiers are used - which is already specified!] FP: Right, fixed. s4.1, last para and s4.3, last para: Does /authz-info have some special meaning? FP: The same as in draft-ietf-ace-oauth-authz. I haven't changed anything here because we use the same phrasing as that document (which we also reference in the terminology). s4.3, para 1: s/ Once receiving the 2.01 (Created) response from the RS/ Once the 2.01 (Created) response is received from the RS/ FP: Fixed. s4.3, Figure 12: I assume the Master Salt is supposed to be a CBOR indefinite length string encoding (it doesn't say so) as it it consists of the concatenated CBOR strings of its component byte strings. It would be strictly correct to start it with 0x5f and end with (0x)ff I would have thought. FP: No, this is to prevent attacks based on confusing the delimitation between the two nonces. Be that as it may, I do not understand why the document is concerned with either CBOR or JSON/base64 encodings of the master salt. It may be that I am missing something, but I didn't think that the master salt was ever put in a protocol message as such (deliberately), but only as one or two of its components such that it could be privately constructed at both endpoints once the three components had been shared, and was just the concatenation of the data bytes of the 3 components rather than involving their lengths. FP: The OSCORE master salt is constructed from two parts: one part agreed beforehand (if present) and the other part from the nonces sent in the message. Since both parties need to agree on this parameter to derive the same keys, the method of encoding used in the key derivation needs to be defined. s6. last para: s/observation/observations/ FP: Fixed. s7, para 3: s/RS pass/RS passes/ FP: Fixed. s9: It is now usual to give the URLs for the various existing registries as normative references. FP: Added (to the existing one). s9.4: I am not aware that a single registry can have different review/specfication requirements for portions of its parameter space. Is it seriously expected that there will be significant numbers of requests for values in this registry? My instinct would be to go for specification required and advise allocation according to the orign and type of the specification. FP: Yes, that is done, for example in the COSE registries. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
