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

[Sandra Murphy <[e-mail]>]

And I missed the fact that Magnus did a review as well.

—Sandy

> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.  If an asymmetric key is configured with a public key and a private key that are not a key-pair, where is that caught?  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?  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?)
> 
> 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.
> 
> Section 4 p34 says "built-in keys MUST be hidden".  That really should be mentioned in Section 3.
> 
> 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.
> 
> 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.
> 
> By the requirement that built-in keys must be hidden, servers that do not support hidden keys can not use built-in keys.  Correct?
> 
>  A hidden root key MAY be either a symmetric key or an asymmetric key.
> 
> Does the same apply to built-in keys?  The examples in Section 3 are all asymmetric keys.
> 
> Is using a built-in key as a root key advisable in operational situations?
> 
> 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.
> 
> 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?
> 
> 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.
> 
> Section 4.3 p35
> 
> 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.)
> 
> 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".
> 
> "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.
> 
> 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.
> 
> 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.
> 
> 
> 
> 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.
> 
> 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?
> 
> 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.
> 
> 
> 
> 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"?
> 
> 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?
> 
> 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?
> 
> 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?
> 
>  [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.
> 
> 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.)
> 
> 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"
> 
> 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.
> 
> "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.
> 
>     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".
> 
> 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].
> 
> Section 2.1.3.7 p 11
> 
>  *  The "keystore-grouping" grouping is defines a keystore instance as
> 
> "is defines" -> "defines"
> 
>  *  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"
> 
> 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
> 
> Section 2.2.1 p 13
> 
>  The following example illustrates keys in <running>.
> 
> Like I said before, maybe a citation of RFC8340 here.
> 
> 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?
> 
>       <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.
> 
> 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?
> 
> 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?
> 
> 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?
> 
> (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?
> 
> (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?
> 
> 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.
> 
> 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.";
> 
> 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.
> 
> 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.
> 
> 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?
> 
> 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"
> 
> 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>.
> ...
>  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?
> 
>                                        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'.
> 
>   	       	  	     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?
> 
> (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
>  set) of the built-in keys define in <operational>
> 
> "define" -> "defined"
> 
>  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.
> 
> 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.
> 
> Section 4.1 p34
> 
>  not support hidden keys, then the private data part of key MUST be 
> 
> "of key" -> "of the root key"
> 
> 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.)
> 
> 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.)
> 
> 
>     |  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.
> 
> 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?
> 
> Section 6.2 p 39
> 
> "the the" -> "the"
> 
> 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.
> 
> —Sandy
>