Last Call Review of draft-ietf-netconf-trust-anchors-13
review-ietf-netconf-trust-anchors-13-yangdoctors-lc-schoenwaelder-2021-01-12-00
Request | Review of | draft-ietf-netconf-trust-anchors |
---|---|---|
Requested revision | No specific revision (document currently at 28) | |
Type | Last Call Review | |
Team | YANG Doctors (yangdoctors) | |
Deadline | 2020-08-06 | |
Requested | 2020-07-09 | |
Requested by | Mahesh Jethanandani | |
Authors | Kent Watsen | |
I-D last updated | 2021-01-12 | |
Completed reviews |
Genart Last Call review of -22
by Paul Kyzivat
(diff)
Secdir Last Call review of -24 by Yoav Nir (diff) Secdir Last Call review of -13 by Yoav Nir (diff) Yangdoctors Last Call review of -13 by Jürgen Schönwälder (diff) |
|
Assignment | Reviewer | Jürgen Schönwälder |
State | Completed | |
Request | Last Call review on draft-ietf-netconf-trust-anchors by YANG Doctors Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/yang-doctors/Zy6bHhccIf_sVlLY4-y37LqEpio | |
Reviewed revision | 13 (document currently at 28) | |
Result | Ready w/issues | |
Completed | 2021-01-12 |
review-ietf-netconf-trust-anchors-13-yangdoctors-lc-schoenwaelder-2021-01-12-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-trust-anchors-13.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. - The prefix 'ts' is rather short, the set of two letter prefixes is rather small limited. This comment also applies to the other documents, crypto-types uses 'ct. Perhaps this is not a problem since collisions can be handled but if we go for rather short prefixes, we will have to exercise the collision resolution. (I see that 'ct' has already been used by ietf-complex-types, RFC 6095.) A possible alternative could be to use sec-ct, sec-ts, sec-ks, ...). RFC 8407 provides the following guidelines (Section 4.2): Prefix values SHOULD be short but are also likely to be unique. Prefix values SHOULD NOT conflict with known modules that have been previously published. Well, having short and terse prefixes is actually nice and enhancing programmer readability. - s/is defines a truststore/defines a truststore/ - s/to be grouped references/to be grouped and referenced/ - In 2.2.1, I was not sure what CA certificates are and what EE certificates are. I then tried to guess EE = end entity cert, but this does not explain CA since the term used in crypto types is trust anchor cert. The description in the XML clarified that my guess was kind of correct. Perhaps explain upfront what these acronyms mean? Or perhaps the acronyms can be avoided by simply spelling things out? They do not appear to be used frequently. - s/<!-- Entity Certs/<!-- End Entity Certs/ - s/(Section 2.1.3.2), groupings/(Section 2.1.3.2) groupings/ - Is the feature truststore-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? - In the YANG module, you seem to use Truststore to refer to /ts:truststore but in the surrounding text you also use just truststore. 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 /ts:truststore but that may be my subjective preference. Well, I started to use 'central truststore' below, not sure this is better as the term also would benefit from being defined.) - Does it make sense to be explicit about the fact that the cert/key references are all weak references in the sense that they can exist without the corresponding cert/key being present? In other words, in order to safely delete a cert/key, one would have to check that there are no dangling references left (which is difficult in case references are scattered over multiple (proprietary) data models). For YANG savvy people this may be obvious since there is no require-instance statement in the leafref type definition but not every implementer may recall this - and it would be good to document that using weak references was an explicit design decision and not an oversight. - Does it make sense to spell out that using the grouping in other YANG modules requires to define additional reference types since the ones provided by this modules only work for the central truststore store? And this as a consequence may require multiple leafs to refer to keys that exist in different truststores, i.e., having multiple truststores is possible but comes with a cost. - Would it make sense to add to all three documents an objectives section that summarizes the design objectives? For this module, a starting point might be this: 2.1. Objectives The design of the truststore data model was guided by the following design objectives: - provide a central truststore for storing keys or certificates - provide support for storing named bags of keys or certificates - provide types that can be used to reference keys or certificates stored in the central truststore - protect the truststore using suitable access control definitions - provide groupings that support locally configured keys or certificates or references to key or certificate bags in the central truststore - provide groupings that can be used to instantiate truststores in other data models (but this requires to introduce additional types to reference keys or certificates) I do not know whether this list is complete but I find it usually helpful to have the design objectives spelled out. - Section 3 talks about populating <running> with built-in trust anchors. In order for the built-in trust anchors to be referenced by configuration, the referenced certificates MUST first be copied into <running>. The certificates SHOULD be copied into <running> using the same "key" values, so that the server can bind them to the built- in entries. Is the idea that this copy operation is an explicit management operation or can implementations populate <running> with this data automatically? - Section 4.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.