Subject: [Tsv-art] Tsvart early review of draft-ietf-ipsecme-iptfs-03

From: Joseph Touch via Datatracker <[email-masked]>
Date: Thu, 03 Dec 2020 22:26:12 -0800

Reviewer: Joseph Touch
Review result: Not Ready

This documents most significant transport issue is the use of path MTU
discovery. Sec 2 recommends the use of path MTU discovery [RFC1191] [RFC8201],
but this is known to be problematic black holing where ICMPs are blocked or
filtered (which should be noted everywhere PMTU is suggested, including Sec
4.2). It would be more appropriate to incorporate PLPMTUD [RFC4821] [RFC8899],
especially in conjunction with congestion control, as both assume bidirectional
stateful ingress/egress coordination. This is particularly important given the
sending side’s prerogative to change the size of the tunnel EMTU_S (see below).

The most significant concern is not at the transport layer – it is the
assumption of in-order delivery and insufficient consideration of the impact of
loss at the network layer. Loss could orphan received fragments – for which a
timeout should be recommended. Loss or reordering could generate faulty
reassembly at the egress – which is actually very likely here, e.g., when a
short packet is followed by a sequence of packets that are exactly the tunnel
MAP (as defined in draft-ietf-intarea-tunnels). As noted below, persistent
fragmentation could make this situation worse, esp. in the presence of frequent
reordering or loss.

Other significant issues, largely at the network/tunnel layer:

There is no clear utility in having the blockoffset point past the end of the
current packet. It serves – at best – as only a partially useful check on the
next packet. I.e., if the two blockoffsets disagree, presumably a packet is
lost – but if they agree, it cannot be concluded that a packet is not lost. It
is sufficient that it points to the end of the tunnel packet.

Although the mechanism allows for padding, it appears too aggressive in trying
to be work-conserving. Consider a tunnel that could support 1000B payloads; if
a stream of packets comes in as 200B, 1000B, 1000B, 1000B, etc. (more 1000B
packets), EVERY 1000B would be fragmented, which means loss of a single tunnel
packet would cause two inner packets to be incomplete. The protocol should
allow padding in some cases to avoid fragmentation, e.g., every 100th packet it
might allow insertion of padding rather splitting a packet across the stream.
The heuristic shown here is just an example.

Fig 1 shows the blocksize as always occurring in the latter half of a word; is
this always the case? If so, it would be useful to indicate the left side of
that word as “ESP – con’t”. If not, other examples should be provided.

At the end of Sec 2.2, the blockoffset helps recover the next inner packet, but
the term “full” in that sentence implies the inner packet is entirely inside
that outer packet (which it may not be).

The  option of congestion control and the claim of “unidirectional” should be
discussed more consistently (i.e., mention the need for bidirectional channels
when mentioning thec claim of unidirectionality).

Like all tunnels, this approach needs to more clearly indicate a number of
different MTU values and how they interact [draft-ietf-intarea-tunnels], both
for the underlying network and the tunnel provided: -       EMTU_S -      
EMTU_R -       Path MTU -       Link MTU

It may also be useful to use the terms developed in that doc, e.g., tunnel link
packet (the packet that traverses a tunnel) as well as inner vs. outer
fragmentation, tunnel maximum atomic packet, etc.

There are a number of other tunnel considerations that should be addressed,
again as discussed in draft-ietf-intarea-tunnels. These include: -       The
impact of tunnel traversal on the inner hopcount/TTL (there should be none, as
per that doc; hopcount should be adjusted by routers, not tunnels) -      
Impact of errors in the tunnel on the ingress -       Specification of the
EMTU_R of the tunnel itself, and how that is determined -       What the
ingress should do if an incoming packet exceeds EMTU_R -       It should be
noted that this is a unicast tunnel -       What you expect if there are
multiple paths between the tunnel ingress and egress -       Does the tunnel
itself have a flow ID? (if the outer packet is IPv6) If so, is it fixed or
intended to vary arbitrarily (and if so, how)? -       If the outer packet is
IPv4, do you expect to use DF=1? If not, how are you handling ID issues in RFC
6864? If so, it might be useful to add a reminder that the ID can be anything
(including constant, which may be useful in avoiding a covert channel).

---

From: Christian Hopps <[email-masked]>
Date: Fri, 4 Dec 2020 03:14:36 -0500

Hi Joseph,

First, thanks for your review. This is an initial reply to clarify a couple points [inline].

> On Dec 4, 2020, at 1:26 AM, Joseph Touch via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Joseph Touch
> Review result: Not Ready
> 

Thanks, I will look into adding text on this. An implementation can use receipt of the time echo in the CC header as an ACK of receipt of a tunnel packet that included that time val so probing using this method could be suggested as well. For non-CC the user is expected to be in control of the path domain (otherwise it wouldn't be able to correctly set the fixed send rate) so no PMTU (or simple PMTU if they really wish to use it -- unlikely) should suffice.

> fragmentation could make this situation worse, esp. in the presence of frequent

In-order deliver is not assumed. Correct reordering is required, and is done on ingress by IPTFS through use of the ESP sequence number. IPsec/ESP includes a sequence number and a window for anti-replay protection. This also provides for loss detection (as the sliding window moves) allowing for reaping of orphaned fragments.

Perhaps we should add a bit of text highlight this though?

(FWIW my test suite for our implementation of IPTFS includes a fairly comprehensive set of tests of out-of-order delivery including drops. :)

> is sufficient that it points to the end of the tunnel packet.

No harm either though, right? Having implemented this, I can tell you that it does help detect bugs in ones code while in development. :)

> The heuristic shown here is just an example.

This is allowed by the protocol. There is no restriction specified on when an implementation can insert padding in the tunnel packet stream. In fact unless the tunnel is being 100% utilized the expectation is that it will be sending extra padding often -- that being one key to the traffic flow security.

Your suggestion I think is that it could be beneficial to consider reducing the available bandwidth by forcing padding to align packets even if in the presence of an offered load >= 100% of the tunnel capacity . Again, this is certainly allowed, but when and if to do this seems to be a great topic for research, and would be outside the scope of this initial protocol specification I think.

Do you think we should add text explicitly mentioning that one could do this if one wanted to though?

> that word as “ESP – con’t”. If not, other examples should be provided.

The "..." is just omitting the details of other IPTFS fields which are normatively specified in section 6. The intent was to give an overview of what is being discussed in the text below, but not normatively define the header format.

> that outer packet (which it may not be).

will remove "full".

> when mentioning thec claim of unidirectionality).

Will do.

> (including constant, which may be useful in avoiding a covert channel).

Will go back through the document keeping this in mind.

Thanks again,
Chris.

---

From: Joseph Touch <[email-masked]>
Date: Fri, 4 Dec 2020 08:42:38 -0800

Hi, Christian,

> On Dec 4, 2020, at 12:14 AM, Christian Hopps <[email-masked]> wrote:
> 
> Hi Joseph,
> 

Agreed. It would just be useful to not imply “just do PMTU” because we know it isn’t a solution.

> Perhaps we should add a bit of text highlight this though?

That would help. I checked and reordering doesn’t appear to be mentioned at all.

It will be important to also include limits on reordering - how many packets to hold back before allowing a drop in the sequence and what to do if a packet shows up late. I.e., this is a bit more complex than just saying “reordering is required”.

> No harm either though, right? Having implemented this, I can tell you that it does help detect bugs in ones code while in development. :)

It’s useful as a check, but it’s important to explain what the check means.

There are four cases (BO = blockoffsets, seq = ESP sequence number)

                              ESP - no gap         ESP - gap
---------------------------------------------------------------------
BO - aligned          OK                         BO-FP-err
BO - misaligned    SN-FP-err              Typ-err

Typ-err is the typical error when a packet is lost, because that should result in both an ESP seqno gap and blockoffset misalignment.

SN-FP-err is when there’s no ESP seq no gap but the blockoffsets are misaligned. This should only ever happen if the seqno rolls around and you missed it, which you should have some other mechanism to prevent (i.e., drop old packets when they’re half the sequence space old - which is easy to predict because you know the tunnel packet generation rate).

BO-FP-err is when there’s an ESP seq no gap but the blockoffsets are aligned. This is just a false positive (thus “FP” in my notation). S

In any of the error cases, you do not reassemble.

So checking blockoffset alignment only helps you know whether your sequence number check and timeout discard is working correctly.

Protocol implementation correctness should be checked by test vectors, not during normal code operation IMO. At best, it’s unnecessary complexity. At worst, it gives you a false sense of wanting to reassemble when you shouldn’t.

> Your suggestion I think is that it could be beneficial to consider reducing the available bandwidth by forcing padding to align packets even if in the presence of an offered load >= 100% of the tunnel capacity .

Not just then, but also when offered load is <100% too. Even in those cases, it’s possible that it might be better to add padding than to try to pack. Think of it this way -
- if the remaining space in a tunnel packet is small
AND
- if the next packet would fit in the tunnel without splitting it up if it started the next packet
AND
- if the amount of wasted space doesn’t make the ingress incoming packet queue backup too much
THEN
- pad out the current tunnel packet and start a new one

I.e., this can all be based on a queue on the ingress. When the queue backs up, push towards work-conserving. Otherwise, push for split-reduction.

The depth of the queue should be configurable - and a property of the tunnel, because it adds delay. 

> Again, this is certainly allowed, but when and if to do this seems to be a great topic for research, and would be outside the scope of this initial protocol specification I think.
> 

I’d suggest mentioning that it’s allowed, describing a possible way to do it, and explaining its cost (esp. the added delay). It may be “research” to get it right, but having something like that as part of the system may be important to having it work over lossy paths.

> The "..." is just omitting the details of other IPTFS fields which are normatively specified in section 6. The intent was to give an overview of what is being discussed in the text below, but not n…

That’s not clear. You should put something in that area to say “IPFTS Sec 6 HDR” or something.

>> that outer packet (which it may not be).

Thanks - that’s what I was thinking would suffice.

> Will go back through the document keeping this in mind.

I’ll issue an update to that doc shortly. It’s on my list to do a deep pass to get that one out the door - it’s been a long time coming, but it’s already impacted the terminology of a number of other docs that have been published.

FWIW, I’d be glad to provide feedback on an update, esp. on that last issue.

Joe

---

From: Christian Hopps <[email-masked]>
Date: Sat, 19 Dec 2020 04:17:48 -0500

Changes are underway. One comments inline.

> On Dec 4, 2020, at 11:42 AM, Joseph Touch <[email-masked]> wrote:
> 
> Hi, Christian,

[...]

> There are four cases (BO = blockoffsets, seq = ESP sequence number)

Conversely, test-vectors are harder to write w/o this and must be external, whereas this is a simple sanity check that can be added inline (perhaps conditionally) to code which has been shown to catch bugs. This will help implementations be more robust.

Regarding complexity, having implemented this I can say there really is no complexity :) You set the block offset to the remaining inner packet length to be sent (or 0 if no packet fragmentation is in progress). This value is sitting right there in the code so you can either plug it in or use some marker value (you suggest the length to the end of the payload). On receive the code is identical: does it exceed the length of the payload or not.

Thanks,
Chris.

---

From: Christian Hopps <[email-masked]>
Date: Sat, 19 Dec 2020 17:16:52 -0500

> On Dec 4, 2020, at 3:14 AM, Christian Hopps <[email-masked]> wrote:

Looking through this list I realized one thing. We are not technically defining a new tunnel here. We are defining a new payload for the already defined IPsec/ESP tunnel. So many of these points are covered by the base ESP tunnel document. Where we do differ is that we do not pre-fragment packets (i.e., we do not use IP fragmentation or have a tunnel MTU) on ingress as the encapsulation supports fragmentation and reassembly by design of any sized IP packet. This fact is noted in the text while talking about the DF bit mapping (or lack thereof).

"IP-TFS never IP fragments the inner packets and the inner packets will not affect the fragmentation of the outer encapsulation packets."

Do you think I should expand a bit more on that text to anything more clear?

>> again as discussed in draft-ietf-intarea-tunnels. These include:

>> -       The

This is covered by IPsec (https://tools.ietf.org/html/rfc4301#section-5.1.2 <https://tools.ietf.org/html/rfc4301#section-5.1.2> section 5.1.2.1 in particular).

>> Impact of errors in the tunnel on the ingress

What would you suggest saying about this? Broadly I'm thinking "errors in the tunnel will affect inner packet delivery?" Seems a bit obvious so I think I may be missing what your after.

>> -       Specification of the
>> EMTU_R of the tunnel itself, and how that is determined -

There is no maximum, all IP packet sizes are supported. :)

>>       What the

We inherit these from the base IPsec/ESP tunnel mechanism that we use.

Thanks,
Chris.

---

From: Joseph Touch <[email-masked]>
Date: Sat, 19 Dec 2020 16:41:41 -0800

> On Dec 19, 2020, at 1:17 AM, Christian Hopps <[email-masked]> wrote:
> 
>> On Dec 4, 2020, at 11:42 AM, Joseph Touch <[email-masked] <mailto:[email-masked]>> wrote:
>> 
>> Hi, Christian,
> 
> [...]
> 

Test vectors run only during testing. By putting this test in regular operation, you’re adding overhead to check that the code is working every time it runs.

It’s your call; I was just pointing out that this is very inefficient.

> Regarding complexity, having implemented this I can say there really is no complexity :)

If the LOC > 0, then it’s not “no complexity”.

> You set the block offset to the remaining inner packet length to be sent (or 0 if no packet fragmentation is in progress). This value is sitting right there in the code so you can either plug it in or use some marker value (you suggest the length to the end of the payload). On receive the code is identical: does it exceed the length of the payload or not.

Agreed, it’s simple - but it’s nonzero effort and code. Again, at run-time. For a check that basically keeps saying the code is working, rather than checking an actual packet error.

Again, recommending inefficient code is up to you.

Joe

---

From: Joseph Touch <[email-masked]>
Date: Sat, 19 Dec 2020 16:57:10 -0800

> On Dec 19, 2020, at 2:16 PM, Christian Hopps <[email-masked]> wrote:
> 
> 
> 
>> On Dec 4, 2020, at 3:14 AM, Christian Hopps <[email-masked] <mailto:[email-masked]>> wrote:
> 

It’s the ways in which you differ that need to be addressed.

> "IP-TFS never IP fragments the inner packets and the inner packets will not affect the fragmentation of the outer encapsulation packets."

It doesn’t IP fragment, but it does fragment. There are still fragmentation and reassembly considerations as a result.

> Do you think I should expand a bit more on that text to anything more clear?

Yes - the point is to explain how much you rely on some properties of ESP to avoid replays and maintain ordering and explain that’s why some of the typical reassembly issues aren’t relevant. But not all..

> This is covered by IPsec (https://tools.ietf.org/html/rfc4301#section-5.1.2 <https://tools.ietf.org/html/rfc4301#section-5.1.2> section 5.1.2.1 in particular).

It should be noted as how that impacts the methods of this doc.

> What would you suggest saying about this? Broadly I'm thinking "errors in the tunnel will affect inner packet delivery?" Seems a bit obvious so I think I may be missing what your after.

You should describe how you expect ICMP errors arriving at the ingress to be handled when they arise because of a packet traversing the tunnel. If it’s “how ESP handles it”, fine, but mention that.

> There is no maximum, all IP packet sizes are supported. :)

OK, then mention that. It’s different from the EMTU_R of ESP, for example.

> We inherit these from the base IPsec/ESP tunnel mechanism that we use.

Some of it you do inherit, but others you do not (as you note above). They all need to be addressed.

I.e., you’re potentially putting multiple IP packets inside one tunnel packet; what does the header of the tunnel packet look like? You can’t simply say “do what ESP does” because ESP allows only one IP packet as payload. I.e., see Sec 5.1.2.1 - all those values “copied from the inner packet” - how do they work when you have more than one packet and the values vary across those payloads??

That’s the point here - you can definitely say “do what ESP does” when that’s enough, but it’s clearly not. The ways in which you differ need to be spelled out explicitly.

Joe

---

From: Christian Hopps <[email-masked]>
Date: Sat, 19 Dec 2020 22:47:51 -0500

> On Dec 19, 2020, at 7:41 PM, Joseph Touch <[email-masked]> wrote:
> 
> 
> 
>> On Dec 19, 2020, at 1:17 AM, Christian Hopps <[email-masked] <mailto:[email-masked]>> wrote:
>> 

The test is already performed to determine if the fragment is complete or not, regardless of whether you use the real value or some marker value.

> If the LOC > 0, then it’s not “no complexity”.

There really is no complexity added.

copylen = min(inner_remain, outer_remain)
ohdr->block_offset = inner_remain; /* Only Diff */
memcpy(outer_buf, inner_buf, copylen);
if (inner_remain > copylen) more to go...

vs.

copylen = min(inner_remain, outer_remain)
ohdr->block_offset = copylen; /* Only Diff */
memcpy(outer_buf, inner_buf, copylen);
if (inner_remain > copylen) more to go...

Except the former allows for creating more robust implementations, and also differentiates the case where a fragment just fits or not for the receiver. The latter ends up stripping away useful information. Both ways point the block offset past the end of payload. I see the loss for stripping the information here, I don't see the gain.

Thanks,
Chris.

---

From: Christian Hopps <[email-masked]>
Date: Sat, 19 Dec 2020 23:44:20 -0500

Hi Joe,

Thanks  so much for your review and feedback. I have just published a new version of the draft that I believe covers the concerns you raised in the review.

New: https://tools.ietf.org/html/draft-ietf-ipsecme-iptfs-05 <https://tools.ietf.org/html/draft-ietf-ipsecme-iptfs-05>
Changes: https://www.ietf.org/rfcdiff?url2=draft-ietf-ipsecme-iptfs-05 <https://www.ietf.org/rfcdiff?url2=draft-ietf-ipsecme-iptfs-05>

Thanks,
Chris.

> On Dec 19, 2020, at 7:57 PM, Joseph Touch <[email-masked]> wrote:
> 
> 
> 
>> On Dec 19, 2020, at 2:16 PM, Christian Hopps <[email-masked] <mailto:[email-masked]>> wrote:
>> 
>> 
>> 
> _______________________________________________
> IPsec mailing list
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/ipsec <https://www.ietf.org/mailman/listinfo/ipsec>

---

From: Joseph Touch <[email-masked]>
Date: Wed, 23 Dec 2020 14:33:47 -0800

Looks good - thanks.

Joe

> On Dec 19, 2020, at 8:44 PM, Christian Hopps <[email-masked]> wrote:
> 
> Hi Joe,
> 
>> On Dec 19, 2020, at 7:57 PM, Joseph Touch <[email-masked] <mailto:[email-masked]>> wrote:
>> 
>> 
>> 
> Tsv-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tsv-art