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

[Kent Watsen <[e-mail]>]

Hi Lada,



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

Done.



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

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



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

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



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

Noted.


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

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

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.


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


K.