TCP Encapsulation of Internet Key Exchange Protocol (IKE) and IPsec Packets
draft-ietf-ipsecme-rfc8229bis-09

# 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