Subject: [yang-doctors] Yangdoctors last call review of draft-ietf-netconf-tcp-client-server-09

From: Ladislav Lhotka via Datatracker <[email-masked]>
Date: Fri, 09 Apr 2021 02:58:30 -0700

Reviewer: Ladislav Lhotka
Review result: Ready with Issues

The modules are well designed and nicely documented, both in the descriptions
and text of the Internet-Draft.

**** Comments

- Sections 2.1.4, 3.1.3, 4.1.3: the sentence 'The "..." module does not contain
any protocol-accessible nodes.' is misleading in that the modules do define
data nodes that are intended to be protocol accessible after the corresponding
grouping is used. I know this is a part of the NETCONF/YANG lingo, but another
formulation that clearly says what's going on might be preferable.

- Sections 2.2, 3.2 and 4.2: the XML snippets use document elements
"tcp-common", "tcp-client" and "tcp-server", but these containers are not
defined in the corresponding modules. This is confusing, my suggestion is to
rewrite the examples in the JSON representation where no such top-level node is
necessary.

- What is the purpose of "tcp-connection-grouping" if it only uses
"tcp-common-grouping" and nothing else? Why cannot "tcp-common-grouping" be
used directly?

- The "local-port" parameter defined in ietf-tcp-client seems dubious from the
security viewpoint in that fixing the source port makes it easier for attackers
to steal the connection (see RFC 6056). If the feature
"local-binding-supported" is needed at all, I'd suggest to mention this in
Security Considerations.

- The module ietf-tcp-client uses the placeholder "RFC AAAA", which is not
defined in the Editorial Note.

**** Nits

- RFC 7950 is cited repeatedly (6 times) in a general context, e.g. whenever
YANG 1.1 is mentioned. It should suffice to use the citation at the first
appearance.

- sec. 1.3: s/in compliant/is compliant/

- in 3 places: s/illustatrating/illustrating/

---

From: Kent Watsen <[email-masked]>
Date: Mon, 19 Apr 2021 20:30:14 +0000

Hi Lada,

Thank you for your review!

Below are responses to your comments.

K.

> and text of the Internet-Draft.

 ;)

> formulation that clearly says what's going on might be preferable.

Fair enough.  How about this?

   The "ietf-tcp-..." module does not itself define
   any protocol-accessible nodes.  This module defines
   only "grouping" statements that are used by other
   modules to instantiate protocol-accessible nodes.

> rewrite the examples in the JSON representation where no such top-level node is

Indeed, and my validation script even takes the special step to convert the groupings to containers in order for the validation to succeed.   In other modules, I created a “foobar-usage.yang” module to instantiate the grouping statements.

Hmmm, I was about to create the JSON, but realized that it would be easier to strip out the first-and0-last lines from the XML examples.  I understand that they're not a valid XML documents, but snippets have been used before…

FWIW, all the examples are still validated, so there’s otherwise no loss in correctness.

> "tcp-common-grouping" and nothing else? Why cannot "tcp-common-grouping” be used directly?

I added this comment:

    Whilst this grouping appears to not add any value beyond that given
    by "tcp-common-grouping", it is defined so as to provide space for a
    future peer module (e.g., "tcp-system-grouping" that defines additional
    configuration nodes used to configure a TCP stack at an operating
    system level.

Makes sense?

> to steal the connection (see RFC 6056).

Interesting read.  But note that the RFC regards primarily cleartext protocols.  An axiom in security circles is “obscurity is not security”, i.e., randomizing client ports to make off-path guessing harder is not much of a win.  

In our case, and pretty much every IETF protocol now, a secure transport layer is used, and having a predictable 5-tuple can greatly reduce the attack-surface in firewall rulebases.

As it stands, it’s configuration enabled by an optional feature (your next comment) and, besides, the wildcard values (e.g., “0”) may configured, enabling [potentially-random] operating-system selected values - so no harm?

> If the feature

“Needed” is an interesting take.   My feeling is that the model is incomplete otherwise.  From a Security Considerations perspective, I’d like to put something like:

   “Implementations are RECOMMENDED to implement the 'local-binding-supported’ feature for cryptographically-secure protocols so as to reduce to attack surface presented by firewalls.”

Is that what you had in mind?   If nothing else, having such a statement in the draft would certainly catch the eye of the SecDir reviewer.  Thoughts?

> defined in the Editorial Note.

Fixed.

> YANG 1.1 is mentioned. It should suffice to use the citation at the first

All but one citation removed.

> - sec. 1.3: s/in compliant/is compliant/

Fixed (in all the drafts!)

> - in 3 places: s/illustatrating/illustrating/

Fixed (in all the drafts!)

Thanks again!

K.

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 20 Apr 2021 14:42:25 +0200

>    modules to instantiate protocol-accessible nodes.

What about using just the last sentence? It says all.

> FWIW, all the examples are still validated, so there’s otherwise no loss in correctness.

Maybe it is not necessary to validate these short fragments. In any case, a casual reader might be confused - where do these elements come from? As a minimum, some explanation should be added.

>     configuration nodes used to configure a TCP stack at an operating

OK, so this new grouping could use equally well either "tcp-connection-grouping" or "tcp-common-grouping", and add more configuration nodes. Having two names for the same thing is confusing.

> Interesting read.  But note that the RFC regards primarily cleartext protocols.  An axiom in security circles is “obscurity is not security”, i.e., randomizing client ports to make off-path guessin…

I don't think this can be labelled as security by obscurity. Without cryptographic protection, it is basically the only available measure against traffic injection. In DNS, for example, source port numbers (and transaction IDs) with insufficient entropy are considered security issues.

> “Needed” is an interesting take.  My feeling is that the model is
> incomplete otherwise.  From a Security Considerations perspective,

I was thinking it might be needed for cases like NETCONF call home. Otherwise, it is not common to be able to configure the port for the side that performs active open.

> I’d like to put something like:
>

I don't understand what firewalls have to do with this issue. On the other hand, I would also mention the reason why this is RECOMMENDED, and cite RFC 6056.

> Is that what you had in mind?   If nothing else, having such a statement in the draft would certainly catch the eye of the SecDir reviewer.  Thoughts?

Of course, a SecDir review would be more authoritative here.

Cheers, Lada

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

---

From: Kent Watsen <[email-masked]>
Date: Thu, 22 Apr 2021 01:01:27 +0000

Hi Lada,

> What about using just the last sentence? It says all.

Done.

> Maybe it is not necessary to validate these short fragments. In any case, a casual reader might be confused - where do these elements come from? As a minimum, some explanation should be added.

Done - the following XML-comment has been added just each such XML example:

    <!-- The outermost element below doesn't exist in the data model. -->
    <!--  It simulates if the "grouping" were a "container" instead.  -->

I think it critical to validate *every* example, as it’s too hard to maintain otherwise.  ;)

> OK, so this new grouping could use equally well either "tcp-connection-grouping" or "tcp-common-grouping", and add more configuration nodes. Having two names for the same thing is confusing.

Removed, after reviewing the latest "draft-ietf-tcpm-yang-tcp” draft.

> I don't think this can be labelled as security by obscurity. Without cryptographic protection, it is basically the only available measure against traffic injection. In DNS, for example, source port…

Noted.

> I was thinking it might be needed for cases like NETCONF call home. Otherwise, it is not common to be able to configure the port for the side that performs active open.

Uncommon, but not unheard of, nor bad practice.

Ever since I worked at company developing FW/IDP devices and a NOC/SOC, I’ve always pushed for and advocated the ability to configure local address/ports.

> I don't understand what firewalls have to do with this issue.

Because knowing the source addr/port enables some of the 5-tuple numbers to move from an “any” value to specific values.  This benefits both the egress and ingress sides.

> On the other hand, I would also mention the reason why this is RECOMMENDED, and cite RFC 6056.

Done.

K.