Subject: Re: [Netconf] draft-ietf-netconf-server-model-05

From: Kent Watsen <[email-masked]>
Date: Sat, 17 Jan 2015 01:54:22 +0000

Hi Alan,

Another very thorough review - thank you!

Below are responses to your questions.

Thanks
Kent

>Page 6, Section 2.4, typo
>-------------------------
>
>s/mapping to be configured."/mapping to be configured./
>
>(Delete the quotation mark.)

Done, thanks!

>Page 7, Section 2.6.6, possible improper hyphenation
>----------------------------------------------------
>
>The text reads:
>
>   If a persistent connection is desired, it is the responsibility of
>   the connection-initiator to actively test
>
>Not sure, but I think "connection-initiator" should not be hyphenated.

Agreed, removed.

>Page 8, Section 3.1.1, clarification
>------------------------------------
>
>   module: ietf-netconf-server
>      +--rw netconf-server
>         +--rw session-options {session-options}?
>            +--rw hello-timeout?   uint32
>            +--rw idle-timeout?    uint32
>
>
>The meaning of the braces { } around session-options initially was not
>clear.  It appeared to be a typo instead of brackets [ ].  After a lot
>of looking at the usage of the braces, it appears to indicate when the
>"session-options" feature is present?
>
>Perhaps adding something like the following explanation to section 1.2
>(on Pages 4 and 5):
>
>   o  Braces "{" and "}" enclose feature names, and indicate that the
>      named feature must be present for the following part of the data
>      model to be present.

Agreed, added.

>Page 14, Section 3.2, ietf-netconf-server.yang, nit
>---------------------------------------------------
>
>  grouping session-options-container {
>    description
>      "";
>    container session-options {
>
>I suggest removing the empty description.  I think the reason/purpose of
>the grouping is obvious enough that a description clause does not assist
>the reader's understanding.
>
>Many of the grouping statements in the  ietf-netconf-server.yang  module
>and the  ietf-restconf-server.yang  module have empty descriptions.  This
>same comment applies to them also.

The empty descriptions are an annoying artifact of the `pyang --ietf`
option requiring descriptions on these nodes.  But I agree that no
description is needed and that it looks odd as is.  I could copy/paste the
entire description or has one reference the other...hmm, how about this -
look for "(see above)" below:

OLD:

  grouping session-options-container {
    description
      "";
    container session-options {
      description
        "NETCONF session options, independent of transport or
         connection strategy.";

NEW:

  grouping session-options-container {
    description
      "NETCONF session options, independent of transport or
       connection strategy.";
    container session-options {
      description
        "(see above)";

If not, do yo have a better suggestion?

>Page 18, Section 3.2, ietf-netconf-server.yang, nit
>---------------------------------------------------
>
>        container connection-type {
>          description
>           "Indicates the kind of connection to be maintained.";
>          choice connection-type {
>            default persistent-connection;
>
>Possibly change "maintained" to "used"?
>
>Persistent connections might be "maintained"; but "maintaining" a
>periodic connection seems confusing.

Agreed - done.

>Page 19, Section 3.2, ietf-netconf-server.yang, clarification
>-------------------------------------------------------------
>
>                leaf linger-secs {
>                  type uint8;
>                  units seconds;
>                  default 30;
>                  description
>                   "The amount of time the NETCONF server should wait
>                    after last receiving data from or sending data to
>                    the NETCONF client's endpoint before closing its
>                    connection to it.  This is an optimization to
>                    prevent unnecessary connections.";
>                }
>
>In the behavior of the periodic connection (with the 30 second
>linger-secs), 
>what happens if the NETCONF server holds the idle periodic connection
>open 
>for 29 seconds, then another message is exchanged on that connection?
>
>Based upon the description clause, I would expect the message would reset
>a linger timer, so the server would wait another 30 seconds before closing
>the connection.  This behavior is not completely spelled out, though.

Yes, this is the expected behavior - does it need to be spelled out?

>In this scenario, if one of the NETCONF peers repeatedly sends a message
>just before the linger timeout expires, the periodic connection will
>never 
>be closed.  Is this the desired behavior for the periodic connection, or
>would it make sense to have timer that forces a close after a certain
>maximum time period?

Yes, it could turn out that the "periodic" connection stays open forever
due to always having activity.  This is the desired behavior, as the goal
is to avoid the computationally-expensive reconnection.

>This same comment applies to other linger-secs values in the
>ietf-netconf-server.yang  module and the  ietf-restconf-server.yang
>module.

Understood, but I currently don't believe the text is inaccurate.  I'm not
planning to make any changes yet...

>Page 21, Section 3.2, ietf-netconf-server.yang, nit
>---------------------------------------------------
>
>  grouping tls-container {
>    description
>      "";
>    container tls {
>      description
>        "Configures TLS properties not specific to the listen
>         or call-home use-cases";
>
>Perhaps change the description text to:
>
>      description
>        "Configures TLS properties for authenticating clients.";

Agreed - done!

>Are there other possibilities are there other than the listen or call-home
>use cases?

There may be, but your point is that it's better to say what you do rather
than what you don't do, which is spot on.

>Page 23, Section 3.2, ietf-netconf-server.yang, question
>--------------------------------------------------------
>
>  grouping certificates-container {
>    description
>      "";
>    container certificates {
>      description
>        "Parent container for the list of certificates.";
>      leaf-list certificate {
>        type string;
>        min-elements 1;
>        description
>          "An unordered list of certificates the TLS server can pick
>           from when sending its Server Certificate message.  The value
>           of the string is the unique identifier for a certificate
>           configured on the system.  How valid values are discovered
>           is outside the scope of this module, but they are envisioned
>           to be the keys for a list of certificates provided
>           by another YANG module";
>        reference
>          "RFC 5246: The TLS Protocol, Section 7.4.2";
>      }
>    }
>  }
>
>
>What happens if the min-elements is not satisfied?

How is this possible?  A TLS server must have a server-cert defined...

>Also, what is the use case for the server having more than one
>certificate 
>it can send in it Server Certificate message?  How would the server
>choose 
>which certificate to send in its Server Certificate message?  Does this
>description need a comment about "How a certificate is chosen for sending
>in a Server Certificate message is outside the scope of this module"?

The server may have more than one certificate using different algorithms
or key-strengths.  The server may use one certificate for when listening
on port 6513 and another when it calls home.

>These questions also apply to the certificates-container grouping in the
>ietf-restconf-server.yang module.

Understood, but no change planned yet.

>Page 25, Section 3.2, ietf-netconf-server.yang, clarification
>-------------------------------------------------------------
>
>          strategy).  The interval timer is reset after each
>          transmission, thus an unresponsive NETCONF client will
>          be dropped after ~count-max * interval-secs seconds.";
>
>I suggest changing the text to:
>
>          strategy).  The interval timer is reset after each
>          transmission, thus an unresponsive NETCONF client will
>          be dropped after approximately (count-max * interval-secs)
>          seconds.";
>
>Upon a first read of this description, the tilde ~ looked like the
>C-language operator.  I had to think about the text a bit more.
>
>This same comment applies in the ietf-restconf-server.yang module, on
>page 36.

Right you are, this was slang text - fixed.

>Page 26, Section 4.1.1, typo
>----------------------------
>
>The text reads:
>
>   module enables configuration for listening for remote connections, as
>   described in [draft-ietf-netconf-restconf] and
>   [draft-ietf-netconf-call-home].  Feature statements are used to limit
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>Should the text be changed to delete the flagged text above and read
>as follows?
>
>   module enables configuration for listening for remote connections, as
>   described in [draft-ietf-netconf-restconf].

Yes, done.

>Also, the text reads:
>
>   to use (i.e.  SSH, TLS), and the IP address and port to listen on.
>                 ^^^
>
>Should the text be changed to delete the flagged text above and read
>as follows?
>
>   to use (i.e. TLS), and the IP address and port to listen on.

Correct, for RESTCONF, only TLS is supported (copy/paste error)

>Page 27, Section 4.1.2, typo
>----------------------------
>
>The text reads:
>
>   the transport to be used (i.e.  SSH, TLS), and a list of remote
>                                   ^^^
>
>Should the text be changed to delete the flagged text above and read
>as follows?
>
>   the transport to be used (i.e. TLS), and a list of remote

Correct again, for RESTCONF, only TLS is supported (copy/paste error)

>Page 30, Section 4.2, ietf-restconf-server.yang, nit
>----------------------------------------------------
>
>      list endpoint {
>        key name;
>        description
>          "List of endpoints to listen for connections on.";
>        leaf name {
>          type string;
>          description
>            "An arbitrary name for the listen endpoint.";
>        }
>
>Should RESTCONF be added to these descriptions like so?
>
>      list endpoint {
>        key name;
>        description
>          "List of endpoints to listen for RESTCONF connections on.";
>        leaf name {
>          type string;
>          description
>            "An arbitrary name for the RESTCONF listen endpoint.";
>        }

Sure, and I made the comparable change to the ietf-netconf-server.yanf
file as well

>Page 30, Section 4.2, ietf-restconf-server.yang, question
>---------------------------------------------------------
>
>The following snippet of the data model uses "mandatory true;".
>
>        choice transport {
>          mandatory true;
>          description
>            "Selects between available transports.";
>          case tls {
>            container tls {
>
>I have no idea, but should more schema nodes in both the ietf-restconf-
>server.yang module and the ietf-netconf-server.yang module be marked by
>"mandatory true;"?

I think that they are all set correctly...I just checked

>The correct use of the "mandatory true" is unclear to me, based upon
>the description of the "mandatory" statement in sections 3.1, 7.6.5,
>7.9.4, of RFC 6020.  But that is a side issue.

Key fields are implicitly mandatory.  Fields that have default values are
explicitly not-mandatory.   Marking a node mandatory means that end-user
config is required.  For instance, the end-user *must* supply an IP
address to connect to for call-home connections (that's mandatory=true)

>Correctly identifying schema nodes with "mandatory true" seems high-effort
>and relatively low value.  The best way to address this particular
>comment 
>might be to ignore it.

Too late!  ;)

>Page 30, Section 4.2, ietf-restconf-server.yang, question
>----------------------------------------------------------
>
>          case tls {
>            container tls {
>              description
>                "TLS-specific listening configuration for inbound
>                 connections.";
>              uses address-and-port-grouping {
>                refine port {
>                  default 6513;
>                }
>              }
>              uses certificates-container;
>            }
>          }
>
>Is refining the default value for the port to 6513 intentional, or a
>cut-and-paste error?  This is the same port number IANA assigned for
>NETCONF over TLS.

It was.  This was previously fixed here:
https://github.com/netconf-wg/server-model/issues/26

>Page 31, Section 4.2, ietf-restconf-server.yang, typo
>-----------------------------------------------------
>
>        choice transport {
>          mandatory true;
>          description
>            "Selects between SSH and TLS transports.";
>                             ^^^^^^^^^^^
>
>Should the "SSH and TLS" be removed?  Is it envisioned that RESTCONF
>may operate over transports other than TLS, or should this not even
>be a choice because only TLS is supported?

Fixed.  I had purposely left the choice to cover future augmentations.

>Page 36, Section 4.2, ietf-restconf-server.yang, wording
>--------------------------------------------------------
>
>  grouping keep-alives-container {
>    description
>      "";
>    container keep-alives {
>      description
>        "Configures the keep-alive policy, to proactively test the
>         aliveness of the RESTCONF client, in order to know when a
>         new call home connection should be established.";
>             ^^^^^^^^^
>
>The listen-container grouping on Page 30 also uses the
>keep-alives-container,
>so perhaps this description text should not be specific to call home.

Right you are.  Fixed in ietf-netconf-server.yang as well.

>Page 38, Section 6, "Security Considerations", technical
>--------------------------------------------------------
>
>The text reads:
>
>   There are a number of data nodes defined in the "ietf-netconf-server"
>   YANG module which are readable and/or writable that may be considered
>   sensitive or vulnerable in some network environments.  Write and read
>   operations to these data nodes can have a negative effect on network
>   operations.  It is thus important to control write and read access to
>   these data nodes.  Below are the data nodes and their sensitivity/
>   vulnerability.
>
>Then lists a few containers considered security sensitive.
>
>Am I missing something, but shouldn't way more schema nodes than just
>the few listed in section 6 be recognized as security sensitive?
>
>As an example, in the ietf-netconf-server.yang module, aren't the
>following
>nodes security sensitive?
>
>   module: ietf-netconf-server
>      +--rw netconf-server
>         +--rw listen {listen}?
>            +--rw max-sessions?   uint16
>            +--rw endpoint* [name]
>               +--rw name           string
>               +--rw (transport)
>               |  +--:(ssh) {ssh}?
>               |  |  +--rw ssh
>               |  |     +--rw address?     inet:ip-address
>               |  |     +--rw port?        inet:port-number
>               |  |     +--rw host-keys
>               |  |        +--rw host-key*   string
>               |  +--:(tls) {tls}?
>               |     +--rw tls
>               |        +--rw address?        inet:ip-address
>               |        +--rw port?           inet:port-number
>               |        +--rw certificates
>               |           +--rw certificate*   string
>               +--rw keep-alives
>                  +--rw interval-secs?   uint8
>                  +--rw count-max?       uint8
>
>If these nodes aren't properly secured, then couldn't an attacker
>just delete all of the listen endpoints, and disable access to the
>server?  Losing control of the NETCONF server (and presumably the
>network device it runs on) seems like it fits the description of
>adversely affect network operations, and so should be listed under
>security considerations.
>
>The same could be said of the call-home nodes, and probably other nodes.

This issue is addressed here:
https://github.com/netconf-wg/server-model/issues/24

This is a bit tricky.  On one hand, the entire configuration has to be
protected from unauthorized access and modification.  But with NACM, the
issue is notched up a level to cover what *authorized* users are allowed
to access and/or modify.  I'll look at this again when working on #24...

Thanks again,
Kent

---

From: Alan Luchuk <[email-masked]>
Date: Mon, 19 Jan 2015 15:03:29 -0500 (EST)

Hello,

>If not, do yo have a better suggestion?

Leave the empty comments.  No sense in making unnecessary changes.

>planning to make any changes yet...

I appreciate your clarification in this E-mail.  

I agree that leaving the current text unchanged unless someone else has 
the same question is a reasonable course of action.

>How is this possible?  A TLS server must have a server-cert defined...

I'll have to study more about the usage of the YANG min-elements statement.  
For this particular case, I'm not clear on what happens if a NETCONF server 
is provisioned with NO certificates.  In this case, the TLS components can't 
work, but the min-elements constraint can't be satisfied for NETCONF 
retrieval operations either (presumably exchanged over SSH).

>Understood, but no change planned yet.

OK.  Not trying to beat a dead horse, but it still seems like the data model 
would allow many (e.g., 12) certs to be provisioned on the NETCONF server.  
Let's say that all of the certs use the same cryptographic cipher suites and 
key strengths.  The certs only differ in the keys (key pairs).  It seems 
like any of the certs would be equally good for sending in the Server 
Certificate message.  For this situation, I'm still not clear on how the 
NETCONF server would choose one of the certs to send in a Server Certificate 
message.  I would probably write the code so that the server would always 
send the first certificate in an ordered list of some kind, and ignore all 
the subsequent certs.

Have I missed documentation somewhere that describes how the server chooses
the certificate to send?

>I think that they are all set correctly...I just checked

Sounds fine.  Thanks for checking.

>to access and/or modify.  I'll look at this again when working on #24...

OK.

FWIW, my first quick thought about this is that unauthorized configuration 
of any of the data nodes could have a negative effect on network operations.
I'd probably protect write access to all data nodes in the data model. 
Careful consideration of each node might show that some nodes are less 
critical, but the cost/benefit of analyzing each node would be insufficient 
to justify the analysis effort.

Not sure about which nodes should be guarded against read access.  I'd 
probably guard all nodes against read access.  But again, an analysis 
would be a good, but possibly high-effort/low-benefit thing to do.

Regards,
--Alan

 ------------------------------------------------------------------------------
 Alan Luchuk               SNMP Research, Inc.          Voice:  +1 865 573 1434
 Senior Software Engineer  3001 Kimberlin Heights Road  FAX:    +1 865 573 9197
 luchuk at snmp.com        Knoxville, TN  37920-9716    http://www.snmp.com/
 ------------------------------------------------------------------------------

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 19 Jan 2015 21:54:23 +0100 (CET)

> look for "(see above)" below:

pyang --ietf implements the rules defined in RFC 6087.

If you really don't have anything to say, remove the statement and
ignore the error - we do not want unnecessary noise in our modules!

This said, I think that these groupings really should have a proper
description.  Or rather, I'd prefer to not define the groupings at
all, but rather inline their definitions.   But if you do think that
these groupings are reusable, the description should explain what they
are.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 20 Jan 2015 10:51:38 +0100

> pyang --ietf implements the rules defined in RFC 6087.

I think enforcing descriptions via 2119 keywords is inappropriate and,
as in this case, counter-productive. It should be left to the module
author's judgement where to place descriptions. Some kind of general
advice and explanation why descriptions are useful would IMO be more
helpful.

It is often the case that a grouping contains just a single data node,
and then it mostly doesn't make sense to have a description on the
grouping and then on the data node again.

> ignore the error - we do not want unnecessary noise in our modules!

But we also don't want unnecessary noise in stderr - it's not only
annoying but also useful warnings then can get lost.

Lada

> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf

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

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 20 Jan 2015 11:06:42 +0100

> grouping and then on the data node again.

SMIv2 mandated descriptions and while sometimes one could have done
without, I believe the fact that they were simply required was overall
a big win and helped to avoid endless debates. In this particular
case, the description of the grouping should explain what the grouping
is good for and when to use it and the description of the container
should describe what the container is good for. The container and its
description is there after expansion of the grouping, the description
of the grouping goes away with the expansion. In other words, I like
to get a description of the container that says what the container
does without having to worry whether the container was coming from a
grouping or not. So I think both descriptions do actually serve
different purposes and both make sense to have.

/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: Ladislav Lhotka <[email-masked]>
Date: Tue, 20 Jan 2015 11:28:03 +0100

> On 20 Jan 2015, at 11:06, Juergen Schoenwaelder <[email-masked]> wrote:
> 

OK, so let’s take RFC 7407 as an example, of which you are an co-author: out of eight groupings defined in its modules, only *one* has a description. This violates a MUST in RFC 6087.

Lada

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 20 Jan 2015 11:56:44 +0100

> OK, so let’s take RFC 7407 as an example, of which you are an co-author: out of eight groupings defined in its modules, only *one* has a description. This violates a MUST in RFC 6087.

This is why reviews are important.

/js

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 20 Jan 2015 12:32:47 +0100

> On 20 Jan 2015, at 11:56, Juergen Schoenwaelder <[email-masked]> wrote:
> 

Actually, ietf-routing is 6087-compliant, which leads to embarrassing definitions like this:

     grouping address-family {
       description
         "This grouping provides a leaf identifying an address
          family.";
       leaf address-family {
         type identityref {
           base address-family;
         }
         mandatory "true";
         description
           "Address family.";
       }
     }

Lada

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 20 Jan 2015 13:50:56 +0100

>          "This grouping provides a leaf identifying an address

The value of groupings with one leaf can be debated.

But whatever example you will post here, it won't change my general
position that it is better to require descriptions than relying on
authors to provide them when really needed (because then you will then
have to convince authors to write descriptions they do not want to
write because for them everything is of course obvious).

A shortcoming of groupings is that descriptions are kind of forced to
be generic and the more semantically meaningful descriptions are then
lacking. Perhaps I could compromise if a definition in a grouping
without a description MUST have a description added in a refine
statement of the uses statement, that is if there is

  uses address-family {
    refine address-family {
      description
	"The address family related to ABV and used to do XYZ.";
    }
  }

Anyway, this is NETCONF writing a YANG 1.0 model using existing RFCs
so this is off topic I think.

/js

---

From: Andy Bierman <[email-masked]>
Date: Tue, 20 Jan 2015 08:08:32 -0800

>          "This grouping provides a leaf identifying an address

This is a poor example of a description-stmt.

Consider the possibility that tools can extract the descriptions
and display them to users in a context-specific manner.
They may not be read from a YANG file.

Consider the possibility that the term "address family" is
generic and not that helpful.

IMO either a description-stmt or reference-stmt MUST be present
in published YANG modules.

> Lada

Andy

---

From: Kent Watsen <[email-masked]>
Date: Tue, 20 Jan 2015 16:46:37 +0000

On 1/20/15, 5:06 AM, "Juergen Schoenwaelder"
<[email-masked]> wrote:

>In this particular

Thank you Juergen - this makes good sense to me.

I'll revise the description statements in the server-model accordingly.

Thanks again,
Kent

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 20 Jan 2015 17:53:34 +0100

> On 20 Jan 2015, at 17:08, Andy Bierman <[email-masked]> wrote:
> 

That’s because the leaf is in a grouping, so it is in fact generic - it is then used in different contexts. Actually it might make more sense to add a description to each “uses” statement.

For a human reader, repeating essentially the same information in several places (grouping, data node, typedef) just adds clutter to the module.

Lada

---

From: Kent Watsen <[email-masked]>
Date: Sat, 24 Jan 2015 01:14:14 +0000

Hi Alan,

>>description is needed and that it looks odd as is.

I added description for the grouping statements per Juergen's suggestion.

This issue was tracked here:
https://github.com/netconf-wg/server-model/issues/29.

The diff for the change is here:
https://github.com/netconf-wg/server-model/commit/fe5186bc4fcfd7db84220bfc0
f8d725124e8435c

FWIW, the above diff also address issue #30
(https://github.com/netconf-wg/server-model/issues/30), which adds a
description for braces to the tree-diagram section, and other edits per
your suggestions.

>retrieval operations either (presumably exchanged over SSH).

If the concern is for edit operations, isn't the answer that a server with
no certificates would then exclude the "certificates" parent container?  -
note that it is not marked mandatory true...

>Have I missed documentation somewhere that describes how the server

Per RFC 5246, the client sends cipher_suites in its ClientHello.  Then the
server selects one of the ciphers in its ServerHello (presumably the first
one matching it has a suitable certificate for) and then, as you say, it
sends its server certificate in its Certificate message.

Your example says all the certs have the same cipher suite - in that case,
I agree it makes no sense to configure more than one cert in the list.

>would be a good, but possibly high-effort/low-benefit thing to do.

You're probably right about this - I'll keep this in mind when working on
issue #24

Thanks again,
Kent