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

[Kent Watsen <[e-mail]>]

Hi Rob,

Thanks again for the great review.  Comments below.

Kent


> On Jun 23, 2023, at 11:41 AM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:
> 
> 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.

Ack.  The issue itself is being discussed in the your AD-review of the trust-anchors draft (though it should be on the NETMOD list).  But the real question is if this comment impacts *this* draft at all?  This draft includes this text already:

          The primary characteristic of the built-in keys is that they are provided
          by the server, as opposed to configuration.  As such, they are present
          in <operational>, and <system> [I-D.ietf-netmod-system-config],
          if implemented.

Thus, this draft defers all discussion of, e.g., if just the "necessary nodes” MUST be copied or not, to the “system-config” draft.  Is there any change needed in this draft?


> 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.

I just reviewed the SHOULDs in the SC section and I’m okay with them as they are.  This item is considered resolved.



> 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.

Done.  This item is considered resolved.



> (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.

I wrote an application that augments in its own keystore-reference here.  So there’s that data-point.

Expanding the name, i.e., s/keystore-reference/central-keystore-reference/ is more descriptive or, at least, consistent with the use of word “central” in this (and the trust-anchor) draft.  I cannot deny this.  

Okay, all drafts switched to both "central-truststore-reference” and "central-keystore-reference”.  This affected YANG modules and examples in all but the “crypto-types” draft.

This item is considered resolved.


> (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.

Removed.  Also removed similar phrase instances both in ietf-keystore.yang and ietf-truststore.yang.

This item is considered resolved.



> (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.

Grrr.  In another of our AD-review threads, we’re discussing that just the "necessary fields” are copied and, presumably would not include data that might change via a software update.  I wrote before that the “necessary fields” would include the keys and any nodes referenced by “must” or “when” expressions.  I forgot to add that “necessary fields” also includes “mandatory true” nodes, in order to support offline-validation (again, the need to support offline-validation is kicking us!).

Sadly, for the suite of client-server drafts, so much is “mandatory true”, and rightfully so (IMO), when the trust-anchors and/or keys are NOT built-ins (i.e. not “hidden”).   What we want, programmatically, is a way for some fields to be "mandatory false” when or:origin=“system”.  This, assuming that the “or.origin” attribute is in <running> at all (so far the system-config draft doesn’t state that <running> needs to carry this attribute for system data).

Thoughts?





> (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.

or.origin=“or:intended” is set at the top-level “keystore” container, which is then inherited by the "asymmetric-keys” container, as you thought.  But the "Manufacturer-Generated Hidden Key” <asymmetric-key> container flips it to or:origin="or:system”.  And then a client configured a "Deployment-Specific LDevID Cert” cert underneath the asymmetric-key, so it explicitly flips it back to or:origin="or:intended”.   Makes sense?



> (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?

Point.  But the YANG uses “encrypted-by”, so I was trying to follow that convention.  Makes sense?


> 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.

Done.  This item is considered resolved.



> (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"

s/the keystore/the central keystore/

I think that this better describes the groupings that are defined in the draft.

This item is considered resolved.


> (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.

Removed.  This item is considered resolved.


> (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 ..."?

Done.  This item is considered resolved.


> (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?

Done.  This item is considered resolved.




> (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.

Indeed, but this pyang’s doing!?


> (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.

Added this to the Editor Note section:

       Section <xref target="proto-access-nodes"/> presents a tree-diagram that has been
       wrapped due to automation.  Please manually edit the tree-diagram so that it doesn't
       have to be wrapped.  Please work with the author to make this edit.</t>

This item is considered resolved.


> (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.

Done - This item is considered resolved.



> (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 ..."

Done - This item is considered resolved.



> (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'?

s/never known/never be known/  - This item is considered resolved.



> (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/.././

Fixed - This item is considered resolved.


> Regards,
> Rob

Kent