Last Call Review of draft-ietf-netconf-keystore-20
review-ietf-netconf-keystore-20-yangdoctors-lc-schoenwaelder-2021-01-14-00
Request | Review of | draft-ietf-netconf-keystore |
---|---|---|
Requested revision | No specific revision (document currently at 35) | |
Type | Last Call Review | |
Team | YANG Doctors (yangdoctors) | |
Deadline | 2020-08-07 | |
Requested | 2020-07-14 | |
Requested by | Mahesh Jethanandani | |
Authors | Kent Watsen | |
I-D last updated | 2021-01-14 | |
Completed reviews |
Genart Last Call review of -29
by Reese Enghardt
(diff)
Secdir Last Call review of -30 by Magnus Nyström (diff) Yangdoctors Last Call review of -02 by Jürgen Schönwälder (diff) Secdir Early review of -18 by Magnus Nyström (diff) Secdir Last Call review of -19 by Sandra L. Murphy (diff) Yangdoctors Last Call review of -20 by Jürgen Schönwälder (diff) |
|
Assignment | Reviewer | Jürgen Schönwälder |
State | Completed | |
Request | Last Call review on draft-ietf-netconf-keystore by YANG Doctors Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/yang-doctors/TXSozJxbSoMtvJOjaN1jj1jo7IQ | |
Reviewed revision | 20 (document currently at 35) | |
Result | Ready w/issues | |
Completed | 2021-01-14 |
review-ietf-netconf-keystore-20-yangdoctors-lc-schoenwaelder-2021-01-14-00
The crypto modules aim at providing a flexible reusable infrastructure of groupings for modeling cryptographic keys and related concepts. The flexibility of the definitions provided of course comes with a certain amount of complexity. From a YANG perspective, draft-ietf-netconf-keystore-20.txt is in a good and close to publish state (a couple of minor issues left). I also tried to understand what is being modeled here and hence I also have some questions concerning the concepts modeled and I hope these are easy to answer/resolve as well. - I have compiled the YANG modules using yanglint 0.16.105. - Compared to the other two I-Ds in the batch, this I-D has a more verbose introduction (appreciated) and it also has a terminology section (which never hurts). I do not know whether Kent has the energy to align the I-Ds in their intro style. - s/in Examples (Section 2.2)./in Section 2.2./ - Is the feature keystore-supported really needed? Does the YANG library not already provide the information whether a module has been implemented or just imported to access type and grouping definitions? OK, I know see that this is used to make definitions conditional, hence it makes sense. This means that my comment on truststore-supported in the other review can be ignored, I found the answer. - My question concerning two-letter prefixes applies to this I-D as well. - In the YANG module, you seem to use Keystore to refer to /ks:keystore but in the surrounding text you also use just keystore. I am not sure it is necessary to have the capitalized version but if people think its necessary, it makes sense to define the difference and to make sure the proper capitalization is used throughout the document. (If it is necessary somewhere to be explicit, I would rather use /ks:keystore but that may be my subjective preference. In the other I-D review, I used the term 'central truststore', which is not a good term either, the term 'well-known keystore' may be a better alternative.) - Many of the groupings either symmetric-key-ref or asymmetric-key-ref and while the groupings seem to offer flexibility to instantiate other keystores, I have some doubts that this actually works unless you augment in other reference types. Looking at the ex-keystore-usage module, I find in there the usage of ks:symmetric-key-ref and ks:asymmetric-key-ref and they refer to the well-known keystore, not the one defined in the example module. YANG's reference mechanism via leafrefs is not really supporting well what you try to do. I understand the flexibility you want to achieve but it seems YANG 1.1 does not really support this well enough. What you would need is a leafref type that can be "anchored" at different places but we don't really have this... You hint at this in the definition of the keystore-grouping: "Grouping definition enables use in other contexts. If ever done, implementations SHOULD augment new 'case' statements into local-or-keystore 'choice' statements to supply leafrefs to the new location."; It seems the SHOULD really is a 'must' (I do not care about capitalization); if you do not add your own leafrefs, things will not work or be majorly surprising. If I am correct, then there should be stronger warnings upfront that simply reusing the groupings is not enough and that the example module is actually an incomplete example... The same may apply to some of the groupings in the truststore drafts. - There is a live discussion concerning the built-in keys, which obviously applies here as well. Perhaps the conclusion is that what we have is the best solution. This is just here as a reminder in case there are changes. - Section 4 points to keys being compromised 'when in transit' but I think we also want to protect keys at rest, i.e., sitting in a backup. - I am wondering whether key encryption also applies to the related truststore document. - Expand RMA in "RMA scenarios" or simply avoid the acronym (its only used once). - s/"default-deny-all)/"default-deny-all")/ - Section 4.3 talks about _highly_ restricted access mechanisms and _highly_ authorized clients and I am usually a bit confused what _highly_ means. But I am YANG doctor, not a security reviewer. ;-) - Section 5.2 says: This module does not define any RPCs, actions, or notifications, and thus the security consideration for such is not provided here. Well, the module actually instantiates certificate-expiration notifications. - Registrant Contact: should be changed to the IESG.