Hi Michael, I sincerely appreciate your review and comments. I will work for this draft by removing some unnecessary parts and improving your commented ones in the NSF Capability YANG Data Model.
Thanks. Best Regards, Paul On Sat, Nov 7, 2020 at 7:49 AM Michael Scharf via Datatracker < [email protected]> wrote: > Reviewer: Michael Scharf > Review result: Almost Ready > > This is a TSV-ART review of draft-ietf-i2nsf-capability-data-model-13. This > review focuses *only* on transport protocol topics. The document may have > other, more general problems. Apparently this is already discussed in the > IESG. > > Most YANG model entries related to Internet transport protocols are poorly > defined. The definition, description, and the references need to be > rewritten > to actually reflect the current use of transport protocols in the Internet. > Precise definitions and/or better references are needed in many places. > > In particular the selection of "capabilities" related to TCP is not well > motivated in the document. The definitions in the YANG data model may be > pretty > disconnected from actual capabilities of TCP middleboxes. It might be quite > challenging to comprehensively describe in a YANG model all possible TCP > processing in sophisticated TCP middleboxes. But even if the objective of > the > document was to focus on basic state-less processing of TCP, the document > does > not describe all capabilites that can be found e.g. in ACL lists in > state-of-the-art routers or firewalls. In contrast, the document lists > some TCP > capabilities that may have no or only very limited real-world benefit, at > least > to the best of the knowledge of the reviewer. At minimum, the document > would > have to better reason its selection of capabilities, in particular > regarding > processing of TCP. > > Detailed comments regarding the YANG Model: > > 1. identity ipv4-tos { > base ipv4-capability; > description > "Identity for IPv4 Type-Of-Service (TOS) > condition capability"; > reference > "RFC 791: Internet Protocol - Type of Service"; > } > > According to RFC 3168, this field is subdivided into a DSCP and an ECN > field. > Thus, it is unclear what this identity refers to. Does it refer only to the > DSCP part (for instance, RFC 8329 lists only DSCP)? Or does it also > include the > two ECN bits? If so, what capability regarding the two ECN fields would > this > refer to? Given the ongoing discussion on use of ECN bits, it would be a > surprise if there was industry consensus on what capabilities an NSF should > have regarding ECN bits. > > Instead of a reference to the protocol specification, it would be useful to > have a reference to a precise definition of this capability. This comment > also > applies in many other places. > > Formally, the reference to RFC 791 seems insufficient, as the ToS > definition in > that document has been updated by RFC 2474, RFC 3168, etc. > > 2. identity ipv6-traffic-class { > base ipv6-capability; > description > "Identity for IPv6 traffic class > condition capability"; > reference > "RFC 8200: Internet Protocol, Version 6 (IPv6) > Specification - Traffic Class"; > } > > The previous comment also applies here. RFC 8200 explains the subdivisions > of > the "traffic class" field. At least regarding the two ECN bits it is > entirely > unclear what capability this identity would refer to. > > 3. identity tcp-capability { > base condition; > description > "Base identity for TCP condition capabilities"; > reference > "RFC 793: Transmission Control Protocol"; > } > > State-of-the-art TCP is not only defined by RFC 793. For instance, RFC 1122 > updates and clarifies RFC 793. Regarding the TCP header, a better reference > would be draft-ietf-tcpm-rfc793bis, which is in WGLC in TCPM and will > obsolete > RFC 793. A modern TCP stack implements many features beyond RFC 793. RFC > 7414 > provides a roadmap. > > This comment about reference RFC 793 also applies to the following > identities. > > Independent of the question where the TCP header is defined, listing the > standard for the protocol may not be a precise definition of a capability > inside the network. TCP standards describe the protocol in the connection > endpoints. That can be *very* different to the handling in a middlebox, > which > may not even be specified in any IETF document (sometimes for good > reasons). > > 4. identity exact-tcp-window-size { > base tcp-capability; > description > "Identity for exact-match TCP window size condition capability"; > reference > "RFC 793: Transmission Control Protocol - Window Size"; > } > > identity range-tcp-window-size { > base tcp-capability; > description > "Identity for range-match TCP window size condition capability"; > reference > "RFC 793: Transmission Control Protocol - Window Size"; > } > > >From this document it is entirely unclear why a match on the window size > would > matter. It is well-known that many firewalls process quite a number of TCP > header fields. But the absolute value of the (receive) window alone is > actually > not very useful. A well-known middlebox capability would e.g. be some > checks > whether sequence numbers are valid. This may require logic for processing > the > window and other parameters. As far as I can tell, that logic does not need > (only) an exact or range match for the window value. > > In addition, for this field it is clearly not appropriate to only > reference RFC > 793. RFC 7323 changes the semantics of this header field by scaling, which > is > used by basically all modern TCP stacks. > > As far as I know, the RFC 7323 scaling of the window can only correctly be > reverse-engineered by an NSF that can keep per-connection state (i.e., > records > the scaling factor in the SYN). It is unclear how this model deals with an > NSF > that decodes the wrong window sizes, i.e., an NSF that cannot correctly > decode > the actual parameter in the endpoints due to lack of knowledge of the scale > factor. > > 5. identity tcp-flags { > base tcp-capability; > description > "Identity for TCP flags condition capability"; > reference > "RFC 793: Transmission Control Protocol - Flags"; > } > > Further TCP flags are defined in RFC 3168. As already noted, > draft-ietf-tcpm-rfc793bis might be a better reference regarding the current > definition of the TCP flags. > > The TCPM working group also currently standardizes further semantics for > the > TCP flags in draft-ietf-tcpm-accurate-ecn. Internet transport protocols > evolve > over time... In order to be meaningful, this document would have to be much > more precise what is meant by "tcp-flags". > > 6. Selection of parameters (and omissions) in the YANG model > > As already noted, the selection of TCP-related capabilities in this YANG > model > is not well motivated and actually surprising (at least to me). It is > pretty > well-known in industry - and the IETF - what capabilities widely deployed > TCP > middleboxes have. For instance, widely deployed TCP middleboxes are known > to > process TCP options. There are also well-known studies in this field > (e.g., "Is > it still possible to extend TCP?" by Michio Honda et al.). A YANG model > that > focuses on some few fixed TCP header fields seems entirely disconnected > from > running code in the Internet. For instance, why are TCP options omitted? > > 7. identity exact-udp-total-length { > base udp-capability; > description > "Identity for exact-match UDP total-length condition capability"; > reference > "RFC 768: User Datagram Protocol - Total Length"; > } > > identity range-udp-total-length { > base udp-capability; > description > "Identity for range-match UDP total-length condition capability"; > reference > "RFC 768: User Datagram Protocol - Total Length"; > } > > These definitions need to take into account that the UDP total length can > be > smaller than the IP transport length. See draft-ietf-tsvwg-udp-options for > more > details. > > 7. identity sctp-capability { > description > "Identity for SCTP condition capabilities"; > reference > "RFC 4960: Stream Control Transmission Protocol"; > } > > In addition to SCTP, the IETF has also standardized e.g. DCCP. For > instance, > RFC 8329 also mentions DCCP. Well, DCCP is to the best of the knowledge of > the > reviewer not widely deployed, i.e., maybe it could indeed be omitted. But > https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml > lists > other parameters. Which criteria determines that SCTP must be included in > this > document, but other protocols are not considered? > > Comments regarding the Appendix: > > - The examples in the appendix need to be checked against the YANG model > and > they have to be updated. For instance, in example A1, there is an entry > "<tcp-capability>exact-fourth-layer-port-num</tcp-capability>". That > apparently > contradicts the definition in the document. > > - Example 1 seems unrealistic. For instance, I cannot think of any firewall > that can only process TCP port numbers, but not UDP port numbers. > > - I suggest to avoid the terms "forth-layer"/"forth layer" in this > document. > For instance, IETF documents use the term "transport layer" or "transport > protocol" to refer to TCP, UDP, etc. > > > _______________________________________________ > I2nsf mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/i2nsf > -- =========================== Mr. Jaehoon (Paul) Jeong, Ph.D. Associate Professor Department of Computer Science and Engineering Sungkyunkwan University Office: +82-31-299-4957 Email: [email protected], [email protected] Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php <http://cpslab.skku.edu/people-jaehoon-jeong.php>
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
