Subject: [Netconf] mbj review of draft-ietf-netconf-server-model-06

From: Martin Bjorklund <[email-masked]>
Date: Tue, 17 Mar 2015 15:27:56 +0100 (CET)

Hi,

Here is my WGLC review of draft-ietf-netconf-server-model-06.

o  I suggest you create a terminology section.

   The term "application" is still used w/o being defined.  Also, the
   YANG model has this:

      list application {
        key name;
        description
          "List of NETCONF clients the NETCONF server is to initiate
           call-home connections to.";
        leaf name {
          type string;
          description
            "An arbitrary name for the remote NETCONF client.";
        }

  Which to me seems as if the term "application" is equivalent to
  "NETCONF client", but I don't think that's the intention.  (Or is
  it?  If so, why do we have two terms for the same thing?)

o  2.6.7: s/as transpired/has transpired/

o  3.1.2  has a reference to 6242; this should probably reference 6242
   and the tls document

o  3.1.2: s/Feature statements/"feature" statements/

o  This is the first of our modules to use import-by revision.
   Esp. since it's meaning is at best underspecified in 6020, I
   suggest you remove the revision-dates.

o  For feature "ssh" and "tls":

   Update the description text to:

     "The ssh feature indicates that the server supports the
      SSH transport protocol for NETCONF.";

   and similar for the tls text.

o  I and others have commented before that the usage of groupings in
   this module makes the model more difficult to read.  I still think
   this is true.  It is very difficult to see the structure in this
   module.   Most groupings in this module are described as:

      "This grouping is used only to help improve readability
       of the YANG module.";

   I suggest you remove these groupings - to improve readability ;-)

o  The other groupings are described as:

      "This grouping is used by both the ssh and tls containers.";

   This doesn't tell the reader what the grouping does.

o  The hello-timeout has this range:

          range "0 | 10 .. 3600";

   Why not allow values 1..9?  This seems like an arbitrary range to
   me.

   Also, maybe we should discuss an alternative design for this leaf,
   and other similar leafs.

    type union {
      type uint32 {
        range "1..3600";
      }
      type enumeration {
        enum "infinity";
      }
    }

  I don't know which design pattern is best.

o  The hello-timeout description talks about the "hello PDU".  This
   should be "hello message".

   The text also mentions "hello-wait" state.  There is no such
   defined state - although I do understand what you mean.  I suggest
   you simply remove the last part of the sentence.

   Also, the text talks about denial of service attacks; maybe you
   should mention this leaf in the Security Considerations section.

o  list endpoint / choice transport

   The text says:

            "Selects between SSH and TLS transports."

   I suggest:

            "Selects between available transports.";

   (this text is already in the call-home transport choice)

o  call-home / application / connection-type

   This choice has "persistent-connection" as the default.  Why is
   this the default?  Wouldn't it be better to not have a default, and
   possibly make the choice mandatory.

o  call-home / application / connection-type

  The call-home draft mentions this objective:

   o  The network element may proactively call home after being powered
      on for the first time in order to register itself with its
      management system.

  How is this use case supported in this model?

  It also mentions:

   o  The network element may access the network in a way that
      dynamically assigns it an IP address and it doesn't register its
      assigned IP addressed to a mapping service, thus complicating the
      ability for a management system to connect to it.

  How is this use case supported in this model?

o  call-home / application / connection-type / persistent-connection

  By default, if we configure a call-home "application", we have a
  persistent-connention with keep-alives being sent on the transport
  layer ever 15 seconds.  But we also have the idle-timeout that works
  on the messages layer default to 3600 seconds.

  So it seems to me that unless there is NETCONF traffic, the
  persistent connection will be closed by the idle-timeout after 3600
  seconds.  But since the connection was persistent, the device will
  immediately re-establish the connection, and so on.

o  reconnect-strategy / start-with

   The description says:

                   NETCONF servers
                   SHOULD support this flag across reboots.";

   "support this flag" is a bit vague.  I think you mean that servers
   SHOULD keep track of the last connected client in non-volatile
   storage.

o  reconnect-strategy / count-max

  Would "max-attempts" be a better name than "count-max"?

o  ordered-by system

  Some lists have an "ordered-by system" statement.  Since this is the
  default, I suggest you remove it.

o  call-home / application / connection-type / periodic-connention

   The leaf timeout-mins is of type uint8.  Isn't this a bit small?  I
   think this should be uint16.

   Also, the text says that a device "should" pro-actively connect to
   the client if it has messages to send.  Maybe this is is good
   enough, but in a deployment with *lots* of devices, maybe you want
   to suppress such messages to a time window when the client is
   prepared to handle it.  Again, maybe this use case is better done
   with a new type of connection-type ("absolute-time" or something),
   and such a container can be augmented later.  I just wonder if you
   thought about this use case.

o  tls / client-auth

   Why do we need both trusted-client-certs and cert-maps?

   Also, I think the text doesn't describe how the "trusted-ca-certs"
   leaf-list should be used.  The cert-to-name list has this:

         2) If the cert-to-name list entry's fingerprint value
            matches that of a locally held copy of a trusted CA
            certificate, [...]

   So I *think* the intention is that the configured
   "trusted-ca-certs" leaf list is supposed to provide these "locally
   held copy".

   Also, is it a bug or a faeture that we have two separate leaf-lists
   of trusted-ca-certs, one for ssh and one for tls?

o  endpoints address / address

  s/The hostname or IP address or hostname/
    The hostname or IP address/

o  container keep-alives

  s/Section 4/Section 5/

o  keep-alives / interval-secs

  s/a message/a transport-protocol specific message/

  Also, is the word "NETCONF client" correct here?  Ths point is that
  the keep-alive message in on the transport layer.

o  keep-alives / count-max

  Would "max-attempts", or "retries" be better names than
  "count-max"?

  The description mentions "The interval timer", but no such timer has
  been defined.

o  Appendix A.2

  The prefix "x509c2n" is not defined.

  When I fixed that, the example valdiates.

o  I have validated the examples in A.1 and A.3.

I plan to implement the data model in this document for NETCONF over
SSH, and RESTCONF over TLS "soon".

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 17 Mar 2015 16:09:44 +0100

>   I don't know which design pattern is best.

If we adopt this pattern, we should probably make this

   enum "infinity" { value 0; }

in order to be explicit about the numeric value assigned to this
enum via auto-numbering.

/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: Thu, 19 Mar 2015 23:04:52 +0000

Martin,

Thank you for your review.  I'll make the changes you recommended.

>   Why not allow values 1..9?  This seems like an arbitrary range to

This YANG snippet came from Andy.  I'm not sure why 1-hour was picked.

>  How is this use case supported in this model?

The zerotouch draft discusses how a network element can obtain an initial
configuration during factory-default power on.  Example A.3 in that draft
uses 
the module defined in this draft.  The server-model enables NETCONF Call
Home to be configured, that's all.  Is there a specific change in the text
you're hoping to see in this draft?

>  How is this use case supported in this model?

This bullet point is supported by the NETCONF Call Home, by the device
initiating the TCP connection.  That is, the solution does not rely on
the network element's IP address in any way, it could be static, dynamic,
NAT-ed, or whatever.  The server-model merely enables NETCONF Call Home
to be configured, but doesn't realize this benefit itself.  Is there a
specific change in the text you're hoping to see in this draft?

>   think this should be uint16.

Perhaps.  256 minutes is a little more than 4 hours, which in my world
seems like a crazy amount of time for a device to not be connected to its
management system.  That said, I grant you that there are likely some
elements that don't call home but once a day, or maybe even longer.
Further, it's no effort to change to a uint16, so why not?

But I wonder, would it be better to have a 2-tuple whereby one value is a
number and the other is an enum for minutes, hours, and days?

>   thought about this use case.

I think you mean "to a time window when the *server* is prepared to handle
it."

The thinking behind this is that the message could be a fault or security
alert that should be delivered with haste, without regards for the
data-collection system's scalability.  That said, I can easily support the
idea of the device buffering low-priority messages and perhaps high-
priority messages too, if that's how it's been designed or configured
- so be it.

Perhaps the SHOULD should be a MAY?  The point is to call out that it's
possible for a device to connect sooner than it's periodic timeout would
have it do.  It's one thing to connect just in case the NETCONF client
has pending messages for the device, and yet another for if the device
itself have something to send.  Maybe there should be two timeouts - one
for each of these cases?  Or nix the whole idea with the assumption the
device has a replay-buffer that the NETCONF client can pull previous
messages from?

>   Why do we need both trusted-client-certs and cert-maps?

Trusted-client-certs (and trusted-ca-certs) is used by the TLS-layer to
validate the client's certificate.  Separately, cert-maps is used by the
NETCONF/RESTCONF layers to extract a username from the client's
certificate.  Makes sense?

>   "trusted-ca-certs" leaf list is supposed to provide these "locally

Agreed.  This text was inherited from 5539bis, but looking at it now, it
also doesn't look right to me.

>   of trusted-ca-certs, one for ssh and one for tls?

It was meant to maximize flexibility, but I don't think it makes sense for
the netconf server to differentiate trusted CA or client-certs by
transport protocol.  Do you agree?

>  the keep-alive message in on the transport layer.

Unsure.  On one hand, each NETCONF message is a transport-level message
but, on the other hand, it is technically a transport-level message that
matters.   Would you rather see "SSH/TLS client"?

>SSH, and RESTCONF over TLS "soon".

Exciting.  Hopefully one more round of edits will stabilize the model.
I will update the reference code on Github to support the final model
as well.

Thanks,
Kent

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 20 Mar 2015 16:01:09 +0100

> number and the other is an enum for minutes, hours, and days?

I do not think that a 2-tuple makes the _programmatic_ interface
easier.

/js

---

From: Per Hedeland <[email-masked]>
Date: Fri, 20 Mar 2015 16:32:19 +0100

> I do not think that a 2-tuple makes the _programmatic_ interface

Maybe ietf-yang-types should have something along the lines of
xsd:duration (http://www.w3.org/TR/xmlschema-2/#duration)?

--Per Hedeland

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 20 Mar 2015 16:57:21 +0100

> xsd:duration (http://www.w3.org/TR/xmlschema-2/#duration)?

Yes, this might be a reasonable way to go if we want to move away from
a number with a fixed unit.

/js

---

From: Martin Bjorklund <[email-masked]>
Date: Sun, 22 Mar 2015 10:21:56 -0500 (CDT)

> you're hoping to see in this draft?

If all that is needed in this case is inital call-home, how would the
call home feature be used in this case?  I assume it could be set up
to do a persistent connecttion, and one the management application
receives this initial "registration", it can reconfigure the device to
not have call home enabled.

> specific change in the text you're hoping to see in this draft?

It seems to me that neither persistent nor periodic call home would be
correct for this case.  Wouldn't the device call home after start up,
and after that let the client initiate future sessions as needed?  And
the client cannot disable call home like above, b/c if it gets another
ip address after a reboot, the call home is necessary.  Do we need a
new connection-type for this?

> seems like a crazy amount of time for a device to not be connected to its

I am thinking about a use case like TR-069, where there are *lots* of
CPEs and they don't have to be connection very often.

> I think you mean "to a time window when the *server* is prepared to handle

No I meant the client.  Maybe the client needs to spread out the
connections from the servers in order to scale.

> have it do.

Ok.

> It's one thing to connect just in case the NETCONF client

I think the MAY solves the issue.  More elaborate connection schemes
can be added in the future.

> certificate.  Makes sense?

This could be made more clear.  The description for trusted-certs says

      "A list of client certificates that a NETCONF server can use
       to authenticate a NETCONF client's certificate".
          ^^^^^^^^^^^^

But maybe this whole thing goes away if certificates are not stored in
this data model at all, just like the host keys; see Juergen's
comment.

> transport protocol.  Do you agree?

Yes.

> matters.   Would you rather see "SSH/TLS client"?

I think so, yes.

/martin