Hi Matt, Thanks for the review. On 4/7/17, 1:58 PM, "Matthew Miller" <linuxwolf+ietf@xxxxxxxxxxxxxxxx> wrote: >Reviewer: Matthew Miller >Review result: Almost Ready > >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 treat these comments just >like any other last call comments. > >For more information, please see the FAQ at > ><https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > >Document: draft-ietf-rtgwg-yang-key-chain-17 >Reviewer: Matthew A. Miller >Review Date: 2017-04-07 >IETF LC End Date: 2017-04-07 >IESG Telechat date: 2017-04-13 > >Summary: > >This document is almost ready to be published as a Proposed Standard, >once the issues noted herein are resolved. > >Major issues: > >NONE > >Minor issues: > >* Forgive me for my limited knowledge of YANG, but is there a reason >key-strings are only representable as either a YANG string or >hex-string type, and not the YANG binary type? Let me ask why I¹d want to use this type? I can get all the entropy I want with a hex string and implementations are familiar with this representation. I¹m not really fond of the obscure base64 representation used by the YANG type and if one consults Benoit¹s search tool, the type is not widely used. http://www.yangcatalog.org/yang-search/yang-search.php > >* This document does not provide much guidance around AES key wrap >other than it can be used and the KEK is provided >out-of-band/-context. >For instance, AES key-wrapped key-strings probably require using >"hexidecimal-string". Yes - I¹ll add that. > Also, assuming I'm reading the model >correctly, >it appears this feature applies to the whole chain, which I think is >worth calling out. In YANG, if you support a feature, it is for the entire model. The per-chain boolean indicates whether or not it is applies to all the keys in that particular key-chain. I will clarify this. > >* This document warns against using the "clear-text" algorithm, which >the >reader is lead to understand is for legacy implementation reasons. >However, is there not a similar concern with cryptographically weak >algorithms, such as md5 and (arguably) sha1? I can add something for MD5. > >Nits/editorial comments: > >* In Section 3.2. "Key Chain Model Features", the word "of" is >missing >between "configuration" and "an" in the phrase "support configuration >an >acceptance tolerance". Good catch. I will fix. > >Non-nits: > >* I note that idnits is calling out some odd spacing issues, but I >think >they are safe to ignore. Though the line numbers don¹t match the draft, I was able to fix these. Thanks, Acee >