Thanks for your review, Carsten. I apologize for not responding to it until now. It had gotten sorted into a folder and I wasn't aware of it until Kathleen brought it to my attention. Replies are inline below...
> -----Original Message----- > From: apps-discuss [mailto:[email protected]] On Behalf Of > Carsten Bormann > Sent: Thursday, October 02, 2014 12:22 AM > To: [email protected]; draft-ietf-jose-json-web- > [email protected] > Cc: [email protected]; [email protected] > Subject: [apps-discuss] Appsdir review for > draft-ietf-jose-json-web-algorithms- > 33 > > I have been selected as the Applications Area Directorate reviewer for this > draft > (for background on appsdir, please see > http://trac.tools.ietf.org/area/app/trac/wiki/ApplicationsAreaDirectorate > ). > > Please resolve these comments along with any other Last Call comments you > may receive. Please wait for direction from your document shepherd or AD > before posting a new version of the draft. > > Document: draft-ietf-jose-json-web-algorithms-33 > Title: JSON Web Algorithms (JWA) > Reviewer: Carsten Bormann > Review Date: 2014-10-02 > IESG Telechat date: 2014-10-02 > > Summary: This draft is ready for publication as a standards track RFC, with a > few > nits corrected. > > However, some additional editorial improvements might improve the security > outcome when it is referenced by application developers. > > Major issues: None. > > Minor issues: > > 5.2: > Add a reference that defines PKCS #7 padding. I'll plan on adding a reference to RFC 2315 for this. > 5.2.2.2 > Does "the PKCS #7 padding is removed" entail checking all of its bytes? Should this be changed to "the PKCS #7 padding bytes are checked and then removed"? > 6.2.1 > Is the intention that the sentence containing "point compression is not > supported" also applies to any future registered value of "crv"? > A similar comment applies to other specifications in 6.2.1.x, e.g., the > reference > to SEC1 representation to x and y. It would apply to any future curves registered that use the "x", "y" point representation. Others could define new key types that use a different representation or we could refine the definition of "kty":"EC" to make the representation specific to the "crv" value. A discussion happened on the JOSE thread "JWK Elliptic Curve key representations and new curves". The conclusion suggested by Richard Barnes and seconded by Stephen Farrell was to table defining any new key representations for new elliptic curves until the CRFG recommendations to TLS have been made. That ended the discussion for now. > 6.2.1.1 > »Additional "crv" values MAY be > used, provided they are understood by implementations using that Elliptic > Curve > key.« How are conflicts between such implementation defined values and future > registered values handled? Conflicts are avoided using the IANA JSON Web Key Elliptic Curve registry defined in Section 7.6. > 6.3.2: > The MAY accept partially overrides the MUST include? > Is the latter thus really a SHOULD? "d" is REQUIRED. The others SHOULD be included, and if any are included, the others MUST be included. Those are requirements on producers. Consumers may choose to accept keys that don't include all the CRT parameters. Given that they can be computed from "n", "e", and "d" and sometimes they're not available, a previous reviewer had asked this language about consumers to be included. It's a case of "Be conservative in what you send, be liberal in what you accept". It's not clear to me that there's any value in relaxing the requirements on producers. > 7.1: > It is interesting that a mere registration (vetted only by a DE) can change > the > IETF consensus base specifications by making an algorithm "Required". It is expected that the designated experts will be appointed, in part, for their cryptographic expertise. > 8. > I am unable to find a "security considerations" section in NIST SP 800-38A. > 800-38D at least has a "practical considerations" section, is that meant? > (Etc., I haven't checked all the references.) In general, I believe a security > considerations section is most useful where it provides more directed guidance > instead of saying the equivalent of "here is a textbook". There are 14 subsections with directed guidance, plus more in the JWS, JWE, and JWK specifications. If there is other directed guidance you'd like to see added, please supply proposed text. Having skimmed through it, I agree that 800-30A doesn't contain clear enough security guidance to merit inclusion in the list. I'll remove it. If there are other specifications that you'd like to see added or removed, please let me know. > 8.7 is not clear: is it NOT RECOMMENDED to reuse an entire set of key material > (including IV), or to reuse any part of it? I agree that this is ambiguous, as worded. Its advice seems to be so broad as to be impractical, as never reusing a key means that a new key would have to be redistributed for each encryption, which then requires a key to use for the distribution... This text came from http://tools.ietf.org/html/draft-miller-jose-jwe-protected-jwk-02#section-8.1, which was incorporated into the document after a working group decision to do so. Matt Miller, you were the author of this text. Could you clarify what the intent was? Thanks. > Nits/editorial comments: > > 6.3.2.x: > The constant repetition of »It is represented as the base64url encoding of the > value's unsigned big endian representation as an octet sequence. > The octet sequence MUST utilize the minimum number of octets to represent > the value.« almost ensures that an implementer will stop reading the details > (well, I did, and I did not write a program to verify the same phrase is used > everywhere; if any parameter were using a different encoding, that sure would > be missed). Why not define another abstraction like base64url and use this? I did just verify that they are all consistent. Yes, if you're reading the spec linearly I'm sure it feels repetitive, but if you're an implementer going straight to a definition to implement it, having a complete description all in one place is valuable. I understand your request but it's not clear to me that this rises to the level that a separate definition that the developer then has to go find is warranted. > 6.2.3.1: This is not a positive integer? 6.2.3.x mentions this otherwise. I may not understand this remark. I'm guessing you're referring to 6.3.2.1 and the fact that the description includes the word "unsigned". This is there to make it clear that no sign bit will be present in the representation - which is especially important since the high-order bit is always set for "n" and "d" for correctly generated keys. > 7.1.1 > »Example description« is not a useful example for an "Algorithm Description". > (Same comment for 7.x.1.) See the actual registrations in Section 7.1.2 for more useful examples. > 8.3: > s/because it/because it is/ Good catch > [sec1] > (Given the date, this is probably referencing V2.0 of this spec.) Hmmm - the version I have cached locally came from http://www.secg.org/collateral/sec1_final.pdf and is dated September 20, 2000. But the site appears to be down at present. The version referenced by Wikipedia at http://www.secg.org/download/aid-385/sec1_final.pdf is missing too. http://secg.org/download/aid-780/sec1-v2.pdf is missing too. Can you point me to a good link? RFC 5915 includes this reference: [SECG1] Standards for Efficient Cryptography Group (SECG), "SEC 1: Elliptic Curve Cryptography", Version 2.0, May 2009. Worst comes to worst, I'll either reference that or explicitly reference the 1.0 version. But if I'm going to reference the 2.0 version, I'd like to be able to read a copy so that I can verify that the way we're using it is consistent with the actual spec, including the section numbers. > [usascii] > The reference to ANSI X3.4:1986 should probably be replaced by a reference to > RFC 20. There is little reason to reference a somewhat hard to obtain > external > document ($60!) when we have an RFC about the same subject. OK > (Tables in Appendix A need some formatting.) Agreed. It's on my to-do list. Jim Schaad recently told me to "Add a width attribute to the ttcol with either "digits" (for fixed width) or "digits%" (for percent width) to the column in question". I plan to give it a try. > _______________________________________________ > apps-discuss mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/apps-discuss Thanks again, -- Mike _______________________________________________ jose mailing list [email protected] https://www.ietf.org/mailman/listinfo/jose
