Re: [IPsec] Paul Wouters' Discuss on draft-ietf-ipsecme-ikev2-multiple-ke-10: (with DISCUSS and COMMENT)

Paul Wouters <paul@nohats.ca> Tue, 29 November 2022 18:49 UTC

Return-Path: <paul@nohats.ca>
X-Original-To: ipsec@ietfa.amsl.com
Delivered-To: ipsec@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5951AC15258C; Tue, 29 Nov 2022 10:49:29 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.093
X-Spam-Level:
X-Spam-Status: No, score=-2.093 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_BLOCKED=0.001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=nohats.ca
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OqyGBqDPSr8h; Tue, 29 Nov 2022 10:49:25 -0800 (PST)
Received: from mx.nohats.ca (mx.nohats.ca [193.110.157.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7A63AC1524D1; Tue, 29 Nov 2022 10:49:22 -0800 (PST)
Received: from localhost (localhost [IPv6:::1]) by mx.nohats.ca (Postfix) with ESMTP id 4NMBFr3zP3z27r; Tue, 29 Nov 2022 19:49:20 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nohats.ca; s=default; t=1669747760; bh=Drk8PvMNZiXMwgs+m6d8iA4BMBNNbGMC2YNCin9+xGo=; h=Date:From:To:cc:Subject:In-Reply-To:References; b=TPJ/sQC+9cz9nZyu9o7GOJ9HxMFPhMc8BjSUCzERrlrJlodn9IlYREy5PIh06/S9O Siyn1yUTQe58xAXo5HuzdcgCzC6j8TbkCLt1YDJhHxmc29U8mBSWemmR4anY/ntUrt ZaAFoAurDu9xD9sova06a7wkA5zFvZxGjox27rDg=
X-Virus-Scanned: amavisd-new at mx.nohats.ca
Received: from mx.nohats.ca ([IPv6:::1]) by localhost (mx.nohats.ca [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id edv1QP-nWEeU; Tue, 29 Nov 2022 19:49:18 +0100 (CET)
Received: from bofh.nohats.ca (bofh.nohats.ca [193.110.157.194]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx.nohats.ca (Postfix) with ESMTPS; Tue, 29 Nov 2022 19:49:18 +0100 (CET)
Received: by bofh.nohats.ca (Postfix, from userid 1000) id 5767C41448F; Tue, 29 Nov 2022 13:49:17 -0500 (EST)
Received: from localhost (localhost [127.0.0.1]) by bofh.nohats.ca (Postfix) with ESMTP id 55AA041448E; Tue, 29 Nov 2022 13:49:17 -0500 (EST)
Date: Tue, 29 Nov 2022 13:49:17 -0500
From: Paul Wouters <paul@nohats.ca>
To: Valery Smyslov <svan@elvis.ru>
cc: 'Paul Wouters' <paul.wouters@aiven.io>, 'The IESG' <iesg@ietf.org>, draft-ietf-ipsecme-ikev2-multiple-ke@ietf.org, ipsecme-chairs@ietf.org, "ipsec@ietf.org WG" <ipsec@ietf.org>, Tero Kivinen <kivinen@iki.fi>
In-Reply-To: <150601d90418$3cf314b0$b6d93e10$@elvis.ru>
Message-ID: <d1d99f3b-f3c3-1bb4-756b-075d636ea328@nohats.ca>
References: <166966973968.20669.3414159618699952233@ietfa.amsl.com> <150601d90418$3cf314b0$b6d93e10$@elvis.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"; format="flowed"
Archived-At: <https://mailarchive.ietf.org/arch/msg/ipsec/qLWuoFoAPm2afa7dtuLqYiNE2Mo>
Subject: Re: [IPsec] Paul Wouters' Discuss on draft-ietf-ipsecme-ikev2-multiple-ke-10: (with DISCUSS and COMMENT)
X-BeenThere: ipsec@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Discussion of IPsec protocols <ipsec.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ipsec>, <mailto:ipsec-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ipsec/>
List-Post: <mailto:ipsec@ietf.org>
List-Help: <mailto:ipsec-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ipsec>, <mailto:ipsec-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 29 Nov 2022 18:49:29 -0000

On Tue, 29 Nov 2022, Valery Smyslov wrote:

>> ### IANA entries mentions in the Introduction ?
>>
>> Shouldn't the introduction mention this draft introduces the IKE_FOLLOWUP_KE Exchange
>> and the STATE_NOT_FOUND Notify Message Type, along with additional entries to the
>> (now renamed) Key Exchanges Methods registry?
>
> I'm a bit puzzled here. The introduction currently is silent about the details of the proposed
> extension. Do you want to add them there? Or did you mean the Abstract,
> (which indeed contains some details) to be augmented with more details?

Yes I meant the abstract :)

>> ### Additional key exchanges DoS ?
>>
>> The following paragraph raised an issue for me:
>> ```
>>    If the responder selects NONE for some Additional Key Exchange types
>>    (provided they are proposed by the initiator), then the corresponding
>>    IKE_INTERMEDIATE exchanges MUST NOT take place.
>> ```
>> If the initiator's local policy requires at least one Additional Key Exchange,
>> an attacker sending back a quick reply with only NONE replies would be a DoS.
>> I think similar to like the original IKE_SA_INIT, perhaps we need to give some
>> advise that if the local policy would lead to permanent failure, that it
>> should wait for other (more legitimate) responses to this IKE_SA_INIT ?
>
> As you pointed out, the situation is exactly the same as described
> in Section 2.4 of RFC 7296:
>
>   One type of DoS attack on the initiator of an IKE SA can be avoided
>   if the initiator takes proper care: since the first two messages of
>   an SA setup are not cryptographically protected, an attacker could
>   respond to the initiator's message before the genuine responder and
>   poison the connection setup attempt.  To prevent this, the initiator
>   MAY be willing to accept multiple responses to its first message,
>   treat each response as potentially legitimate, respond to each one,
>   and then discard all the invalid half-open connections when it
>   receives a valid cryptographically protected response to any one of
>   its requests.  Once a cryptographically valid response is received,
>   all subsequent responses should be ignored whether or not they are
>   cryptographically valid.
>
> Since the draft adds nothing new into the negotiation mechanism
> in the IKE_SA_INIT, I see no need to reiterate this advice here
> (otherwise we may want to repeat a lot of text from base IKEv2 spec :-)).

I don't think it would hurt pointing to Section 2.4 of RFC 7296. I see
this as a likely possible implemention mistake.

>> ### ADDITIONAL_KEY_EXCHANGE
>> ```
>>    After IKE SA is created the window size may be greater than one and
>>    multiple concurrent exchanges may be in progress, it is essential to
>>    link the IKE_FOLLOWUP_KE exchanges together
>> ```
>> I had some trouble figuring out why these are needed. For Child SA rekeys, these
>> would not be needed, because we would have an old SPI and MSGID that would make the
>> order obvious. But for adding addtional Child SA's, we have no old SPI. But we have
>> a new SPI on the initiator (and then a new SPI on the responder in the answer. Since
>> these are coupled by MSGID, I wonder if ADDITIONAL_KEY_EXCHANGE is really needed?
>> Looking at the useful appendix examples, I realise that the IKE_FOLLOWUP_KE exchange
>> does not have an SA payload so no SPI, so it makes sense to me now. Perhaps a sentence
>> in the document would be useful to explain this?
>
> How about:
>
>   After IKE SA is created the window size may be greater than one and
>   multiple concurrent exchanges may be in progress, it is essential to
>   link the IKE_FOLLOWUP_KE exchanges together and with the
>   corresponding CREATE_CHILD_SA exchange. Due to the fact
>   that once IKE SA is created all IKE exchanges are independent and
>   don't have built-in means to link one with another, a new status type
>   notification ADDITIONAL_KEY_EXCHANGE is introduced for this purpose.
>
>  (feel free to edit the text).

Looks good, thanks.

>> I still do not know why not to use the SPI as value for ADDITIONAL_KEY_EXCHANGE instead
>> of an opaque linking blob? The SPI is traditionally our linking blob.
>
> The opaque blob can be an SPI. Or something else, that the responder
> thinks better suits for this purpose. In my implementation it is the
> MID of the exchange containing the previous key exchange (either
> CREATE_CHILD_SA or IKE_FOLLOWUP_KE).

Harumph. I guess. ok.

>> Could the IKE_FOLLOWUP_KE set the SPI value in the IKE header instead of using
>> a new ADDITIONAL_KEY_EXCHANGE payload and use that with the MSGID as linking blob?
>
> Hm, we don't have a room in the IKE header for a new SPI :-)
> Am I missing your point?

Never mind - I was wrong :)

>> ### State loss issue
>> ```
>>    After receiving this notification the initiator MAY start
>>    a new CREATE_CHILD_SA exchange which may eventually be followed by
>>    the IKE_FOLLOWUP_KE exchanges, to retry the failed attempt.  If the
>>    initiator continues to receive STATE_NOT_FOUND notifications [...]
>> ```
>> How could this happen? If the state was lost, eg due to reboot, there would
>> need to come a new IKE SA, that can then send a new CREATE_CHILD_SA. I don't
>> see how that could lead to another STATE_NOT_FOUND. But the paragraph then
>> also continues with "and delete [the] IKE SA". But this IKE SA is brand new?
>> I would just remove this entire paragraph as I think this cannot happen. Or
>> at least it is not a special case and existing abort code handles this already.
>
> No, this is not concerned with reboots. It is concerned with the situation
> when it takes too long for the initiator to compute the next public key
> value to send to the responder (the delay may also be caused by lost packets).
> So, when the responder first receives the CREATE_CHILD_SA exchange
> and negotiates using multiple key exchanges, it expects that some
> IKE_FOLLOWUP_KE exchanges shortly follow (with some definition
> of "shortly"). So, it creates a state that would contain all the shared
> key computed as a result of these exchanges. But the responder
> cannot keep this state forever (for example, the initiator may
> be switched off and never send next request), so the responder
> have to do housekeeping and remove this state if the responder
> doesn't receive the next IKE_FOLLOWUP_KE exchange in a timely fashion
> (again, with some definition of "timely fashion"). Then consider the
> situation when initiator cannot make the next request within
> this period (either it takes too long to compute the key or some request
> messages were lost and the responder received the n-th retransmission).
> In this case the responder receives the IKE_FOLLOWUP_KE exchange,
> but it doesn't already have the state to which it can add this new
> shared key, so it responds with NO_STATE_FOUND. This is non-fatal
> notification, so the initiator may start all from scratch (from the
> CREATE_CHILD_SA exchange) in a hope that this time its performance
> or network conditions would be better.
>
> Does this help?

Yes, but then I think you need to split the text that is currently in
one large paragraph into multiple ones, because the start of the
paragraph has:

    It is possible that due to some unexpected events (e.g. reboot) the
    initiator may lose its state

So it is not clear that a large part of this paragraph does not in fact
involve "state lost due to reboot". So please split the paragraph a bit
into pieces to make this more clear.

>> ### IKE session resumption
>>
>> Should there be a section updating RFC 5723 Section 5.1, or is the method there
>> specified quantum-safe if the initial IKE SA was protected using this document's
>> mechanism? See https://www.rfc-editor.org/rfc/rfc5723.html#section-5.1
>>
>> I think the IKE resumption can work "as normal", as no KE payload is
>> involved in the resumption, but it would be nice if a sentence somewhere
>> in this document could confirm this.
>
> Yes, I agree. We didn't add anything since there is no interaction with
> RFC 5723 (it works "as normal"). We can add some text if you think
> it is needed.

Just a one sentence somewhere would be nice yes.

>> Also RFC 5723 states:
>> ```
>> The keys and cryptographic protection algorithms should be at
>>       least 128 bits in strength.
>> ```
>> IF we live in Grover universe, perhaps that should be 256 bits in strength? And since
>> we are making things quantum safe with this document, perhaps we should then at least
>> state session tickets should be 256 bits. Note if we do, then this document must
>> Update: RFC 5723. Perhaps this note on 5723 can be added in the Security Considerations
>> Section paragraph that talks about Grover and Shor.
>
> The recent recommendations from NIST for the safe symmetric key size went back to 128 bits,
> because the Grover algorithm appears to require so large amount of quantum memory,
> that it makes it almost impractical.
>
> Note, that the Security Considerations section of the draft says:
>
>  It was previously believed that one needed to double
>   the key length of these algorithms.  However, there are a number of
>   factors that suggest that it is quite unlikely to achieve the
>   quadratic speed up using Grover's algorithm.  According to NIST
>   [NISTPQCFAQ], current applications can continue using AES algorithm
>   with the minimum key length of 128 bit.

Thanks for the clarification. That resolves my issue with no new text needed.

>> ### non-fatal NO_PROPOSAL_CHOSEN?
>> ```
>>    In this case, the responder may respond with
>>    non-fatal error such as NO_PROPOSAL_CHOSEN notify message type.
>> ```
>> Technically, this error is non-fatal. But in this context, wouldn't it be fatal if the
>> responder insists on additional exchanges during the initial exchange and the initiator
>> doesn't suppor this? It is sort of a lame duck IKE SA ? :)
>
> It is non-fatal in this context, since it is sent in the IKE_AUTH
> just to prevent Child SA to be created. IKE SA will be created
> in any case (provided no authentication issues arise).
>
> The responder has already agreed to skip additional key exchanges
> for the initial IKE SA (since it agreed on the proposals in the IKE_SA_INIT).
> But the responder doesn't want to create any Child SA without
> PQ protection, so it purposely makes it failed (since the initiator
> doesn't support Childless IKE SA, which is a "proper" way
> to handle this situation). Then the responder may immediately rekey IKE SA
> with PQ algorithms (or wait the initiator to do this).

Hmmm. I don't fully agree but can see you point. As it is not really
important, no new text is needed.

>> Also the "may" responder is unclear to me. What other response could there be and why?
>
> The responder has few choices here. It expects the initiator
> starts childless IKE_AUTH, but it didn't happen.
> First the responder may fail the IKE SA itself (e.g. by sending SYNTAX_ERROR).
> It will be a fatal error. Then, the responder may agree to create
> the Child SA, but immediately delete it (no guarantee that
> no data will be sent over it). The suggested method
> is a bit of protocol hack, but it allows to avoid creating
> of weak Child SA and at the same time it keeps chances
> that the IKE SA be upgraded to be PQ-safe.

Yeah these are all pretty unlikely and unsafe things to do :P

>> ### misplaced text?
>> ```
>>    Note that if the initial IKE SA is used to transfer sensitive
>>    information, then this information will not be protected using the
>>    additional key exchanges [...]
>> ```
>> This paragraph appears in the Section "Interaction with Childless IKE SA", but should probably
>> be moved to the Security Considerations section.
>
> The Security Considerations has already some text that is related to this (the last para).
> The reason this text is in the "Interaction with Childless IKE SA" is to emphasize,
> that with the approach described in this section (Childless initial IKE SA, then rekey IKE SA with PQ algorithms)
> the initial IKE SA will be not PQ-safe, that may be inappropriate for some use cases.
>
> However, referencing G-IKEv2 seems to be wrong here (it has nothing to do with Childless IKE SA),
> so I moved the text referencing G-IKEv2 to the Security Considerations section.

Thanks.

>> ### IKE_FOLLOWUP_KE name
>>
>> I find the name IKE_FOLLOWUP_KE a little confusing, as this exchange applies to
>> IKE and IPsec SA rekey negotiations. Why is it not called FOLLOWUP_ADDITIONAL_KE ?
>> Or CREATE_CHILD_SA_FOLLOWUP(_KE) (a sort of bad name too but that at least follows the
>> bad name from the original IKEv2 spec)
>
> If you remember, we have had some discussion in the ipsecme WG,
> and then you suggested that all new IKE exchanges start with IKE_ prefix.
> We just followed this suggestion :-)

Ah yes, I remember :)

> Well, I don't care much what the name is, so I rely on my co-authors on this.
> I only hope it won't be too long and will be easy to pronounce :-)

Just give it some thought, but if we leave it as is that is fine too.

>> ### authentication ?
>>
>> ```
>>         This document does not address authentication since it is less urgent
>>         at this stage.
>> ```
>> While true, it does state that PPKs can be used. It might also want to say that
>> no IKE protocol level changes would be needed for authentication. A new RFC 7427
>> Digital Signature algorithm that is quantum-safe could be defined for X.509 and would
>> become available immediately without any IKEv2 level changes. So in a way, this
>> issue will be addressed but no IKEv2 document is needed for that. Perhaps this
>> can be clarified in the draft?
>
> I'm not so sure. I'd like to avoid any speculations on this topic in this draft.
> For example, the following draft does make few changes to IKEv2
> (or at least uses the features that are not yet standardized):
>
> https://www.ietf.org/archive/id/draft-guthrie-ipsecme-ikev2-hybrid-auth-00.txt
>
> There may also be issues with the size of AUTH and CERT payloads...

Ok fine with me.

>> Related to this is text in the Security Considerations:
>>
>> ```
>>    In particular, the authenticity of the SAs established
>>    under IKEv2 is protected using a pre-shared key, RSA, DSA, or ECDSA
>>    algorithms.
>> ```
>> This text is also incorrect as RFC 7427 allows us to use post-quantum authentication
>> algorithms that have a SubjectPublicKeyInfo (SPKI) definition. There might not be any
>> now, but there will presumbly be some in the future.
>
> Do you want to augment this text? Any suggestions?

How about:

    In particular, the authenticity of the SAs established under IKEv2
    are not protected with post-quantum algorithms.

>> ### AH
>>
>> Section 4 lists AH. Is there much point in using this document when deploying AH?
>> The idea was the protect against _future_ quantum computers breaking encryption,
>> not MITM style packet modification. So using AH (or ESP_NULL) with this document
>> seems pointless :)
>
> I see your point. But until the AH is formally deprecated, one is free to use it :-)
> Note also, that the draft has another application - to be able to combine
> several key exchange algorithms so that the resulting key depends on all.
> For example, if each side trusts only its own key exchange algorithms and absolutely
> doesn't trust the peer's ones. This application is described in the introduction
> and it is orthogonal to post-quantum cryptography.
>
> So, keeping this case in mind, I'd rather keep the table as is.

Fair enough :)

>> And the Security Considerations kind of agree with me here:
>> ```
>>    Until quantum computers
>>    become available there is no point in attacking the authenticity of a
>>    connection because there are no possibilities for exploitation.
>> ```
>
> I see no contradiction to my point :-)

The contradiction is using this document for AH, which adds no value at
all as per this claim :P

I agree this is all rather academic and just scars of age. So fine with
no changes.

>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------

(cutting the agreed parts out)

>> ### Section 2
>>
>> I would probably have moved the Design Criteria to a later Section or Appendix,
>> after the entire protocol specification and Security Considerations. It's nice
>> to know the background, but this is "optional information" and shouldn't be as
>> much at the focus at the start of the document. (this is comment, you are fine
>> to disagree and leave it where it is)
>
> I don't disagree with you, but I'd like to hear from my co-authors.

Ok.

>> ### Design Criterea
>>
>> One design criteria I do not see mentioned is "limit the extra number of RTTs as
>> much as possible". I do believe that was an important design criterea ?
>>
>> The "future proof" design criterea is probably better named as "not
>> post-quantum specific" ?
>
> Well, I'd rely on my co-authors.

Ok.

>> ### IKE_INTERMEDIATE "is encrypted"
>> ```
>>    The additional key exchanges are performed using
>>    IKE_INTERMEDIATE messages; because these messages are encrypted, the
>>    standard IKE fragmentation mechanism is available.
>> ```
>> I think this is confusing. It is not really "because" they are encrypted that
>> the fragmentation mechanism "is available". Additionally, "encrypted" probably
>> should clarify the level of encryption - eg it would not me post-quantum safe.
>> And of course it does not need to be. Mabye something like:
>>
>>    The additional key exchanges are performed using IKE_INTERMEDIATE messages
>>    that follow the IKE_SA_INIT exchange. This is to allow for standard IKE
>>    fragmentation mechanisms to be available for the potentially large
>>    post-quantum Key Exchange algorithm payloads. The IKE_SA_INIT exchange does
>>    not [cannot?] support fragmentation.
>
> Then how about:
>
>            The additional key exchanges are performed using IKE_INTERMEDIATE messages
>            that follow the IKE_SA_INIT exchange. This is to allow for standard IKE
>            fragmentation mechanisms (which cannot be used in IKE_SA_INIT) to be available for the potentially large
>            post-quantum Key Exchange algorithm payloads.

That works for me.

>> ###
>> ```
>>       and that hybrid key exchange is not needed.
>> ```
>> Maybe:
>>
>>       and that a hybrid key exchange containing a classic (EC)DH is no longer
>>       needed.
>
> I'd rather to keep the text as is. My co-authors might disagree :-)

Ok.

>> ### Section 3.1 Childless?
>> ```
>>      Following that, the IKE_AUTH exchange authenticates peers
>>      and completes IKE SA establishment.
>> ```
>> This made me wonder if it was required to do a Childless IKE SA. I think a
>> clarification is in order here. Perhaps:
>>
>> Following that, the IKE_AUTH exchange comples at normal and authenticates the
>> peers, completes the IKE SA establishment and when not childless, a Child SA is
>> also established.
>
> I'm not sure this clarification is needed. This section is focused on IKE SA
> establishment, so if we go into all the details, readers may not follow the main idea.

Well, when reading I was wondering if this had to be childless, so I
think it would be good to remove that confusion. But whatever you decide
is fine. This is just my comment.


>> ### Section 3.1 ASCII art
>>
>> Probably, it should be clarified here that {} means "encrypted", or point a
>> sentence on syntax to the explanation in RFC7296? While this is obvious to
>> readers of IKEv2 documents, this document has not actually stated that this is
>> the meaning. Additionally, perhaps introduce a {{foo}} that means encrypted
>> safely for classic and quantum ?
>
> We can add a sentence that diagrams follow RFC 7296 notation
> (although to be frank, I don't remember which other IKE extension
> documents have this text). What my co-authors think?
>
> And with regard to {{foo}} - I think here we may confuse
> readers since the draft explicitly allows combining
> non-PQ key exchange algorithms (in which case
> we cannot claim the result is quantum safe).

Fair enough. I just checked the IKEv2 resumption RFC and it does not do
this either.

>> ### duplicated algorithms
>>
>> ```
>>    MUST NOT contain duplicated algorithms
>> ```
>> But it goes on saying this _can_ be possible, if the algorithm properties are
>> the same, so this sentence needs to reflect that to avoid misimplementation.
>> Maybe:
>>
>> MUST NOT contain duplicated algorithms with identical attributes
>
> I suggest then:
>
> MUST NOT contain duplicated algorithms (those with identical Transform ID and attributes)
>
> since "algorithm" != "transform"

Works for me.

>> ### Section 3.2.1.  MUST stop SA
>> ```
>>    the initiator should log the
>>    error and MUST stop creating the SA or delete the SA if it is a
>>    rekey.
>> ```
>>
>> There is ambiguity here on the "delete the SA if it is a rekey". I think you
>> mean to say to stop creating or delete the SA being negotiated, and not to
>> delete the SA that was attempted to be rekeyed. How about the simpler:
>>
>> the initiator should log the error and MUST abort the exchange with a permanent
>> error.
>
> No, the text is correct (and we suggest to delete the SA that was attempted to be rekeyed).

Oh, then I do disagree here.

> The reason - the initiator has received invalid message from the responder.
> It cannot abort the exchange since the exchange has already completed.
> This is similar to the situation in IKEv2 when the initiator sends, for example,
> a single proposal to use AES-GCM, and the responder responds with Chacha-Poly
> (which was not proposed at all). The only thing the initiator can do in this case -
> delete the SA.

The situation is not the same. In your example there is no working SA,
so you can only delete the partial one. Unless you suggest you would
also do this in CREATE_CHILD_SA when the algorithms suddenly mismatch.
I also think that is only a reason to ignore the CREATE_CHILD_SA, and
not a reason to destroy the existing IKE SA (and thus any existing Child
SA)

> I think that the text may be clarified a bit. How about?

I guess first we need to agree on what to do.....

>> ### Section 3.2.1 IKE_INTERMEDIATE
>> ```
>>     then the corresponding IKE_INTERMEDIATE exchanges MUST NOT take place.
>> ```
>> You are already then clarifying this statement, but perhaps to avoid
>> misimplementing, rewrite this to:
>>
>> then the corresponding Additional Key Exchange(s) in the IKE_INTERMEDIATE
>> exchanges MUST NOT take place. If there is no Additional Key Exchange left to
>> negotiate, this could mean that there is no more need to perform any
>> IKE_INTERMEDIATE exchanges. [and remove the following paragraph completely]
>
> I'd rather to use your text partially:
>
> then the corresponding Additional Key Exchange(s) in the IKE_INTERMEDIATE
> exchanges MUST NOT take place.
>
> This is correct clarification. The rest of your text is not completely correct -
> there may be needs to perform IKE_INTERMEDIATE for purposes other
> than additional KE.

That is what I wrote? If there is no Additional Key Exchange, it could
mean you don't need to do IKE_INTERMEDIATE? I found it more clear then
your following paragraph :)

> I'll left the rest of the para as is.

Maybe reread it one more time, but whatever you decide is fine.

>> ### Section 3.2.2
>> ```
>>    The other keying materials SK_d, SK_ai, SK_ar, SK_ei,
>>    SK_er, SK_pi, SK_pr are updated as:
>>
>>    [...]
>> ```
>> Why not say: The other keying materials SK_d, SK_ai, SK_ar, SK_ei, SK_er,
>> SK_pi, SK_pr are generated from the SKEYSEED(n) as per RFC 7296.
>
> No objection.

Just to clarify, where I wrote [...] I meant to cover the illustration
under the text as well. The point of this change was to not repeat a
stock 7296 exchange (which might confuse implementers in believeing this
is different from stock 7296)

>> ### Section 3.2.3
>> ```
>>    This exchange is the
>>    standard IKE exchange, except that the initiator and responder signed
>>    octets are modified as described in [RFC9242].
>> ```
>> Instead of "modified", which might mislead the reader into thinking this
>> documents "modifies" that process, I would say:
>>
>> This exchange is the standard IKE exchange as described in [RFC7296] and
>> [RFC9242].
>
> How about
>
> This exchange is the standard IKE exchange as described in [RFC7296] with
> the modification of AUTH payload calculation described in [RFC9242].
>
> This way we emphasize that the exchange is not completely standard :-)

Works for me.

>> ### Section 3.2.4 missed rename
>> ```
>>     the peers may optionally perform a Diffie-Hellman key exchange
>> ```
>> Should this not also be renamed into: perform an additional Key Exchange Method
>
> Good catch. But I think the correct text is:
>
> 	the peers may optionally perform a key exchange to add a fresh entropy into the session keys.
>
> We here talk about generic key exchange (single or multiple).
> We add a notice about a possibility to perform multiple (i.e. additional)
> key exchanges a couple of sentences later.

Agreed.

>> ### Section 3.2.4 Simultanious rekey
>> ```
>>    the initiator of this exchange just stops the
>>    rekeying process and it MUST NOT initiate the IKE_FOLLOWUP_KE
>>    exchange.
>> ```
>> should this clarify with: and MUST delete the state and MUST NOT send a
>> Delete/notify  ?
>
> I don't think so. When initiator stops rekeying process, it obviously
> deletes associated state. And there is no point to send Delete,
> since the SA is not created yet (it would be created only when all
> IKE_FOLLOWUP_KE exchanges are complete).

Ok.

>> ### Section 3.2.5 Childless IKE SA
>>
>> This section explains how to use establish Child SAs without using the
>> IKE_INTERMEDIATE exchange.
>>
>> I guess I would prefer that there are not two ways to do something, as IKEv2 is
>> already complex enough. But I guess the infrastructure needed for rekeying
>> causes this additional method to creep in whether we want it or not.
>
> There was a request from some folks who were concerned
> with the possibility of DoS attack due to exchange of
> large amount of information with unauthenticated peer.
> So we added the possibility to avoid this concern.

Fair enough.

>> ### I did?
>> ```
>>     Thanks to Paul Wouters for reviewing the document.
>> ```
>> I have no memory of this. Or was this pro-actively added? More serious, I guess
>> I did review this a long long time ago when the document looked very different
>> :)
>
> Anyway, I think that your thorough review definitely deserves this text to persist :-)

Thanks :)

>> ### Example is a bit contrived :)
>> ```
>>        Transform AKE1 (ID = PQ_KEM_1)
>>        Transform AKE1 (ID = PQ_KEM_2)
>>        Transform AKE1 (ID = NONE)
>>        Transform AKE2 (ID = PQ_KEM_3)
>>        Transform AKE2 (ID = PQ_KEM_4)
>>        Transform AKE2 (ID = NONE)
>>        Transform AKE3 (ID = PQ_KEM_5)
>>        Transform AKE3 (ID = PQ_KEM_6)
>>        Transform AKE3 (ID = NONE)
>> ```
>> I understand this is just an example to show the processing, but it would be a
>> little odd that both the order of (1|2) before (3|4) before (5|6) would matter
>> if these sets are all themselves optional - they are "optional requirements" ?
>> :)
>
> We can shuffle them :-) But will this help readers?

No I agree to leave it as is. It was just a comment.

>> ```
>>         Nonetheless, it is possible to
>>         combine this post-quantum algorithm with a FIPS complaint key
>>         establishment method so that the overall design is FIPS
>>         compliant
>> ```
>>
>> I would change "is FIPS compliant" to "remains FIPS compliant"
>
> OK.

Also be aware you wrote "FIPS complaint" not "FIPS compliant". Some
might agree that FIPS is a complaint though ;-)

>
>> ```
>>     Simply increasing the key length can dwarfed this attack.
>
> ????

My problem is that I don't understand what it means to "dwarf" an
attack. My brain thinks of throwing Gimli,Dwalin, Balin and Thorin
into the attack.

Perhaps you mean "reduce" or "largely counter" ? :-)

Thanks for the quick response!

Paul