Re: [netconf] early secdir review of draft-ietf-netconf-keystore-19

[Kent Watsen <[e-mail]>]

[+netconf (for visibility) and -keystore.authors (since its just me)]


Hi Sandy,

This is a massive review.  Thank you for that, but it demands a massive response…I hope you’re ready for it  ;)

Before jumping in, I want to clarify something that I’ll also make a point to add to the Introduction section, which is that the ietf-keystore YANG module defines a configuration model supporting existing practices; it does not intend to define new ways for doing things.  For instance, the sections regarding built-in keys, hidden keys, key-encryption keys, and master keys all attempt to support existing practices.  The point being that, in thinking about what these sections are saying, trust that this draft isn’t defining new behavior, and folk's experience with systems is the best guide.

Also, note that I did update Section 4 (Encrypting Keys in Configuration) per Magnus’s review.  That update was sent to the SecDir list on the 10th (with GitHub commit here [1]).  While the section was effectively entirely rewritten, the update essentially accomplishes just one thing, which is the swap a couple terms as follows: r/root key/master key/ and r/shared root key/key encryption key/.  Your review precedes this update, but my responses below will, when appropriate, reflect the current terms.

[1] https://github.com/netconf-wg/keystore/commit/5b3b9f3db0d6b7027ddf5f0e983ab958e8802251 <https://github.com/netconf-wg/keystore/commit/5b3b9f3db0d6b7027ddf5f0e983ab958e8802251>


PS: I just updated all the drafts with the updates from this email.  The bad news is that the Datatracker is still using the *previous* draft version for hyperlinks, so be sure to manually increment whenever opening a referenced document.

Thanks,
Kent



> On Aug 12, 2020, at 9:10 AM, Sandra Murphy <[e-mail]> wrote:
> 
> This is a requested SECDIR early review of draft-ietf-netconf-keystore-19, not an IETF Last Call review.
> 
> I have reviewed this document as an early review assigned by the security directorate, as part of the directorate's ongoing effort to review documents headed eventually to the IESG.  Document editors and WG chairs should treat these comments just like any other comments.
> 
> I am a neophyte at Yang so any comments I make here should be understood with that in mind.  (The recent mpls and netconf discussion of the MPLS module reuse of an RPC and the difference between "destination address" and "local label" have further impressed upon me just how much I do not know about YANG.)
> 
> Outline of this review is:
> 
> News of draft status
> Summary of the draft
> General and issue comments
> Page-by-page comments and nits
> 
> 
> News of draft status:
> ---------------------
> 
> This document was in WGLC when assigned and it appears that the WGLC has not yet been decided.
> 
> In the netconf session at IETF108, the wg discussed the need for a raw password feature, which might end up as a new grouping being added to the draft-ietf-netconf-crypto-types and therefore might induce changes to this draft as well.

True, it was discussed @108, but that change in crypto-types will not affect this draft (however, it will affect the tcp-client-server, ssh-client-server, and http-client-server drafts)


> Summary of the draft:
> ---------------------
> 
> The draft abstract says:
>  This document defines a YANG 1.1 module called "ietf-keystore" that
>  enables centralized configuration of both symmetric and asymmetric
>  keys.
> 
> Keys may be symmetric or asymmetric, and maybe be of types "cleartext", "hidden" (as in a TPM), or "encrypted". Keys can be configured locally (which means within the data model) or as a reference to a keystore item.  One server might have multiple keystores in use at any one time.
> 
> The document defines groupings that might prove useful for other YANG modules that import this one.
> 
> The document describes the data model for the keystore and provides many examples.  The descriptions of the data model and examples are presented in different modes as you read through the document, in tree diagrams, XML, and text that (surely?) is YANG (I think).  I believe the YANG text is authoritative.

True, the YANG module in Section 2.3 is the raison d’être for the draft.



> For other readers, here's the sequence of description modes.
> 
> Section 2.1 Mostly tree diagram descriptions of the groupings defined in this document.
> 
> Section 2.2.1-2.2.2 Examples (Keystore Instance and Cert Expiration) given in XML.
> 
> Section 2.2.3 A module, containing those groupings of Section 2.1 whose titles' prefixes are "local-or-keystore-", is described in what I believe would be called YANG (p16-18) and then in a tree diagram (p19-23).
> 
> Section 2.3 This section describes what I believe to be the normative Yang module text.

Yes, the YANG module in 2.3 is the normative module text for the draft.


> 
> General and issue comments:
> ---------------------------
> 
> This document is very well written.  The examples were very helpful, particularly to this reader who was unfamiliar with Yang.  I can't imagine the patience and care it took to keep so many descriptions in close alignment.

My secret is:

1) maintain all YANG modules and examples a stand-alone files.
2) each time building the draft (`make clean; make`), dynamically:
      a) validate all YANG modules
      b) validate all examples against the YANG modules
      c) generate all tree diagrams
      d) stitch tree-diagrams, examples, and YANG modules into an uber xml2rfc file

This way, everything stays connected and up-to-date.  I takes effort to set up, but it pays for itself over time (not only for the author, but also for WG participants who might otherwise discover anomalies that could’ve been caught).


> The document describes two new typedefs: asymmetric-key-ref and symmetric-key-ref.  Those typedefs are never further explained.  I believe (based only on the string "ref" in the name and usage as "keystore-reference") that these are intended to be references to named items in other parts of the keystore which themselves contain keys.  (Sheesh it takes a lot of words!) And I hope/believe that the asymmetric-key-ref is to a structure that contains a key-pair.

Yes, you got it. 

FWIW, Section 2.1.2 (Typedefs) says: "The leafrefs refer to symmetric and asymmetric keys in the keystore.  These typedefs are provided primarily as an aid to downstream modules that import the "ietf-keystore" module.”   Not good enough?


> The text typically says "asymmetric key" without distinguishing whether it is talking about the public key, the private key, or a key-pair.  Presumably "asymmetric key" means a key-pair and the implementations of operations know from context which key in the key-pair is needed.

Yes, that is what is intended.  Is that nomenclature okay or should “key-pair” be postpended throughout?

In contrast, note that when referring to asymmetric key halfs, the text attempts to always call that it out, e.g., “the public half of the asymmetric key”.


> I am not familiar with the Yang system, so I wonder if there is any mechanism in Yang's use to check if the assumptions of the data model are followed.  (1) If an asymmetric key is configured with a public key and a private key that are not a key-pair, where is that caught?  (2) If an asymmetric key and an associated cert are configured, but the public key part of the structure is not the public key mentioned in the cert, where is that caught?  (3) If the data model says that a component is an asymmetric-key-ref, but the de-ref'd value is not an asymmetric key (key type?), where is that caught, like maybe when the value is stored in the keystore?  (Do the questions even make sense?)

Yes, the questions (now numbered 1-3) make sense.

For #1) I just updated the “description” for the " asymmetric-key-pair-grouping” in the "ietf-crypto-types” module (in draft-ietf-netconf-crypto-types):

OLD:
      "A private key and its associated public key.";

NEW:
      "A private key and its associated public key.  Implementations
       SHOULD ensure that the two keys are a matching pair.”

[Note: “SHOULD” is used instead of “MUST” because the fallback is to simply *trust* that the clients never configure mismatched values.]

For #2) Already the description statements for various groupings in the "ietf-crypto-types” module have suitable statements:

  grouping end-entity-cert-grouping {
    description
      "An end entity certificate, and a notification for when
       it is about to (or already has) expire.  Implementations
       SHOULD assert that, where used, the end entity certificate
       contains the expected public key.”;
    <snip/>
  }

  grouping asymmetric-key-pair-with-cert-grouping {
    description
      "A private/public key pair and an associated certificate.
       Implementations SHOULD assert that certificates contain
       the matching public key.”;
    <snip/>
  }

  grouping asymmetric-key-pair-with-certs-grouping {
    description
      "A private/public key pair and associated certificates.
       Implementations SHOULD assert that certificates contain
       the matching public key.”;
    <snip/>
  }


For #3) There are three parts to this one.  

3a) First, when configuring a key into the Keystore, I made the following adjustments:

For "symmetric-key-grouping”:
  OLD:
      description
        "Identifies the symmetric key's format.”;
  NEW:
      description
        "Identifies the symmetric key's format.  Implementations
         SHOULD ensure that incoming symmetric key value is encoded
         in the specified format.”;

For "public-key-grouping”:
  OLD:
      description
        "Identifies the public key's format.";
    }
  NEW:
      description
        "Identifies the public key's format. Implementations SHOULD
         ensure that incoming public key value is encoded in the
         specified format.";
    }

For asymmetric-key-pair-grouping:
  OLD:
      description
        "Identifies the private key's format.";
    }
  NEW:
      description
        "Identifies the private key's format.  Implementations SHOULD
         ensure that incoming private key value is encoded in the
         specified format.";
    }



3b) Second, regarding if a format mismatch can occur, this is not possible as, e.g., the “key-format” for the symmetric-key (see below) uses an “identityref" that indicates that set values must derive from the "symmetric-key-format” (i.e., NOT either the “public-key-format” or “private-key-format” identities):

    leaf key-format {
      nacm:default-deny-write;
      type identityref {
        base symmetric-key-format;
      }



3c) Third, regarding if the thing referenced is what its supposed to be, note in the YANG module has this definition:

  typedef asymmetric-key-ref {
    type leafref {
      path "/ks:keystore/ks:asymmetric-keys/ks:asymmetric-key"
         + "/ks:name";
    }

Thus, if a configuration points to “foo”, but no asymmetric key called “foo” exists, then a YANG validation would fail when the configuration is attempted to be set.


QUESTION: should these implementation-notes be *moved* into Security Considerations?    One one hand, there’s is best IETF-form but, on the other hand, there is best YANG-form.   From both PoVs, I think a case can be made but, if no one cares, I’ll leave it be.


> Section 3 says
>                            Built-in "encrypted" keys MAY be copied
>  into other parts of the configuration so long as they maintain their
>  reference to the other built-in key that encrypted them.
> 
> I could see that built-in keys should be encrypted by other built-in keys (I don't see what other keys are available at the building-in time to encrypt them, but proof by "I don't see" is unconvincing).  Is that required?  I don't think the data model represents built-in keys as a distinct type, so I'm not sure the data model could represent the built-in-ness requirement.  Could it ever be that a built-in key could be encrypted by a non-built-in key?  I.e., does something prevent it.

From a terminology perspective, equate “built-in” with “generated by the manufacturer and/or in the device’s factory default configuration".

To the last question, no, a built-in key cannot be encrypted by a non-builtin key.  That would be putting the cart in front of the horse, so to speak.

But, to the crux of your question, that sentence simply acknowledging what’s possible.  For instance, imagine a device ships with a *hidden* built-in key called “foo” and an encrypted built-in key called “bar”, where bar is encrypted by foo.   Presumably both foo and bar would appear in the Keystore in <operational> and, if a user wanted to use bar as a KEK, they would copy it into the exact same spot in <running> (i.e., in Keystore, using the same name, e.g., “bar").  This sentence acknowledges that the key might possibly be renamed “baz” or possibly be copied into a “local definition” somewhere (i.e., not in Keystore at all).  In such cases, the configured-key is still valid so long as the base64 and the leafref pointing to “foo” remain.  It’s a corner case that’s unlikely to trigger in practice, because why would you, though theoretically possible without a technical reason for a constraint.  Makes sense?  



> Section 4 p34 says "built-in keys MUST be hidden".  That really should be mentioned in Section 3.

As mentioned in an email I wrote on Thu (Aug 13th), I don’t think this MUST is enforceable and so am dialing it back something akin to a SHOULD or RECOMMENDED via this text:

            A MEK is commonly a globally-unique built-in (see <xref target="built-ins"/>)
            asymmetric key for which the private key, due to its long lifetime, is hidden 
            (<relref section="2.1.4.5." target="I-D.ietf-netconf-crypto-types"/>) and the 
            public key is contained in an identity certificate (e.g., IDevID).



> Section 3 and 4 about built-in keys and root keys and hidden keys:
> 
> Section 4 p34 says
> 
>  The root key SHOULD be a hidden key, i.e., one whose private data has
>  no presence in <running> or <operational>
> 
> and
>                                   Given the long lifetime of built-in
>  keys (see Section 3), built-in keys MUST be hidden.

But this last part is no longer a “MUST” (see above)

> and Section 3 p32 says
> 
>  The key characteristic of the built-in keys is that they are provided
>  by the system, as opposed to configuration.  As such, they are
>  present in <operational>.
> 
> So built-in keys must be hidden, which means they are candidates to be root keys, but they are "present in operational".  By the first sentence, their private data is not in <operational>, so the configuration must use the hidden-private-key or hidden-key key-type for the secret parts.

Yes

> By the requirement that built-in keys must be hidden, servers that do not support hidden keys can not use built-in keys.  Correct?

Not anymore, since it can’t be enforced.  The better statement might be: "it is highly RECOMMENDED that builtin keys are [permanently] hidden and, if this is not possible, highly restricted access mechanisms are used to limit the built-in key's secret data to only highly authorized clients (e.g., an organization’s crypto officer)."



>  A hidden root key MAY be either a symmetric key or an asymmetric key.

True.   Asymmetric is the common case but its possible that a MEK could be a symmetric key, such that *only* a, e.g., TPM, is able to encrypt/decrypt certain keys (e.g., KEKs)


> Does the same apply to built-in keys?  The examples in Section 3 are all asymmetric keys.

Yes (see example in previous response)


> Is using a built-in key as a root key advisable in operational situations?

Absolutely.  This is commonly done and (I think) recommended by TCG.


> Presumably if a root key is not a built-in key and not a hidden key, then its secret data must be of cleartext-key type.  Encrypting the root key is not possible, there is no other single key available (or it would be the root key).  Has any consideration been given to dual root keys?  I could see that that would be too complex to manage.

Your presumptions are correct.   There certainly could be more than one MEK (root key).  This model doesn’t constrain that possibility from being exposed to users.


> Section 4.1 p35
> 
> The use of the root key mentions encrypting with an asymmetric key, but not signing with an asymmetric key.  Are signing and other crypto services not a part of the netconf view?

Signing is using something a device might do to prove to a remote peer that it originated some data.   In the context of the Keystore, it’s clearly desirable to configure encrypted keys but not clear there is a need to configure signed keys.  At least, I’m unaware of the use case.   Certainly any asymmetric key could be used to sign data, but the API (i.e., RPCs) enabling that are currently outside the scope of this document.



> Section 4.2 p35
> 
>  Each time a new key is to be configured, it SHOULD be encrypted by
>  the root key.
> 
> Would it be good to discuss how to configure a root key?  If the root key is not a built-in key and there is no support for a hidden key, then a cleartext key would be required, correct?  An encrypted key would not be possible because it would require a second key that would have to be available in the keystore - which means that would be the root key.

I added:

   "How to configure a MEK during the manufacturing process is outside
     the scope of this document.”

And:

     "It is highly RECOMMENDED that MEKs are built-in and hidden but, if
      this is not possible, MEKs highly restricted access mechanisms SHOULD 
      be used to limit access to the MEK's secret data to only highly 
      authorized clients (e.g., an organization’s crypto officer).  In this
      case, it is RECOMMENDED that the MEK is not built-in and hence is,
      effectively, just like a KEK.”


> Section 4.3 p35

Heads up: this is the section that was rewritten based on Magnus’s review.  The “shared root key” is now called “key encryption key” (KEK) and the “root key” is now “master encryption key” (MEK) in the -20 version I shared as an attachment last week.  

PS: Magnus suggests renaming “Master Encryption Key” to “Master Key”; I haven’t done this yet hence why I’m still using the MEK term…


> This process for migrating a configuration to a new server would also work for replacing the root key on a server without requiring re-encrypting all the keystore's encrypted keys.  (Mind you, I don't know how a non-builtin, non-hidden root key is configured from a cold start.)

Yes.


> The text calls the key a "shared root key" - but the use in this process of migrating a configuration does not require that the key be shared other than between the "first" and "second" servers.  I don't see that there's any reason to share the "shared root key" more widely; each migration between two servers could use a different "shared root key".

Agreed.


> "This shared root key would only need to be known to an organization's crypto officer." -- until it is decrypted in a server for use in encrypting and decrypting local keys, right?  I don't know how YANG implementations hold the root keys that are constantly in use but hopefully access is carefully controlled.

To your question, right, only the crypto officer and then later the device itself should ever know the key.

Regarding the second part of your comment, please note that the YANG model isn’t forcing any behavior.  From my previous comments above, I think you see that the draft “recommends” certain behaviors but, otherwise, enables implementations do present an interface reflecting a device’s internal behavior.


> A key common to all servers in an operational environment has been found to be unwise because an exposure at one server has a global impact.  The text on p 36 says "The crypto officer can then safely handoff the encrypted shared key to other administrators responsible for server installations, including migrations." but that should be taken as a handoff as necessary at each occasion, not taken as a handoff to all other servers jointly, at the same time.

Agreed.  I think that it understood that the shared KEK would be encrypted for the minimal subset of devices…presumably just a single failover device but, perhaps, a set of devices constituting a cluster.


> Section 5.2 p38
> 
> This section points to the NETCONF protocol's support for mutual authentication, but in this particular case, I believe that the mandatory support for confidentiality is very important and should be mentioned, in particular for the configuration of cleartext keys.

Please be aware that this section is following (roughly) the template defined here: https://tools.ietf.org/html/rfc8407.html#section-3.7.1 <https://tools.ietf.org/html/rfc8407.html#section-3.7.1>.  This template ware reviewed by previous Security Area ADs .

FWIW, the first paragraph says “… Both of these protocols have mandatory-to-implement secure transport layers (e.g., SSH, TLS) with mutual authentication.”, so “confidentiality isn’t completely unstated….

What text would you like to see added here?


> I had one puzzle I could never figure out - why the keystore-reference for the grouping local-or-keystore-asymmetric-key-with-certs-grouping (Sect 2.1.3.5) is an asymmetric-key-ref type.  Other uses of asymmetric-key-ref are consistent with it being a reference to a key only, but here I expected the reference would be to a structure that contains the key and its associated certs.  The descriptions in the Sect 2.3 Yang Module p29 says the grouping contains the cert and its keys.

Good catch.  Yes, there is some wonkiness here.  (Continued in next comment)

> Is the asymmetric-key-ref in some way useable as both a reference to an asymmetric key and an asymmetric key and its associated certs?

Indeed.  The reality is that the Keystore only supports the notion of keys that may have associated certificates.  In all cases, the “local” definition is as described, but sometimes the “keystore” definition is a bit off.   

The only perfect* mapping is the “local-or-keystore-asymmetric-key-with-certs-grouping” grouping, as that is exactly what the Keystore has in it.  

The "local-or-keystore-end-entity-cert-with-key-grouping” isn't that bad either, as it points to specific cert, which is under a key…the only wonkiness is that the key may have other certs under it too, but it isn’t a big deal.  

The "local-or-keystore-asymmetric-key-grouping” is the worst mapping because, while it does point to a key, the key may have certs under it.  The expectation is that this won’t be a problem in context because the code is, presumably, only interested in the key (not the certs), as that is exactly all the code would get if the “local” definition is used.


> At first I thought the "associated certs" in the groupings were the certificate chain certs - the subject cert, the issuer cert, etc.  I am not sure they are - might they be many certificates for the same public key, perhaps for different algorithms?  I think that should be explained.

Each “certificate” is actually the "end-entity-cert-cms" CMS structure defined in the “crypto-types” draft.  Each CMS MAY contain a chain of certs for that end-entity “certificate”, in quotes because the “certificate” can also include associated issuer certs, as needed for the context.

Therefore, that a key may have a multiplicity of associated certs (i.e. CMSs) is to support things such as: different algorithms, different Subject/SAN, different Issuers, etc.  Makes sense.

Happy to make things more clear, can you propose the text you’d like to see?




> Page-by-page comments and nits:
> -------------------------------
> 
> Section 1 p 3
> 
>  there are groupings that defined enabling a key to be either
> 
> Did you mean the past tense here?  Or did you mean "define"?

Fixed —> "define".

> Section 1.1 p 4
> 
> In the diagram, what is the meaning of the lines - what relationshiop do they represent - e.g., "refers to", "uses", "augments", something else?

"The relationship between” —> "The normative dependency relationship between"


> It looks like netconf-client-server has no relationship (whatever "relationship" is) to http-client-server, restconf-client-sever has no relationship to ssh-client-server, and http-client-server has no relationship to keystore or truststore.  Is that all correct?

Yes.  NETCONF doesn’t have an HTTP-based transport.  RESTCONF doesn’t have an SSH-based transport.  HTTP may be “mixed-in” with TLS to created a protocol stack, but doesn’t itself depend on TLS.


> Section 1.3 p 5
> 
>  This document in compliant with Network Management Datastore
>  Architecture (NMDA) [RFC8342].
> 
> RFC8342 is listed in the informative section - what does it mean to be compliant with an informative reference?

r/in compliant/is compliant/

This text is via https://tools.ietf.org/html/rfc8407#section-3.5.   It means that the data model has been developed assuming that there exists dedicated <operational> datastore that contains the “operational” value for the system as a superset of configured nodes.  


> 
>  [Std-802.1AR-2009] certificate) are expected to appear in
>  <operational>
> 
> I did not catch that the term <operational> was part of R%FC8342, and went looking for a reference.  I found it in many places, like RFC6241.  For those just that unobservant, could you put a citation here to RFC8342, which I think is the proper reference for that term.  Note: <running> appears later, and a citation to RFC8342 is probably proper there as well.  Yes, I know, probably about as necessary as a reference to "packet", but it would have spared me, maybe other non-Yang clueful folk will be reading this also.

Quite right - I added a “Terminology” section with references for "<running>” and “<operational>” (RFC 8342), as well as “client" and “server” (RFC 6241).



> Section 2 p 5
> 
>  This section defines a YANG 1.1 [RFC7950] module that defines a
>  "keystore"
> 
> Should that be "ietf-keystore"?  Or do you mean keystore in a general sense, in which case I think the quote marks make it look like a name, not a general sense.  (I went looking for a definition of a "keystore", but that could just be me.)

Part of the issue is that the term “keystore" isn’t defined anywhere. I just added the following in the new “Terminology” section:

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

That said, the issue with the paragraph you cite is worse than you point out, as it cites some but not all notable aspects about the ietf-keystore module.  I just rewrote that paragraph to a simpler description of the subsections to come.

OLD:
         This section defines a YANG 1.1 <xref target="RFC7950"/> module
         that defines a "keystore" and groupings supporting downstream
         modules to reference the keystore or have locally-defined definitions.

NEW:
         This section defines a YANG 1.1 <xref target="RFC7950"/> module
          called "ietf-keystore".  A high-level overview of the module is provided in
          <xref target="overview"/>. Examples illustatrating the module's use 
          are provided in <xref target="examples">Examples</xref>. The YANG
          module itself is defined in <xref target="keystore-yang-module"/>.





> Section 2.1.1.  p6
> 
>  The following diagram lists all the "feature" statements defined in
> 
> Everywhere beyond this page it says "tree diagram".  RFC8340 is cited in Section 2.1.3.1, but this is the first use (unless I'm wrong about the "tree diagram"), so the citation should go here.
> 
>  The following diagram lists the "typedef" statements defined in the
>  The following diagram lists all the "grouping" statements defined in
> 
> diagram -> "tree diagram"

The thing that you don’t know is that those diagrams (plus another that lists the “groupings” defined in the document) are NOT RFC8340 tree diagrams, they are using a format I made up that happens to look like the RFC 8340 diagrams.  

I recommend the NETMOD WG consider an rfc8340-bis to define the new tree diagram outputs for a list of features, a list of typedefs, and a list of groupings.

In the meanwhile, I updated this draft to explicitly state that particular diagrams are not tree diagrams as follows:

      |  The diagram above uses syntax that is similar to but not
      |  defined in [RFC8340].



> Section 2.1.3.3 - 2.1.3.6:
> 
> " offer an option as to if an asymmetric key is defined" - personally, I would say "as to whether".  I don't know if the RFC-Editor (and rfc style guide) care.

Fixed —> replaced all "as to if” strings to “for whether” - better?


> "augmented in if, e.g.," - I actually thought this was an editing error, until heard "augment in" used as a word in the IETF session.  Web search shows "augment in" is a term of art in netconf!  But some uses add a hyphen, which I think is a good idea.

Added to the new “Terminology” section:

“””
The sentence fragments "augmented" and "augmented in" are used herein as the verbified form of the "augment" statement defined in <relref section="7.17" target="RFC7950"/>.
“"”



>     reference a asymmetric key in an alternate location.
> 
> a asymmetric -> an asymmetric.  A global change might be good, as well as changing "an symmetric" to "a symmetric".

Both fixed - thanks!


> Section 2.1.3.4
> 
>  *  For the "local-definition" option, the defintion uses the
>     "asymmetric-key-pair-grouping" grouping discussed in
>     Section 2.1.3.4 of [I-D.ietf-netconf-crypto-types].
> 
> It looks to me like it is actually Section 2.1.4.4 of [I-D.ietf-netconf-crypto-types].

Fixed!  “2.1.3.4” -> “2.1.4.5"

This is one issue with the new “relref” element in the xml2rfc v3 syntax, a section number (e.g., “2.1.3.4”) has to be hardcoded, there’s no option to use the “anchor” value, so misalignments may occur unknowingly.



> Section 2.1.3.7 p 11
> 
>  *  The "keystore-grouping" grouping is defines a keystore instance as
> 
> "is defines" -> "defines"

Fixed


>  *  For asymmetric keys, each "asymmetric-key" uses the "asymmetric-
>     key-pair-with-certs-grouping" grouping discussed Section 2.1.3.10
>     of [I-D.ietf-netconf-crypto-types].
> 
> "discussed" -> "discussed in"

Quite right, and I found another like it, fixed both.


> Why do none of the asymmetric keys here use a grouping that is the key only, like the ct:asymmetric-key-pair-grouping used in Sectoin 2.1.3.4 of this document (discussed in 2.1.4.4 of [I-D.ietf-netconf-crypto-types]).
> 
>       |     |     +--rw encrypted-private-key
>       |     |        +--rw encrypted-by
>       |     |        |  +--rw (encrypted-by-choice)
>       |     |        |     +--:(symmetric-key-ref)
>       |     |        |     |  +--rw symmetric-key-ref?    leafref
>       |     |        |     +--:(asymmetric-key-ref)
>       |     |        |        +--rw asymmetric-key-ref?   leafref

The section you quoted is for the "encrypted-private-key” case of a “choice" statement called "private-key-type”.  This “choice” also supports "cleartext-private-key”, that is the raw key only, is that what you mean?


> Section 2.2.1 p 13
> 
>  The following example illustrates keys in <running>.
> 
> Like I said before, maybe a citation of RFC8340 here.

The new Terminology section defines “<running>" (and “<operational>) pointing to RFC8342 (NMDA).  [PS: RFC 8340 is for “Tree Diagrams”]


> Section 2.2.3 p 17 has the phrase "following non-normative ".  Does that belong here?  Or because this is XML, it can not be confused to be a normantive Yang module?

It was/is a little bit of everything   \_o_/

I created a new introduction:

	<t>These examples assume the existence of an example
	 module called "ex-keystore-usage” having the namespace
	 "http://example.com/ns/example-keystore-usage".</t>

And then changed the line as follows:

OLD:
            <t>The following non-normative module is defined to illustrate these
              groupings:</t>
NEW:
            <t>Following is the "ex-keystore-usage" module's YANG definition: 



>       <symmetric-key>
> 	      <encrypted-by>
>               <asymmetric-key-ref>hidden-asymmetric-key</asymmetric-k\
>  ey-ref>
>             </encrypted-by>
> 
> The "hidden-asymmetric-key" key appears later in the asymmetric-keys part of the keystore example on p15.  I don't know how to prevent that forward sort of reference for the readers, or if it is a problem.

This is not a problem, as ordering doesn’t matter with YANG-defined models.  


> Section 2.2.3 p 18
> 
>      list end-entity-cert-with-key {
>        key name;
>        ...
>        uses ks:local-or-keystore-end-entity-cert-with-key-grouping;
>        description
>          "An end-entity certificate, and its associated private key,
>           that may be configured locally or be a reference to a
>           specific certificate (and its associated private key) in
>           the keystore.";
> 
> You can't rely on descriptions, but I see no explicit reference to the private key in that end-entity cert grouping.  That grouping is defined in Section 2.1.3.6 p 9, and the keystore-reference is an asymmetric-key-certificate-ref-grouping.  That grouping has an asymmetric-key-ref and a cert.  The description above says "and its associated private key".  So is the asymmetric-key-ref a ref to the private key or to a key pair?

Fixed.  “associated private key” -> "associated asymmetric key"

        "An end-entity certificate and its associated asymmetric
         key, that may be configured locally or be a reference
         to another certificate (and its associated asymmetric
         key) in the keystore.”;



> Section 2.3 p 25
> 
>    typedef symmetric-key-ref {
>      type leafref {
>        path "/ks:keystore/ks:symmetric-keys/ks:symmetric-key"
>           + "/ks:name";
>      }
> 
> Does default-deny-write belong here, or added each time an item is defined of this type?

No, because it is a typedef, and typedefs don’t support extensions (search RFC 7950 for "typedef-stmt” and "extension-stmt”).


> Section 2.3 p 26
> 
>        case asymmetric-key-ref {
>          leaf asymmetric-key-ref {
>            type leafref {
>              path "/ks:keystore/ks:asymmetric-keys/"
>                   + "ks:asymmetric-key/ks:name";
>            }
>            description
>             "Identifies the asymmetric key used to encrypt this key.";
>          }
> 
> (a) "the asymmetric key used to encrypt" - the public key part is used to encrypt, so is the asymmetric key just the public key or a key-pair where the appropriate part of the key-pair is used according to context?

OLD:
            "Identifies the asymmetric key used to encrypt this key.";
NEW:
           "Identifies the asymmetric key whose public key
            encrypted the associated key.";


> (b) "encrypt this key" - The "this key" part is appropriate if the assymmetric-key-ref is being used in an encrypted-by-choice structure, but there are lots of places in this text where the type is not in such a structure.  Is it the case that the uses always resolve to be encrypting a key?

Right, per above, now "the associated key” and the “description" statement for "encrypted-by-choice-grouping” now says:

      "A grouping that defines a choice that can be augmented into
       the 'encrypted-by' node presented by the 'symmetric-key-grouping'
       and the 'asymmetric-key-pair-grouping' groupings defined in
       RFC AAAA.”;

Indicating that the “associated key” is defined in those two groupings.  Good enough?


> (c) "encrypt" - Is it always the case that asymmetric keys in the keystore will be used to encrypt something?  There is no expectation that the asymmetric key-pair will be used to sign something?

An asymmetric-key may be used to sign something, but this module has no need to do that.  For instance, the module doesn’t support the notion of a “signed key” (a la “encrypted key”), beyond its support for certificates.  This doesn’t preclude a consuming application from using an asymmetric key to sign data if needed.


> Section 2.3 p27
> 
>      description
>        "A grouping that expands to allow the symmetric key to be
>         either stored locally, within the using data model, or be
>         a reference to a symmetric key stored in the Keystore.";
> 
> The following is wordsmithing the description text, so a very-nitty-nit.
> 
> ", or be" -> ", or"  (the "be" is before the "either", don't need it)
> 
> A reader might be confused as to whether the disjunction was two disjuncts or three.

Fixed, in all four places where “or be” was being used.


> I might suggest using "i.e., within the using data store".  Maybe also
> move the "be" inside the "either or" for scoping
>        "A grouping that expands to allow the symmetric key to either
>         be stored locally, i.e., within the using data model, or be
>         a reference to a symmetric key stored in the Keystore.";

Added “i.e.,”

> The groupings in this section have a descriptoin of the grouping, a description of the key case and a description of the choice.  I thought it worked better to place the description of the choice closer to the "choice local or keystore" rather than at the end.

Moved all “choice” statement descriptions be before the “case” statements.


> And finally. Descriptions very frequently (always?) conclude with a "." but they are very frequently not sentences, so the "." is not necessary.  I do not know if the RFC-Editor or the Style manual cares.

The descriptions should be sentences.  I found only two instances that were missing an opening article (‘A’), which gave the appearance of not being a sentence…but you said there were “many"?


> Section 2.3 p 28
> 
>    grouping local-or-keystore-asymmetric-key-with-certs-grouping
>    ...
>       case keystore {
>          if-feature "keystore-supported";
>          leaf keystore-reference {
>            type ks:asymmetric-key-ref;
>            description
>              "A reference to an asymmetric-key (and all of its
>               associated certificates) in the Keystore.";
> 
> An example of my puzzle about the local-or-keystore-asymmetric-key-with-certs-grouping keystore-reference.  How are the certs to be found if the keystore-reference is to the asymmetric key only?  Or must the asymmetric-key-ref sometimes point just to a key and sometimes to a key+certs, as needed?

An asymmetric key "in the Keystore” always allows certs to be associated.  I understand that this isn’t true for asymmetric keys in general.



> Section 3 p 31
> 
>  In some implementations, a server may support built-in keys.  Built-
>  in built-in keys
> 
> "Built-in built-in" => "Built-in"

Fixed - redundancy removed.


> Section 3 p 32
> 
>  In order for the built-in keys (and/or their associated built-in
>  certificates) to be referenced by configuration, the referenced keys
>  MUST first be copied into <running>. The keys SHOULD be copied into
>  <running> using the same "key" values, so that the server can bind
>  the references to the built-in entries.
> 
>  Built-in "hidden" keys cannot be copied into other parts of the
>  configuration because their private parts are hidden, and therefore
>  impossible to replicate.
> 
> Section 4 p34 says built-in keys MUST be hidden.  So they must be copied to <running> but they can't be copied? Huh?
> 

OLD:
        <t>Built-in "hidden" keys cannot be copied into other parts of the 
          configuration because their private parts are hidden, and therefore
          impossible to replicate.  Built-in "encrypted" keys MAY be copied
          into other parts of the configuration so long as they maintain their
          reference to the other built-in key that encrypted them.</t>
NEW:
        <t>In addition to copying keys into the Keystore in &lt;running&gt;, 
          cleartext and encrypted keys may be copied into other parts of 
          configuration, but they will lose their connection to having been
          a built-in value.  Note that hidden keys cannot be copied into
          other parts of the configuration because doing would lose the key's
          connection to the built-in key, where the key's secret value is 
          stored.  Built-in "encrypted" keys MAY be copied into other parts 
          of the configuration so long as the reference to the other built-in 
          key that encrypted them is maintained.</t>



> The keys SHOULD be copied into
>  <running> using the same "key" values, so that the server can bind
>  the references to the built-in entries.
> 
> The word "key" is overloaded in this document, like "The key characteristic of the built-in keys" and "Key Words".  Here, the reference is surely to a required part of the "list" syntax, so no way to use a different word.  Maybe say 'the same list "key" values' or 'the same values for the list "key" field/node'.

Changed "Key characteristic" to "primary characteristic”.

"Key words” is RFC 2119 language that cannot be altered.

OLD:
          The keys SHOULD be copied into
          <running> using the same "key" values, so that the server can bind
          the references to the built-in entries.
NEW:
          The keys SHOULD be copied
          into &lt;running&gt; using the same value for the list's "key" 
          substatement, so that the server can bind the references to the
          built-in entries.


>   	       	  	     Built-in "encrypted" keys MAY be copied
>  into other parts of the configuration so long as they maintain their
>  reference to the other built-in key that encrypted them.
> 
> (a) Section 4 p34 says "built-in keys MUST be hidden", so is there a way the hidden built-in keys can be encrypted keys?

Under the hood, it is possible that a hidden key is encrypted, but if it is, that information would be hidden too ;)


> (b) So encrypted built-in keys must always be encrypted by other built-in keys?  Is there a way to represent that in the data model?  (I explored this before.)
> 
>  <running> MAY be a subset of the built-in keys define in <operational>
> 
> "define" -> "defined"

FIXED.



>  Only the referenced keys need to be copied; that is, the keys in
>  <running> MAY be a subset of the built-in keys define in
>  <operational>.  No keys may be added or changed (with exception to
>  associating additional certificates to a built-in key); that is, the
>  keys in <running> MUST be a subset (which includes the whole of the
>  set) of the built-in keys define in <operational>.
> 
> I don't understatnd this part.  First it says
> 
> "keys in <running> MAY be a subset of the built-in keys define in <operational>"
> 
> and then
> 
> "keys in <running> MUST be a subset ... of the built-in keys define in <operational>".
> 
> That is inconsistent, is it intended?
> 
> Does the last sentence mean that if there are built-in keys, no other keys may be added to the keystore?  I don't know what is intended here.

Removed the last part: i.e., “the keys in <running> MUST be a subset (which includes the whole of the set) of the built-in keys define in <operational>."

NEW:
        <t>Only the referenced keys need to be copied; that is, the keys
          in &lt;running&gt; MAY be a subset, including the whole of the set, 
          of the built-in keys defined in &lt;operational&gt;.</t>  
        <t>No new built-in keys may be added nor existing built-in changed, 
          with exception to associating additional certificates to an 
          existing built-in key.</t>


> About "exception to" - I use "exeption to <rule>" and "exception for <a special case>".  I don't know if the RFC-Editor or the Style manual cares.

“exception to” —> “exception for”


> Section 4.1 p34
> 
>  not support hidden keys, then the private data part of key MUST be 
> 
> "of key" -> "of the root key"

This sentence no longer exists after the rewrite of the section addressing the comment from Magnus.


> Section 4.1 p35
> 
>              If the hidden root key is asymmetric, then the server
>  SHOULD provide APIs enabling other keys to be both generated and
>  encrypted by it
> 
> The "both generated and encrypted by it" sounds like keys are being generated by the root key.  I don't think that's generally true.  Section 4.2 separates key generation and key encryption steps.  I think this should be clearer.
> 
> (The mention of other crypto services moved to area before nits.)

After addressing the comment from Magnus, the new text is:

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

Better?


> Section 4.2 p35
> 
> (A discussion of issues, not nits, was moved to area before nits.)
> 
> Section 4.3 p35
> 
> (A discussion of issues, not nits, was moved to area before nits.)  
> 
> Section 5.2 p38
> 
> (A discussion of issues, not nits, was moved to area before nits.)

Okay.




>     |  Please be aware that this module uses the "key" and "private-
>     |  key" nodes from the "ietf-crypto-types" module
>     |  [I-D.ietf-netconf-crypto-types], where said nodes have the NACM
>     |  extension "default-deny-all" set, thus preventing unrestricted
>     |  read-access to the cleartext key values.
> 
> This text missed the change A.19 to add a "cleartext" prefix as in ietf-netconf-crypto-types.  The "cleartext" prefix is present in the modules, tree-diagrams, XML, etc., but the references in text here did not change.  (Actually, I see several examples in the text in ietf-netconf-crypto-types where the change was not made, e.g. p 9 says  '-  The "key" node can encode any plain-text key value.' where the preceding tree diagram has a node named "cleartext-key" and older versions said "key".)  I appreciate the discussion of the default-deny-all in Section 3.5 p42 of ietf-netconf-crypto-types and I think that a citation here would be warranted and useful.

Fixed: prefixed “cleartext-“

NEW:
            <t>Please be aware that this module uses the "cleartext-key" and 
              "cleartext-private-key" nodes from the "ietf-crypto-types" module 
              <xref target="I-D.ietf-netconf-crypto-types"/>, where said nodes
              have the NACM extension "default-deny-all" set, thus preventing
              uncontrolled read-access to the cleartext key values.</t>

Also fixed a number of missing “cleartext-“ prefixes in the crypto-types draft.


> BTW.  I don't know the NACM well but could it be said that the default-deny-all prevents uncontrolled access, but does not necessarily prevent unrestricted access, because a present but careless access control might not actually restrict access?

Fixed.  See “uncontrolled” the the NEW above.


> Section 6.2 p 39
> 
> "the the" -> "the"

Fixed (in other documents also) - thanks!

> Section 7.2 p40
> 
> The list of Informative References includes:
> 
>  [I-D.ietf-netconf-keystore]
>             Watsen, K., "A YANG Data Model for a Keystore", Work in
>             Progress, Internet-Draft, draft-ietf-netconf-keystore-17,
>             20 May 2020, <https://tools.ietf.org/html/draft-ietf-
>             netconf-keystore-17>.
> 
> Isn't this a self reference to an earlier version?
> 
> Note: ID nits gives 11 warnings, but all are due to references to older versions of existing drafts or to future versions not yet published.  The latter case seems to be wrong, and the warning usually notes
>    (However, the state information for draft-ietf-netconf-keystore is not
>    up-to-date.  The last update was unsuccessful)
> 
> I have no idea what that means.  The warnings are consistent, so may be something to watch.

Generally speaking, a document should not reference itself!  The self-reference in this document is coming from Section 1.1 (Relation to other RFCs).  The XML feeding this section is from a common snippet of xml2rfc XML that is stitched into the same spot for each of the documents in the collection of RFCs-to-be, per the "Relation to other RFCs” section.   My choices are:

1) Copy-paste the common-text into each document and then customize it by converting the self-reference to something like “<this document>”, and the remove the reference from the "Informative References” section.

or

2) Add an RFC-editor note requesting that customization be made for each document in the collection.

Thoughts?


K.

> 
> —Sandy
>