[Softwires] draft-ietf-softwire-yang-15 posted incorporating IESG eval. (and area review) comments

[[e-mail]]

Hi

We’ve just posted -15 of this draft incorporating the IESG evaluation DISCUSSes/comments and the Secdir/Genart review comments. Many thanks for those.

Thanks,
Ian

Below is the log of DISCUSS/Comments received during the review and the changes that have been incorporated to address them.


-------

DISCUSS Points:

Alissa Cooper (Supported by Adam Roach, Ben Campbell, Warren Kumari and Alexey Melnikov)

The security considerations do not seem to follow the YANG security guidelines
https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines <https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines>. They do not
list the specific writeable and readable subtrees/nodes and why they are
sensitive. The fact that all the writeable nodes could "negatively affect
network operations" seems trivially true for most writeable YANG module nodes.
In the case of these modules, there seem to be multiple different threats
relevant to different nodes, including exposure of data about individual
users/customers, potential for disruption of the operations of the BR or CE,
etc.

The Security Considerations section has been rewritten expressly describing
the threats associated to the relevant nodes.

--

Benjamin Kaduk (supported by Adam Roach, Benjamin Campbell and Alexey Melnikov)
Discuss 1

This document has 7 listed authors/editors. Since, per RFC 7322, documents
listing more than five authors are unusaul, and seven is greater than five,
let's talk about the author count.

The document follow’s Adam Roach’s suggestion (in support of Benjamin’s DISCUSS)
above, and now lists 2 editors and a Contributors section.

—

Benjamin Kaduk (Supported by Warren Kumari)
Discuss 2 

The binding-table-versioning container's "version" leaf is of type uint64
but the in-module description indicates that it is a timestamp. If it is
actually supposed to be a timestamp, then the units and zero time need to
be specified, but it seems more likely that this should just be described
as an abstract version number, if I understand the prose text about this
container correctly.

Description text changed to match the leaf’s type:
Version number for this binding table.

-------------------------

Comments:

Alissa Cooper
Comment 1
I think "external party" would make more sense than "abuse party.”

Sentence reworded (in both instances) to:
When a party who is the victim of abuse presents an external IP address/port,


—-------------

Benjamin Kaduk
Comment 1
Please expand CE on first usage.

The abstract gives the full name and the abbreviation is explained in the Introduction.

Comment 2
Section 4.1

It feels a little strange to put something as generic as
/if:interfaces/if:interface/if:statistics:sent-ipv4-packets in the
ietf-softwire-ce module. Are these counters likely to be useful for other
(non-softwire?) tunneling techniques?

The counters can be re-used bu other modules. No change is needed.

---

Comment 3
Section 5.2

o softwire-num-max: used to set the maximum number of softwire
binding rules that can be created on the lw4o6 element
simultaneously. This paramter must not be set to zero because
this is equivalent to disabling the BR instance.

This seems to leave it ambiguous whether a server should reject an attempt
to set it to zero, or accept it but diable the BR instance.

The text is clear.
range "1..max".

No change is needed.

---

Comment 4
Section 7

       leaf enable-hairpinning {
         type boolean;
         default "true";
         description
           "Enables/disables support for locally forwarding
            (hairpinning) traffic between two CEs.";
         reference "Section 6.2 of RFC7596";

Is a global toggle sufficient or would there be cases where more
fine-grained control would be needed?

A+P is designed to reduce as much as possible the per-subscriber state at the network/BR. Requiring fine-grained control would require some extra state to be maintained, which is not desired. Having the general parameter is sufficient.

---

Comment 5
Section 8

container algo-versioning {
[...]
leaf date {

   type yang:date-and-time;
   description
     "Timestamp when the algorithm instance was activated.

      An algorithm instance may be provided with mapping
      rules that may change in time (for example, increase
      the size of the port set). When an abuse party
      presents an external IP address/port, the version
      of the algorithm is important because depending on
      the version, a distinct customer may be identified.

nit: "abuse party" is probably not a term that everyone is familiar with.
(similarly in br-instances)

Sentence reworded to:
When a party who is the victim of abuse presents an external IP address/port,

---

Comment 6
Section 9
Is there any possibility of a situation where the
invalid-/added/modified-entry notifications cause a substantial amount of
notification traffic (i.e., a DoS level of traffic)?

Added some text among those lines:

This is in theory possible if the BR is under the control of a non-authorized/misbehaving entity. The DDoS can be softened by defining a notification interval, but given that this interval parameter can be disabled or set to a low value by the misbehaving entity, the same problem will be observed.

———————————
Ignas Bagdonas
Comment 1
Is the derived-from() complexity for statistics counters really required given that the module itself is for softwires, and any tunnel using it would be covered?

reply:
Because these counters are applicable to some interface/tunnel schemes, the augment must be anyway subject to a "when derived-from()" clause (as a function of the tunnel type).   

The counters are designed so that other modules can reuse them, if needed. This can be done by using this statement:  

uses softwire-common:traffic-stat;

———————————————
Suresh Krishnan
Comment 1
I would have thought putting in a prioritization mechanism (like RFC8026 does)
for ordering the different mechanisms would have been useful in this YANG
module in order to configure the CE. Was this something that was considered?

responses:
1) We didn't consider 8026-like prioritization at the CE because that feature is not specific to A+P but applies to a bunch of IPv4 service continuity mechanisms including DS-Lite and 464xlat. That feature has to be defined, if needed, in a separate module. 

2) 




Mirja Kühlewind 
Comment 1
Sec 4.2:
"softwire-path-mru: optionally used to set the maximum IPv6
softwire packet size that can be received, including the
encapsulation/translation overhead. Needed if the softwire
implementation is unable to correctly calculate the correct IPv4
Maximum Receive Unit (MRU) size automatically [RFC4213]."
I guess this should both be IPv6…?

IPv4 is correct here. Changed to the wording below to clarify: 

Needed if the softwire implementation is unable to correctly
calculate the correct IPv4 payload Maximum Receive Unit (MRU)
size automatically (see Section 3.2 of [RFC4213)].

---

Comment 2
Why does the description of "rcvd-ipv4-bytes" say
"IPv4 traffic received for processing, in bytes"..?
Does the "for processing" have any special meaning or why is it only phrased
like this for that one entry?

No special meaning is intended. Reworded with:

IPv4 traffic received, in bytes.

Comment 3
Also the description for "sent-ipv4-bytes" and "sent-ipv4-packets" could be
unified.

Reworded with:
Number of IPv4 packets received.


——————
Philip Hallam-Baker (Secdir Review)
Comment 1
The document describes a schema and has appropriately identified the read/write
security concerns arising from it.

One issue that I thing could be usefully spelled out is that the use of
automated tools to decode structures of this type is not merely a programming
convenience. Attempts to parse length delimited objects nested in length
delimited structures using handwritten code is error prone and has led to
introduction of numerous buffer overrun vulnerabilities.

Added the following text to the
Security considerations given in [RFC7950] are also applicable here.

—

Roni Even (Genart review)
Comment 1
In section 2 " The adopts the Network Management Datastore Architecture (NMDA)"
there is a word missing?

Reworded to:
The YANG modules in this document adopt the Network Management Datastore
Architecture (NMDA) [RFC8342]. The meanings of the symbols used in tree diagrams
are defined in [RFC8340].