Re: [tsvwg] Intdir early review of draft-ietf-tsvwg-udp-options-19

[[e-mail]]

Hi, Carlos,

Many thanks for the detailed review. Some responses below.

Joe

> On Feb 9, 2023, at 5:11 PM, Carlos Pignataro via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Carlos Pignataro
> Review result: Ready with Issues
> 
> …
> 
> A. Transport Options for UDP [draft-ietf-tsvwg-udp-options-19]
> 
> As a high-order bit, I would have expected a broader, deeper, and more explicit
> discussion of two areas: (1) considerations relating every endpoint, stack,
> middle-box, security filter, etc., which might complain in the presence of the
> difference in length -- not explicitly defined,

This is first noted in Section 5 and in detail in Section 16,

> and (2) security and privacy
> considerations of having effectively a covert channel, and whether it is best
> to recommend it is not used versus recommending its use.

This is addressed in Sec 22 (though there’s a typo there - it should say:
substantially different than *TCP* options
The key point is that sentence (which admittedly fails its point as written). The previous sentence already recommends not using them where this is a concern.

> For (1), I find the section "Interactions with Legacy Devices" to be "anecdata"
> more than "analytical", and (possibly as intended) not comprehensive nor much
> useful. For example, "worked through NAT devices" is neither qualitative nor
> quantitative, and a single NAT that might drop a UDP optioned packet might be
> too much.

I’m not sure what you’re looking for, but there cannot be a useful quantitative statement (worked through 10 vs 100 means nothing if we can’t know how many different types/firmware variants of NATs are in the Internet).

> As an aside, the qualifier "legacy" sounds like a stretch label --
> every udp stack as semi-obsolete -- though this might be my misinterpretation
> of the nuances of the use and semantics of the word.

Legacy was implied as shorthand for “not updated with these options”; that can be added earlier in the doc.

> For (2), I'd frankly find most useful a discussion on the engineering and
> architectural tradeoffs between, (a) e.g., saying "the inconsistency of length
> fields is largely a security invitation, and as such, though shall be malformed
> packets sent to /dev/null, and if you want options use something else", versus
> (b) saying "largely works and is mega useful and read the security
> considerations". There seems to be an implied assumption, although frankly I
> might have missed text, or it could be documented in the WG mailing list as
> discussion had, and not worth for the I-D.

As noted in Section 16, this inconsistency of length has always been allowed, so it’s not clearly a “malformed” packet. Unexpected values are not security issues - despite numerous RFCs and drafts that continue to draw this incorrect conclusion.

So there’s arguably a philosophical debate to be had between this and a document outlawing length inconsistency, but this document exists in only one side of that debate anyway. So yes, perhaps the other side of the debate was on the list, but I don’t see how it adds to the doc.

> Some more in-lined comments, marked with "CMP":
> 
> Abstract
> 
>  Transport protocols are extended through the use of transport header
>  options. This document extends UDP by indicating the location,
>  syntax, and semantics for UDP transport layer options.
> 
> CMP: I often find the Abstract setting stage for a whole piece of work. These
> could be nits and editorials, though maybe significant to call out: CMP: The
> first sentence is an absolute statement. Are *all* transport protocols (or
> *all* the time) extended as such? Frankly UDP has not been for decades, still
> being a transport protocol, and truly will *not* be either extended with
> "header options" (but trailing ones). For these two things, the very first
> sentence in the document does not, to me, fully parse.

“Header” should have been “metadata carried with the packet”, yes.

> 6. The UDP Surplus Area Structure
> 
>  The remainder of the surplus area consists of options defined using
>  a TLV (type, length, and optional value) syntax similar to that of
>  TCP [RFC9293], as detailed in Section 8. These options continue
> 
> CMP: It seems there are some options that do not use the Length either -- are
> only Type. As such, is length optional in the first sentence, similar to the
> value being optional? Otherwise, the minimum option would be two octets.

TLV is often abbreviated when T implies L and V. So yes, “type, length, and value - where length can be implied and value is optional”. I’ll see how to work that in, but it’s common in TLV representations.

>  For IPv4, IP Total Length field indicates the total IP datagram
>  length (including IP header) and the size of the IP options is
>  indicated in the IP header (in 4-byte words) as the "Internet Header
> [...]
>  UDP options use the entire surplus area, i.e., the contents of the
>  IP payload after the last byte of the UDP payload. They commence
>  with a 2-byte Option Checksum (OCS) field aligned to the first 2-
>  byte boundary (relative to the start of the IP datagram) of that
> [...]
>  UDP options are typically a minimum of two bytes in length as shown
>  in Figure 5, excepting only the one byte options "No Operation"
>  (NOP) and "End of Options List" (EOL) described below.
> 
> CMP: NB: this is a humble, not presumptuous or arrogant. In the text above, as
> well as throughout the document, I read "bytes" which is strictly a
> machine-dependent quantity, and always try to use 'octets' or 'bits' instead.
> RFC 791 says '32-bit words', not '4-byte words'. Octets instead of bytes?

Back in the 1970s/80s there was a distinction between octet and byte, but currently most RFCs just go straight to byte both more common and as unambiguously equivalent to octet.

> 8. UDP Options
> 
>  The Kind field is always one byte. The Length field is one byte for
>  all lengths below 255 (including the Kind and Length bytes). A
> 
> CMP: It is clear from the examples and subsections under Section 9 that the UDP
> Option Length includes the Kind and Length/EL fields themselves. Is that what
> is meant in the parenthetical above? Something like this might be more clear:
> CMP: "The Length field is one octet when its value is below 255. The value of
> the Length  includes the Kind and Length fields, and as such its minimum value
> is 2. [...]"

I will clean that up.

> 9.4. Fragmentation (FRAG)
> 
> CMP: How is FRAG a SAFE UDP Option?

Because FRAG embeds user data in the surplus area. An UNSAFE option is one that cannot be used with user data without also using FRAG.

> 24.1. Normative References
> 
>  [Fa22]    Fairhurst, G., T. Jones, "Datagram PLPMTUD for UDP
>            Options," draft-ietf-tsvwg-udp-options-dplpmtud, Sep.
>            2022.
> 
> CMP: Why is this a Normative Reference?

Because REQ and RES are required in the base implementation but their use is defined in that doc, not in this one.

> I Imagined one I-D defined the base
> option spec, while others can make use of them, and add other options. CMP:
> That is, a unidirectional Normative reference
> draft-ietf-tsvwg-udp-options-dplpmtud -> draft-ietf-tsvwg-udp-options.

The definition of those options IS part of the base spec. It just is unwieldy to include it in the base doc.

> ~~~
> 
> B. Datagram PLPMTUD for UDP Options [draft-ietf-tsvwg-udp-options-dplpmtud-04]
> 
> CMP: One question only:
> 
> 4.1.  Sending Probe Packets using the Echo Request and Response Options
> 
>  A Probe Packet includes the UDP Options area containing a RES Option
>  and any other required options concluded with an EOL Option followed
>  by any padding needed to inflate to the required probe size.
> 
> CMP: Is there an advantage or implication of this approach, versus having a
> PADDING UDP Option which can be passed to UDP on receipt, and verify that
> padding? (modulo the risk of a covert-channel)
> 
> Again, I hope these are clear and useful, and thanks for the review request and
> entertaining these comments.
> 
> Thank you!
> 
> Carlos Pignataro
> 
>