Subject: Re: [Netconf] comments on draft-ietf-netconf-keystore-02

From: Kent Watsen <[email-masked]>
Date: Mon, 10 Jul 2017 19:46:42 +0000

Hi Martin,

Please see below for responses to your comments.

Thanks,
Kent

--

Hi,

Some quick comments on the keystore document:

o  Section 2

  The text has:

   o  A semi-configurable list of private keys, each with one or more
      associated certificates.

  "semi-configurable" sound very odd.  What does it even mean?  I
  think the model needs to be designed so that it works with the
  normal configuration datastores that we have.  They are not
  "semi-configurable".   I suggest s/semi-//

<KENT> This "semi-" was a holdover from before when the private-key
data itself was not directly configurable.  We fixed this by making
the private-key data configurable, and adding an NACM default-deny-all
attribute to it.   This change will be in -03.

o  Section 2

      Private keys MUST be either preinstalled
      (e.g., a key associated to an IDevID [Std-802.1AR-2009]
      certificate), be generated by request, or be loaded by request.

  What does this mean?   The text is on the form: MUST be a, b, or c.
  This seems to imply that there are other things (d, e ...) that it
  MUST NOT be.  What are those?

<KENT> This is related to the issue above.  I just rewrote the whole
paragraph.  Taken together, it now reads:

   o  A configurable list of private keys, each with one or more
      associated certificates.  Private keys may be configured using
      standard configuration mechanisms or, to support requesting the
      server to generate a private key in hardware (e.g., a TPM), the
      private key may also be created via the 'generate-private-key"
      action.  Private keys may also be preinstalled (e.g., a key
      associated to an IDevID [Std-802.1AR-2009] certificate).  Each
      private key may have one or more associated certificates.

o  /keystore/keys/key

   I suggest the "INACCESSIBLE" enum is changed to something more
   descriptive, maybe "protected" or "tpm" or something similar.

<KENT> HARDWARE-PROTECTED?

   Also, what happens if a client creates a key with this enum set?  I
   assume it is ok, but the key can't be used until
   "generate-private-key" has been used?

<KENT> I assume that this would NOT be okay.  The expectation is
that action would be called first.  The description for the 
'generate-private-key' action's 'name' leaf says: 

  "The name this private-key should have when listed
   in /keys/key.  As such, the passed value MUST NOT
   match any existing 'name' value.";

I guess we could change this, to allow what seems to be an out of 
order sequence of operations, but I'd rather update the (now) 
HARDWARE-PROTECTED enum to say that it's a value that cannot be
set by clients.  What do you think?

o  /keystore/keys/key

  I assume that if the client creates a new key with
  "generate-private-key", it shows up in <operational>.  Then the
  client can create an entry in the config with the same name, in
  order to configure a certificate for example.

<KENT> correct, it shows up in <operational>.  Exactly, the client
would create an entry with the same name, e.g., in <running>, to
set associated certificates.

o  leaf algorithm-identifier

  Why is this config if it is only used when the key is generated?
  What happens if a client changes this on an existign key?

<KENT> The leaf needs to be config true to support the case where the
client is creating the private-key via a NC/RC operation.  It would
be a problem if a server allowed a client to change this value for
an existing key, at least not without simultaneously changing the
key's crypto-value as well.  Do you think the draft needs to say
that?

  The model has this which probably should be removed.

             // no 'params' like in RFC 5912? - none are set for
             // algs we care about, but what about the future?

<KENT> removed in my local copy.  I guess that, if we ever need to
support params, we can add them in a revision to this module.

  (there are some other comments that also should be removed, search
  for '//')

<KENT> I removed all of these, but there was one that might be worth
asking here.  It was asking a question about if we should rename:

  /keystore/trusted-certificates/trusted-certificate/certificate

to something like:

  /keystore/trusted-certificates/trusted-certificate/data

or maybe:

  /keystore/trusted-certificates/trusted-certificate/x509

It's just that, it might be good to use a different word so that it
stands out more...  

Thoughts?  Anyone?

Kent

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 11 Jul 2017 10:17:29 +0200 (CEST)

> Please see below for responses to your comments.

I suggest "hardware-protected".  No need to shout.

> set by clients.  What do you think?

I have strong objections to any special magic treatment of
configuration nodes.  Either it is config true, and then it can be
configured by clients, or it is config false and just operational
state.

I think this can be handled just like interfaces; if there is a
configured instance that matches an instance in available in the
hardware, then the config is used, otherwise the config is ignored.
Then the order of configuring vs. calling the action doesn't matter.

> set associated certificates.

It would be good to clarify this in the draft.

> client is creating the private-key via a NC/RC operation.

Why is this leaf needed in this case?

> It would

You have /keys/key/certificates/certificate/value
                                            ^^^^^
(and .../trusted-host-keys/trusted-host-key/host-key)

I don't have a strong opinion, but prefer consistent naming.

/martin

---

From: Kent Watsen <[email-masked]>
Date: Tue, 11 Jul 2017 15:52:47 +0000

Hi Martin,

> I suggest "hardware-protected".  No need to shout.

Done.

> configuration nodes.

Understood.

>  Either it is config true, and then it can be

It is config true.

> Then the order of configuring vs. calling the action doesn't matter.

I'm okay with this.   Looking at how to make the change, I think
the source issue is that the 'algorithm-identifier' leaf is mandatory
true.  This is a problem because, if all the client wants to do is to
set a certificate for an existing 'hardware-protected' key, then it
should not have to pass anything other than the list's key 'name'.
Making matters worse, 'algorithm-identifier' isn't the only mandatory
true leaf, 'private-key' and 'public-key' are also.  I think that
all three of these should not be mandatory true. We can use a 'must'
statement to indicate that the 'algorithm-identifier' leaf must exist
for both the private-key and public-key leafs.  Agreed?

> It would be good to clarify this in the draft.

The description statement for the 'generate-private-key' action says:

    The resulting key is
    considered operational state and hence initially only
    present in the <operational> datastore, as defined in
    [I-D.netmod-revised-datastores].";

Is this good enough, or do you want text in the draft itself?

> Why is this leaf needed in this case?

Because the private-key itself is a binary blob without an ability to
discern its encoding otherwise.  The 'algorithm-identifier' leaf is
used to identify the encoding of the private-key (and public-key).

Ah ha!, now I understand your issue...the description statement on
'algorithm-identifier' is wrong...

OLD
  Identifies which algorithm is to be used to generate the key.
NEW
  Identifies the key's algorithm.  More specifically, this
  leaf specifies how the 'private-key' and 'public-key'
  binary leafs are encoded.

Better?

> I don't have a strong opinion, but prefer consistent naming.

Right, we could change all of them to something like this:

OLD
  /keys/key/certificates/certificate/value
  /trusted-host-keys/trusted-host-key/host-key
  /trusted-certificates/trusted-certificate/certificate

NEW
  /keys/key/certificates/certificate/data 
  /trusted-host-keys/trusted-host-key/data
  /trusted-certificates/trusted-certificate/data

Or we could try to give them all names more meaningful to the
types of data they contain:

NEW2
  /keys/key/certificates/certificate/unsigned-pkcs7
  /trusted-host-keys/trusted-host-key/one-asymmetric-key
  /trusted-certificates/trusted-certificate/x509-certificate

What do you think?

Kent