Last Call Review of draft-ietf-netconf-keystore-20
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.