Ballot for draft-ietf-ipsecme-rfc8229bis
Yes
No Objection
Note: This ballot was opened for revision 07 and is now closed.
# SEC AD comments for draft-ietf-ipsecme-rfc8229bis-07 CC @paulwouters ## Comments I have a number of comments and some small fixes. While the appendix fixes technically might be a DISCUSS, I have faith the authors will pick it up from the comment section too :) ### The Length field plus non-ESP marker plus IKE packet is called "message" at the start in Section 3, but in Section 3.1 it is called an "IKE Header Format" and "IKE message" is used to denote the non-ESP marker plus IKE Header _without_ the Length field. And then it uses "IKE Packet" in the Length field description to mean the thing without the Length and non-esp marker. And then the figure uses "IKE header" were it should probably say "IKE packet". These names should be made more consistent :) ### Section 3.1 and Section 3.2 state: The value in the Length field MUST NOT be 0 or 1. In fact, a lot more values are clearly wrong, like anything under 4 which cannot contain the non-esp marker. Then there is the size of the minimum UDP IKE or ESP packet as well. Why are these two values singled out? ### Section 3.1 states: The IKE header is preceded by a 16-bit Length field in network byte order that specifies the length of the IKE message Section 3.2 states: The ESP header is preceded by a 16-bit Length field in network byte order that specifies the length of the ESP packet Why don't both say either "message" or "packet"? I would say "packet" for both. ### Section 4: at the beginning of any stream of IKE and ESP messages. I would s/any/the TCP/ to avoid people thinking of the inner streams and thinking they need to repeat the IKETCP prefix for each burst of traffic - this mistake was made once in the first version of the Linux kernel code. ### Section 5: when it has been configured to be used with specific IKE peers. I would say "when it has been configured to be optionally used with specific IKE peers. Otherwise, implementers might think it needs to be an on/off setting instead of a "may be used when udp is blocked" setting. Similarly: If a Responder is configured to use TCP encapsulation, I would say "is configured to accept TCP encapsulation" (nit: "it is recommended that Initiators should only use TCP encapsulation" can be written more clearly by omitting the "should") ### If TCP encapsulation is being used for a specific IKE SA, all messages for that IKE SA and its Child SAs MUST be sent over a TCP connection I think "messages" here is unclear. It might be confused for control messages (IKE) only and not mean data (ESP) packets. I would use: If TCP encapsulation is being used for a specific IKE SA, all IKE and ESP packets for that IKE SA and its Child SAs MUST be encapsulated and sent over this TCP connection Note that technically, this is not true because if you want to see if UDP becomes available again, you have to send an IKE packet over UDP, but it is probaly clearer not to mention that here. ### Section 6.1 using the configured TCP port. I would say "using the preconfigured Responder's TCP port" ### The first bytes sent on the stream MUST be the stream prefix value Maybe again say "TCP stream" ? This also made me realize that Section 4. could use a diagram to ensure people do it right, eg: Initiator Responder Prefix|Length|non-ESP marker|IKE packet -> <- Length|non-ESP marker|IKE packet Length|non-ESP marker|IKE packet -> <- Length|non-ESP marker|IKE packet [...] Length|ESP packet -> <- Length|ESP packet ### If a TCP connection is being used to resume a previous IKE session, I would use "continue" instead of "resume" to avoid confusion with Session Resumption. Also instead of "previous IKE session" maybe say "previous encapsulation session"? Note: at this point it has also not been made clear in the draft whether multiple IKE SAs can use the same encapsulation session. That information should probably have been conveyed earlier in the document. This is only stated at the end of Section 6.1 ### Implementations SHOULD NOT tear down a connection if only a single ESP message has an unknown SPI, since the SPI databases may be momentarily out of sync. This advise might be difficult to follow. If this out of sync really happens, it would be likely that a number of ESP packets would be seen before the IKE packet comes in that syncs up the SPI. (assuming this out of sync issue happens when the TCP responder is also the IKE responder to a rekey and once it rekeyed and installed a new IPsec SA it starts sending ESP packets before it sends its IKE rekey reply, or a number of smaller ESP packets arrive sooner somehow?) ### nit: if the Responder chooses to send Cookie request add " a ", eg "a Cookie request". ### Section 6.3 talks a lot about how COOKIE stuff is both useless for TCP and can fail in mysterious new ways. Why not simplify things and say "cookies must (should?) not be verified and fully ignored when over TCP, and only puzzles should be verified" ### Section 6.3.1 assumes the responder knows the rough CPU power of its clients and that these are all in the same ballpark. Is this really the case? ### section 6.4 nit: in case it receives error notification -> in case it receives an error notification ### section 6.5: When negotiating over UDP port 500, IKE_SA_INIT packets include NAT_DETECTION_SOURCE_IP and NAT_DETECTION_DESTINATION_IP payloads An IKE peer is allowed to skip port 500 and use 4500 directly. It would still send the NAT payloads. I would write "When negotiating over UDP, IKE_SA_INIT packets include". ### How about sharing an alternative to the transport mode checksum fixups: Implementations MAY use the information that a NAT is present to omit sending USE_TRANSPORT over TCP, and thus enforce tunnel mode IPsec SA's to avoid the need for checksum fixups for encapsulated packets inside TCP. ### I would personally split out NAT-T and keepalive into its own seperate sections. People already confuse them too often and it is really two completely different things. ### Section 6.6 NAT keep-alive packets over a TCP- encapsulated IPsec connection will be sent as an ESP message with a one-octet-long payload with the value 0xFF. I don't think you can call it an ESP message? I understand you say this so implementers will know there is no non-ESP marker, but its really not an ESP message. Maybe something like: NAT keep-alive packets over a TCP- encapsulated IPsec connection will be sent as a one-octet-long payload with the value 0xFF, preceded by the two byte Length specifying a length of 1. ### for which purpose the standard IKEv2 mechanism of exchanging empty INFORMATIONAL messages is used I believe these INFORMATIONALs are not neccessarilly empty, even though they started out that way. I would just leave out the word "empty". ### Section 6.7 mentions QoS, but we are also working on per-CPU IPsec SA's, which would have the same problem. Perhaps that can be worked into the existing text? I would perhaps also state here that the effects on performance are not very important, as doing TCP encapsulation in itself is already reducing performance significantly. ### nit: Section 7.4 starts with a broken reference to [RFC6311] ### Section 8: TCP encapsulation of IKE should therefore use common TCP behaviors to avoid being dropped by middleboxes. I'm not sure why this text is here? Perhaps you mean to say: If the IKE implementation has its own minimal implementation of TCP, it SHOULD still use common TCP behaviors to avoid being dropped by middleboxes. That at least clarifies that one needs to do nothing if one uses the OS TCP stack. ### Section 9: Implementations SHOULD favor using direct ESP or UDP encapsulation over TCP encapsulation whenever possible. I understand the SHOULD relates to "whenever possible", but since we are talking about "SHOULD favor", I think we can say "MUST favor". It's only favoring after all :) ### Section 10: nit: [RFC5961] is a broken reference. nit: it will be re-created by TCP Originator [add "the"] ### Alternatively, implementations MAY try to re-sync after they receive unknown SPIs by searching the TCP stream [...] This is an odd paragraph. "You can do this, but really it is futile". I would remove it. ### An attacker capable of blocking UDP traffic can force peers to use TCP encapsulation, thus degrading the performance and making the connection more vulnerable to DoS attacks. Note, that attacker capable to modify packets on the wire or to block them can prevent peers to communicate regardless of the transport being used. I don't think this paragraph is useful either. There is nothing an implementer can do with this information. ### TCP Responders should be careful to ensure that (1) the stream prefix "IKETCP" uniquely identifies incoming streams as streams that use the TCP encapsulation protocol and (2) they are not running any other protocols on the same listening port (to avoid potential conflicts). I thought (2) was actually a good thing. One can run a HTTPS server while also acting as a VPN server and demultiplex based on the prefix. So I don't think the advise in (2) is appropriate and just limits the deployment possibilities. ### The successful delivery of valid IKE or ESP messages over a new TCP connection is used I think this should say "valid new IKE or ESP messages" (as explained right below it) Though if an attacker can block packets, they can take a valid new message, and stuff it in their own source IP packet and send it to cause the TCP stream to be deviated. Perhaps recommend sending a liveness probe once a TCP connection is redirected? (although even then, we only detect we are dead - we cannot go back to the original stream) ### Section B.4 1) ----------------- IKE_SA_INIT Exchange ----------------- (IP_I1:UDP4500 -> IP_R:UDP4500) Non-ESP Marker -----------> Initial IKE_AUTH The marker ------ foo ------ has been used to describe what follows, but what follows here is an IKE_AUTH exchange, not an IKE_SA_INIT 6) ------------ Retransmit Message from step 2 ------------- Length + Non-ESP Marker -----------> INFORMATIONAL HDR, SK { N(UPDATE_SA_ADDRESSES), N(NAT_DETECTION_SOURCE_IP), N(NAT_DETECTION_DESTINATION_IP) } <------- Length + Non-ESP Marker HDR, SK { N(NAT_DETECTION_SOURCE_IP), N(NAT_DETECTION_DESTINATION_IP) } This seems to miss the line "INFORMATIONAL" on the responder side. Same for step 7. 1. During the IKE_SA_INIT exchange, the client and server exchange MOBIKE_SUPPORTED notify payloads to indicate support for MOBIKE. See above, step 1 shows an IKE_AUTH, not IKE_SA_INIT so does not match this description. 6. The client sends the UPDATE_SA_ADDRESSES notify payload on the TCP-encapsulated connection. I find this wording misleading. The UPDATE_SA_ADDRESSES payload is not send on the TCP connection, but a whole IKE message containing this payload is sent.
Thank you very much for addressing my DISCUSS concerns.
# Éric Vyncke, INT AD, comments for draft-ietf-ipsecme-rfc8229bis-07 CC @evyncke Thank you for the work put into this document. There must be several use cases for it. Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education). Special thanks to Tero Kivinen for the shepherd's detailed write-up including the WG consensus, but it lacks the justification of the intended status. Thanks as well to Brian Haberman for his INT directorate review, his review has a 'ready' status. I hope that this review helps to improve the document, Regards, -éric ## COMMENTS ### UDP blocked even with QUIC As there are more and more traffic relying on QUIC (a wild guess of mine...), is it still the case that middle boxes are blocking UDP ? Just out of curiosity... feel free to ignore. ### Section 1 ``` Devices on these networks that need to use IPsec (to access private enterprise networks, to route Voice over IP calls to carrier networks, or because of security policies) are unable to establish IPsec SAs. ``` The above appears like an exhaustive list while it is not. Suggest to add ", etc.". ### Section 1 `Some peers default to using UDP encapsulation` is "peer" so well-defined in the IPsec world ? 'some' is also rather vague, perhaps cite one implementation ? or use "some peers may default to" ? ### Section 2 Should "Implementations of this specification" be used in `Implementations MUST support TCP encapsulation on TCP port 4500, which is reserved for IPsec NAT traversal.` ? ### Section 3 No AH Even if AH is nearly no more used, I wonder whether there is a reason why AH is not supported by this I-D. The sentence about AH should really come earlier in the document. ### Section 3 ``` Although a TCP stream may be able to send very long messages, implementations SHOULD limit message lengths to typical UDP datagram ESP payload lengths. ``` What is the 'typical' length ? ### Section 3.1 Suggest to add a description of "Non-ESP" header field below the description of the "length" field rather than in the text above. ### Section 5.1 `Since UDP is the preferred method of transport for IKE messages,` hem... previous text (and common sense) stated that direct is the preferred method. ### Section 6.1 what about IP address changes ? ``` Since new TCP connections may use different ports due to NAT mappings or local port allocations changing, the TCP Responder MUST allow packets for existing SAs to be received from new source ports. ``` For some NAT devices (or IPv6 nodes w/o NAT but with temporary addresses), the IP address could also change. This document should describe what to do in this case. ### Section 6.5 The very last sentence deserves a paragraph on its own. ### Section 6.7 Please add that the DF bit is obvioulsy only for IPv4 (Hi, Tommy, I had to insert my mandatory IPv6-related comment ;-) ) ### Section 9.3 I am not a transport person, but I would have used "MUST NOT" rather than "MAY NOT" in: ``` Individual packets SHOULD NOT use different markings than the rest of the connection, since packets with different priorities may be routed differently and cause unnecessary delays in the connection. ``` Even if somehow obvious, should there be a statement about the IPv6 Flow-ID header field ? ### TCP Fast Open support Is TCP fast open supported by this doc ? ## Notes This review is in the ["IETF Comments" Markdown format][ICMF], You can use the [`ietf-comments` tool][ICT] to automatically convert this review into individual GitHub issues. [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md [ICT]: https://github.com/mnot/ietf-comments
# Internet AD comments for {draft-ietf-ipsecme-rfc8229bis-07} CC @ekline ## Comments ### S1.1 * In "Cellular Network Access", is there a particular TS number to reference for this claim about preferring TLS for IWLAN setup? ### S2 * "Implementations MUST support TCP encapsulation on TCP port 4500": which implementations, exactly? Only TCP-supporting implementations, or all IKE/IPsec implementations? ### S6.1,6.3+,7.1,7.3,B.1,B.3,B.4 * Can the "IKETCP" be sent in a 7413 Fast Open (say, when reconnecting)? Can other IKE initiating messages be included with the SYN? Alternatively: are there concerns with use of Fast Open such that it should be forbidden? I don't see any mention of Fast Open anywhere in this doc, and I kinda think /something/ should maybe be said, but IANATP... (I am not a transport person) ### App. A * Is there an ALPN that is typically used with TLS? ## Nits ### S3.1 * "MUST close TCP connection" -> "MUST close the TCP connection" ### S6.4 * "after receiving error notification" -> "after receiving an error notification"? ### S6.7 * "stack manages DF bit" -> "stack manages the DF bit" ### S9.1 * "between all flows" -> "among all flows", perhaps ### S10 * "Note, that attacker capable to modify" -> "Note that an attacker able to modify" ### Acknowledgements * It seems a bit weird for an Author to Acknowledge himself (Tommy Pauly), but oh well ;-) :-)
I reviewed the diff to RFC 8229 and didn't notice anything of concern to the ART area. I concur with Eric about the shepherd writeup, and in particular the unanswered part of the first question.
Thanks to Joe Touch for the TSVART review. There are no showstoppers in this document, but some non-normative text makes inaccurate statements about TCP and RFC6040, and there are some odd decisions with respect to TLS that are worth asking about: (Sec 9.1) "TCP-in-TCP can also lead to "TCP meltdown", where stacked instances of TCP can result in significant impacts to performance [TCP-MELTDOWN]. 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." Which "window" are you referring to? Receive window, congestion window, or the send buffer? My reading of [TCP-MELTDOWN] is that the tunnel ingress should set its send buffer size to the BDP of the tunnel, which is easier said than done. It appears you are using "window" and "send buffer" interchangeably here in a way that is confusing. (9.5) Implementations SHOULD follow the ECN compatibility mode for tunnel ingress as described in [RFC6040]. In compatibility mode, the outer tunnel TCP connection marks its packet headers as not ECN-capable. If upon egress, the arriving outer header is marked with CE, the implementation will drop the inner packet, since there is not a distinct inner packet header onto which to translate the ECN markings. This is not an accurate summary of compatibility mode. In compatibility mode, the outer packet is marked Not-ECT, which means it will not be marked CE. In normal mode, the outer packet is marked as the inner, and this might result in an outer CE marking. It's a shame that there is no attempt to figure out a mapping between inner and outer that would make normal mode work, as this reviewer is optimistic that ECN marks will become increasingly important. But regardless, this section should be clear and make correct statements. (Appendix A) Why not provide an in-band way to determine TLS support? There could be another port number, or different magic bytes to indicate that TLS handshake messages follow.