Mixing Preshared Keys in the IKE_INTERMEDIATE and in the CREATE_CHILD_SA Exchanges of IKEv2 for Post-quantum Security
draft-ietf-ipsecme-ikev2-qr-alt-08

# Andy Newton, ART AD, comments for draft-ietf-ipsecme-ikev2-qr-alt-08
CC @anewton1998

* line numbers:
  - https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-ipsecme-ikev2-qr-alt-08.txt&submitcheck=True

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Comments

Thanks for writing this draft. The following are just comments to use as you see fit.

### Variable PKK_ID

Given the figure and the description of PKK_ID below, is there a mismatch
on the length. The description says "(variable)" but the figure seems to
indicate a rigid 8 octets.

204                             1                   2                   3
205         0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
206        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
207        |                                                               |
208        ~                             PPK_ID                            ~
209        |                                                               |
210        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
211        |                                                               |
212        +                        PPK Confirmation                       +
213        |                                                               |
214        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

216                 Figure 1: PPK_IDENTITY_KEY Notification Data Format

218        Where:

220        *  PPK_ID (variable) -- PPK_ID as defined in Section 5.1 of
221           [RFC8784].  The receiver can determine the length of PPK_ID by
222           subtracting 8 (the length of PPK Confirmation) from the
223           Notification Data length.

225        *  PPK Confirmation (8 octets) -- value, which allows the responder
226           to check whether it has the same PPK as the initiator for a given
227           PPK_ID.  This field contains the first 8 octets of a string
228           computed as prf( PPK, Ni | Nr | SPIi | SPIr ), where prf is the
229           negotiated PRF; PPK is the key value for a specified PPK_ID; Ni,
230           Nr, SPIi, SPIr -- nonces and IKE SPIs for the SA being
231           established.

## Nits

### Remove "the one"

I think "the one" can be removed from the end of line 91. I would also remove
the word "mostly", but that's just me.

90         calculation.  At the time this extension was being developed, it was
91         a consensus in the IPsecME WG that it is the IPsec traffic the one
92         that mostly needs to be protected.  It was believed that information

### Not Applicable Cells

My first reaction to looking at this table is that "*" was a reference to
a foot note or aside. Maybe putting "n/a" for "not applicable" is better
if your intention is to say neither "Yes" nor "No" but something is needed.

295        Table 1 summarizes the above logic for the responder:

297        +===========+=============+========+===========+====================+
298        |Received   | Supports    |Has one | PPK is    | Action             |
299        |USE_PPK_INT| USE_PPK_INT |of      | mandatory |                    |
300        |           |             |proposed| for       |                    |
301        |           |             |PPKs    | initial   |                    |
302        |           |             |        | IKE SA    |                    |
303        +===========+=============+========+===========+====================+
304        |No         | *           |*       | No        | [RFC8784] (if      |
305        |           |             |        |           | proposed) or       |
306        |           |             |        |           | standard IKEv2     |
307        |           |             |        |           | protocol           |
308        +-----------+-------------+--------+-----------+--------------------+
309        |No         | Yes         |*       | Yes       | Send               |
310        |           |             |        |           | NO_PROPOSAL_CHOSEN |
311        +-----------+-------------+--------+-----------+--------------------+
312        |Yes        | Yes         |Yes     | *         | Section 3.1,       |
313        |           |             |        |           | Paragraph 16, Item |
314        |           |             |        |           | 1 (use this        |
315        |           |             |        |           | extension)         |
316        +-----------+-------------+--------+-----------+--------------------+
317        |Yes        | Yes         |No      | Yes       | Section 3.1,       |
318        |           |             |        |           | Paragraph 16, Item |
319        |           |             |        |           | 2 (abort           |
320        |           |             |        |           | negotiation)       |
321        +-----------+-------------+--------+-----------+--------------------+
322        |Yes        | Yes         |No      | No        | Section 3.1,       |
323        |           |             |        |           | Paragraph 16, Item |
324        |           |             |        |           | 3 (standard IKEv2  |
325        |           |             |        |           | protocol)          |
326        +-----------+-------------+--------+-----------+--------------------+

### Comparison to IKE_AUTH

457        IKEv2 protocol are discussed in [RFC8784].  Compared to use PPKs in
458        IKE_AUTH this specification makes even initial IKE SA quantum secure.

Just a suggestion (if I understood correct):

   Unlike PPKs in IKE_AUTH, this specification makes initial IKE SA quantum secure.

# Internet AD comments for draft-ietf-ipsecme-ikev2-qr-alt-08
CC @ekline

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Comments

### S1

* Thank you for the helpful summary and background.

## Nits

### S1

* "that it is the IPsec traffic the one that mostly needs to be protected"

  ->

  "that it was the IPsec traffic that most needed to be protected"

  perhaps

* "re-creating a new once"

  ->

  "re-creating a new one"

Thanks for producing this I-D for IKE. I really found it helpful to understand how this could work and to explain the new method.

I have two comments that I really think ought to be considered before progression (but which I do not need to DISCUSS:

(1) Please consider whether this ought to use RFC-2119 language, as /MAY/, it could be a protocol action (you or the responsbile AD will know best): "If this is inappropriate for the initiator, it may immediately delete this SA."

(2) Appendix A contains a RFC 2119 keyword: /MAY/. I do not think this is normal to use keywords on appendices (you or the responsbile AD will know best), please consider using a lower case /may/ or simply explain it is an /alternative/.

===

I stumbled several times for non-technical reasons when reading, and therefore I have a few comments that I hope will allow others to also easilly read this useful I-D, for which I have tried to suggest new wording:

Abstract:
"way to get protection" - the word "get" seems to read strange to me, could you consider using "provide" or soemthing similar?
"but protects the initial IKEv2 SA too", could be "but also protects the initial IKEv2 SA."

Introduction:
"At the time this extension was being developed, it was a consensus in the IPsecME WG that it is the IPsec traffic the one
that mostly needs to be protected.", could be: "At the time this extension was being developed, the consensus in the IPsecME WG was to specify protection for the IPsec traffic."
"However, in some situations it is desirable to have this protection for IKE SA", include /the/: "However, in some situations it is desirable to have this protection for the IKE SA".
"An example of such situation is Group", include /a/ and /the/: "An example of such a situation is the Group"
"In this protocol group policy and session keys are transferred from Group Controller/Key Server (GCKS) to Group Members (GM) immediately once an initial IKE SA is created. ", include /a/ and /the/: "In this protocol, group policy and session keys are immediately transferred from a Group Controller/Key Server (GCKS) to the Group Members (GM) once  an initial IKE SA is created."

Spec:
"then the responder MUST return NO_PROPOSAL_CHOSEN notification.", include /a/? "then the responder MUST return a NO_PROPOSAL_CHOSEN notification."
"If the negotiation was successful, the initiator includes one or more PPK_IDENTITY_KEY notification containing PPK identities the initiator believes are appropriate for the IKE SA being created, into the IKE_INTERMEDIATE request.", could be:
"If the negotiation was successful, the initiator includes one or more PPK_IDENTITY_KEY notifications into the IKE_INTERMEDIATE request, containing the PPK identities that the initiator believes are appropriate for the IKE SA being created."
Bullet 1&3:"If the responder is configured with one of the PPKs which IDs were sent by the initiator and this PPK matches the initiator's",
seems hard to parse, maybe /with an ID that was sent/ ... or something similar? (the same comment applies to bullet 3).
Bullet 2:"has some of proposed PPKs", include /the/: "has some of the proposed PPKs"

"In case the responder", could be: "In the case that a responder"

Thanks for working on this document. It's in good shape; I have a few minor issues and several nits that can be incorporated at the discretion of the author and the responsible AD.

In Section 3.1, given the MUST abort, I'm guessing the "should" is not a SHOULD. Consider "should treat this as" => "treats this as" (or "MUST treat this as a fatal error and abort...")

In Section 4, the terms "QR" and "CRQC" need to be introduced with an expansion on first use, or better yet, simply expanded and the abbreviation not used.

===NITS===

Section 1:

"it is the IPsec traffic the one that mostly needs to be protected." => "it is the IPsec traffic that most needs to be protected."

"This allows to leverage fresh" => Either "This allows implementations to leverage fresh" or "This allows the use of fresh"

"The initiator that" => "An initiator that"

CURRENT: If the responder is configured with one of the PPKs which IDs were sent by the initiator and this PPK matches the initiator's one (based on the information from the PPK Confirmation field),
CONSIDER: If the responder supports any of the PPKs which the initiator proposed, based on the PPK ID and PPK Confirmation fields,

"returns PPK identity" => "returns a PPK identity"

CURRENT: If the responder does not have any of the PPKs which IDs were sent by the initiator or it has some of proposed PPKs, but their values mismatch the initiator's ones (based on the information from the PPK Confirmation field), and using PPK is mandatory for the responder, then it MUST return AUTHENTICATION_FAILED notification and abort creating the IKE SA.
CONSIDER: If the responder cannot use any of the PPKs proposed by the initiator, based on the PPK ID and PPK Confirmation fields, it MAY return an AUTHENTICATION_FAILED notification and abort creating the IKE SA if configured to require PPK usage.

CURRENT: If the responder does not have any of the PPKs which IDs were sent by the initiator or it has some of proposed PPKs, but their values mismatch the initiator's ones (based on the information from the PPK Confirmation field), and using PPK is optional for the responder, then it does not include any PPK_IDENTITY notification to the response.
CONSIDER: If the responder cannot use any of the PPKs proposed by the initiator but it is not configured to require PPK usage, it does not include any PPK_IDENTITY notification to the response.

"abort creating IKE SA" => "abort creating the IKE SA"

"selects PPK" => "selects a PPK"

Section 3.1.1:

"applying PPK" => "applying the PPK"

"with PPK" => "with the PPK"

Section 3.2: "supports use PPKs" => "supports using PPKs"

Similar language suggestions from Section 3.1.

Section 3.2.1: "resulted" => "resulting"

Section 4:
- "Compared to use PPKs in IKE_AUTH this" => "Compared to using PPKs in IKE_AUTH, this" or "Compared to the use of PPKs in IKE_AUTH, this"
- "since PPK is used in authentication too, that gives this exchange a QR protection even against active attacker." => "since the PPK is used in authentication too, this exchange is protected even against an active attacker."

Throughout: "Note, that" => "Note that"

Hi Valery, 

Thank you for the effort put into this specification. 

Also, thanks to Luis M. Contreras for the OPSDIR review (his first review for the opsdir team, BTW) and to Valery for engaging with Luis.

Please find some comments below:

# Guidance for those who will deploy

From the perspective of those who will deploy, I found appendix useful…but somehow late. Putting aside that appendix was not referenced in the document, having an applicability section early in the document with a guidance when to use this extension vs. RFC8784 would be my preferred approach. 

# Guards against too frequent changes

For example, we do have 

CURRENT:
   If a fresh PPK is available to both peers at the time when an IKE SA
   is active, peers MAY use this fresh PPK without creating a new IKE SA
   from scratch.  

Is there some kind of timer used as a guard to protect against too frequent changes?

# Preference policy

CURRENT:
   However, if the responder supports both specifications and is
   configured to use PPKs, it has to choose one to use, thus it MUST
   return either USE_PPK_INT or USE_PPK notification in the response,
   but not both.

Is there any provision for policy support here? Also, can we recommend a default value here? 

# Exemplify shortcomings and situations

It would be helpful to exemplify the shortcomings mentioned in:

CURRENT:
   Instead, it is supposed to be
   used in situations where the approach defined there has a significant
   shortcomings. 

Likewise, do we have examples of situations mentioned in the following excerpt?

CURRENT:
   However, if the partners support both use PPKs in
   IKE_AUTH and this specification, then the latter MAY also be used in
   situations where using PPKs in IKE_AUTH suffices.

BTW, what is meant by “partners” mentioned in that text? Do we mean initiator and responder?

# Terminology anchor

As I’m there, maybe consider adding the following (or similar) to Section 2:

NEW:
   This document uses the terms defined in [RFC7296].  In
   particular, readers should be familiar with the terms "initiator" and
   "responder" as used in that document.

Cheers,
Med

Thank you to Meral Shirazipour for the GENART review.

** Section 5.
   This document defines two new Notify Message Types in the "IKEv2
   Notify Message Types - Status Types" registry:

Please correct the name of the registry to be “IKEv2 Notify Message Status Types” (https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xhtml#ikev2-parameters-16)