Review of draft-ietf-tsvwg-udp-options-22

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

The summary of the review is Ready

Thanks for a very clear document. It makes it easier to decide whether
this idea is insane or genius :)

I found no particular security concerns and the Security Considerations
section covers the important parts, mostly regarding DoS protection.
I suspect if any issues show up, it will be mostly transport related,
eg "will this make it through all the internet hops".

Some minor comments/questions:


>> An endpoint supporting UDP options MUST treat unsupported options
   in the UNSAFE range as terminating all option processing.

>> Receivers supporting UDP options MUST silently drop all UDP
   options in a datagram containing an UNSAFE option when any UNSAFE
   option it contains is unknown. See Section 10 for further discussion
   of UNSAFE options.

>> "Must-support" options other than NOP and EOL MUST come before
   other options.

It seems implied, but not clearly stated, that one is supposed to
build up a list of received options without processing them, then
once no unknown (UNSAFE) options are found, start processing the
options. Perhaps it is meant that SAFE options can be processed
directly when read, but this is not clear to me. Especially the
above use of "terminating" seems to suggest processing options
"until" an unknown one is found. I think the text should make this
unambiguous.

>> The default UDP reassembly expiration timeout SHOULD be no more
   than 2 minutes.

Where does 2 minutes come from? That seems like a VERY long expiration
time - as in electrons can circle the earth over 6 times in that time :P
Even taking into account IoT devices waking up from sleep, I would not
expect this to ever legitimately take more than 10s? This could be a DoS
attack vector.


   >> UDP packets with incorrect AUTH HMACs MUST be passed to the
   application by default, e.g., with a flag indicating AUTH failure.

   >> UDP fragments with individual incorrect AUTH HMACs MUST be
   accumulated and passed to the application by default as part of the
   reassembled packet.


   Like all non-UNSAFE UDP options, AUTH needs to be silently ignored
   when failing. This silently-ignored behavior ensures that option-
   aware receivers operate the same as legacy receivers unless
   overridden.

I'm a little confused here. What is the point of ADDING an AUTH option
to UDP when the failure mode doesn't change. I know it says "unless
overridden", but isn't the whole point that if you set whatevever setsocket
option for this, that you WANT this feature to provide the extra security?

I think the text could be a little more clear on what to do, depending
on the socket options and thus what udp option support has been enabled
for the udp socket.

>> All options MUST be processed by receivers in the order
   encountered in the options area.

This comes very late in the text, and probably could be said earlier in
a more relevant place.


   System-level variables (sysctl):

I find the table a bit confusing as it has a column "default" but the
entries themselves also contain the word "Default" in the "meaning" row.
I would remove the word from the "meaning" row.

Why are only net.ipv4 entries listed and not ipv6 ? If these are the same,
maybe use "ipvX" or add a sentence that only v4 is listed but v6 applies
as well.

What are the JUNK entries for as there is nothing in the text indicating
this. (presumbly to perform transport and endpoint testing over the real
internet?)

NITS:

   Additional values in this registry are to be assigned
   from the UNASSIGNED values in Section 8 by IESG Approval or
   Standards Action [RFC8126].

I would just say "The registration policy is IESG Approval or
Standards Action [RFC8126]. (the use of "additional values" makes
things a bit confusing)

   Those assignments are subject to the
   conditions set forth in this document, particularly (but not limited
   to) those in Section 11.

This is also a but awkard. As any standards action can update this document,
the "conditions" can change regardless. Perhaps something along the lines of
"New assignment should follow the constrains set forth in this document to
ensure forward compatibility and migration support" ?

It would be useful to list these "conditions" in one section, or in this case
perhaps remind the reader that all conditions that apply have the ">> " marker.