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

[Kent Watsen <[e-mail]>]

Hi Lada,

Thank you for your review!

Below are responses to your comments.

K.


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

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.



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

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.



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

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?


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

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
> "local-binding-supported" is needed at all, I'd suggest to mention this in
> Security Considerations.

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


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

Fixed.



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

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.