Re: [Tsv-art] Tsvart last call review of draft-ietf-ipsecme-rfc8229bis-06

[Valery Smyslov <[e-mail]>]

Hi Joe,

 

please see inline.

 

Regards,

Valery.

 

From: [e-mail] [mailto:[e-mail]] 
Sent: Monday, May 23, 2022 6:14 PM
To: Valery Smyslov
Cc: tsv-art; [e-mail]; [e-mail]; [e-mail]
Subject: [***SPAM***] Re: [Tsv-art] Tsvart last call review of draft-ietf-ipsecme-rfc8229bis-06

 

Hi Valery,

 

Notes below.

 

Joe





On May 23, 2022, at 4:53 AM, Valery Smyslov <[e-mail]> wrote:

 

Hi Joseph,

thank for your review, much appreciated. More inline.




Reviewer: Joseph Touch
Review result: Ready with Issues

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[e-mail] if you reply to or forward this review.

Overall, this document adds useful clarifications to the original RFC on
tunneling IPsec over TCP. There are a number of issues that should be addressed
as it proceeds, as noted below. All can be addressed relatively directly (i.e.,
none create new open issues).

General comments:

The document lacks (and would benefit from) a section providing details of the
differences in this update.


Good point.

can add the following text at the end of 1.1:

In particular:
o The interpretation of the Length field preceding every message is clarified
o Use of NAT_DETECTION_*_IP notifications is clarified
o Retransmission behavior is clarified
o Using cookie and puzzles is described with more detail
o Error handling is clarified
o Implications of TCP encapsulation on IPsec SA processing are expanded
o Interaction of TCP encapsulation with MOBIKE is clarified
o Section describing interactions with other IKEv2 extensions is added
o Recommendation for TLS encapsulation include using TLS 1.3

Is it OK?

 

Sure; it might also be useful to indicate the section where each is addressed in most detail.

 

          Done.

 

Figures should include captions.


I would leave this to RFC Editor (we tried to keep RFC 8229 text when possible,
and it doesn't have captions too).

 

The RFC Editor isn’t likely to care. These can be added without changing the intent of RFC 8229; in most cases, the caption is fairly obvious from the text.

 

          I’ve added titles for the figures defining headers and prefix formats 

          and removed anchors from figures in Appendix B (so they are now “inline”).

 

Given the new document adds primarily clarifications, it would be preferable if
the header numbering were not gratuitously modified vs. the original. The new
section 2 should be demoted to 1.2 as per the original; this would go a long
way to avoiding unnecessary confusion between the two.


OK, makes sense.




Specific suggestions and concerns:

Section 3 clarifies the meaning of the 16-bit length field as including both
the message and the message length field. This is counterintuitive and
problematic, notably because ESP messages could be up to 65535 bytes long. This
possibility should be addressed (e.g., prohibit tunneling of messages over
65533 bytes).


This is a a good catch, we'll add this clarification.




Section 4.2 claims the length cannot be 0 or 1 bytes; again, this suggests it
might have been better to have the length field no include the length itself.


The design decision that length field includes both the message and the message
length was made back in 2016 when RFC 8229 was developed to align 
it with 3GPP’s recommendation. We are not in a position to change this design.

 

Understood.





Regardless, it seems there are other lengths that are equally invalid (isn’t
there a min ESP header size? What about the IP packet header inside)? The true
min should be indicated.


TCP encapsulation is used for IKE too and "NAT keepalive" messages 
may still be sent by IKE (even they are not needed for TCP), which are 1 byte long.

 

It might be useful to mention that.

 

          This possibility is mentioned in Section 7.6. Added a clarification referencing this section.

 

It's a good question whether empty messages (with Length = 2) are OK.

 

>From receiver's point of view following the Postel's rule I'd simply ignore them 

and don't tear down TCP...



Added a clarification for receiving empty messages.





Section 7.1 suggests closing idle TCP connections to clean up resources; this
is inconsistent with TCP’s basic premise (don’t clean it up until those
resources are used for a new connection). There should be a more direct reason
given for this change.


If a TCP connection is no more associated with any SA, then it SHOULD be 
deleted by TCP Originator. In some cases the TCP FIN/ACK messages 
will not reach the Responder (e.g. due to network problems), 
so this TCP connection will become an orphan on Responder, 
since no new traffic will ever be sent over it. 
We see no reason to waste Responder's resources in this case - this is the reason.
Note, that this recommendation is MAY, you are free to ignore it.

 

It might be useful to indicate that the reason is to conserve responder IPsec resources, i.e., this is (to TCP) an application consideration, not a TCP one.


Done.





Section 7.1 mentions a keep-alive; it would be useful to explain whether this
is intended to use TCP keepalives or IPsec keepalives or both. It may also be
important to indicate how these keepalives might interact. This might refer to
the more detailed discussion in Section 7.6.


Section 7.1 talks about IKE keep-alive messages (more formally called "liveness check" messages).
The idea is that an encrypted and authenticated message must be received,
so that the responder may learn new SPIs, so TCP keep-alives are not suited.

Will add clarification.




Section 7.2 on retransmissions should explain the need for IPsec to continue to
retransmit messages even though the transport ensures reliable delivery (e.g.,
that messages could be ignored or delayed elsewhere in receiver processing).


There is generally no need to do it (except for the situations when TCP connection
is replaced with a new one while waiting for the response). And Section 7.2 
describes it very clear, we think. However, there is no harm if the initiator retransmits 
the request (following IKEv2 recommendations on exponential back-off between retransmissions), 
so some implementations might choose not to complicate retransmission logic and always follow
the same pattern regardless on the transport (note, that TCP encapsulation
adds quite a lot of changes to IKE codebase, so it's a valid desire to minimize them).
Thus "SHOULD NOT" retransmit in the first button in 7.2.

 

It might be useful to indicate this rationale there.


Done.





Section 7.7 discusses the relation of the IPsec DFs to the outer TCP DFs. As
with all tunnels, there need be no direct relation. The outer TCP header acts
as a link layer protocol and its frag/reassembly need have no correlation to
the inner payload. Additionally, it is nonsensical to relate the two for TCP as
a tunnel because it does not preserve message boundaries of the carried IPsec
traffic anyway. It might be useful to mention this, rather than indicating this
as “not possible”. (i.e., even if it were possible, it would be incorrect to do
so).


We believe that is exactly what Section 7.7 1-st bullet says.
Do you want some specific text to be added here?

 

It might be useful to indicate that you’re doing what tunnels (not IPsec tunnels) should be doing, not creating a new solution for this specific approach.

 

          Added a clarification.





(Note, that we are not in a position to discuss generic considerations
of using tunnels, we just explain that what is required 
by RFC 4301, is not possible with TCP. We don't want to discuss
here whether RFC 4301 requirements are wrong).

 

RFC4301’s tunnels are not the tunnel you’re describing here; agreed that those tunnel considerations are not relevant.





Section 10.1 indicates problems with TCP-in-TCP. It would probably be useful to
provide a citation with better treatment of this issue (e.g,,
https://www.spiedigitallibrary.org/conference-proceedings-of-spie/6011/1/Understanding-TCP-over-TCP--
effects-of-TCP-tunneling-on/10.1117/12.630496.short?SSO=1,
https://link.springer.com/chapter/10.1007/978-981-13-3329-3_32). This is more
commonly referred to as “TCP meltdown”; bufferbloat is a different phenomenon
with a different cause (in-network queuing that relies on tail-drop and/or
lacks ECN), and does not appear relevant to the issues presented in this
section.


We'd be happy to include good references, but It seems that both articles are behind paywalls, 
or at least require some subscription to be able to read. Do you have some free references? 
Or probably you can craft a proper text to be included in this section?

 

Being behind a paywall does not disqualify a document as an appropriate citation, especially when that is the primary source.

 

Here’s from/to text:

 

FROM:

   TCP-in-TCP can also lead to increased buffering, or bufferbloat.
   This can occur when the window size of the outer TCP connection is
   reduced and becomes smaller than the window sizes of the inner TCP
   connections.  This can lead to packets backing up in the outer TCP
   connection's send buffers.  In order to limit this effect, the outer
   TCP connection should have limits on its send buffer size and on the
   rate at which it reduces its window size.
TO:
 
   TCP-in-TCP can also lead to “TCP meltdown”, where stacked instances 
   of TCP can result in significant impacts to performance [Ho05].
   For the case in this document, such meltdown can occur when the window 


   size of the outer TCP connection is smaller than the window sizes of the inner TCP
   connections.  The resulting interactions can lead to packets backing up in the outer TCP
   connection's send buffers.  In order to limit this effect, the outer
   TCP connection should have limits on its send buffer size and on the
   rate at which it reduces its window size.

Here’s the bibtex:
@inproceedings{10.1117/12.630496,
author = {Osamu Honda and Hiroyuki Ohsaki and Makoto Imase and Mika Ishizuka and Junichi Murayama},
title = {{Understanding TCP over TCP: effects of TCP tunneling on end-to-end throughput and latency}},
volume = {6011},
booktitle = {Performance, Quality of Service, and Control of Next-Generation Communication and Sensor Networks III},
editor = {Mohammed Atiquzzaman and Sergey I. Balandin},
organization = {International Society for Optics and Photonics},
publisher = {SPIE},
pages = {138 -- 146},
keywords = {TCP, TCP over TCP, TCP tunnel, Performance Evaluation, Goodput, Round-Trip Time},
year = {2005},
doi = {10.1117/12.630496},
URL = {https://doi.org/10.1117/12.630496}
} 
 
              Thank you for the text!
 

Section 11 (security considerations) mentions the new vulnerability introduced
by the outer TCP layer, but only DoS via SYN-flooding. This connection is also
susceptible to RST and other spoofing attacks attacks (RFC4953), which should
be noted as well. Data injection attacks are not possible, but all the rest of
the TCP machinery remains vulnerable.


Good point, will add clarification and cite RFC 4953.

Regards,
Valery.

_______________________________________________
Tsv-art mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/tsv-art