Subject: [IPsec] My review of the draft-ietf-ipsecme-iprfs-05.txt

From: Tero Kivinen <[email-masked]>
Date: Sun, 17 Jan 2021 23:27:14 +0200

First, thanks for the Joe for very good TSV review for this draft.

I am now going through this draft to see whether it is ready for WGLC,
and while reviewing it I have some comments on it. On the other hand I
think it is starting to get ready for WGLC...

In section 2.2.3 fragmentation etc, the draft talks about replay
protection, but in the ESP replay attack protection is optional, and
does not need to be done by the receiver, altough the sequence number
generation MUST be done by sender.

Also normally in ESP seqeuence numbers are not required to be back to
back, there could be cases where they would be incrementing by larger
value, and cases where the sequence numbers are not sent out in order.
This can happen for example if multiple encryption boards are used,
and each of them have separate coordinated sequence number generation.
This can also cause reordering in some cases. The draft now says that
"sender MUST Send the inner-packet fragments back-to-back", which
adds a new requirement for ESP. It might be good idea to add note,
that says this changes requirement from RFC4303, which only requires
that sequence numbers are incremented, not by how much (for example
draft-ietf-lwig-minimal-esp proposes that in some enviroments it might
be better to use time for sequence number generation so they do not
need to store sequence number while sleeping). 

Section 2.2.3 does not change the replay protection parts, just talks
about that similar method than what is used in the replay protection
can be used for reassembly. This may cause issues if the ESP level
does not do replay protection, but the reassembly code assumes it is
done. The text also seems to indicate that the window sizes for the
reassembly and replay protection could be different.

If reassembly and replay windows are not same, I think the smaller one
of them is used, as if replay window is smaller, then too old packets
are dropped before they would reach the larger reassembly window, and
same happens for other way around.

I think we could just say that when using this detection of replays is
mandatory and that replay window and the reassembly window are same.

Also note, that increasing replay window size does not have that much
effect on the memory consumption of the SA, but increasing reassembly
window size does have. I.e., if the quite often used 1024 packet
replay window size is matched with same sized reassembly window of
1024 * 1024 byte packets, that will mean each SA could consume up to 1
MB for reassembly window.

Also even when the 2.2.3 says in that there is no need for timers, I
disagree with that, as during rekey or other operations it might be
possible that the traffic from the SA is moved to the newly created
SA, and then few minutes later the old SA is deleted. Keeping the
reassembly buffers for that long just because there is no incoming
packets is just wasting data. 

In section 2.2.2 and 2.2.3.1 the draft tries to fill the packets to
full, even when there would only be one byte fitting on the frame, and
I think this is bad idea. I would think it would be better to say that
if less than 5% (or some other fixed configurable amount) of the
packet is available and the packet to be added would be fragmented,
then packet is padded instead of fragmenting, and the packet to be
added is moved to next outer packet.

I think there is too much emphasis on not wasting the bandwidth and
too much emphasis on the robustness of the protocol. Small amount of
padding that will avoid fragmentation is much more desirable than not
wasting few % of the bandwidth.

In section 2.2.5 it says that ECN value is not mapped, because of
constant-send-rate, but it leaves out the other part, i.e., if some
device is sending inner data faster than the constant-send-rate then
the IPTFS should set the ECN for inner packets when adding them to the
aggregated frame, just to indicate this information to the other end.

In section 2.2.7 the draft says there is no effective MTU, which is
true, but the question is that should there be one? I mean even if the
original sender will automatically detect that 63kB UDP packets go
through without fragmentation that does not mean that it will be good
idea to use 63kB packets as that will cause latency for all other
packets sharing the same SA. I think it would be useful to be able to
restrict the effective MTU of the tunnel.

In section 2.3 why does the text say:

   In other words, an SA that has
   AGGFRAG_PAYLOAD enabled MUST NOT have non-AGGFRAG_PAYLOAD payloads
   such as IP (IP protocol 4), TCP transport (IP protocol 6), or ESP pad
   packets (protocol 59) intermixed with non-empty AGGFRAG_PAYLOAD
   payloads.

why is that restriction with only non-empty AGGFRAG_PAYLOAD payloads?
When can empty AGGFRAG_PAYLOAD used intermixed with other payloads?

The rekeying of the SA is not specified fully. It does say that
rekeying cannot change the USE_AGGFRAG information, but it does not
say that rekeyed SA actually inherits the USE_AGGFRAG flag. It should
explictly mention that.

Also it does not mention whether frame is allowed to to be fragmented
across the old and new SA. It should say that as sequence numbers are
different for the SAs, the packet can never be fragmented so that part
of it is in old SA, and part of it is in the newly created SA.

In section 2.2.4 draft says: ESP payload length is equal to the
AGGFRAG_PAYLOAD header length. This would indicate that this packet is
not the same length than other frames. I think it would be better to
say that this has just the AGGFRAG_PAYLOAD header, and then rest of
the frame is padding.
-- 
[email-masked]

---

From: Christian Hopps <[email-masked]>
Date: Tue, 19 Jan 2021 11:33:39 -0500

Hi Tero,

I have published a new version of the draft which I believe addresses the issues you raise in your WGLC readiness review below. As such, I'd like to request the document enter WGLC.

Changes indicated inline below.

> On Jan 17, 2021, at 4:27 PM, Tero Kivinen <[email-masked]> wrote:
> 

Modified the text to include "optional" and make more clear we are only referring to the window method that is commonly used for this.

> need to store sequence number while sleeping).

Added paragraph on new "incrementing sequence numbers by one" requirement.

> reassembly and replay protection could be different.

The lack of the assumption is also more clear with the additional text about the window sizes when both IP-TFS and replay protection are in use.

> same happens for other way around.

Added text to point this out.

> mandatory and that replay window and the reassembly window are same.

Continued to leave the option to use replay protection or not to the user. This change isn't required so I'd rather leave it as optional as the base ESP spec does.

> packets is just wasting data.

Added a more text saying timers are OK and could save some memory.

> wasting few % of the bandwidth.

Added sentence describing a method for minimum fragment size configuration as well.

> aggregated frame, just to indicate this information to the other end.

Added paragraph indicating ECN can still be set on inner fragments according to RFC3168.

> restrict the effective MTU of the tunnel.

Added sentence indicating MTU MAY still be explicitly configured on the tunnel.

> When can empty AGGFRAG_PAYLOAD used intermixed with other payloads?

Added sentence explaining this and referring back to "Empty Payload" section which also explains this.

> say that rekeyed SA actually inherits the USE_AGGFRAG flag. It should

Added sentence indicating the inheritance.

> of it is in old SA, and part of it is in the newly created SA.

Added sentence saying inner packets should not be fragmented across different SAs.

> say that this has just the AGGFRAG_PAYLOAD header, and then rest of

These are only used to transmit congestion control information back to the sender on non-IP-TFS SAs, there's no reason to pad them out.

Once again I believe we are ready for WGLC.

Thanks,
Chris.

> --
> [email-masked]
> 
> _______________________________________________
> IPsec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ipsec
>

---

From: Tero Kivinen <[email-masked]>
Date: Mon, 25 Jan 2021 03:53:50 +0200

Christian Hopps writes:
> I have published a new version of the draft which I believe
> addresses the issues you raise in your WGLC readiness review below.
> As such, I'd like to request the document enter WGLC. 

Thanks, I will start WGLC soon. 

> The lack of the assumption is also more clear with the additional
> text about the window sizes when both IP-TFS and replay protection
> are in use.

Yes, the new text seems much better. 

> Continued to leave the option to use replay protection or not to the
> user. This change isn't required so I'd rather leave it as optional
> as the base ESP spec does.

Actually the way you do reassembly already does replay protection, as
if you receive frame with same sequence number you have already
received you drop it. Or can you explain when there would not be
effective replay protection enabled when IPTFS is enabled. It does not
matter whether the replay protection happens in the IPsec replay
protection window or in the reassembly algorith, the offered
functionality is same. 

> section which also explains this.

This I do not understand. I assumed the congestion control only
covered the IPTFS SA, and it was done per SA bases. How does this
congestion control work when it is controlling non IPTFS SAs? Why do
you need it if you are not doing traffic flow confidentiality, but are
only sending frames based on the normal traffic patterns, and the
normal congestion control functions should work normally without
issues?  

> Added sentence saying inner packets should not be fragmented across

I think it should say that implementations MUST NOT send initial
fragments of an inner packet using one SA and subsequent fragments in
a different SA, as allowing this would make reassembly much harder, as
it needs to check that the older frames are the last ones to be sent
on that old SA, and there must not be any others after them etc...

I think it would be better to just assume that those SAs do have
separate windows, thus fragments can never cross over from one SA to
other. 

> These are only used to transmit congestion control information back
> to the sender on non-IP-TFS SAs, there's no reason to pad them out.

So these packets are not same size than others, thus outsiders can
see that these are congestion control information frames, and not real
traffic. Can they also get some other information of the inner flows
by analysing those empty frames?

> Once again I believe we are ready for WGLC.

I will start 3 week WGLC on the document, ending 2021-02-15. 
-- 
[email-masked]

---

From: Christian Hopps <[email-masked]>
Date: Mon, 25 Jan 2021 05:27:38 -0500

> On Jan 24, 2021, at 8:53 PM, Tero Kivinen <[email-masked]> wrote:
> 
> Christian Hopps writes:

Right. From an operational point of view I figured it comes down to whether one gets a specific type of log message/alert.

> normal congestion control functions should work normally without

Correct the CC is only covering an IPTFS SA; however, the receiver has to send back information to the sender for CC to function, that is what the empty payload is for, to send the CC information back over the non-IPTFS SA. This is only going to be used when the user has configured IPTFS in only one direction -- we're covering all the bases here.

> on that old SA, and there must not be any others after them etc...

OK.

> by analysing those empty frames?

See above, but these empty payloads are not being protected by IPTFS they are just for transmitting required info back to the sender.

> I will start 3 week WGLC on the document, ending 2021-02-15.

Thanks!

Chris.