search

tsvwg / draft-ietf-tsvwg-udp-options

1 stars 0 forks source link

Reduce reduce implementation complexity by limiting per-fragment options to specific ones and fixing per-fragment option placement #25

Closed Mike-Heard closed 8 months ago

Mike-Heard commented 12 months ago

For reasons outlined in the mailing list post below, I would like to propose that only those options whose definitions specifically state that they are suitable as per-fragment options be allowed as per-fragment options, and furthermore, that the specifications for all such options specify how they get invoked by the upper layer and how they get reported to the upper layer. AUTH and UENC could provide at least a hypothetical benefit in fragments by protecting the options themselves, including the FRAG options, and therefore could be among the options specified for use in fragments.

I would further propose to change

>> When FRAG is present, it SHOULD come as early as possible in the UDP options list.

to

>> When FRAG is present, it SHOULD come as early as possible in the UDP options list.

in order to most efficiently dispatch packets received from the wire to the reassembly module.

jtouch commented 11 months ago

Agree with the proposed change at the end of the text above.

I disagree that we should a-priori decide which options make sense per-packet vs. per-fragment. All options should be defined as having meaning in both cases; some are less useful in one case vs the other, but that's for users to decide, IMO.

Mike-Heard commented 11 months ago

Agree with the proposed change at the end of the text above.

FWIW, I had intended to say for the proposed new text:

>> When FRAG is present, it MUST come as early as possible in the UDP options list.

In other words, s/SHOULD/MUST. The intent was to require that the FRAG option appear first unless there is another option that must appear before it in order to achieve the required per-fragment effects. An example would be a UENC options that encrypts the FRAG option itself; that UENC option would clearly have to precede the FRAG option.

Joe, perhaps you read the original submission that way, but since I didn't say what I intended, I though I should clarify.

I disagree that we should a-priori decide which options make sense per-packet vs. per-fragment. All options should be defined as having meaning in both cases; some are less useful in one case vs the other, but that's for users to decide, IMO.

Understood, though I remain in disagreement. The reason that I do so is because even if this is theoretically possible, I do not think that in most cases the resultant complexity that is added to the implementation and the upper later interface will give results that are worth the effort. At best such requirements tend to be ignored; at worst, they lead to code paths that are buggy from being largely unused or even become obstacles to implementation and deployment of the useful parts.

For the benefit of anyone who cares to weigh in, the mailing list post where I argue for the opposite position (motivated by an earlier question from Tom Herbert, and omitted from the initial issue submission) can be found at https://mailarchive.ietf.org/arch/msg/tsvwg/bN3oPWngzTr2A_wlrFOy0Yvn-Oo/.

My apologies for a rather careless initial submission ☹

jtouch commented 11 months ago

On Dec 25, 2023, at 11:52 AM, Mike-Heard @.***> wrote:

Agree with the proposed change at the end of the text above.

FWIW, I had intended to say for the proposed new text:

When FRAG is present, it MUST come as early as possible in the UDP options list.

In other words, s/SHOULD/MUST.

That’s just confusing and doesn’t actually do anything vs SHOULD. The “as early as possible” part isn’t defined in a way that forces a MUST.

It’s either first (which, as you note for UENC, it can’t be) or not. If not, MUST has no teeth.

Joe

Mike-Heard commented 10 months ago

On Dec 25, 2023, at 11:52 AM, Mike-Heard wrote:

FWIW, I had intended to say for the proposed new text: >> When FRAG is present, it MUST come as early as possible in the UDP options list. In other words, s/SHOULD/MUST/.

That’s just confusing and doesn’t actually do anything vs SHOULD. The “as early as possible” part isn’t defined in a way that forces a MUST. It’s either first (which, as you note for UENC, it can’t be) or not. If not, MUST has no teeth. Joe

If one accepts this position, which indeed I do not, then every implementation must be prepared to accept per-fragment options (irrespective of KIND) either before or after the FRAG header. If that is the case, it is not possible to count on any performance advantage from having the FRAG header in a specific position, and in that case it would in turn be conceptually much simpler to have all per-fragment options precede the FRAG header. That would allow the FRAG header to be simplified by removing the FRAG.START field (which is similar to a proposal that I floated several years ago).

From an implementation standpoint, however, a better approach would be to require that FRAG come first UNLESS there is an UNSAFE options that is specifically required to come before it (UENC would be the prime and probably only example). This allows an implementation to dispatch fragments to the reassembly (or decryption) engine immediately, instead of having to first parse though a list of options whose processing needs to be held in abeyance until it is determined whether we are or are not dealing with a fragment.

If this seems to complicated to unambiguously specify, then just require that FRAG come first, period. This would preclude UENC form protecting the FRAG header itself, but that is a small loss (and one that was conceded for the IP fragmentation fields by IPSec's AH and ESP).

In no case are small gains in functionality are not worth levying outlandish complexity on all implementations.

jtouch commented 10 months ago

Some points worth repeating:

If we decide so**, we CAN say "FRAG MUST come first when present". What we can't do is say that it "MUST come as early as possible", because that sentence is nonsense. A "MUST" with "as early as possible" is undefined unless you define what's possible, and that possibility has no relation to UNSAFE options.

**We left it as "SHOULD be as early as possible" so we don't have to rescind "MUST be first" if/when we find an option that needs to come earlier.

Note also that coming first doesn't necessarily solve the hardware problem - you still don't know what to do with it until you parse the other per-fragment options.

jtouch commented 10 months ago

PS - it would be useful to provide a summary reminder which options CAN be used in each of the three places: (1) unfragmented packets, (2) as per-frag options of frag packets, (3) as per-packet options of frag packets. In particular, UNSAFE cannot be used as #1; anything that primarily operates on the payload can only be used as #1 or #3 (UENC, APC, and others, e.g., payload compression). FRAG is (by definition) used in only #2. Anything else arguably could go anywhere else, AFAICT, though.

Mike-Heard commented 10 months ago

Some points worth repeating:

  • UENC encrypts only the user data; it (optionally) includes the surplus area as part of the initialization vector (protecting it, i.e., so that the user data is decoded only if the surplus area isn't modified). UENC is not useful per-fragment; it is only useful per-packet, though hidden via the FRAG option.

https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-udp-options-28#section-12.1 says:

  [...] UENC encrypts the user data and (when configured to) the
  portion of the surplus area that occurs after UENC, although
  it can (optionally) depend on options that precede it [...]

From that description, it certainly seems that UENC, were it to appear immediately after the UDP header, could protect fragments in their entirety. Were it to appear as a per-frag option after the FRAG header, it would still protect the fragment data and any per-fragment options that come after it.

  • AUTH can protect the FRAG header within a fragment even if it comes after the FRAG header

Correct.

If we decide so**, we CAN say "FRAG MUST come first when present". What we can't do is say that it "MUST come as early as possible", because that sentence is nonsense. A "MUST" with "as early as possible" is undefined unless you define what's possible, and that possibility has no relation to UNSAFE options.

**We left it as "SHOULD be as early as possible" so we don't have to rescind "MUST be first" if/when we find an option that needs to come earlier.

Note also that coming first doesn't necessarily solve the hardware problem - you still don't know what to do with it until you parse the other per-fragment options.

If we say "FRAG MUST come first when present", then the receiver would immediately know the difference between a UDP fragment and a non-fragmented UDP packet with options but no user data. The former would get dispatched to the reassembly logic; the latter, to the normal (unfragmented) processing path. This would facilitate implementation of the reassembly logic as a "bump in the stack" that takes a sequence of UDP fragments as input and outputs a UDP datagram ready for normal processing as described in https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-udp-options-28#section-11.4. The reassembly logic would, at a minimum, need to provide two flags to the normal path processing indicating (a) whether the packet was a reassembled fragment (so UNSAFE options could be accepted) and (b) whether all fragments were protected by a valid, non-zero OCS (so that a receiver that opts to require OCS protection would still accept the per-datagram options even in if the OCS on the per-datagram option was zero).

PS - it would be useful to provide a summary reminder which options CAN be used in each of the three places: (1) unfragmented packets, (2) as per-frag options of frag packets, (3) as per-packet options of frag packets. In particular, UNSAFE cannot be used as #1; anything that primarily operates on the payload can only be used as #1 or #3 (UENC, APC, and others, e.g., payload compression). FRAG is (by definition) used in only #2. Anything else arguably could go anywhere else, AFAICT, though.

In view of what the spec actually says, I would argue that UENC could usefully appear in #2 or #3. And it is easy to see what to do if UENC appears on a fragment: process it within the reassembly engine and discard it if the decryption fails. The result would be a reassembly timeout.

In the case of AUTH, as it is currently defined, it would be necessary for the reassembly engine to tell the upper layer (a) whether all fragments were covered by AUTH and (b) whether AUTH failed on any fragment. Note how much simpler this would be if the rule were to discard anything with an AUTH failure, as with UENC. This relates to the discussion in Issue #23.

As for "[a]nything else arguably could go anywhere," I would argue that options that are passed to the upper layer (or a library acting on behalf of the upper layer) for processing and possible response are useful only #1 or #3. That includes MDS, MRDS, REQ, RES, and TIME. Allowing those as per-fragment options is possible, but since processing is deferred until after reassembly is complete, it would be necessary for the reassembly logic to collect a list of what was received and send them to the normal processing path (along with the aforementioned flags) to eventually be passed to the upper later or library acting on its behalf. But then what happens? AFAICT, the only logical thing is to prepend this list to the list of per-packet options, and process the options in the order received. If there are any duplicates, only the first instance is retained for processing, as stated in https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-udp-options-28#section-10; the others must be discarded before being passed to the upper layer or library acting on its behalf. I guess I could live with this, given that the spec does not outlaw repeated options in all cases (only for the required ones). But we really should be explicit about what is expected.

jtouch commented 8 months ago

Per discussion on Feb 19 with Gorry Fairhurst and Mike Heard: Noted in the FRAG section why SHOULD first is not a MUST, notably that there are other options that would need to come first if they affect the FRAG option itself, e.g., UENC (newly noted) UCMP (compression). The impact of it not being first is noted. Also added how per-frag options are aggregated for each defined option. The text in -29 should be close...

Mike-Heard commented 8 months ago

IMO, the text in -29/-30/-31 satisfactorily addresses the issues except for some typos and editorial issues. I am therefore closing this issue with the following comments:

For APC and AUTH, it is noted that these options "may fail even where the user data has not been corrupted, such as when its contents have been overwritten." If my understanding is correct, the intent is "may fail even where the user data has not been corrupted, such as when its contents have been intentionally overwritten e.g. by a middlebox to update embedded ports numbers or IP addresses." Whether or not I have accurately captured the intent, it may be useful to make it explicit.

For APC (Section 11.3), I see:

APC is reported to the user, whether used per-datagram or per- fragment (as defined in Section 11.4). In the latter case, it is indicated as success only if reassembly is complete.

My reading of Section 11.3 is that APC covers only the UDP user data, and as such, does not cover fragment data that is present in a FRAG option. On this reading the per-fragment version does not protect anything and either includes the CRC for an empty PDU (0xffffffff for CRC32c) and checks OK or contains something else and checks invalid. IMO, it would be more useful to change the definition of APC to cover the FRAG data if it appears in a UDP fragment or the UDP data otherwise. I am not in any way doctrinaire about this and only point this out so that implementers get clear/unambiguous instructions. This is a separate matter than the one for which this issue was opened, so we may want to open a new issue.

Section 12.1. UNSAFE Compression (UCMP) has a typo:

OLD:

The UNSAFE Compression (UENC, Kind=192) option is reserved [...]

NEW:

The UNSAFE Compression (UCMP, Kind=192) option is reserved [...]

In the references I see:

[To24] Touch, J., "The UDP Authentication Option," draft-touch- udp-auth-option, Jul. 2018.

We need a corrected publication date when this draft is submitted.

jtouch commented 8 months ago

In order: