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

From: Magnus Nyström <[email-masked]>
Date: Sun, 9 Aug 2020 22:29:32 -0700

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

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.

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.

*Editorial:*

( A few nits that I found )

Section 1:

   - Substitute "Trusted Protection Module" with "Trusted Platform Module"
   - The term "HSM" is used without explanation
   - Last sentence (starting: "It is also possible...") is garbled

Section 1.1:

   - Sentence starting "Links the each" is garbled

Section 3:

   - Second sentence starts "Built-in built-in keys..."

/M

---

From: Kent Watsen <[email-masked]>
Date: Tue, 11 Aug 2020 01:26:00 +0000

[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 <[email-masked]> 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:
> 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

---

From: Magnus Nyström <[email-masked]>
Date: Mon, 10 Aug 2020 23:28:00 -0700

Hi Kent,
On the metadata, yes, I did catch that the actual values are protected. My
suggestion (it is entirely up to the group of course) was just to consider
if any of the data that will be readable may warrant a similar restriction
as you have for the actual key values.

On the root key etc., - OK, I understand now. I am also good with your
other updates.

Thanks!
/M

On Mon, Aug 10, 2020 at 6:26 PM Kent Watsen <[email-masked]> wrote:

> [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
>
>
> 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:*
>
> The GitHub commit enacting the updates is here:
> https://github.com/netconf-wg/keystore/commit/5b3b9f3db0d6b7027ddf5f0e983ab958e8802251
>

-- 
-- Magnus

---

From: Kent Watsen <[email-masked]>
Date: Wed, 12 Aug 2020 14:35:09 +0000

Hi Magnus,

I just want to confirm, did you read the new Section 4 (Encrypting Keys in Configuration) in the plain-text draft I attached in my previous response?  - it was effectively completely rewritten to align with the Key Encryption Key and Master Key terminology...

PS: Sandy’s response is massive, but it refers to the old Section 4 text so, before digging into it, I want to ensure that you’re happy with the new text.

Kent

> On Aug 11, 2020, at 2:28 AM, Magnus Nyström <[email-masked]> wrote:
> 
> Hi Kent,
>> normally refers to an asymmetric key, but in this text it could be either

---

From: Magnus Nyström <[email-masked]>
Date: Wed, 12 Aug 2020 23:26:12 -0700

Hi Kent,
I did review the updated Section 4 and I think it looks good. A couple of
minor suggestions:

   - You could replace "Master Encryption Key (MEK)" with just "Master Key
   (MK)," it is a more common term I think.
   - In addition, I would suggest replacing 'In order to decouple the
   crypto officers from the regular administrators, a second KEK, called the
   "master encryption key" (MEK), is used." with "In order to decouple the
   crypto officers from the regular administrators, a second KEK, called the
   "master encryption key" (MEK), MAY be used."
   (The "MAY" shows there are more options)

Thanks,
/M

On Wed, Aug 12, 2020 at 7:35 AM Kent Watsen <[email-masked]> wrote:

> Hi Magnus,
>
> I just want to confirm, did you read the new Section 4 (Encrypting Keys in
> Configuration) in the plain-text draft I attached in my previous response?
> - it was effectively completely rewritten to align with the Key Encryption
> Key and Master Key terminology...
>
> PS: Sandy’s response is massive, but it refers to the old Section 4 text
> so, before digging into it, I want to ensure that you’re happy with the new
> text.
>
> Kent
>
>
>

-- 
-- Magnus

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 13 Aug 2020 10:44:27 -0400

I interpreted the “shared root key” as a key encryption key — I don’t think you eliminate any issues.

(The term is common, but I haven’t found an ietf definition to reference, so I can’t say it *is* a key encryption key.)

wrt: “one can rotate or renew the key encryption key” 

I thought the migration process might work to rotate the root key, not the “shared root key”.  So the “first server” is server Bob with root key Bob.Tues-key and “second server” is server Bob with root key Bob.Wed-key.  Do you think that works?

Magnus’ mention of “renew” brings up the topic of key lifetimes, which I realize I did not think to mention.  (Yep, out of all those words, I did not mention something I should have.)  This draft includes expiration times for the certificate, but no key lifetimes, here or in the crypto-types draft.  RFC8177 does include lifetimes for keys, for the purpose of establishing valid intervals of use and key rollover.  Was key lifetime considered?

—Sandy

> On Aug 10, 2020, at 9:26 PM, Kent Watsen <[email-masked]> wrote:
> 
>>

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 13 Aug 2020 11:04:57 -0400

Yep, sorry.  A comment that I felt needed a table of contents, wow.

In my defense, this was my first view of the draft, not a “why didn’t you mention that earlier” comment.

This was the result of an initial review before the IETF, a full day of review (my first look at a YANG module) and a full day of transcribing (and resolving) comments.

—Sandy     

> On Aug 12, 2020, at 10:35 AM, Kent Watsen <[email-masked]> wrote:
>

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 13 Aug 2020 11:16:17 -0400

> I just want to confirm, did you read the new Section 4 (Encrypting Keys in Configuration) in the plain-text draft I attached in my previous response? 

Taking a look at -20, it looks like Section 4 is the only change (of substance).

I see that you removed the early paragraph that says the root key should be hidden but might be unhidden with careful access control.  Have you abandoned the idea of an unhidden root key (aka MEK)?

The revisions have eliminated the "Given the long lifetime of built-in keys (see Section 3), built-in keys MUST be hidden.”  Did you intend to eliminate that?

Are MEK and KEK the only keys that can be used to encrypt other keys?  I thought the YANG model permitted using a reference to any keystore key in the encrypted-by field.

—Sandy

---

From: Kent Watsen <[email-masked]>
Date: Thu, 13 Aug 2020 17:32:17 +0000

Hi Sandy,

Thank you for your most awesome review!

I’m preparing a response but it’s taking time to get through it all.  Early to mid next week seems about right.

Kent

> On Aug 13, 2020, at 11:04 AM, Sandra Murphy <[email-masked]> wrote:
>

---

From: Kent Watsen <[email-masked]>
Date: Thu, 13 Aug 2020 18:23:00 +0000

Hi Sandy,

> I see that you removed the early paragraph that says the root key should be hidden but might be unhidden with careful access control.  Have you abandoned the idea of an unhidden root key (aka MEK)?

It has not been intentionally abandoned, for the simple reason that not all devices may support “hidden” keys and so access-control is all they can do.  Sadly, this is in fact what most systems do today.

While the "root key” was generally mapped to “MEK", the section containing the text you mention was more about a KEK.  Thus I made this tweak to my local copy:

OLD:
           In other deployments, an organization's crypto officer, possessing the
            KEK's cleartext value, configures the same KEK on the second server,
            presumably as a hidden key so that the cleartext value is not disclosed
            to regular administrators.

NEW:
           In other deployments, an organization's crypto officer, possessing the
           KEK's cleartext value, configures the same KEK on the second server,
           presumably as a hidden key or a key protected by access-control 
           (e.g., NACM's "default-deny-all), so that the cleartext value is not
           disclosed to regular administrators.

> The revisions have eliminated the "Given the long lifetime of built-in keys (see Section 3), built-in keys MUST be hidden.”  Did you intend to eliminate that?

Kind of.  I feel that this draft cannot be overly proscriptive. The closest new text follows with an enhancement that (I think) strikes the right balance (i.e. the “MUST” in the quoted text becomes —> "commonly…hidden”).  However, I tweaked the text a bit more to be even more like that from before:

OLD:
            A MEK is commonly a globally-unique built-in (see <xref target="built-ins"/>)
            asymmetric key for which the private key is hidden and the public key is
            contained in an identity certificate (e.g., IDevID).

NEW:
            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.4." target="I-D.ietf-netconf-crypto-types"/>) and the 
            public key is contained in an identity certificate (e.g., IDevID).

> Are MEK and KEK the only keys that can be used to encrypt other keys?  I thought the YANG model permitted using a reference to any keystore key in the encrypted-by field.

Correct.  In fact, the YANG model itself doesn’t know anything about MEKs or KEKs; it only has keys, which MAY be builtin and MAY be encrypted or hidden.  Effectively the model supports any key encrypting another key and, in that sense, any key can be a KEK.

Sections 3 and 4 attempt to highlight best/common practice (for those that might not know any better) without limiting possible variations (for those that think they do or otherwise are more adventurous).  ;)

Kent

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 13 Aug 2020 14:40:36 -0400

> On Aug 13, 2020, at 1:32 PM, Kent Watsen <[email-masked]> wrote:
> 
> Hi Sandy,
> 

No problem whatsoever.  I have some work to catch up on.  :-)

—Sandy

---

From: Kent Watsen <[email-masked]>
Date: Thu, 13 Aug 2020 19:09:50 +0000

Hi Sandy,

I think you’re making two comments, but there may be more!

1) is there support for key-lifetime/expiration?

The answer is “no” (or, perhaps, “not yet”), for the simple reason that key file formats (e.g., OpenSSL RSAPrivateKey, ECPrivateKey, AESKey, etc.) do not have fields for validity periods.  Do you think this module should add such a field (like RFC 8177)? - if it did, then the module certainly could also define a key-expiration notification.  Please advise.

2) Can KEKs be renewed / rolled over?

Yes, but not like in RFC 8177.  I’m not a routing person, but my understanding to that RFC 8177’s approach is visible in routing protocol messages...

In contrast, configuration is being set and there may only be the “current” key.  That said, a renew/rollover might be implemented as follows:

   - crypto-officer generates a new KEK
   - for each device having the same KEK (e.g., primary/failover devices):
        - crypto-officer encrypts the KEK with the device’s MEK
        - crypto-officer hands off the encrypted-KEK to a regular admin
        - regular-admin loads the encrypted KEK into the device
            - note that, at this time, no switchover has occurred yet
        - for each key on the device encrypted by expiring KEK:
            - regular-admin requests the device to encrypt the key using
               the new KEK (no switchover has occurred yet).  In pseudocode:
               new-encrypted-key = new-kek.encrypt(cleartext-key) (see [1])
            - regular-admin replaces the key (encrypted by the expiring
              KEK) with the new (really the same) key (encrypted by
              The new KEK).
                           
[1] this entails the regular admins knowing the cleartext clears, which is NOT best practice, but a necessary(?) consequence of the decision discussed here: https://tools.ietf.org/html/draft-ietf-netconf-crypto-types-17#section-3.2.   

To be clear, there were before two RPCs: generate-symmetric-key() and generate-asymmetric-key().  If IETF were to rescind this decision, or when a future effort fills in the missing piece, then it would make sense to additional RPC called, perhaps, reencrypt-symmetric-key() and reencrypt-asymmetric-key().

[Update, due to second-guessing myself] Hmmm, the reasons for why the two “generate” RPCs couldn’t be defined may not apply/limit the creation of said “reencrypt” RPCs…is this something you think should be added?

Regardless if said “reencrypt” RPCs are added or not, do you think the draft should extend “Encrypting Keys in Configuration” section with the above sketch of steps?

Please let me know if there were more than these two questions in your comment.

Thanks,
Kent

> On Aug 13, 2020, at 10:44 AM, Sandra Murphy <[email-masked]> wrote:
>