Subject: Yangdoctors last call review of draft-ietf-netconf-keystore-02

From: Jürgen Schönwälder <[email-masked]>
Date: Mon, 17 Jul 2017 01:43:28 -0700

Reviewer: Jürgen Schönwälder
Review result: Not Ready

draft-ietf-netconf-keystore-02

- s/YANG data module/YANG module/ or s/YANG data module/YANG model/

- Find a better phrase for 'hold onto'. Does the module allow
  configuration of trusted private keys and certificates? If so, say
  so.

- What is 'extends special consideration'?

- What is a 'semi-configurable' list?

- s/is MAY/MAY/ (does this may need to be allcaps?)

- s/an optional attributes/optional attributes/

- the module seems to use actions but the surrounding text talks about
  rpcs

- does the generate-private-key really take a key length as a parameter
  as promised in section 2?

- section 2.2. talks about 'examples above' but there are none 'above'

- Have the examples been validated? Is rsa1024 a valid identityref
  value?

- There are a number of binary fields where you show explanatory text
  in the examples instead of encoded data. Is this always clear?

  <private-key>...</private-key> <!-- elipsis placeholder for a base64 encoded key -->

- I am not sure I understand trusted-host-keys. On systems that have
  multiple user accounts, you usually have a per account list of
  trusted host keys in addition to a global system wide list. Perhaps
  make it clear that this models the global known hosts list only?

- Is it helpful that some examples use XML / NETCONF while others use
  JSON / RESTCONF or is this possibly confusing? If you include JSON
  examples, do you not need to reference RFC 7951?

- s/(c) 2014/(c) 2017/

- reference statement of the revision statement seems to have the
  wrong title

- Should the document title be aligned with other YANG module
  definitions, so it is easier to spot it?

- Since crypto algorithms come and go, is it reasonable to have the
  identities defined in a rather static RFC? Would an IANA maintained
  identity module perhaps make more sense?

- Oh, I now see that the key length is embedded in the algorithm
  identity. Perhaps clarify this earlier, see my confused comments
  above. Anyway, I think this makes the need for an IANA maintained
  module even stronger.

- "If additional algorithms are needed, they MAY be augmented in by
  another module" seems confusing; I think all that is needed is to
  define a new identity, right? If so, there is no augmentation in the
  YANG sense.

- The description of the keystore container talks about passwords and
  I am not sure this is correct. I am also not sure the terms 'active
  material' and 'passive material' really help.

- The description of keys says "A list of keys maintained by the
  keystore.". I think it would be more helpful to say "A list of
  public-private key pairs maintained by the keystore." Every list
  element seems to have two keys.

- What about the comment in the definition of algorithm-identifier?
  Looks like an open issue or something that should be resolved before
  publishing this document.

- Add a reference clause to the definition of private-key? Do we
  really need the INACCESSIBLE enum and the union? There is another
  open question right below the private-key definition...

- Add a reference clause to the definition of public-key?

- Why do certificate names have to be unique across all keys?

- Since the certs of a key do not contain a signature, where are signed
  certificates stored or are they outside the scope of the model?

- All references to RFCs and I-Ds in description clauses must resolve
  to regular citations somewhere and references in the references
  section.

- Is there any text needed to explain what happens if actions can't be
  carried out, e.g., the algorithm identity is not supported? Can a
  client actually learn which algorithm identities are supported or is
  the approach simply trial and error?

- I am a bit concerned about the two list names 'trusted-certificates'
  and 'trusted certificate' which mean very different things and can
  be potentially confused. (The same for 'trusted-host-keys' and
  'trusted-host-key'.) [No, I do not have a good naming proposal that
  I like, perhaps some brainstorming helps.]

- I am not sure what 'trusted certificate' here really means. What you
  are modeling is a set of sets of certificates. What such a set means
  depends on where this list is used. I am not sure what it means for
  a certificate in such a set to be 'trusted'.

- What about the // rename to 'data' comment? I like it, renaming to
  list certificate { key name; leaf name; leaf data; } seems less
  confusing (and it is even shorter). Perhaps something along this
  line would be better (not sure I like certificate-set)

	 +--rw certificate-set* [name]
         |  +--rw name                   string
         |  +--rw description?           string
         |  +--rw certificate* [name]
         |     +--rw name    string
         |     +--rw data?   binary

- Why is the certificate (data) leaf in trusted-certificate not
  mandatory? Does it make sense to have list elements where there
  is only a name?

- I am wondering whether it is really a good idea to handle both X509
  keys and certificates as well as some SSH host keys in this document.
  Perhaps it is better to really split things. Also think about devices
  that only do SSH or only to TLS.

- I am not sure I fully understood the call home statement in the
  description of trusted-host-key. This probably needs more or better
  explanation.

- Are SSH implementation encoding keys in ASN.1/DER? I doubt this very
  much. Perhaps this SSH stuff should be moved to a separate I-D since
  I believe this needs more work.

- You cite RFC 6020 (YANG 1) but you use actions which were only added
  in YANG 1.1. I think you should only cite YANG 1.1 (RFC 7950).
 
- The prefix is 'ks' and not 'kc' as stated in the IANA considerations
  section.

- Security considerations talk about 'RPC operations' but the data
  model actually uses actions. Perhaps simply talk about 'operations'.

---

From: Kent Watsen <[email-masked]>
Date: Wed, 19 Jul 2017 21:06:59 +0000

Hi Juergen,

Thanks for your review.  Below are my responses to your comments.

PS: why is [email-masked] in the CC?

Kent

--

<KENT> Fixed.

<KENT> Done.

<KENT> reworded.

<KENT> fixed (per Martin's comment earlier)

<KENT> previously fixed and case-lowered.

<KENT> fixed.

- the module seems to use actions but the surrounding text talks about rpcs

<KENT> fixed.

<KENT> no it doesn't (anymore). fixed.

<KENT> fixed.

<KENT> Just did, found no issues.

<KENT> This issue has been raised by others as well.  I used to
have full base64 values, but it made the examples very difficult
to read.  Hmmm, I like your suggestion, but the resulting line
is much longer than allowed.  I could line-wrap, but the result
looks worse.  Any other suggestions?

<KENT> it's a list of lists, so there can be many different
sets a host-keys defined, with each application/use pointing
to its own entry.  The ietf-ssh-client module's grouping has
a leafref to a specific entry.  FWIW, this grouping is
used by ietf-netconf-client module.  At the moment, there
no other uses of this grouping.  I think that you may be
getting confused between the key the server presents (the
host-key) versus the key user presents.
.

- Is it helpful that some examples use XML / NETCONF while
  others use JSON / RESTCONF or is this possibly confusing?
  If you include JSON examples, do you not need to reference
  RFC 7951?

<KENT> I thought there was a criticism a while back that
drafts should show balance in their examples?  In the 
meanwhile, I reworded the sentence to look more like the
other two examples and, in doing so, removed the "JSON
encoding" string altogether.

- s/(c) 2014/(c) 2017/

<KENT> fixed, in all my drafts.

<KENT> fixed.

<KENT> I don't understand, what do you mean?

<KENT> Unsure, but I wouldn't suspect needing to make that
kind of update for a long time.  If we were to define a 
module for algorithm identities, it might lead to us wanting
to define every algorithm (not just public-key algs), which
could take some time...

<KENT> yes, but your comment before was valid.  I don't see why
this makes a stronger case though...

<KENT> Fixed.

<KENT> agreed. Fixed.

<KENT> Fixed.

<KENT> comment removed per Martin's review.

<KENT> added.

<KENT> added.

<KENT> because otherwise leafrefs might be ambiguous, right?

<KENT> I'm confused, certificates are signed structures already...

<KENT> Fixed.

<KENT> If an action fails, then standard NC/RC error is returned.
There is no support to learn which algorithms a server supports,
but by supporting this module, the server must support all of
these algorithms.

<KENT> okay, let's keep this one open.

<KENT> yes, the certs may be used by different modules for
different use-cases, but they'd always be trusted.  E.g.,
the certs preinstalled by a browser...

<KENT> "set" doesn't sound too bad - dunno.  Regarding moving
"data", see the tail-end of the email I sent on July 11.

<KENT> I was supposed to be mandatory.  Fixed.

<KENT> I believe that it is correct for the keystore to
contain all these things, and I think it's most readable
for them all to be defined in this document.  What do
others think?

<KENT> it was dumb, I removed the whole paragraph.

<KENT> Good point.  Just now realizing that ietf-system's
'authorized-key' has a workable definition.  I just switched
to using it.

<KENT> Fixed.

<KENT> holdover from when draft was called "keychain".  Fixed.

<KENT> Fixed.  (Benoit should update his template as well)

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 20 Jul 2017 18:59:42 +0200

> PS: why is [email-masked] in the CC?

I used the datatracker form and it does what it does.
 
> looks worse.  Any other suggestions?

Assuming people will figure out that ... is just a marker, what about
this?
 
   <private-key>...</private-key> <!-- base64 encoded key  -->

> host-key) versus the key user presents.

I though these are the authorized host keys? Yes, I am confused. If
these are user public keys, they should not be called host keys. But
yes, I am confused (but I started to read the other documents).

> <KENT> I don't understand, what do you mean?
 
We often use "[A] YANG Data Model for ....".

https://en.wikipedia.org/wiki/YANG#Usage_in_Standards

The current name does not say YANG (sometimes I search for YANG in the
RFC index) and Keystore is also pretty broad given the related work we
have.
 
> to define every algorithm (not just public-key algs), which

I think an IANA maintained registry can be as incomplete or complete
as the I-D. I understand that the mechanics of working out the proper
registry can be painful (there likely are already N registries) but
clearly crypto algorithm identities must be easily extensible.

> this makes a stronger case though...

Well, the notion of what is strong changes every N years.

> <KENT> because otherwise leafrefs might be ambiguous, right?

OK. Perhaps explain this detail so it is obvious to everybody.

> <KENT> I'm confused, certificates are signed structures already...

Well, the description of certificates/certificate/value says:

   An unsigned PKCS #7 SignedData structure, as specified
   by Section 9.1 in RFC 2315, containing just certificates
   (no content, signatures, or CRLs), encoded using ASN.1
   distinguished encoding rules (DER), as specified in
   ITU-T X.690.

Yes, I am confused how this works.
 
> but by supporting this module, the server must support all of

This may raise a red flag by the security people. They take crypto
agility serious and there might be very valid reasons to not support
some of the algorithms at a certain point in time. Actually, there is
a difference between supporting (in the sense of implemented) and
enabled (in the sense can be used).

Anyway, this is usually regulated by protocol specifications,
protocols often define mandatory to implement algorithms etc. Is the
keystore going to interfer with this? Or should it better be silent
about what is mandatory?
 
> the certs preinstalled by a browser...

If an application (i.e. a RESTCONF server) trusts some set of
certificates, the certificates in that set become trusted by that
specific application. But without concrete context, isn't it unclear
what is trusting a listed certificate and why?
 
> "data", see the tail-end of the email I sent on July 11.

Aha. Not sure this resolves this (or I looked at the wrong message).

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 20 Jul 2017 19:18:30 +0200

> clearly crypto algorithm identities must be easily extensible.

I looked a bit more and you define

  identity key-algorithm {
    description
      "Base identity from which all key-algorithms are derived.";
  }

plus a bunch of concrete algorithms. draft-ietf-rtgwg-yang-key-chain-24
defines

     identity crypto-algorithm {
       description
         "Base identity of cryptographic algorithm options.";
     }

and then a bunch of concrete algorithms (hashes and symmetric ones).
They also do not expect IANA to maintain things. I would love if
security area people would help us with getting this right, well
perhaps they jump in during secdir review.

/js

---

From: Kent Watsen <[email-masked]>
Date: Thu, 27 Jul 2017 22:37:50 +0000

Hi Juergen,

> perhaps they jump in during secdir review.

FWIW, the two sets of algorithm identities are disjoint.  The ones in
the keystore draft are all public-key algorithms.  As for the key-chain
draft, all but one of the identities are hmac algorithms, with the last
one being for a key derivation function.

It would be best to address this in the WG, for visibility.  I think
that it's possible to request an early secdir review, or maybe we can 
ask about just this concern.  This is a chair-action. 

Kent

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 28 Jul 2017 07:21:27 +0200

> ask about just this concern.  This is a chair-action. 

I all for secdir review for this document. That said, perhaps it is OK
to have the identities as they are and to define a proper IANA
registry later. The price might be that some identities are defined
twice but that would likely not be a significant implementation cost.
Its a trade off between (i) moving fast and maybe requiring to adapt
later or (ii) trying to work everything out while doing the first
version and hence being slow.

/js