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

[Ladislav Lhotka <[e-mail]>]

Kent Watsen <[e-mail]> writes:

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

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

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

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.

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

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.

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

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.

>
> 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 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:
>
>    “Implementations are RECOMMENDED to implement the 'local-binding-supported’ feature for cryptographically-secure protocols so as to reduce to attack surface presented by firewalls.”

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

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

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