Subject: [yang-doctors] YANG doctor review of draft-ietf-rtgwg-yang-key-chain-13

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Feb 2017 15:01:01 +0100

Hi,

I was assigned to be the YANG doctor for this document. Here is my
review:

**** General comments

***** Configuration and state data
 
      The module "ietf-key-chain" represents state data in a way that
      departs from the convention that has been used so far in most
      IETF modules, i.e. separate configuration and state data
      trees. I know this is a controversial topic but I also won't
      hide that I prefer the "traditional" arrangement. In any case,
      this data model organization clutters the output of the NETCONF
      "get" operation. I assume in most (all?) cases the "*-state"
      parameters will be the same as the configured value.

***** Cryptographic algorithm types

      What is the reason for representing these as a YANG choice with
      empty leaves? I think it would be more natural to use a single
      leaf, either an enumeration or (if extensibility is important)
      identityref.

***** Reusability

      The module defines key-chain as a grouping with the aim of
      making it reusable in other modules. However, this approach has
      known problems that are discussed in
      draft-ietf-netmod-schema-mount. I am not sure how relevant they
      are in this case but, for one, the "key-chain-ref"
      type is not applicable if the "key-chain" grouping is used in
      another module. An alternative is not to use the grouping and
      rely on schema mount.

***** Key string style

      The difference between ASCII and hexadecimal
      formats of key strings should be explained. I understand that
      the latter is a hash of the key and, if so, I'd suggest to
      include "hexadecimal-string" also in state data.

      Also, I believe that storing clear-text key in configuration is
      insecure and Security Considerations should warn against it.

***** Example

      It might be useful to include an appendix with example instance data.

**** Specific comments
  
***** Sec. 2

     - paragraph 2: s/where ever/wherever/

***** Sec. 3

     - paragraph 1: replace both Key-Id a Key-ID with Key ID (the
       latter is used in other places of the text).

     - paragraph 2: the suggested way of supporting assymetric keys
       looks like a hack, I would suggest a more explicit
       representation, e.g. using a choice.

***** Sec. 4

     - The module has inconsistent indentation: up to "grouping
       crypto-algorithm-types", top-level statements are indented with
       four spaces, the subsequent ones with five spaces.

***** Sec. 6

      - The statement "Given that the key chains themselves are
        sensitive data, it is RECOMMENDED that the NETCONF
        communication channel be encrypted." is misleading because RFC
        6241 requires that transport protocols for NETCONF guarantee
        confidentiality (and RFC 8040 does the same for RESTCONF).

Lada

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 13 Feb 2017 15:33:06 +0100 (CET)

Hi,

I had reasons to read this document the other day, and I have some
additional comments:

o  The revision history should be trimmed to just a single entry.  The
   idea is to only keep revision only for published versions (in this
   case RFC versions).

o  Lists should be named with the singular form, so "list
   key-chain-entries" should be "key-chain-entry".  But I wonder about
   the terminolgy; the draft text says:

     A key chain is a list of elements each containing a key

   so I think the YANG names should reflect this terminology.  Thus I
   suggest that "key-chain-list" is renamed to "key-chain" and
   "key-chain-entry" to "key".   If you agree, the top-level container
   should probably be renamed to "key-chains" (and "key-chains-state")
   also.

>       parameters will be the same as the configured value.

But this module does have two separate top-level trees, /key-chain and
/key-chain-state.  What do I miss?

>        four spaces, the subsequent ones with five spaces.

I suggest you run "pyang -f yang --yang-canonical" on your module.  It
will produce a module that is consistent with our IETF modules in
terms of indentation and module layout.

/martin

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 13 Feb 2017 14:46:47 +0000

Hi Lada, 

Thanks for the review, an initial read indicates some good comments. As
editor, I will work with my co-authors to respond to them. However, see
one question below.

>      parameters will be the same as the configured value.

Are you sure you are looking at the right version of the draft? We went
back and forth several times on this and the -13 has a separate key-chain
and key-chain-state data node at the top level as you suggest.

Thanks,
Acee

---

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Feb 2017 16:07:46 +0100

> On 13 Feb 2017, at 15:33, Martin Bjorklund <[email-masked]> wrote:
> 
> Hi,
> 

I think that at least in some cases, e.g. if a module stays relatively long in the draft stage, it makes sense to record all revisions so as to be able to tell them apart.

6087bis says:

   It is not required to keep the full revision history of draft
   versions (e.g., modules contained within Internet-Drafts).  That is,
   within a sequence of draft versions, only the most recent revision
   need be recorded in the module.

So it doesn't preclude recording "intermediate" revisions. I would personally leave it to each author's discretion.  

>   should probably be renamed to "key-chains" (and "key-chains-state")

I agree.

> /key-chain-state.  What do I miss?

Hmm, I don't know what I saw, maybe I got confused by the groupings. I am sorry for the noise, please ignore this item.

Thanks, Lada

---

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Feb 2017 16:09:38 +0100

> On 13 Feb 2017, at 15:46, Acee Lindem (acee) <[email-masked]> wrote:
> 
> Hi Lada, 
> 

Sure, see my response to Martin - I must have suffered from a mental blackout. :-)

Sorry for that, Lada

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 13 Feb 2017 16:27:22 +0100

> So it doesn't preclude recording "intermediate" revisions. I would personally leave it to each author's discretion.  

This must be new text, I liked RFC 6087 more; I do not want to know
about all the intermediate editing steps someone did, regardless how
long it did to finish the editing. Since we ask module authors to
update the revision date on intermedia Internet-Draft versions, there
is also no ambiguity.

/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: Kent Watsen <[email-masked]>
Date: Mon, 13 Feb 2017 16:31:35 +0000

> update the revision date on intermedia Internet-Draft versions, there

I agree.  Having the intermediate revisions listed at all was only intended for while a draft was work in progress.  The final RFC module's history should only reflect other published modules.

BTW, Acee, I noticed in Section 2:

   The module name was change from ietf-key-chain to ietf-routing-key-
   chain to avoid disambiguate it from the ietf-system-keychain module
   defined in [NETCONF-SERVER-CONF].  However, due to popular demand,
   the module name has been restored to simply ietf-key-chain.

Please note that the NETCONF keychain model has since been renamed and pulled out into its own draft (draft-ietf-netconf-keystore), since folks remained confused after your draft didn't change its name.   No worries, "keystore" is a more apt name for my draft's models anyway.

However, with the new names now being sufficiently different, I think that the naming history in the paragraph above can be removed.  That said, it would still be useful for your draft to point readers to the other "key chain" draft, in case they land on yours by accident, or are otherwise interested in seeing how keys are handled there.

Kent

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 13 Feb 2017 17:23:00 +0000

Hi Kent, 

On 2/13/17, 11:31 AM, "Kent Watsen" <[email-masked]> wrote:

>I agree.  Having the intermediate revisions listed at all was only
>intended for while a draft was work in progress.  The final RFC module's
>history should only reflect other published modules.
>
>Please note that the NETCONF keychain model has since been renamed and
>pulled out into its own draft (draft-ietf-netconf-keystore), since folks
>remained confused after your draft didn't change its name.   No worries,
>"keystore" is a more apt name for my draft's models anyway.
>
>However, with the new names now being sufficiently different, I think
>that the naming history in the paragraph above can be removed.

Sure. 

>That said, it would still be useful for your draft to point readers to
>the other "key chain" draft,

The other draft is not a ³key chain² draft and I was happy to see the
misnomer corrected.

>in case they land on yours by accident, or are otherwise interested in
>seeing how keys are handled there.

I don¹t feel this is necessary.

Thanks,
Acee

---

From: Kent Watsen <[email-masked]>
Date: Mon, 13 Feb 2017 19:45:08 +0000

> The other draft is not a ³key chain² draft and I was happy to see the

I think you're overstating a router-centric view.  As explained before,
both OSX and Linux (and maybe more) have "keychain" utilities that the
other draft's YANG module is modeled after. 

> I don¹t feel this is necessary.

But it would be helpful.

K.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 13 Feb 2017 20:11:50 +0000

>other draft's YANG module is modeled after.

I understand that you feel strongly that your work is very important but,
like the basic routing protocols, there are network devices other than
routers that will implement protocol authentication based on key-chains.

>> I don¹t feel this is necessary.

I disagree and believe this would set a bad precedent. Can you point to
other RFCs that have references to other documents in case readers were
looking for those documents rather than the document itself? Were you
planning on adding references in the keystore model to other documents
that readers may have been searching for?

Thanks,
Acee

---

From: Kent Watsen <[email-masked]>
Date: Mon, 13 Feb 2017 20:29:59 +0000

>routers that will implement protocol authentication based on key-chains.

You misunderstand, this has nothing to do with me.  It's simply a fact that
"key chain" means different things to different groups of people.

>that readers may have been searching for?

The keystore draft already has an informative reference to your draft.

K.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 13 Feb 2017 20:46:35 +0000

>The keystore draft already has an informative reference to your draft.

I looked at the text and reference in the keystore draft and I still don’t
feel that this will be important once the two documents are published.

Acee

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 13 Feb 2017 23:02:04 +0100

> Please note that the NETCONF keychain model has since been renamed and pulled out into its own draft (draft-ietf-netconf-keystore), since folks remained confused after your draft didn't change its…

As said before, the module names I have seen so far to not indicate
very well the difference between these two models and where they are
applicable.

/js

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Wed, 15 Feb 2017 17:45:10 +0000

Hi Lada, 

I¹ve discussed your comments with my co-authors and we are incorporating
most of them. See inline.

>      parameters will be the same as the configured value.

Already discussed. 

>      leaf, either an enumeration or (if extensibility is important)

We are moving to identities.

>      another module. An alternative is not to use the grouping and

We will collapse unneeded levels of grouping. However, note that using
grouping is a common YANG model practice (e.g., RFC 8022). We¹ll also
remove the statement regarding the top level usage of key-chain
from the text.

>      include "hexadecimal-string" also in state data.

The hexadecimal option is not a hash. Rather, it a specification option
that allows more entropy (256 hex unique hex digits versus printable ASCII
characters). We¹ll explain this in the text.

>      insecure and Security Considerations should warn against it.

I¹ll add this. 

>      It might be useful to include an appendix with example instance

Helen has provided some examples.

>     - paragraph 2: s/where ever/wherever/

Fixed. 

>       latter is used in other places of the text).

Fixed. 

>       representation, e.g. using a choice.

I hate to add multiple ways to do the same thing - especially when the
more general specification has been in use for 15-20 years.

>       four spaces, the subsequent ones with five spaces.

We will fix. 

>        confidentiality (and RFC 8040 does the same for RESTCONF).

We will rewrite since NETCONF is always encrypted.

Thanks,
Acee

---

From: Jeff Tantsura <[email-masked]>
Date: Wed, 15 Feb 2017 17:49:32 +0000

Fantastic!
Thanks both Lada and Acee for quick resolution!

Regards,
Jeff

> On Feb 15, 2017, at 17:45, Acee Lindem (acee) <[email-masked]> wrote:
> 
> Hi Lada, 
>

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Wed, 15 Feb 2017 20:48:00 +0000

Hi Martin, 

On 2/13/17, 10:07 AM, "Ladislav Lhotka" <[email-masked]> wrote:

>I think that at least in some cases, e.g. if a module stays relatively
>long in the draft stage, it makes sense to record all revisions so as to
>be able to tell them apart.
>
>6087bis says:
>
>So it doesn't preclude recording "intermediate" revisions. I would
>personally leave it to each author's discretion.

I think now that we are close to publication, removing them is a good
idea. 

>>   key-chain-entries" should be "key-chain-entry".

Ok. 

>>  But I wonder about
>>   "key-chain-entry" to "key".

You need to read the rest of the sentence to see that there is a list of
associated items in the key-chain-entry. It would incorrect to refer to
the aggregate as simply a key.

>>If you agree, the top-level container

I have made this change.

Thanks
Acee 

>Hmm, I don't know what I saw, maybe I got confused by the groupings. I am
>sorry for the noise, please ignore this item.
>
>Thanks, Lada
>
>> 
>>> ***** Sec. 4
>>>

---

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 16 Feb 2017 08:58:59 +0100

Hi Acee,

with the indicated changes, I believe the document is ready to be
published. A few comments are inline.

> remove the statement regarding the top level usage of key-chain

Groupings are good for relatively small sets of reusable parameters but
not so much for making the whole module "relocatable". And, of course,
they make the data model more difficult to read.

> characters). We¹ll explain this in the text.

I see, but you don't have any length restriction on either keystring or
hexadecimal-string, so both can be made arbitrarily strong, no?

> more general specification has been in use for 15-20 years.

It is up to you to decide, I just find it cleaner to say explicitly that
a key is not to be used for sending rather than make the send-lifetime
interval empty.

But I know from another context (DNS) that admins are often fond of
peculiar hacks they have been using for ages, and reluctant to abandon
them even if the new solution is arguably better.

Thanks, Lada

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 16 Feb 2017 08:59:28 +0100 (CET)

Hi Acee,

I saw the new revision, thank you for addressing my comments.

One comment inline.

> >> On 13 Feb 2017, at 15:33, Martin Bjorklund <[email-masked]> wrote:

[...]

> the aggregate as simply a key.

Ok, this makes sense.  The YANG module now uses the term "key entry"
for this.

> >>   should probably be renamed to "key-chains" (and "key-chains-state")

For consistency with other models, and maybe simpler mapping between
the config and state, you may want to remove the "-state" suffix on
the descendant nodes from "key-chains-state".  Specifically, change:

  list key-chain-state

to

  list key-chain

and

  list key-entry-state

to

  list key-entry

Then we can talk about a configured "key chain" vs the operationally
uses "key chain".

/martin

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 16 Feb 2017 15:42:45 +0000

Hi Martin, 

>Ok, this makes sense.  The YANG module now uses the term "key entry"

Actually, I reversed this and went with the shorter identifier since it is
unambiguous. I also went through the text and changed to this terminology.

>the descendant nodes from "key-chains-state".  Specifically, change:

I made this change as well. The only place the ³-state² suffix is present
is at the top level keychains-state.

Thanks,
Acee

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 16 Feb 2017 15:57:50 +0000

Hi Lada, 

>they make the data model more difficult to read.

I’ve collapsed some additional groupings that are not used for common
reuse in the -15 revision that I will post today.

>hexadecimal-string, so both can be made arbitrarily strong, no?

An implémentation would definitely impose a length restriction. It seems
that the IETF models, in general, do not impose restrictions.  What is the
policy here? 

>a key is not to be used for sending rather than make the send-lifetime

Maybe I’ve just worked with key-chains too long but this seems intuitive
enough to me. 

There is also an indication in the operational state whether a key is
active for sending or acceptance.

Thanks,
Acee

---

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 16 Feb 2017 17:17:30 +0100

> On 16 Feb 2017, at 16:57, Acee Lindem (acee) <[email-masked]> wrote:
> 
> Hi Lada, 
> 

OK.

> that the IETF models, in general, do not impose restrictions.  What is the

I'd suggest to include minimum length for both in this module, and implementations can specify maximum supported length through a deviation if needed.

> active for sending or acceptance.

OK.

Lada