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

From: Alan Luchuk <[email-masked]>
Date: Thu, 15 Jan 2015 17:56:40 -0500 (EST)

Hello,

I have fairly carefully read  draft-ietf-netconf-server-model-05.txt,  and
have the comments included below.  I hope the comments are helpful/useful
to the NETCONF WG.  

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/
 ------------------------------------------------------------------------------

Page 6, Section 2.4, typo
-------------------------

s/mapping to be configured."/mapping to be configured./

(Delete the quotation mark.)

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.

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.

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.

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.

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.  

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? 

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

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

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

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?

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

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

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.

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

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.

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

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

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;"?

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.

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.

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.

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?

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.

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.