[yang-doctors] Yangdoctors last call review of draft-ietf-netconf-trust-anchors-13

[Jürgen Schönwälder via Datatracker <[e-mail]>]

Reviewer: Jürgen Schönwälder
Review result: Ready with Issues

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.