Subject: [IPsec] IKEv2-bis, comments up to 2.16

From: Yaron Sheffer <[email-masked]>
Date: Wed, 20 Jan 2010 11:22:03 +0200

Abstract: "It replaces" -> "This specification replaces" (otherwise "it" could refer to the protocol itself).

1: Remove bracketed first paragraph of the Introduction.

1: "IKE message flow" -> "An IKE message flow".

1.1.3: "IPsec- protected" - remove space.

1.2: in the table, add "IKE header (not a payload)" - the other items in the table are all payloads.

1.2: The "E" payload is only mentioned here and in the Next Payload table. It is really the SK payload. I suggest to eliminate the confusing "E" notation, and replace it with SK.

1.3.2: for some reason we support negotiation of DH parameters when rekeying the IKE SA. So we should say that the INVALID_KE_PAYLOAD response is possible here, too.

1.3.3: in the exchange, replace "N" by "N(REKEY_SA)".

1.4: "The processing of an INFORMATIONAL exchange is determined by its component payloads." Adding "The payloads are processed in strict left-to-right order" would make it deterministic, and is probably what people do today.

1.4.1: "in your INFORMATIONAL" -> "in the INFORMATIONAL". In general the section is very wordy yet confusing: if each peer is responsible for deleting (=sending a delete payload for) its own incoming SAs, then why should we say that "one never sends delete payloads for the two sides of an SA in a single message".

1.4.1: the last paragraph springs a surprise by defining the behavior of IKE SA deletion while discussing an unlikely "messing up" of Child SAs. IKE SA deletion deserves its own subsection.

2: the last paragraph, beginning "The UDP payload" is out of place here.

2.1: "allowed to forget response" -> "allowed to forget a response"

2.1: please delete the "zero non-ESP marker" from the list of things that may be mutable. It is kind of funny.

2.1: either here or in 1.5 we should say something about allowing limited retransmission of the rare one-way IKE messages, for reliability.

2.3: do we allow to send SET_WINDOW_SIZE in the initial exchange? We don't say we don't - although we do say that it would only be effective starting with IKE_AUTH. But I believe we should not accept it until both peers are fully authenticated.

2.4: this sentence is inconsistent: "The INITIAL_CONTACT notification, if sent, MUST be in the first IKE_AUTH request or response, not as a separate exchange afterwards; however, receiving parties MAY ignore it in other messages." If it MUST be only at a certain location, then it should be ignored (or even rejected) in others.

2.6, first paragraph: the historical discussion going back to the previous century is very confusing to a newcomer: instead of saying what a cookie is, we tell a story. I suggest to remove this discussion or move it to the end of the section.

2.6: "they used to generate cookies" -> "used to...".

2.6: just a comment - I don't want to reopen this discussion: I think only the SPIi and Ni values should be hashed into the cookie, and not the IP. In particular because only the SPI identifies the SA, and only Ni is needed to identify a half-open SA.

2.6: can we separate the cookie discussion into its own subsection? For two reasons: it is an implementation hint, rather than a specification (as opposed to the normative text on SPIs earlier in 2.6); and it is not very important, given the prevalence of DDos.

2.8: this sentence  "It should do so if there has been no traffic since the last time the SA was rekeyed" is useless: it talks about local policy for deletion, which we are not specifying. The policy "delete the SA if there's been no traffic for the last 5 minutes" would be just as valid as this one.

2.8: the sentence "The node that initiated the surviving rekeyed SA should delete the replaced SA after the new one is established." is out of context.

2.8: the sentence beginning "From a technical correctness and interoperability perspective" is not strictly correct: there is still a race condition between the first packets on the SA and the CCSA response. Immediately starting to send is practical but not "technically correct".

2.8: "The responder can be assured that the initiator is prepared to receive messages on an SA if either (1) it has received a cryptographically valid message on the new SA." this should really be "...on the new SA's dual SA" (or better wording thereof) since there's a pair of SAs, and we are determining the state of one based on the other. In general I think we are using the terms SA and SA Pair interchangeably, which is rather confusing.

2.8: "An initiator MAY send a dummy message on a newly created SA if it has no messages queued in order to assure the responder that the initiator is ready to receive messages." A dummy message on an IPsec SA is way out of scope. Whether such messages even exist is a matter of local implementation. We certainly don't have IPsec-level keepalives. Overall, the last 3 paragraphs of 2.8 (starting "There are timing windows") are a mess: you have to read them several times before you realize that this is not normative text, rather it is several implementation alternatives.

2.8.2: this is inconsistent, and probably incorrect - the IKE SA with the lowest nonce wins here, whereas in 2.8.1 the Child SA with the lowest nonce loses.

2.8.2: we should add a sentence on what happens if the peer receives TEMPORARY_FAILURE and does not understand it (because it's new to this spec).

2.9: typo: "wants to sen".

2.9.1: "those traffic" -> "this traffic".

2.12: the sentence "Decisions as to..." is grammatically incorrect. Should be: "Decisions as to whether and when to reuse Diffie-Hellman exponentials are private in the sense that they do not affect interoperability."

2.15: the value GenIKEHDR is confusing, because it is actually NOT including in the calculation. Is suggest to replace it with a clarifying sentence.

2.16: it would be useful if we added the explicit calculation of AUTH. For example, is the padding string required for EAP as well? There are two cases, with MSK and with SK_pi+SK_pr.

2.16: if the responder sends an EAP Failure, is this IKE message acknowledged by the initiator?

2.16: the sentence saying "some simpler variations are documented here and in Section 3.16" may have been true once; it isn't now. In particular in view of my next comment.

3.16: I suggest to remove the table quoted from the EAP RFC. There are dozens of methods now in the IANA registry, many of which are preferable to the ones mentioned here.

---

From: Tero Kivinen <[email-masked]>
Date: Wed, 20 Jan 2010 14:22:01 +0200

I only commented to those cases where I disagreed with you (mostly I
think we do not need to make the change). The other comments were ok
for me.

Yaron Sheffer writes:
> 1.3.2: for some reason we support negotiation of DH parameters when
> rekeying the IKE SA. So we should say that the INVALID_KE_PAYLOAD
> response is possible here, too.

I think generic text in section 1.3 should be enough for it:

   If a CREATE_CHILD_SA exchange includes a KEi payload, at least one of
   the SA offers MUST include the Diffie-Hellman group of the KEi.  The
   Diffie-Hellman group of the KEi MUST be an element of the group the
   initiator expects the responder to accept (additional Diffie-Hellman
   groups can be proposed).  If the responder selects a proposal using a
   different Diffie-Hellman group (other than NONE), the responder MUST
   reject the request and indicate its preferred Diffie-Hellman group in
   the INVALID_KE_PAYLOAD Notification payload.  There are two octets of
   data associated with this notification: the accepted D-H Group number
   in big endian order.  In the case of such a rejection, the
   CREATE_CHILD_SA exchange fails, and the initiator will probably retry
   the exchange with a Diffie-Hellman proposal and KEi in the group that
   the responder gave in the INVALID_KE_PAYLOAD.

and this covers all subsections of 1.3 including 1.3.2, so I think
that is enough, and no additional text is needed.

> 1.4: "The processing of an INFORMATIONAL exchange is determined by
> its component payloads." Adding "The payloads are processed in
> strict left-to-right order" would make it deterministic, and is
> probably what people do today. 

There should be no need to make the processing deterministic, as at
least I cannot think of case where the processing order would matter,
and I do not want someone to start complaining if someone process them
in "wrong" order.

I do not think the addition should be done.

> 2.1: either here or in 1.5 we should say something about allowing
> limited retransmission of the rare one-way IKE messages, for
> reliability.

I have always assumed that you do not retransmit those, you simply
rate limit sending of them, and if other end sends further packets
which trigger sending new one-way IKE messages, then you will send
next one-way IKE message.

I.e. the retransmission is done automatically by the other end either
retransmitting its message (IKE packets) or other end sending more ESP
packets.

If we add text, I would rather add text saying you MUST NOT retransmit
those one-way IKE messages as they are one-way messages. You may send
new (identical) one-way IKE messages in case the same situation is
triggered again (rate limited) because other end sent another packet.

This would be consistent that attacker sending only one IKE or ESP
packet can cause only one one-way IKE message to be sent. If it wants
to peer to send another, it again needs to send another packet (i.e.
no amplification provided for attacker).

> 2.3: do we allow to send SET_WINDOW_SIZE in the initial exchange?

Yes.

> We don't say we don't - although we do say that it would only be
> effective starting with IKE_AUTH. But I believe we should not accept
> it until both peers are fully authenticated.

Regardless of the SET_WINDOW_SIZE the window size is always one until
the initial exchanges completes and initial exchanges include
IKE_INIT_SA and IKE_AUTH, thus after those have completed the parties
have been authenticated, and as you cannot have any other exchanges
while you have initial exchanges still in progess it does not really
matter.

I do not think this requires new text (except perhaps to clarify that
SET_WINDOW_SIZE can be in the initial exchange (usually in IKE_AUTH)).

We do send it in the IKE_AUTH (the same one having SA and TS payloads
if multiple IKE_AUTH messages), in the IKE SA rekey, and in the next
exchange (regardless of type) if it is changed on the fly. 

> 2.4: this sentence is inconsistent: "The INITIAL_CONTACT
> notification, if sent, MUST be in the first IKE_AUTH request or
> response, not as a separate exchange afterwards; however,
> receiving parties MAY ignore it in other messages." If it MUST be
> only at a certain location, then it should be ignored (or even
> rejected) in others.

No. The first one says where you MUST send it, the second part says
what you do if someone does not follow that MUST, which tells you MAY
ignore it.

There is no inconsistancy there.

The text is written like this as this was changed between RFC4306 and
draft-ietf-ipsecme-ikev2. The RFC4306 does not specify the location,
but IKEv2bis does, and because of this old implementations might send
INITIAL_CONTACT notifications in wrong place, and the text says that
you are free to ignore it in those places.

Support for INITIAL_CONTACT is MAY anyways (both sending and
processing it on receive).

So no need to change anything there.

> 2.8: this sentence "It should do so if there has been no traffic
> since the last time the SA was rekeyed" is useless: it talks about
> local policy for deletion, which we are not specifying. The policy
> "delete the SA if there's been no traffic for the last 5 minutes"
> would be just as valid as this one.

Yes, but the local policy given in the IKEv2bis is very common and
useful one. I do not think the text is useless, and as it does not
mandate implementations doing anything, it just says what they might
want to do, I do not see any reason to remove it.

> 2.8: the sentence beginning "From a technical correctness and
> interoperability perspective" is not strictly correct: there is
> still a race condition between the first packets on the SA and the
> CCSA response. Immediately starting to send is practical but not
> "technically correct".

For the protocol point of view it is techinally correct. I.e. after
responder has sent the response to the CREATE_CHILD_SA, for its point
of view the SA is ready and it can be used immediately.

There is race condition built in to the protocol, which is why the
text is there, i.e. it says it is techinally correct, but may still
cause packets to be dropped.

I do not see any need to change this text.

> 2.8.2: we should add a sentence on what happens if the peer receives
> TEMPORARY_FAILURE and does not understand it (because it's new
> to this spec).

There is text for that alread which says (section 3.10.1):

   Types in the range 0 - 16383 are intended for reporting errors. An
   implementation receiving a Notify payload with one of these types
   that it does not recognize in a response MUST assume that the
   corresponding request has failed entirely.  

So I do not see need to change the the 2.8.2. If change is needed
perhaps adding pointer to the 3.10.1, but I think even that is not
needed). 
-- 
[email-masked]

---

From: Yaron Sheffer <[email-masked]>
Date: Wed, 20 Jan 2010 17:47:22 +0200

Hi Tero, thanks for your response. Some comments below.

	Yaron

> -----Original Message-----
> From: Tero Kivinen [mailto:[email-masked]]
> Sent: Wednesday, January 20, 2010 14:22
> To: Yaron Sheffer
> Cc: [email-masked]
> Subject: [IPsec] IKEv2-bis, comments up to 2.16
> 
I agree.

> I do not think the addition should be done.

I'm not convinced. CP payloads can do many things, including side effects on other network components. Notifications are even more open ended, with every IKE extension adding a new one. We should recommend some deterministic processing.
> no amplification provided for attacker).
I agree.

> exchange (regardless of type) if it is changed on the fly.
OK.

> So no need to change anything there.
OK, the original sentence would suddenly make sense if the word "however" were removed.

> want to do, I do not see any reason to remove it.

I disagree. If I rekey the SA once every 24 hours (which is a reasonable policy), this would not be useful at all.
> I do not see any need to change this text.
This is getting philosophical, and it's not worth arguing about.

> perhaps adding pointer to the 3.10.1, but I think even that is not

No, this is a very special case - because such implementations are still "RFC-compliant". So we should explain how the behavior would still be reasonable with such peers.
> --
> [email-masked]
> 
> Scanned by Check Point Total Security Gateway.

---

From: Tero Kivinen <[email-masked]>
Date: Wed, 20 Jan 2010 18:29:01 +0200

Yaron Sheffer writes:
> I'm not convinced. CP payloads can do many things, including side
> effects on other network components. Notifications are even more
> open ended, with every IKE extension adding a new one. We should
> recommend some deterministic processing.

None of the current notifications or configuration payloads have such
deterministic processing requirements, and if some extension adds new
notifications of configuration payloads, then that document should
also add requirement for their deterministic processing.

For example in our implementation it is current impossible to process
payloads in the order they appear in the packet, as we first parse all
payloads from the packet, and during that tome process some of the
payloads (for example configration payloads, and some notify
payloads), but then for example delete payloads are processed later
during INFORMATIONAL exchange processing. Also most of the notify
payloads are processed later the initial pass just goes through those
we need to see early (for example the NAT_DETECTION_* payloads), but
some like INITIAL_CONTACT notifications are processed much later (i.e.
only after the IKE_AUTH AUTH payloads has been verified).

Adding requirement for order of payload processing would make
implementation quite a much harder, and as there is currently no
reason for it, I do not think it is good idea to add such requirement. 

> OK, the original sentence would suddenly make sense if the word

That would be ok for me.

> No, this is a very special case - because such implementations are
> still "RFC-compliant". So we should explain how the behavior would
> still be reasonable with such peers. 

But if someone makes special code for their implementation to
do something when it receives unknown error notify
(TEMPORARY_FAILURE), wouldn't it be better to instead make code that
understands the notify...

I mean the current code out there following the RFC4306 should be ok,
with new error codes. If it is changed in any way it should be changed
to send and understand TEMPORARY_FAILURE, not to add some special case
for unknown notifications.

Or are you thinking we should explicitly say that "if implementations
follow that 3.10.1 MUST then they work fine when they receive
TEMPORARY_FAILURE notifications even when they do not understand it"?
-- 
[email-masked]

---

From: Yaron Sheffer <[email-masked]>
Date: Wed, 20 Jan 2010 19:10:25 +0200

Hi Tero,

I meant exactly what your last paragraph says: the simultaneous rekey text is descriptive. We should describe what happens when a "new" peer meets an "old" peer, given that BOTH are legitimate.

Thanks,
	Yaron
 
> Sent: Wednesday, January 20, 2010 18:29
> To: Yaron Sheffer
> Cc: [email-masked]
> Subject: RE: [IPsec] IKEv2-bis, comments up to 2.16
> 
> Yaron Sheffer writes:
[snip]

---

From: Paul Hoffman <[email-masked]>
Date: Wed, 20 Jan 2010 13:57:00 -0800

Thanks for the in-depth review; I expect we will hear more for sections after 2.16. I have incorporated all the editor comments and opened issues for many of the technical ones. Here is my list of ones that I have rejected; it does not include ones that Tero responded to and you agreed to drop. Please let me know if you have any issues with this list.

=====

[[ Response: New requirement; not OK. ]]
 
=====
1.4.1: In general the section is very wordy yet confusing: if each peer is responsible for deleting (=sending a delete payload for) its own incoming SAs, then why should we say that "one never sends delete payloads for the two sides of an SA in a single message".
 
[[ Response: The wordiness helps implementers who were not here for the design of IKEv2. ]]

=====
 
[[ Response: it is optional behavior and makes sense. If you want a section on IKE SA deletion, you have to write it. I propose that it might be very late for doing that. ]]

=====
 
[[ Response: It is useful in the general sense of "IKE Protocol Details and Variations". It is described in more detail later. ]]

=====

[[ Response: that would be a new prohibition. I don't see text in the current doc indicating this. ]]
 
=====

[[ Response: I found those paragraphs clear, but I might have missed something. Suggest rewording in a new ticket if you wish. ]]
 
=====
 
[[ That is what we had earlier, and the WG reversed that and wanted it in the diagram. ]]

--Paul Hoffman, Director
--VPN Consortium

---

From: Yaron Sheffer <[email-masked]>
Date: Thu, 21 Jan 2010 00:26:24 +0200

Hi Paul,

Thanks for rejecting my issues so quickly :-) Please see some comments below. I have deleted issues where I accept the rejection.

	Yaron

> 1.4.1: the last paragraph springs a surprise by defining the behavior
> of IKE SA deletion while discussing an unlikely "messing up" of Child
> SAs. IKE SA deletion deserves its own subsection.
> 
> [[ Response: it is optional behavior and makes sense. If you want a
> section on IKE SA deletion, you have to write it. I propose that it
> might be very late for doing that. ]]
>
I suggest replacing the last paragraph by the following text (not a new section, though):

Similarly to ESP and HA SAs, IKE SAs are also deleted by sending an Informational exchange. Deleting an IKE SA implicitly closes any remaining Child SAs negotiated under it. The response to a request that deletes the IKE SA is an empty INFORMATIONAL response.

Half-closed ESP or AH connections are anomalous, and a node with auditing capability should probably audit their existence if they persist. Note that this specification nowhere specifies time periods, so it is up to individual endpoints to decide how long to wait. A node MAY refuse to accept incoming data on half-closed connections but MUST NOT unilaterally close them and reuse the SPIs. If connection state becomes sufficiently messed up, a node MAY close the IKE SA, as described above. It can then rebuild the SAs it needs on a clean base under a new IKE SA.
 
> [[ Response: It is useful in the general sense of "IKE Protocol Details
> and Variations". It is described in more detail later. ]]
> 
Yeah well, it still sticks out like a sore thumb. And it's fully described in 2.23, so it's redundant here.
> =====
> 2.3: do we allow to send SET_WINDOW_SIZE in the initial exchange? We
> don't say we don't - although we do say that it would only be effective
> starting with IKE_AUTH. But I believe we should not accept it until
> both peers are fully authenticated.
> 
In fact we do: "The window size is always one until the initial exchanges complete." And Tero mentioned correctly that the "initial exchanges" are both IKE_SA_INIT and IKE_AUTH. The current text is fine.
> =====
> 2.8: "An initiator MAY send a dummy message on a newly created SA if it
> has no messages queued in order to assure the responder that the
> initiator is ready to receive messages." A dummy message on an IPsec SA
> is way out of scope. Whether such messages even exist is a matter of
> local implementation. We certainly don't have IPsec-level keepalives.

> Overall, the last 3 paragraphs of 2.8 (starting "There are timing
> windows") are a mess: you have to read them several times before you
> realize that this is not normative text, rather it is several
> implementation alternatives.
> 
> [[ Response: I found those paragraphs clear, but I might have missed
> something. Suggest rewording in a new ticket if you wish. ]]
> 
I will pass on the last 3 paragraphs. But please open an issue regarding the "dummy message" text (the first part of my text above).

---

From: Tero Kivinen <[email-masked]>
Date: Thu, 21 Jan 2010 11:08:35 +0200

Yaron Sheffer writes:
> Similarly to ESP and HA SAs, IKE SAs are also deleted by sending an
> Informational exchange. Deleting an IKE SA implicitly closes any
> remaining Child SAs negotiated under it. The response to a request
> that deletes the IKE SA is an empty INFORMATIONAL response. 
> 
> Half-closed ESP or AH connections are anomalous, and a node with
> auditing capability should probably audit their existence if they
> persist. Note that this specification nowhere specifies time
> periods, so it is up to individual endpoints to decide how long to
> wait. A node MAY refuse to accept incoming data on half-closed
> connections but MUST NOT unilaterally close them and reuse the SPIs.
> If connection state becomes sufficiently messed up, a node MAY close
> the IKE SA, as described above. It can then rebuild the SAs it needs
> on a clean base under a new IKE SA.

I support doing that change, as it does clarify things.
-- 
[email-masked]

---

From: Paul Hoffman <[email-masked]>
Date: Thu, 21 Jan 2010 09:03:38 -0800

>I support doing that change, as it does clarify things.

I'll make this change unless anyone objects.

--Paul Hoffman, Director
--VPN Consortium

---

From: Yaron Sheffer <[email-masked]>
Date: Fri, 22 Jan 2010 20:09:12 +0200

Hi Tero,

I was ready to accept your answer, until I came across the following sentence in -07.

"Payloads are processed in the order in which they appear in an IKE message by invoking the appropriate processing routine according to the "Next Payload" field in the IKE header and subsequently according to the "Next Payload" field in the IKE payload itself until a "Next Payload" field of zero indicates that no payloads follow."

This seems to say exactly what I was proposing! Did I miss another statement in the document where we say the opposite (that payload order doesn't matter)?

Thanks,
	Yaron

---

From: "Valery Smyslov" <[email-masked]>
Date: Fri, 22 Jan 2010 21:55:14 +0300

Hi Yaron,

> "Payloads are processed in the order in which they appear in an IKE 
> message by invoking the appropriate processing routine according to the 
> "Next Payload" field in the IKE header and subsequently according to the 
> "Next Payload" field in the IKE payload itself until a "Next Payload" 
> field of zero indicates that no payloads follow."
>
> This seems to say exactly what I was proposing! Did I miss another 
> statement in the document where we say the opposite (that payload order 
> doesn't matter)?

The text you cited doesn't impose any requirements in terms of RFC2119.
It's just a general rule which is to be followed in most cases. But it 
doesn't
mean that there must be no exceptions to this rule.

I think, Tero is absolutely right here -  requiring payloads to be processed
in the same order as they appear in the message will make implementations
more complex. Moreover, VendorID payloads MUST be processed prior
to any other, otherwise we lose an ability to use any private values in the 
same
message (and, as a result - no private values in IKE_SA_INIT at all,
as there are no prior messages).

Regards,
Valery Smyslov.

---

From: Yaron Sheffer <[email-masked]>
Date: Fri, 22 Jan 2010 21:02:35 +0200

Hi Valery,

I would like to hear other people's opinion on this issue. At the very least, the text I'm citing should be clarified.

BTW, Vendor ID payloads can appear anywhere in the message (per Appendix C), so you can just put them first and you're fine.

Thanks,
	Yaron

> From: Valery Smyslov [mailto:[email-masked]]
> Sent: Friday, January 22, 2010 20:55
> To: Yaron Sheffer; Tero Kivinen
> Cc: [email-masked]
> Subject: Re: [IPsec] IKEv2-bis, comments up to 2.16
> 
> Hi Yaron,
> 
> > Hi Tero,
> >

---

From: Tero Kivinen <[email-masked]>
Date: Mon, 25 Jan 2010 11:10:27 +0200

Yaron Sheffer writes:
> message by invoking the appropriate processing routine according to
> the "Next Payload" field in the IKE header and subsequently
> according to the "Next Payload" field in the IKE payload itself
> until a "Next Payload" field of zero indicates that no payloads
> follow." 

That says there is nothing in the spec which requires you to do out of
order payload processing. It does not say you cannot do that, and as
there is not really anything where the result would be different
depending in which order you process payloads the way implementation
processes payloads does not really matter.

On the other hand the parsing needs to be done in the order the
payloads are in the packet (it would be very hard to do in any other
order :-), and if the parsing causes for example
UNSUPPORTED_CRITICAL_PAYLOAD, then that should be sent before the rest
of the payloads are parsed (i.e. the first unsupported critical
payload should trigger that error). 

> statement in the document where we say the opposite (that payload
> order doesn't matter)? 

Nope, but the text above does not forbid any other processing order,
as long as the end result will be same, or at least this is how I have
interpreted it.

The text is not MUST in interoperability sense, it is just text
explaining how the processing of packets happens, and as there is not
really a way to detect how the implementation processes packets from
the outside, it does not need MUST in interoperability sense.

Your text would add MUST in interoperability sense, and I would like
to see how you are going to test that some product really supports
that MUST. 
-- 
[email-masked]