Re: [IPsec] Paul Wouters' Yes on draft-ietf-ipsecme-rfc8229bis-07: (with COMMENT)

[Valery Smyslov <[e-mail]>]

Hi Paul,

thank you for the very thorough review (as usual) :-). Please see inline.

> Paul Wouters has entered the following ballot position for
> draft-ietf-ipsecme-rfc8229bis-07: Yes
> 
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this introductory
> paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-
> ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-ipsecme-rfc8229bis/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> # 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 :)

Sure :-)

> ###
> 
> 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 :)

I agree that there is some mess in using different terms (message, packet etc.).
Most of this mess was in RFC 8229 and we tried to keep the text when possible.
Those cases, while formally not accurate, didn't cause confusion (in our opinion), so no attempts 
to fix them were done (note, that similar mess exists in RFC 7296 as a leftover from 5996 and 4306,
and at the time 7296 was being developed my attempts to fix some terms were rejected
by the WG with the resolution - those places never caused confusion, so let them be :-)).

That being said, I think that we can fix at least some of these discrepancies.

So, I've change in sections 3.1 and 3.2:

s/IKE Header/IKE Message
s/ESP Header/ESP Packet

including titles and figures.

> ###
> 
> 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?

Because only these two values don't allow to continue parsing the TCP stream
(to find the next Length field in the stream).

We try to keep layer separation here - obviously, the content of the packets
may be malformed, but it is checked by another layer. The checks you described
are identical for both UDP and TCP encapsulation and are not specific for this draft.

BTW, the value 3 for the Length field is valid, even if it is under 4  :-) - 
in case of NAT keepalive packet (discouraged, but not prohibited over TCP).

> ###
> 
> 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.

As I said there is a little mess, but traditionally we use in IPsec terms "IKE message"
and "ESP packet". "IKE packet" is also used but somehow rarely. We just try to follow
this tradition :-)

> ###
> 
> 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.

OK, changed.

> ###
> 
> 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.

Well, I think these implementation details need not to be covered in the spec.
While UDP is preferred, there may be situations, where it is known for sure
that UDP is permanently blocked, so there is no need to try it each time introducing additional delay.
For example, our implementations may be configured with three choices (per peer) - 
never use TCP, may use TCP if UDP is blocked, always use TCP.

I'd leave the text as is.

> Similarly:
> 
>         If a Responder is configured to use TCP encapsulation,
> 
> I would say "is configured to accept TCP encapsulation"

Hm, I think "use" is more generic, after accepting TCP
the responder will also send something over it :-)

I suggest:

s/is configured to use/is configured to accept and use

is it OK?

> (nit: "it is recommended that Initiators should only use TCP encapsulation" can
> be
> written more clearly by omitting the "should")

OK, I removed "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

Since we traditionally call "IKE message" and "ESP packet", I suggest:

         If TCP encapsulation is being used for a specific IKE SA, all
         IKE messages for that IKE SA and ESP packets for 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.

Agree.

> ###
> 
> Section 6.1
> 
>          using the configured TCP port.
> 
> I would say "using the preconfigured Responder's TCP port"

OK.

> ###
> 
>         The first bytes sent on the stream MUST be the stream prefix value
> 
> Maybe again say "TCP stream" ?

OK.

> 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

OK, I added the diagram and also moved the figure 3 closer to the beginning of the section for readability.

> ###
> 
>         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.

OK.

> Also instead of "previous IKE session" maybe say "previous encapsulation
> session"?

Then what is "encapsulation session"? This term is not defined in the spec.
I think we may leave the text as is for simplicity, although I see your point.

> 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

Moved last para of 6.1 to just before this text.

> ###
> 
>         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?)

There may be delays while installing the ESP SAs depending on the implementation.
E.g. if you have an asynchronous API between IKE and kernel.

> ###
> 
> nit:
> 
>         if the Responder chooses to send Cookie request
> 
> add " a ", eg "a Cookie request".

Done.

> 
> ###
> 
> 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"

I think it's too radical change. Cookies can still be verified with TCP
if no source IP and port are included into their calculation.

> ###
> 
> 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?

With puzzles some estimation should be made anyway.
In any case, 6.3.1 is only about optimization, so wrong guess won't hurt too much.

> ###
> 
> section 6.4
> 
> nit: in case it receives error notification -> in case it receives an error
> notification

Fixed.

> ###
> 
> 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".

Good catch, fixed.

> ###
> 
> 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.

This is not specific to TCP encapsulation. And I think that the selection of mode
may be driven by various considerations, so avoiding checksum fixups
is usually not the primary one.

> ###
> 
> 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.

Done.

> ###
> 
> 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.

OK (except that the Length is 3, since it includes the length of the Length field).

> ###
> 
>         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".

Well, actually any successful IKE exchange serves as a liveness check,
so we may want to leave out INFORMATIONAL too :-)

But the point is that RFC 7296 specifies sending empty INFORMATIONAL
request as a dedicated mechanism for liveness check.

If you mean that an "empty" message in fact contains the Encrypted payload,
then I believe we are going too far in an attempt to be accurate and 
this would confuse implementers. Sure any message is sent encrypted
once IKE SA is established, we are speaking about its internal content...

So, I'd leave the text as is.

> ###
> 
> 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.

Sure.

> ###
> 
> nit: Section 7.4 starts with a broken reference to [RFC6311]

This seems to be a tooling problem when html is produced from txt
(the -07 version was published in txt since there were problems 
with xml upload). The reference is OK in xml and is OK in html when rendered
from xml (see for example the -06 version of the draft).

> ###
> 
> 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.

OK, makes sense. Added this clarification.

> ###
> 
> 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 :)

OK, English is difficult :-)

> ###
> 
> Section 10:
> 
> nit: [RFC5961] is a broken reference.

The same tooling problem as with [RFC6311].

> nit: it will be re-created by TCP Originator [add "the"]

Fixed.

> ###
> 
>         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.

We describe an alternative way as well as its drawbacks.
It's not always futile, but it may happen be such.

I'd leave it for completeness, and TSV guys (Joe Touch) also wanted it be documented.

> ###
> 
>         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.

Not implement TCP encapsulation :-)

Seriously, I think it's better to file all the possible problems,
even if little can be done in some cases.

> ###
> 
>         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.

I tend to agree with you. I removed (2).

> ###
> 
>         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)

Added this clarification.

> 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)

True.

> ###
> 
> 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

Fixed. Also added full 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.

Exactly. Fixed. Also fixed some other nits in this diagram. Thank you for catching.

>    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.

Fixed.

>    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.

Changed to:

              The client sends the UPDATE_SA_ADDRESSES notify payload in the 
              INFORMATIONAL exchange on the
              TCP-encapsulated connection.

Thank you again for the very thorough review! Much appreciated.

Regards,
Valery.