Re: [secdir] (Early) Secdir review of draft-ietf-netconf-keystore

[Kent Watsen <[e-mail]>]

[CC-ing netconf-chairs]


Hi Magnus,

Thank you for the review.  Below are some comments, as well as a link to the GitHub commit, and an updated plain-text version of the draft.

Thanks,
Kent


> On Aug 10, 2020, at 1:29 AM, Magnus Nyström <[e-mail]> wrote:
> 
> I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors.  Document editors and WG chairs should treat these comments just like any other last call comments.
> 
> This document describes a new YANG model for centralized configuration of cryptographic keys in the context of NETCONF and RESTCONF.
> Section 4 describes a process to encrypt all "private" keys in a configuration. It is unclear if this refers only to the asymmetric keys in the configuration (as "private keys" normally refer to asymmetric keys) or also symmetric keys. It appears to apply to all cryptographic keys, and it therefore seems better to replace this usage of "private" with "symmetric and asymmetric private keys."

Good catch.  r/all the private keys/both the symmetric and asymmetric keys/


> Likewise, the use of the term "root key" is a bit unclear. A "root key" normally refers to an asymmetric key, but in this text it could be either.
> 
> More broadly, it may be advantageous to consider a hierarchy, where a key encryption key is used to encrypt all keys in a configuration and this key encryption key in turn is protected by a root (or master) key. This way, two instances only need to share the key encryption key and there's no need for shared root keys (which may present an issue). Likewise, one can rotate or renew the key encryption key if desired, whereas this usually is harder for root keys.

What you say is the intention, but the terminology used seems to be off.   The alignment needed is:

  Root key —> Master Encryption Key (MEK)
  Shared root key —> Key Encryption Key (KEK)



> Section 5.2 first states: "The NACM "default-deny-all" extension has not been set for any data nodes defined in this module," ostensibly because "none of the readable data notes ... are considered sensitive." I note here that meta-data about keys oftentimes can provide valuable information about those keys and suggest considering if the default-deny-all extension should be set by default after all.

I think there is a subtly you may be missing.  The keystore module uses the key definitions defined in the crypt-types draft.  In that draft, the key data *is* flagged with the “nacm:default-deny-all” extension.  So the key data itself (e.g., an ECPrivateKey or AESKey, in OpenSSL parlance) is protected.  Is that sufficient to protect the key metadata mentioned...or are you concerned about the “key-format” and/or “public-key” nodes being readable?


> Editorial:
> 
> ( A few nits that I found )
> 
> Section 1: 
> Substitute "Trusted Protection Module" with "Trusted Platform Module"
> The term "HSM" is used without explanation
Both fixed in my local copy - thanks!

> Last sentence (starting: "It is also possible...") is garbled

Now reads:

          "It is also possible that a 
          system implementing this module possesses a multiplicity of
          operating system level keystore utilities and/or HSMs."

Better?



> Section 1.1:
> Sentence starting "Links the each" is garbled
> Section 3:
> Second sentence starts "Built-in built-in keys..."

Both fixed in my local copy - thanks!




The GitHub commit enacting the updates is here: https://github.com/netconf-wg/keystore/commit/5b3b9f3db0d6b7027ddf5f0e983ab958e8802251 <https://github.com/netconf-wg/keystore/commit/5b3b9f3db0d6b7027ddf5f0e983ab958e8802251>

The plain-text version of the updated draft is attached.

Thanks again!
Kent


> /M