Re: [IPsec] WGLC for draft-ietf-ipsecme-ikev2-multiple-ke

[Valery Smyslov <[e-mail]>]

Hi Paul,

thank you for the review. Please find my comments inline.

> > This is the start of 2 week WGLC on the
> > draft-ietf-ipsecme-ikev2-multiple-ke document, ending 2021-08-10.
> 
> Note that this document has a prerequisite on the intermediate exchange,
> so even if it passed WGLC/IETF LC before intermediate exchange, it will
> have to wait in an RFC Editor cluster for the other draft.

I see no problem here. Actually, it was expected :-),
for this reason the intermediate exchange draft was 
sent to LC earlier, so that it would be ahead of multiple ke draft.
Unfortunately, things slowed down a bit, but it is still ahead.

> > Please submit your comments to the list, also send a note if you have
> > reviewed the document, so we can see how many people are interested in
> > getting this out.
> 
> I have reviewed the document. In general I support this document. I
> really like the idea of renaming the DH Registry to KE. I do think it
> is not ready yet though. My comments and questions follow below.
> 
>  	Key exchange methods negotiated via Transform Type 4 MUST always take
>  	place in the IKE_SA_INIT exchange.  Additional key exchanges
>  	negotiated via newly defined transforms MUST take place in a series
>  	of IKE_INTERMEDIATE exchanges, in an order of the values of their
>  	transform types, so that key exchange negotiated using Transform Type
>  	n always precedes that of Transform Type n + 1.
> 
> I don't understand this section, specifically the use of "Transform Type 4"
> and "Transport Type n+1", as we only have transform type 5 and nothing
> higher and that is Extended Sequence Number.

A one para above new Transform Types are introduced:
Additional Key Exchange 1, Additional Key Exchange 2,
Additional Key Exchange 3, etc. Once added to IANA registry,
they will get their numbers (hopefully in an order of their names).
The text is trying to say, that when additional KE are being negotiated,
the order in which they will take place is defined by the order
of assigned Transform Types (which will correspond to the order of their names).

In other words, if at the end of negotiation peers decided that 
they will perform base KE (negotiated via Transform Type 4, which this draft
renames to "Key Exchange Method (KE)") 2048-bit MODP Group, 
and two additional Key Exchanges - SIKE_P434, negotiated in 
"Additional Key Exchange 3" transform, and FRODOKEM_640_AES,
negotiated in "Additional Key Exchange 5" transform, then
they agree to use 2048-bit MODP Group in the IKE_SA_INIT,
then SIKE_P434 in the first IKE_INTERMEDIATE
followed IKE_SA_INIT and FRODOKEM_640_AES in the next
IKE_INTERMEDIATE.

If, for example, they negotiated the same group in Transform Type 4,
but two additional Key Exchanges - SIKE_P434, negotiated in 
"Additional Key Exchange 4" transform, and FRODOKEM_640_AES,
negotiated in "Additional Key Exchange 1" transform, then
they will perform 2048-bit MODP Group in the IKE_SA_INIT,
then FRODOKEM_640_AES in the first IKE_INTERMEDIATE
followed IKE_SA_INIT and SIKE_P434 in the next
IKE_INTERMEDIATE.

> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xhtml#ikev2-parameters-8
> 
> I think it might be trying to say if there are more than one Key Exchange,
> that the subsequent key exchange should follow in the next IKE message
> exchange (eg in a round of IKE_INTERMEDIATE) ?

It is implied here (and defined in Section 3.1). But the real message here is that the order of key exchanges 
performed in a sequence of IKE_INTERMEDIATE is defined by the order of "Additional Key Exchange *"
transforms via which they are negotiated.

>  	Each IKE_INTERMEDIATE exchange MUST bear exactly one key exchange method.
> 
> I don't understand why there is this limitation. What if some Key
> Exchange mechanism will require 2 RTTs. Why preventively forbid that?

We didn't consider such exotic key exchange methods and, I think,
if they appear, they will deserve a separate document.
But the real message here is that you cannot perform
several key exchanges using a single IKE_INTERMEDIATE exchange.
In other words, you cannot put several public keys
of different Key Exchange methods in a single IKE_INTERMEDIATE 
message.

>  	Additional key exchange methods are proposed
>  	using Additional Key Exchanges transform types.  All these transform
>  	types are optional, the initiator is free to select any of them for
>  	proposing additional key exchange methods.  Consequently, if none of
>  	Additional Key Exchange transforms are included in the proposal, then
>  	this proposal indicates performing standard IKEv2, as defined in
>  	[RFC7296].
> 
> So how does an intiiator convey that it deems an additional Key Exchange
> to be mandatory?

	If the initiator includes "Additional Key Exchange N" transform,
	then the responder MUST choose a single method from 
	those included in this transform. If the list of included
	methods doesn't contain NONE, then the responder
	have to choose one of them, so performing this KE 
	method is mandatory. To make it optional the initiator
	includes NONE among other methods. This allows
	the responder to select NONE and thus to skip doing
	this additional KE.

>  	If the initiator includes any transform of type n (where
>  	n is among Additional Key Exchanges) in the proposal, the responder
>  	MUST select one of the algorithms proposed using this type.  A
>  	transform ID NONE may be added to those transform types which contain
>  	key exchange methods that the initiator believes are optional.
> 
> And so I again do not understand this. What is "n" here? a new transform
> type ? ( eg n=6 ??)  or a new entry in the Transform Type 4 Key Exchange
> registry?

	n here is new Transform Type. We define "Additional Key Exchange 1",
	"Additional Key Exchange 2", "Additional Key Exchange 3" ...
	up to "Additional Key Exchange 7". 

> At his point, the Additional Key Exchange is introduced, and I am
> beginning to understand things. This should really be explained before
> the text I pointed at above to make any sense to the reader. And see
> below on placing "Additional Key Exchanges" into the "Key Exchanges"
> Registry.

Actually, the new transforms are introduced in 3.2.1, but I see your point
and probably we should add more clarifications before this section.

(I also noted that Additional Key Exchange transforms are mentioned at the end of 
3.1 before their formal introduction, that must be fixed).

> The next part explains the CREATE_CHILD_SA and IKE_FOLLOWUP_KE exchanges. I
> personally would prefer that a different exchange than CREATE_CHILD_SA
> is used if the completion of such an exchange does not lead to a fully
> rekeyed state. This use of completing a CREATE_CHILD_SA and being in a
> state that is not "rekeyed" or "failed" complicates the state machine.

That's true. However, there is a tradeoff here. If you defined a new two-message exchange,
that performed multiple key exchange methods at once, then 
the initiator would include public keys for all KE methods it proposed
which is terrible for performance reasons, since the responder may
reject most of them. The message size is also would grow dramatically,
and in case the responder selected a tiny subset of what was proposed,
this would be extremely inefficient.

>  	Each IKE_FOLLOWUP_KE exchange MUST bear exactly one key exchange method.
> 
> See above. Why this preventative limitation?

Again, for simplicity it is prohibited to perform several key exchanges in a single IKE exchange
(IKE_INTERMEDIATE or IKE_FOLLOWUP_KE).

>  	The data associated with this notification is a blob meaningful only to the responder
> 
> Why a blob? Why not the imminent new SPI it generated for this new IKE SA?
> If you really want a blob, there should be an example of how to generate
> the blobs. I don't see any such guidance in the document.

The purpose of this data blob is to allow the responder to link 
CREATE_CHILD_SA and subsequent IKE_FOLLOWUP_KE between
themselves in case the initiator creates several Child SAs
(or rekeys them, or rekeys IKE SA) simultaneously.

In this situation the following sequence is plausible:

   Initiator                             Responder
   ---------------------------------------------------------------------
   HDR(CREATE_CHILD_SA), SK {SA, Ni, Kei, TSi, TSr} -->
                             <--  HDR(CREATE_CHILD_SA), SK {SA, Nr, KEr, TSi, TSr
                                      N(ADDITIONAL_KEY_EXCHANGE)(link01)}

at this point some event triggers the initiator to start
another CREATE_CHILD_SA, while the first sequence
is not complete yet:

   HDR(CREATE_CHILD_SA), SK {SA, Ni, Kei, TSi, TSr } -->
                             <--  HDR(CREATE_CHILD_SA), SK {SA, Nr, KEr, TSi, TSr
                                      N(ADDITIONAL_KEY_EXCHANGE)(link11)}

note, that without any additional means the responder
at this point is unable to say whether next IKE_FOLLOWUP_KE
belongs to the first CREATE_)CHILD_SA ir to the second.
To make it possible the responder sends back ADDITIONAL_KEY_EXCHANGE
with some blob only meaningful to it, that allows it to link
the incoming IKE_FOLLOWUP_KE with proper CREATE_CHILD_SA.

   HDR(IKE_FOLLOWUP_KE), SK {KEi(2),
    N(ADDITIONAL_KEY_EXCHANGE)(link11)} -->
                                  <--  HDR(IKE_FOLLOWUP_KE), SK {KEr(2)}

   HDR(IKE_FOLLOWUP_KE), SK {KEi(1),
    N(ADDITIONAL_KEY_EXCHANGE)(link01)} -->
                                  <--  HDR(IKE_FOLLOWUP_KE), SK {KEr(1)}

The draft doesn't give any advice of how to generate this blob,
because it's a local matter of responder.

> Below is an example of three additional key exchanges.
> 
>     Initiator                             Responder
>     ---------------------------------------------------------------------
>     HDR(CREATE_CHILD_SA), SK {SA, Ni, KEi} -->
>                               <--  HDR(CREATE_CHILD_SA), SK {SA, Nr, KEr,
>                                        N(ADDITIONAL_KEY_EXCHANGE)(link1)}
> 
> 
> Why does the initiator not start out with a N(ADDITIONAL_KEY_EXCHANGE) ?
> There has been an Additional Key Exchange in the initial exchanges, so
> why not start out with one in the rekey from the initiator?

Because N(ADDITIONAL_KEY_EXCHANGE)(data blob) is for the responder 
to allow it to properly link IKE_FOLLOWUP_KE with this particular CREATE_CHILD_SA.
At the time initiator sends CREATE_CHILD_SA request it doesn't know
yet what to put into (data blob) of ADDITIONAL_KEY_EXCHANGE.
Note, that this data may be dynamic and is completely unrelated
to the data blob sent in previous exchanges. 

> What would an initiator do if the responder omited N(ADDITIONAL_KEY_EXCHANGE) ?

If the responder omits N(ADDITIONAL_KEY_EXCHANGE) then from its
point of view no more IKE_FOLLOWUP_KE is expected in the context
of this CREATE_CHILD_SA. If from responder point of view they negotiated 
more additional key exchanges than were performed, then it should be treated
in the same way, as if some mandatory payload is absent (e.g. the responder
omitted KE payload in CREATE_CHILD_SA when some key exchange is negotiated).
I think it's a fatal error.

> It has given no indication it might be mandatory from the initiator's point of view.

You are right, this must be clarified in the draft.

>  	It is possible that due to some unexpected events (e.g. reboot) the
>  	initiator could forget that it is in the process of performing
>  	additional key exchanges and never starts next IKE_FOLLOWUP_KE
>  	exchanges.  The responder MUST handle this situation gracefully
> 
> And wouldn't that solve this weird state issue if the initiator already
> signalled this clearly in CREATE_CHILD_SA, so the responder could in
> that case already return an error in the CREATE_CHILD_SA exchange?

Sorry, I didn't follow your idea. Can you elaborate?

> Note that I find the argument weird, why would an IKE peer forget some
> of its state after a reboot? Both peers should always remember AKE's
> were used and have to be used again upon (PFS) rekeys.

It's true, but in this case the initiator forgets that it started CREATE_CHILD_SA
(with AKEs) before reboot. In this case it never continues this sequence
after rebooting and restoring last stable IKE SA state.

>  	it MUST send back a new error type notification STATE_NOT_FOUND.
>  	This is a non-fatal error notification
>  	[...]
>  	If the initiator receives this notification in
>  	response to IKE_FOLLOWUP_KE exchange performing additional key
>  	exchange, it MUST cancel this exchange and MUST treat the whole
>  	series of exchanges started from the CREATE_CHILD_SA exchange as
>  	failed.
> 
> So why use a non-fatal error notification that leads to a guaranteed failure ?

Because the failure may be recoverable - just start new CREATE_CHILD_SA
with AKEs. I see no need to delete IKE SA in this case.
Of course, if the failure persisted after several attempts, then
the only thing peer can do - delete SA.

> Note there seems to be a notion of AKE's having continuous state, as
> opposed to (EC)DH where you start a new state with the rekey exchange.
> Perhaps this should be clarified at the beginning of the document?

Can you please provide text candidate?

>   This document adds the following Transform Types to the "Transform
>     Type Values" registry:
> 
>     Type     Description                   Used In
>     -----------------------------------------------------------------
>     <TBA>    Additional Key Exchange 1     (optional in IKE, AH, ESP)
>     <TBA>    Additional Key Exchange 2     (optional in IKE, AH, ESP)
>     <TBA>    Additional Key Exchange 3     (optional in IKE, AH, ESP)
>     <TBA>    Additional Key Exchange 4     (optional in IKE, AH, ESP)
> 
> 
> Why are the descriptions referring to "additional" key exchange? I would
> assume the registry is just a list of Key Exchange types, and whether
> one of these is "additional" or not depends on its use in IKE ? That is,
> one of the Key Exchanges that today is "additional" might one day be
> used as the only Key Exchange without Additional Key Exchanges? If we
> really are making some of these exchanges as "additional use only", then
> we should really create a new transform type registry for AKE that is
> separate from the Key Exchange Type (4).

Additional here means that KE methods negotiated via these transforms
are always performed in addition to KE method, negotiated via Transform Type 4.
All these transform types share the same registry of Transform IDs with Transform Type 4.
Note, that these transform types have different semantics than other IKEv2 
transforms - they don't concern with own algorithm type (they all
are concerned with KE methods), they only allow to negotiate
which KE methods perform additionally to a base KE method
and in which order. If one day you don't need to perform additional key exchanges
then just don't use these transforms.

>  	the key lengths of these
>  	transforms SHALL be at least 256 bits long in order to provide
>  	sufficient resistance to quantum attacks.
> 
> I would use MUST instead of SHALL.

OK.

>  	The main focus of this document is to prevent a passive attacker
>  	performing a "harvest and decrypt" attack.  In other words, an
>  	attacker that records messages exchanges today and proceeds to
>  	decrypt them once he owns a quantum computer.  This attack is
>  	prevented due to the hybrid nature of the key exchange.  Other
>  	attacks involving an active attacker using a quantum-computer are not
>  	completely solved by this document.  This is for two reasons.
> 
> I think this part and the text right underneath this belongs in the
> Introduction more than it belongs in the Security Considerations
> section.

This probably makes sence. I'll discuss this with my co-authors.

>  	Unfortunately, this design is susceptible to the following
>  	downgrade attack.
> 
> I'm not convinced this is an issue actually. If you really do think a
> proper configuration would not sufficiently protect against this, the
> initiator could send a notify in the second IKE_SA_INIT of the rejected
> KE value from the previous response, so that this attack would be
> detected.

I won't comment on this - this is just an appendix with early 
design alternatives. I'll let my co-authors to comment on this.

>  	We discarded this approach because we believe that the working
>  	group may not be happy using the RESERVED field to change the
>  	format of a packet and that implementers may not like the
>  	complexity added from checking the fragmentation flag in each
>  	received payload.
> 
> I think that is exactly why we have RESERVED fields. As implementer, I
> don't think that this would have been too complex. But I think using
> INTERMEDIATE is fine, especially if multiple solutions can use this
> method for their own usage, and having a generic method is better than
> having specific methods per postquantum/hybrid solution.

OK, we are in agreement :-)

> Nits:
> 
> a typo:  CRETE_CHILD_SA
> 
> 
> this side MUST not initiate -> this side MUST NOT initiate

Will fix, thanks.

Thank you again for thorough review.

Regards,
Valery.

> Paul
> 
> _______________________________________________
> IPsec mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ipsec