Hi Tero, Thanks for the comments! Responses inline.
Best, Tommy > On Jan 11, 2017, at 7:04 AM, Tero Kivinen <[email protected]> wrote: > > This draft should be quite ready for the WGLC, so I will start that > shortly, but here are comments from my chair review of the draft. > > Consider these comments just like any WGLC comments. > > -- > > In section 6, there is text saying: > > In order to close an IKE session, either the initiator or responder > SHOULD gracefully tear down IKE SAs with DELETE payloads. Once all > SAs have been deleted, the initiator of the original connection MUST > close the TCP connection. > > I do not understand the reasoning behind the MUST, or actually also > whether it is that it MUST be initiator of the original connection > closing, it or that connection MUST be closed after all SAs are > deleted. > > If it is supposed to say that once all SAs are deleted the connection > MUST be deleted, then it does not really matter who will close it, as > once all SAs are gone, there cannot be any new IKE exchanges on the > TCP connection. > > If it is trying to say that only original initiator can close the > connection, just in case it wants to reuse it for new IKE INIT > message, then the text needs to change to say that. > > I.e., what is this MUST trying to say? This MUST should probably be downgraded to a SHOULD, and add explanation about the reuse case. The point of this text was to make sure that initiators don’t needlessly leave TCP connections open to responders, and take up resources on the responder. A better text would be: "In order to close an IKE session, either the initiator or responder SHOULD gracefully tear down IKE SAs with DELETE payloads. Once all SAs have been deleted, the initiator of the original connection SHOULD close the TCP connection if it does not intend to use the connection for another IKE session to the responder. If the connection is left idle, and the responder needs to clean up resources, the responder MAY close the TCP connection." > > -- > > > The streams of data sent over any TCP connection used for this > protocol MUST begin with the stream prefix value followed by a > complete message, which is either an encapsulated IKE or ESP message. > ... > > I think this should be rephrased in a way that when initiator creates > new TCP connection, it MUST send the stream prefix value first, > followed by a IKE or ESP message. > > The responder might receive them in pieces, because of the TCP, so > responder should not consider it an error to only receive stream > prefix and parts of the first message. If it sees partial message it > should wait for the rest of the message to come or untile timeout > occurs. Note, the responder do need timeout in this case, as we do > want to clean up the state if we have partial messages in the TCP > stream, even when the TCP connection itself is not broken. Good point, I will clarify the text to make it clearer that the prefix value must be sent first on any new connection, and that the responder has to deal with parsing partial messages. > > -- > > If the connection is being used to resume a previous IKE session, the > responder can recognize the session using either the IKE SPI from an > encapsulated IKE message or the ESP SPI from an encapsulated ESP > message. > > There should be note here and later explaining that this IKE or ESP > message must pass authentication checks, and MUST be fresh. I.e., > replaying old IKE or ESP message over tcp stream must not move traffic > to new TCP connection. This text here tells which IKE it belongs, but > later three is text saying: > > It should choose the TCP > connection on which it last received a valid and decryptable IKE or > ESP message. > > so that should also include note, that messages must be fresh. Note, > that definition of fresh is not obvious. For the ESP it is not enough > that it is unseen message in the replay window, as the replay window > might be quite large, and acquiring ESP message not seen by the other > end is quite possible. I think it is best to require that the ESP > message must be with higher sequence number ever seen before... This > should be happening anyways, as there will not be reordering happening > inside the TCP connection, so this will not cause issues for normal > cases. > > Same for the IKE SA, i.e. the message-id must be larger than anything > seen before, not just something that is in the window. I will add text to specify that the IKE and ESP messages must be valid (pass authentication and decryption), and have higher IDs than previous messages. > > Also why this text has only lowercase "should", not uppercase MUST? I may want to upgrade it to a SHOULD. If we are in a state in which there are, for a time, two TCP connections up, the only reliable way to return a message from the responder to the initiator is to use the most recently-used TCP connection. However, if a responder implementation did send a response packet on the less-recently used connection, it *might* get back to the initiator. This would probably require some special knowledge about how the implementations were using connections, but if that use case comes up in the future, I don’t think sending a packet on the other connection would necessarily make the implementation out-of-spec. However, if consensus is that this should be a MUST rather than a SHOULD, I’m fine with that! > > -- > > If either initiator or responder receives a stream that > cannot be parsed correctly, it MUST close the TCP connection. > > I think this needs more text. I.e., if it receives random garbage, > then it must close the TCP connection. But if it receives > authenticated IKEv2 message, which passes IKE SA message > authentication, but which is cannot be parsed correctly (i.e., result > in the INVALID_SYNTAX error), then it will return INVALID_SYNTAX > error, tear down the IKE SA (as specified in the RFC7296) and close > the TCP connection. Good point. I’ll try to word this generically—i.e., any parsing of the IKE and ESP frames should cause the connection to be closed, and any errors within IKE that cause the IKE session to be torn down should do the same. > > -- > > Multiple IKE SAs SHOULD NOT share a single TCP connection. > > I think this needs to be MUST NOT. I mean if we assume that the first > IKE or ESP message coming from the newly formed TCP connection tells > which IKE SA this TCP connection belongs, we cannot really use one TCP > connection for multiple IKE SAs. > > Of course we should allow IKE SA rekeying over the same TCP > connection, but I think that is only exception to the multiple IKE SAs > using same TCP connection, and in that case the one end will delete > the old rekeyed IKE SA soon. > > So I would change that to MUST, and add note about rekeying case. Sure, that sounds fine. I could imagine that if two IKE SAs were on the same connection, another message > > -- > > In section 7 we should also mention that the fact whether there is NAT > or not in the path can affect the TCP and UPD packet checksum fixup > for transport mode traffic (RFC7296 section 2.23.1). Will add. > > -- > > In section 8, there is text: > > When an IKE session is transitioned between networks using MOBIKE > [RFC4555], the initiator of the transition may switch between using > > MOBIKE is not property of the networks, it is property of the > implementations. I.e., change this to say "When an IKE session is > created between implementations using MOBIKE, …" Yes, that’s not clear. I meant “When an IKE session that has negotiated MOBIKE is transitioning between networks, the initiator…" > > -- > > In section 9: > > fragmentation. Even if fragmentation is negotiated, an > implementation MAY choose to not fragment when going over a TCP > connection. > > why allow fragmentation over TCP connection? I think we could say that > fragmentation SHOULD NOT be used when sending messages over TCP > connection? The only reason I can think why someone would use > fragmentation is that they are using mobike and the initiator is using > UDP and tries to send some large message which requires fragmentation > on UDP first, and then when the packets do not get through it tries to > move to use TCP, and in that case it MUST forward the IKE packet just > as it was sent over the UDP to the TCP, so the packet will have > fragmentation headers in it. > > So I would say MUST accept packets with fragmentation from TCP if > fragmentation is supported and enabled. SHOULD NOT fragment packets if > sending them to the TCP connection. We can switch it to a SHOULD NOT for sending fragments, and a MUST for accepting fragments. Depending on how implementations are built, the IKE logic may not be co-located with the module handling UDP or TCP, and so I expect some implementations to still use fragmentation in the naive case. > > -- > > In the security considerations section we should also mention that > attackers can do quite a lot of same tricks explained in the MOBIKE > [RFC4555] security considerations. Perhaps put pointer to there, and > think that list through and list which of those can be done also when > using TCP connection. The TCP connection has same kind of properties > than MOBIKE, as it will allow changing the source IP and port of the > incoming connection, and SGW will update its mapping to match the > latest connection… > Will add the reference and section discussing the implications of changing the source IP and port mappings. > -- > > In section B.1, the IKE messages should have "Non-ESP Marker" visible > on the figure 4. Will add. > > -- > > In section B.3, there should be the one mandatory IKE or ESP packet > after the "IKETCP" stream prefix. Perhaps use ESP here as example… Will add. > > -- > > In section B.4 step 6 should note, that this message in step 6, is > actually retransmission of the message sent in step 2, i.e., it is > exactly same IKE message including message-id and the contents. Good point, that will make the diagram clearer. > -- > [email protected] > > _______________________________________________ > IPsec mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/ipsec _______________________________________________ IPsec mailing list [email protected] https://www.ietf.org/mailman/listinfo/ipsec
