Subject: [Netconf] review of draft-ietf-netconf-netconf-client-server-04

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 4 Aug 2017 18:08:48 +0200

Here is my review of draft-ietf-netconf-netconf-client-server-04.

Some comments may also apply to
draft-ietf-netconf-restconf-client-server-04.

- Introduction

  I think the document should reference RFC 6242 (NC over SSH) and RFC
  7589 (NC over TLS).  I also like the inline citation style more than
  the extra paragraph pointing to the RFCs and I am not sure it is
  necessary to reference the TLS and SSH specifications directly given
  that the SSH and TLS specifics are dealt with in another document.

  OLD

   This document defines two YANG [RFC7950] modules, one module to
   configure a NETCONF client and the other module to configure a
   NETCONF server.  Both modules support both the SSH and TLS transport
   protocols, and support both standard NETCONF and NETCONF Call Home
   connections.

   NETCONF is defined by [RFC6241].  SSH is defined by [RFC4252],
   [RFC4253], and [RFC4254].  TLS is defined by [RFC5246].  NETCONF Call
   Home is defined by [RFC8071]).

  NEW

   This document defines two YANG [RFC7950] modules, one module to
   configure a NETCONF [RFC6241] client and the other module to
   configure a NETCONF server.  Both modules support both NETCONF over
   SSH [RFC6242] and NETCONF over TLS [RFC7589] and NETCONF Call Home
   connections [RFC8071].

- endpoint list and choices and other inherited comments

  See my discussion in my restconf-client-server comments; I think the
  proper construction is to have a list of endpoints where each
  endpoint is contains a choice. There are more comments that equally
  apply to the netconf-client-server model so whatever the resolution
  is, we need to apply the changes consistently.

- section 2.3

  The statement about the imports before the module definition is
  incorrect.

- copyright and authors

  The copyright year and the authors need to be aligned.

- example addresses

  The examples should use IANA reserved example IP addresses instead
  of 11.22.33.44, potentially including some IPv6 addresses.

- section 3.3

  The statement about the imports before the module definition is
  incorrect.

- hello-timeout

  Do servers really wait 600 seconds (= 10 minutes) by default for a
  hello message? There is also a comment whether this should be
  removed altogether. I would be in favour of removing this. We also
  do not care about how long the SSH layer is waiting for the hello
  string or how long the TLS layer is waiting for TLS records to flow.
  If needed, this can be added later.

- design considerations

  It needs to be decided what to do with this. Either this becomes a
  (short) complete list of objectives (and then we would need the same
  for the twin RESTCONF document) or we drop this. In general, I am a
  fan of having objectives spelled out at the beginning since this
  makes it clear what to expect. Otherwise, there is just a ton of
  YANG.

- security considerations

  There is a warning that max-sessions != 0 may be exploited for a
  denial of service. So how useful is max-sessions to have in the
  first place?

  Remove the boilerplate text about RPCs since there are no
  RPCs. Replace the other NONE markers will a useful discussion of the
  subtrees.

- tree diagrams outdated

  The tree diagrams list the choice :(password) which is not anymore
  in the YANG definitions, so apparently the tree diagrams are
  outdated.

/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, 14 Aug 2017 21:16:48 +0000

Hi Juergen,

>   SSH [RFC6242] and NETCONF over TLS [RFC7589] and NETCONF Call Home

Better - thanks for the text.

>  is, we need to apply the changes consistently.

I moved everything to the "list with embedded choice" structure before,
to both drafts, so we have consistency.

>  The statement about the imports before the module definition is

Fixed.

>  The copyright year and the authors need to be aligned.

Copyright fixed before.  Same comment regarding authors as before.

>  of 11.22.33.44, potentially including some IPv6 addresses.

Fixed.

>  The statement about the imports before the module definition is

Fixed.

>  If needed, this can be added later.

Removed.  It seems like the kind of thing that could be a local
implementation decision.  It would be a very rare thing to need
to customize the server's default hello-timeout value. 

>  makes it clear what to expect. Otherwise, there is just a ton of

Right, this is the issue Tom raised.  It seems like you both prefer to
keep it and/or have something before all the YANG starts.  As you point
out, the same should be in its twin document.  Do you want to take a
shot at shortening it?

>  denial of service. So how useful is max-sessions to have in the

It's important to production servers to constrain resources.
That said, we could debate if it needs to be a configurable 
value.  Another fix to the security-consideration text is 
either remove '0' as an option or re-define '0' to mean 
"use system preference".  Thoughts?

>  RPCs. Replace the other NONE markers will a useful discussion of the

This is being discussed in another thread.

>  in the YANG definitions, so apparently the tree diagrams are

Searching the netconf-client-server draft, there are four matches for
the string "password", all within the tree diagrams (as you say). This
password field is coming from the ssh-client-grouping (from draft-ietf-
netconf-ssh-client-server).  Everything seems correct to me.  Can you
be more specific?

Thanks,
Kent

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 15 Aug 2017 08:30:10 +0200

> "use system preference".  Thoughts?

The question is whether a simple minded max-session really helps since
it opens the floor to new attacks (and I note that it defaults to no
limit anyway). Anyway, I assume 'session' here means an established
NETCONF session, so the client must have successfully authenticated.
Perhaps my interpretation of what 'session' means is wrong and you
mean the max number of established TCP connections? But then the
denial of service potential is even worse.

> netconf-ssh-client-server).  Everything seems correct to me.  Can you

OK. Looks like I overlooked this.

/js

---

From: Kent Watsen <[email-masked]>
Date: Mon, 21 Aug 2017 17:13:12 +0000

Hi Juergen,

Now we're down to just this last open discussion point.

> denial of service potential is even worse.

I think that this field came from what YumaPro supports, hopefully
Andy chimes in.  Just the same, the intent is for authenticated 
sessions (standard TLS timeout and hello timeout should shed idle
TCP and TLS sessions respectively).

To be clearer here, we're discussing a very benign form of DoS; as
you say, the clients are authenticated.  The issue would manifest 
itself when clients are somehow malfunctioning, not closing sessions
as expediently as they should.

K.

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 21 Aug 2017 19:30:15 +0200

> as expediently as they should.

But then you probably want an idle timeout mechanism and not a hard
max-session limit. If malfunctioning clients do not close sessions,
you lock yourself out sooner or later with a max-session limit.
Anyway, I think it should be clarified what 'session' means. The
question is whether these leafs are commonly implemented and generally
believed to be useful in order to standardize them.

/js

---

From: Andy Bierman <[email-masked]>
Date: Mon, 21 Aug 2017 10:32:12 -0700

Hi,

max-sessions is for constraining resources, not a security parameter.
It can be removed if it is not common.

Andy

On Mon, Aug 21, 2017 at 10:13 AM, Kent Watsen <[email-masked]> wrote:

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

---

From: "t.petch" <[email-masked]>
Date: Wed, 23 Aug 2017 10:19:58 +0100

---- Original Message -----
From: "Andy Bierman" <[email-masked]>
Sent: Monday, August 21, 2017 6:32 PM

> It can be removed if it is not common.

I see max-sessions used widely in Enterprise server software, not just
for Operations, since the software is licensed for the maximum number of
users it will support concurrently.  When that limit gets close, in
everyday use, then some users cannot access the server at peak times.
Users then logon first thing and stay logged on all day.  Then an idle
timeout is introduced to counter this practice.  So I would say that if
you have max-sessions, you must have an idle timeout.

Of course, users then get in the habit of hitting the 'Enter' key every
15/30/60 minutes to keep their sessions alive, coming back early from
coffee or lunch when necessary (well, I do); as yet, I have not seen the
counter to that.

Tom Petch

---

From: Kent Watsen <[email-masked]>
Date: Wed, 23 Aug 2017 12:33:50 +0000

Just adding to how common max-sessions may be, note that
sshd_config has the "MaxSessions" field...

FWIW, I know that JUNOS limits the number of sessions, but
doesn't have a config knob to set it.

K.

---- Original Message -----
From: "Andy Bierman" <[email-masked]>
Sent: Monday, August 21, 2017 6:32 PM

Tom Petch

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 23 Aug 2017 15:08:43 +0200

> sshd_config has the "MaxSessions" field...

You have to pick examples carefully:

  MaxSessions
       Specifies the maximum number of open shell, login or subsystem
       (e.g. sftp) sessions permitted per network connection.

Note the 'per network connection' - so this is not the max-session
we were dealing with I think. There is also this:

  MaxStartups
       Specifies the maximum number of concurrent unauthenticated con-
       nections to the SSH daemon.

Again, something slightly different and they can do random early drop
instead of 'you are locked out once it is full'. Anyway, I did not see
a limit for all established SSH sessions for all connections (and this
might be difficult given their software architecture - perhaps they
would have it otherwise). So a point for each of us.
 
> doesn't have a config knob to set it.

This does not make a strong argument that this is a commonly requested
feature to configure. ;-) Point goes to me.

/js

PS: I do not really care much, but I do have fun with this.

---

From: Kent Watsen <[email-masked]>
Date: Wed, 23 Aug 2017 14:39:37 +0000

Okay, I've removed the 'max-sessions' nodes from all the modules
(NC/RC * client/server) as well as removed the paragraph from the
Security Considerations sections.

Thanks,
Kent

  MaxSessions

  MaxStartups

/js