[netconf] AD review of draft-ietf-netconf-keystore-28

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Kent,

Another really well written document that is very clear and easy to read, hence all my comments are only minor or nits.  The only notable one is about copying the parts of the key into running, i.e., whether those parts of the key definition should be mandatory.  There are benefits both ways and it isn't really clear which way this is going to go with the Netmod system datastore discussion.

I didn't flag it, but I did question whether the SEC ADs may want some of the SHOULDs in the security considerations to be MUSTs, but I will defer to your judgement on this for now, and obviously their judgement when it comes to IESG review.


Moderate level comments:  None.

Minor level comments:

(1) p 8, sec 2.1.3.2.  The "asymmetric-key-certificate-ref-grouping" Grouping

   *  This grouping is usable only when the keystore module is
      implemented.  Servers defining custom keystore locations MAY
      define an alternate grouping for references to the alternate
      locations.

I would suggest "can" rather than "MAY" here.


(2) p 8, sec 2.1.3.3.  The "inline-or-keystore-symmetric-key-grouping" Grouping

     grouping inline-or-keystore-symmetric-key-grouping:
       +-- (inline-or-keystore)
          +--:(inline) {inline-definitions-supported}?
          |  +-- inline-definition
          |     +---u ct:symmetric-key-grouping
          +--:(keystore) {central-keystore-supported,symmetric-keys}?
             +-- keystore-reference?   ks:symmetric-key-ref

How likely is it that there will be extra keystores augmented in?  Specifically, I wonder whether "central-keystore-reference" would be a better leaf name here?  If you decide to change here, I presume that you will check/change other similar occurences.


(3) p 29, sec 2.3.  YANG Module

          Servers that do not 'implement' this module, and hence
          'central-keystore-supported' is not defined, SHOULD
          augment in custom 'case' statements enabling references
          to the alternate keystore locations.";

I'm not sure about the "do not 'implement' this module" text.  Could this comment just be predicated on whether the server supports the feature?  This same comment/resolution probably applies to similar descriptions below.


(4) p 30, sec 2.3.  YANG Module

             description
               "A reference to an symmetric key that exists in
                the keystore, when this module is implemented.";

I would suggest removing the "when this module is implemented" part of the description, since that is implicit on any data node predicated on a feature.


(5) p 36, sec 3.  Support for Built-in Keys

   <keystore xmlns="urn:ietf:params:xml:ns:yang:ietf-keystore"
     xmlns:ct="urn:ietf:params:xml:ns:yang:ietf-crypto-types">
     <asymmetric-keys>
       <asymmetric-key>
         <name>Manufacturer-Generated Hidden Key</name>
         <public-key-format>ct:subject-public-key-info-format</public-k\
   ey-format>
         <public-key>BASE64VALUE=</public-key>
         <hidden-private-key/>
         <certificates>
           <certificate>
             <name>Manufacturer-Generated IDevID Cert</name>
             <cert-data>BASE64VALUE=</cert-data>
           </certificate>
           <certificate>
             <name>Deployment-Specific LDevID Cert</name>
             <cert-data>BASE64VALUE=</cert-data>
           </certificate>
         </certificates>
       </asymmetric-key>
     </asymmetric-keys>
   </keystore>

Related to my comment on the other draft, it would have been nice if the public-key-format, public-key, hidden-private-key and cert-data fields could be kept out of running, but presumably it is too undesirable to make all of these fields mandatory false (e.g., by the a 'use' refinement statement)?

I think that if, after copying, the public-key is changed, then this will end up causing a configuration error if the key is ever updated out of band, since the updated publci-key field would overrite the value in system (assuming tha names match), which could the public and private keys to be out of step.  I.e., there is a benefit of keeping them out of running if possible.


(6) p 37, sec 3.  Support for Built-in Keys

   <keystore xmlns="urn:ietf:params:xml:ns:yang:ietf-keystore"
     xmlns:ct="urn:ietf:params:xml:ns:yang:ietf-crypto-types"
     xmlns:or="urn:ietf:params:xml:ns:yang:ietf-origin"
     or:origin="or:intended">
     <asymmetric-keys>
       <asymmetric-key or:origin="or:system">
         <name>Manufacturer-Generated Hidden Key</name>
         <public-key-format>ct:subject-public-key-info-format</public-k\
   ey-format>
         <public-key>BASE64VALUE=</public-key>
         <hidden-private-key/>
         <certificates>
           <certificate>
             <name>Manufacturer-Generated IDevID Cert</name>
             <cert-data>BASE64VALUE=</cert-data>
           </certificate>
           <certificate or:origin="or:intended">
             <name>Deployment-Specific LDevID Cert</name>
             <cert-data>BASE64VALUE=</cert-data>
           </certificate>
         </certificates>
       </asymmetric-key>
     </asymmetric-keys>
   </keystore>

I would have thought that or:intended would also be applied to the asymmetric-keys container.  Then the existing or:origin="or-intended" on'Deployment-Specific LDevID Cert' can be removed, and "or:origin="or-system" could be added to the other certificate that is not in running.


(7) p 39, sec 4.3.  Migrating Configuration to Another Server

   The following diagram illustrates this idea:

In the diagram, the direction of the arrows and 'encrypted by' can perhaps be read either way.  E.g., I can read that diagram as saying that the MK-1 key is encrypted-by "shared KEK".  Would it make sense to use "encrypts" rather than "encrypted by" and reverse the direction of the arrows?



Nit level comments:

(8) p 0, sec 

   The "Relation to other RFCs" section Section 1.1 contains a self-
   reference to this draft, along with a corresponding Informative
   Reference in the Appendix.

Please clarify this instruction, as per the other reviews.


(9) p 3, sec 1.  Introduction

   The "ietf-keystore" module defines many "grouping" statements
   intended for use by other modules that may import it.  For instance,
   there are groupings that define enabling a key to be either
   configured inline (within the defining data model) or as a reference
   to a key in the keystore.

Should this be "the keystore" or "a keystore"


(10) p 3, sec 1.  Introduction

   Implementations may utilize zero or more operating-system level
   keystore utilities (e.g., "Keychain Access" on MacOS) and/or
   cryptographic hardware (e.g., TPMs).

I suggest eliding "zero or more", which is implies by the may.


(11) p 5, sec 1.3.  Terminology

   The term "keystore" is defined in this document as a mechanism that
   intends safeguard secrets placed into it for protection.

I found this sentence to be somewhat clunky to read.  Perhaps just "that safeguards secrets ..."?


(12) p 7, sec 2.1.3.1.  The "encrypted-by-choice-grouping" Grouping

   The grouping's name is intended to be parsed "(encrypted-
   by)-(choice)-(grouping)", not as "(encrypted)-(by-
   choice)-(grouping)".

Possibly dropping the "-choice-" part of the name, i.e., encrypted-by-grouping, might help avoid potential confusion?


(13) p 11, sec 2.1.3.7.  The "keystore-grouping" Grouping

     grouping keystore-grouping:
       +-- asymmetric-keys {asymmetric-keys}?
       |  +-- asymmetric-key* [name]
       |     +-- name?                                         string
       |     +---u ct:asymmetric-key-pair-with-certs-grouping
       +-- symmetric-keys {symmetric-keys}?
          +-- symmetric-key* [name]
             +-- name?                        string
             +---u ct:symmetric-key-grouping

The first "string" looks too far right.


(14) p 12, sec 2.1.4.  Protocol-accessible Nodes

   =============== NOTE: '\' line wrapping per RFC 8792 ================

For readabilty of this diagram, it might be better to have an RFC editor note, asking them to fix this manually, e.g., to wrap the feature statements and add in the missing '|' characters.


(15) p 27, sec 2.3.  YANG Module

     feature central-keystore-supported {
       description
         "The 'central-keystore-supported' feature indicates that
          the server supports the keystore (i.e., implements the
          'ietf-keystore' module).";
     }

Perhaps "fully implements the ...", since technically a server could 'implement' this module but not support the central-keystore-supported feature.


(16) p 29, sec 2.3.  YANG Module

     grouping inline-or-keystore-symmetric-key-grouping {
       description
         "A grouping that expands to allow the symmetric key to be
          either stored locally, i.e., within the using data model,
          or a reference to a symmetric key stored in the keystore.

Will readers generically undersatand the "using data model"?  Perhaps "i.e., within the data model that uses this grouping, or a ..."


(17) p 38, sec 4.2.  Configuring Encrypted Keys

   Implementations SHOULD provide an API that simultaneously generates
   and encrypts a key (symmetric or asymmetric) using a KEK.  Thusly
   newly generated key cleartext values may never known to the
   administrators generating the keys.

'may never known' => 'are never known'?


(18) p 42, sec 5.3.  The "ietf-keystore" YANG Module

   All the writable data nodes defined by this module, both in the
   "grouping" statements as well as the protocol-accessible "keystore"
   instance, may be considered sensitive or vulnerable in some network
   environments..  For instance, any modification to a key or reference
   to a key may dramatically alter the implemented security policy.  For
   this reason, the NACM extension "default-deny-write" has been set for
   all data nodes defined in this module.

s/.././

Regards,
Rob