Subject: [IPsec] Review of draft-ietf-ipsecme-tcp-encaps-05

From: "Valery Smyslov" <[email-masked]>
Date: Thu, 2 Feb 2017 15:13:51 +0300

Hi,

here is my review of draft-ietf-ipsecme-tcp-encaps-05.
The draft is in a good shape, but a bit of final polishing is required.

1. The terms "tunnel", "tunneled" are used throughout the document
    when referring to ESP SA. I think it is technically incorrect, since 
    ESP supports both tunnel and transport modes, so the document
    should use more appropriate terms like "IPsec SA", "ESP packets" etc.

2. Section 4.
    This value is
   only sent once, by the Initiator only, at the beginning of any stream
   of IKE and ESP messages.

   If other framing protocols are used within TCP to further encapsulate
   or encrypt the stream of IKE and ESP messages, the Stream Prefix must
   be at the start of the Initiator's IKE and ESP message stream within
   the added protocol layer [Appendix A].

Using "Initiator" is wrong here, since the TCP stream may be re-established
after the peers rekeyed IKE SA and changed their roles (so that original
Initiator becomes Responder). It either must be "original Initiator" 
(with all explanations who it is, as in Section 6) or, preferably, 
"originator of TCP stream" (or smth like that).

Actually, "Initiator" is used in a lot of places throughout the document 
and in most cases it means "original Initiator of the first IKE SA in a series
of possible successive rekeys of this SA". It is good to look through 
all these uses and either clarify this term in all places it is used, or add 
some para in the beginning of the draft, e.g. like it is done in RFC4555:

   In this document, the term "initiator" means the party who originally
   initiated the first IKE_SA (in a series of possibly several rekeyed
   IKE_SAs); "responder" is the other peer.  During the lifetime of the
   IKE_SA, both parties may initiate INFORMATIONAL or CREATE_CHILD_SA
   exchanges; in this case, the terms "exchange initiator" and "exchange
   responder" are used.  The term "original initiator" (which in [IKEv2]
   refers to the party who started the latest IKE_SA rekeying) is not
   used in this document.

I also noticed that both "Initiator" and "initiator" are used in the document
(as well as "Responder" and "responder"). It's better to use one form
(preferably with capital letter, like in RFC7296). RFC Editor will change 
them to one form in any case, so you can ease her work a bit.

I'd also suggest to introduce some term for the party, that creates 
TCP session (e.g. "TCP originator") and use it where appropriate,
so that IKE roles are clearly distinct from TCP roles. In this case
you can get rid of "Initiator" in most places replacing it with "TCP originator".

3. Section 6.
   When the IKE initiator uses TCP encapsulation for its negotiation,...

"its negotiation" is confusing here, since TCP encapsulation is not negotiated.
I suggest removing "for its negotiation" completely (or change to "for IKE SA negotiation").

4. Section 6.
   Once all
   SAs have been deleted,...

"all SAs" is confusing. Later in this section it is stated that multiple IKE SAs
MUST NOT share a single TCP connection. Is this a leftover from an early design?

5. Section 6.
   If the connection is being used to resume a previous IKE session...

For clarity:  s/connection/TCP connection

6. Section 6.
   A responder SHOULD at any given time send packets for an IKE SA and
   its Child SAs over only one TCP connection.

Why only Responder? What about Initiator? I think this requirement
is valid for both.

7. Section 6.
   In order to be considered valid for choosing a TCP
   connection, an IKE message successfully decrypt and be authenticated,
   not be a retransmission of a previously received message, and be
   within the expected window for IKE message IDs.  Similarly, an ESP
   message must pass authentication checks and be decrypted, not be a
   replay of a previous message.

"an IKE message successfully decrypt" - is it OK with the grammar?
Shouldn't it be: "an IKE message must be successfully decrypted"?

What about ES, I think that it is a good thing to add 
a requirement, that ESP window size MUST be set to 1 if TCP
encapsulation is in use. Larger windows are useless with TCP,
since there is no packet reordering. On the other hand, setting 
window size to 1 will effectively block an attack vector that was described
by Tero, when an attacker sends old packet with fake TCP header.
If window size is 1, then this packet will be dropped immediately
without any changes in the ESP logic.

8. Section 6.
    Multiple IKE SAs MUST NOT share a single TCP connection.

Please add clarification: "unless one of them is a rekey of another,
in which case two SAs sharing a single TCP connection coexist for 
a short period of time" (or smth similar).

Regards,
Valery.

---

From: Tommy Pauly <[email-masked]>
Date: Fri, 03 Feb 2017 13:13:42 -0800

Hi Valery,

Thanks so much for your comments! I have posted a new version of the draft here:
https://tools.ietf.org/html/draft-ietf-ipsecme-tcp-encaps-06

Responses inline.

Best,
Tommy

> On Feb 2, 2017, at 4:13 AM, Valery Smyslov <[email-masked]> wrote:
> 
> Hi,
> 

Great point. I have removed the references to tunnel, and replaced with SA, generally.

> them to one form in any case, so you can ease her work a bit.

I've switched to use the capitalized version of Initiator and Responder.

> you can get rid of "Initiator" in most places replacing it with "TCP originator".

I added the terms "TCP Originator" and "TCP Responder" in the terminology section, with an explanation of this distinction. I've used the new terms where appropriate throughout the document.

> I suggest removing "for its negotiation" completely (or change to "for IKE SA negotiation").

Removed the phrase.

> MUST NOT share a single TCP connection. Is this a leftover from an early design?

Switched to "Once the SA has been..."

> For clarity:  s/connection/TCP connection

Switched to "if a TCP connection".

> Why only Responder? What about Initiator? I think this requirement

Slightly modified the phrasing. The paragraph above describes the Initiator/Originator behavior.

> Shouldn't it be: "an IKE message must be successfully decrypted"?

Thanks for catching! Fixed.

> without any changes in the ESP logic.

Added this recommendation to the security considerations.

> a short period of time" (or smth similar).

Added the clarification.

---

From: "Valery Smyslov" <[email-masked]>
Date: Tue, 7 Feb 2017 16:54:53 +0300

Hi Tommy,

thank you for the quick update. A couple nits:

1. Section 1.2:
s/the the/the

2. Section 1.2:
       The TCP  
       Originator MUST be the same as the "Original Initiator", or the  
       Initiator of the first IKE SA exchange for a given IKE session.

Again, this is confusing. In RFC7296 the term " Original Initiator" is defined as:
    The "original initiator
     always refers to the party who initiated the exchange that resulted
     in the current IKE SA.  In other words, if the "original responder"
     starts rekeying the IKE SA, that party becomes the "original
     initiator" of the new IKE SA."

"the Initiator of the first IKE SA exchange for a given IKE session" is also
wrong, since IKE session means IKE SA and it can be a rekey of previous
IKE SA when entities swap their original roles.

This is now what we want it to mean, so an additional clarification is needed.
Something along the lines:

    The TCP Originator MUST be the same as the entity who originally
    initiated the first IKE SA (in a series of possibly several rekeyed
    IKE SAs).

Otherwise it looks good.

Regards,
Valery.

> I added the terms "TCP Originator" and "TCP Responder" in the terminology section, with an
explanation of
> this distinction. I've used the new terms where appropriate throughout the document.
> 
> >
> > 3. Section 6.

---

From: Tommy Pauly <[email-masked]>
Date: Tue, 07 Feb 2017 09:44:19 -0800

Hi Valery,

Thanks for the feedback! I'll clarify that the TCP Originator is the same as the original initiator of the first IKE SA, as well as fixing the typographical errors.

If anyone else has more feedback, please chime in! I'll wait a day or so before updating the draft, to batch any other changes that should be made.

Thanks,
Tommy

> On Feb 7, 2017, at 5:54 AM, Valery Smyslov <[email-masked]> wrote:
> 
> Hi Tommy,
> 
> _______________________________________________
> IPsec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ipsec

---

From: Tommy Pauly <[email-masked]>
Date: Thu, 09 Feb 2017 09:04:41 -0800

Hello,

I've posted a new draft with a fix for the TCP Originator vs Original Initiator explanation, and a couple typos.

https://tools.ietf.org/html/draft-ietf-ipsecme-tcp-encaps-07

I believe this addresses all outstanding comments!

Thanks,
Tommy

> On Feb 7, 2017, at 9:44 AM, Tommy Pauly <[email-masked]> wrote:
> 
> Hi Valery,
>