Re: [IPsec] Secdir early review of draft-ietf-ipsecme-g-ikev2-08

[Valery Smyslov <[e-mail]>]

Hi Russ,

thank you for your review. Please see inline.

> -----Original Message-----
> From: Russ Housley via Datatracker [mailto:[e-mail]]
> Sent: Friday, April 14, 2023 3:56 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: Secdir early review of draft-ietf-ipsecme-g-ikev2-08
> 
> Reviewer: Russ Housley
> Review result: Not Ready
> 
> I reviewed this document as part of the Security Directorate's ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written primarily for the benefit of the Security Area
> Directors.  Document authors, document editors, and WG chairs should
> treat these comments just like any other IETF Last Call comments.
> 
> Document: draft-ietf-ipsecme-g-ikev2-08
> Reviewer: Russ Housley
> Review Date: 2023-04-05
> IETF LC End Date: 2022-04-30
> IESG Telechat date: Unknown
> 
> Summary: Not Ready
> 
> Major Concerns:
> 
> Throughout: RFC 7296 says that SK operations are "Encrypted and
> Authenticated".  However, several places we see SK{}.  What is being
> authenticated and encrypted in such cases.  Similarly, there are
> several places where all of the items in SK{ ... } are optional.
> What is being authenticated and encrypted if they are all absent?
> Maybe I am missing something, but I think some discussion of the
> notation would be helpful.

SK{...} denotes the "Encrypted payload" (Section 3.14 of RFC 7296).
An empty Encrypted Payload is used in RFC 7296 in several cases - 
when peer should send something to complete the exchange, but 
there is really nothing meaningful to send. In reality the content
of the "empty" payload is not null - it contains the mandatory Pad Length
field and an optional padding (for counter-based encryption algorithms
there is no padding). So, when sending SK{}, a peer actually sends 
one encrypted zero byte (for counter-based modes) and authenticates
the whole message including the IKE header.

There is nothing new in G-IKEv2 with this regard comparing to RFC 7296.
I don't think any clarification about notation is needed - we have dozens of IKEv2 
extension RFCs which already use this notation with no clarification and in addition 
readers are supposed to be familiar with RFC 7296. Or should we say this explicitly?

> Section 1: RFC 3740 defines a GSA as:
> 
>    A bundling of Security Associations (SAs) that together define how
>    a group communicates securely.  The GSA may include a registration
>    protocol SA, a rekey protocol SA, and one or more data security
>    protocol SAs.
> 
> However, this introduction only talks about two aspects of the GSA.
> Please state which data security protocol will be used.  I assume it
> supports both ESP and AH.

That's true. I've added a clarification:

          This specification relies on ESP and AH as protocols for Data-Security SAs.

> Section 1: The IKE_INTERMEDIATE exchange is discussed later in the
> document, but the Introduction does not lay the ground work for it.

The introduction is mostly concerned with group key management
architecture and terminology, since they a bit different from what 
IPsec folks are accustomed to. IKE_INTERMEDIATE is not specific
to group key management, so it is not mentioned there. 
It is mentioned later to make clear that besides IKE_SA_INIT+GSA_AUTH 
other exchanges, defined as IKEv2 extensions, can also be used.

If I missed your point, then can you please elaborate?

> Section 2.4.1.1: Please provide some explanatory text prior to:
> 
>    DataToAuthenticate = A | P
>    GsaRekeyMessage = GenIKEHDR | EncPayload
>    GenIKEHDR = [ four octets 0 if using port 4500 ] | AdjustedIKEHDR
>    AdjustedIKEHDR =  SPIi | SPIr |  . . . | AdjustedLen
>    EncPayload = AdjustedEncPldHdr | IV | InnerPlds | Pad | PadLen | ICV
>    AdjustedEncPldHdr = NextPld | C | RESERVED | AdjustedPldLen
>    A = AdjustedIKEHDR | AdjustedEncPldHdr
>    P = InnerPlds

This notation follows a similar one from RFC 7296 (Section 2.15)
and RFC 9242 (3.3.2). 

We can add some text before this, for example:

	The input to the authentication algorithm that computes 
	the content of the AUTH payload can be described as:

Is it enough?

> Section 2.4.1.2: The following seems impossible to implement:
> 
>    *  The GCKS must always use IKE fragmentation based on a known
>       fragmentation threshold (unspecified in this memo), as there is no
>       way to check if fragmentation is needed by first sending
>       unfragmented messages and waiting for response.
> 
> There is no hint about how to learn the fragmentation threshold, but
> the GCKS MUST NOT use fragmentation unless the fragmentation threshold
> is known.

It is assumed that fragmentation threshold in this case is pre-configured by administrator.

Should we clarify this? For example:

   *  The GCKS must always use IKE fragmentation based on a pre-configured
      fragmentation threshold (unspecified in this memo), as there is no
      way to check if fragmentation is needed by first sending
      unfragmented messages and waiting for response.

Is it enough?

> Section 4.4.2.1.1: It says: "To specify the details of the signature
> algorithm a new attribute Algorithm Identifier (<TBA by IANA>) is
> defined."  Shouldn't this simply reference RFC 7427?

No, it is irrelevant here. Here the draft discusses the way the GCKS informs the GMs about the authentication
method it will use for multicast rekey messages. This is accomplished via the newly defined 
IKEv2 Transform Type "Authentication Method (AUTH)" (not to be mixed with "Authentication Payload (AUTH)"). 
In case this transform specifies a digital signature algorithm as an authentication method, a newly defined
IKEv2 Transform Attribute "Algorithm Identifier" inside this transform further specifies which digital signature
algorithm to use. IKEv2 transforms and attributes and their use in IKEv2 are defined 
in Sections 3.3.1 and 3.3.5 of RFC 7296.

RFC 7427 is referenced later in this para, when the content of this attribute is discussed.

> Section 4.5.1: Some key wrap algorithms do not have an an explicit IV.
> See RFC 3394 for one example.  How is a zero-length IV handled?

Generally, there is no problem if IV is not used - the draft says:

   *  IV (variable) -- Initialization Vector used for encryption.  The
      size and the content of IV is defined by the employed encryption
      transform.

So, if the algorithm doesn't require IV, then this field is absent (implicitly has zero length).

However, RFC 3394 is not relevant in this case. The draft says:

   The keys are encrypted using algorithm that is used to encrypt the
   message the keys are sent in.  It means, that in case of unicast IKE
   SA (used for GMs registration and rekeying using GSA_INBAND_REKEY)
   the encryption algorithm will be the one negotiated during the IKE SA
   establishment, while for a GSA_REKEY message the algorithm will be
   provided by the GCKS in the Encryption Algorithm transform in the GSA
   payload when this multicast SA was being established.

It means that the key wrapping algorithm must be from the 
IKEv2 encryption transforms, i.e. it is must be among those defined in 
https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xhtml#ikev2-parameters-5

Actually, there are some algorithms with implicit IV in this registry , but they are prohibited 
for using in IKEv2 (and thus in G-IKEv2), they can only be used in ESP. With regard
to multicast ESP SAs, there is further restrictions on using these transforms - 
Replay protection must be on (which implies that there is the only sender for multicast SA).
This should be explicitly spelled out in the draft, I'll add a new section.

Bottom line - there is currently no algorithms defined for use in G-IKEv2 rekey SA without explicit IV
(and I believe algorithms with no/implicit IV must be prohibited there).

> Section 5: It says:
> 
>    S     A single attribute of this type must be present
> 
>    M     Multiple attributes of this type may be present
> 
>    []    Attribute is optional
> 
>    -     Attribute must not be present
> 
> I think these should be MUST, MAY, OPTIONAL, and MUST NOT statements.

Perhaps. Let it be so.

> Minor Concerns:
> 
> Section 1.2: Please add some punctuation between the term being
> defined and the definition.  
> Section 2.2 uses "--" for this purpose.

Actually, this is more a tooling issue. I tried to use some xml2rfc v3 syntax,
since using v3 is generally being promoted by IESG.
With v3 there is a special tag for definitions and this tag doesn't provide
any means for specifying a punctuation delimiter between the term
and its definition. The only option is to make definition starting from 
new line. I'll add the newline attribute if it's OK with you.

> Further, it would be very helpful to
> the reader if you say the source of each defined term, instead of
> the catch all phrase at the top of the list of defined terms.

It is a bit difficult. Same terms are often defined differently in RFC 3740,
RFC 5374, RFC 4046 and RFC 6407. The problem is also that each of those 
sources doesn't use terms from the previous RFCs, instead it redefines them :-(
I tried to pick most appropriate ones (from my POV), sometimes compiled 
a definitions from different sources or truncated them.
If more strict definitions are needed, then I'd rather ask for help - 
currently there seems to be a mess in related RFCs.

> Section 1.2: Please add "CPI", "LKH", and "NULL Authentication" to
> the definitions.

LKH is there (the last term).

For the CPI I prefer to expand on first (and the only) use in the document.

With regard to "NULL Authentication" -- the more I think about this, 
the more I'm sure that referring to "NULL Authentication" in the draft
was a wrong idea. The draft re-uses the code point for NULL Authentication,
which is wrong. I'll re-write the related text and use value 0 (currently "Reserved",
can be renamed to "NONE") to refer to the implicit authentication. 
Thank you for bringing my attention to this issue.

> Section 2.1.1, 2nd paragraph: I cannot parse it. I think it is trying
> to say:
> 
>    Section 2.23 of [RFC7296] describes how IKEv2 deals with NATs.
>    G-IKEv2 registration doesn't create any unicast IPsec SAs, thus if
>    a NAT is present between the GM and the GCKS, there is no unicast
>    ESP traffic to encapsulate in UDP.  However, the actions described
>    in this section regarding the IKE SA MUST be honored.  If the GM
>    and the GCKS use UDP port 848 for the IKE_SA_INIT exchange, they
>    MUST behave in the same manner as if they had used UDP port 500.

This para is already changed as per Gorry proposed. How about (combining his and yours changes):

          Section 2.23 of [RFC7296] describes how IKEv2 supports paths with NATs.
          G-IKEv2 registration SA doesn't create any unicast IPsec SAs, thus if
          a NAT is present between the GM and the GCKS, there is no unicast
          ESP traffic to encapsulate in UDP. However, the actions described 
          in this section regarding the IKE SA MUST be honored. 
          The behavior of GMs and GCKS MUST NOT depend on the port used to create the initial IKE SA.
          For example, if the GM and the GCKS used UDP port 848 for the IKE_SA_INIT exchange, they
          will operate the same as if they had used UDP port 500.

Is it OK?

> Section 2.3.2: It says: "When a secure channel is already established
> between a GM and the GCKS, ...".  I think it would be more clear to
> say: "After the successful completion of the GSA_AUTH exchange, ...".
> If there is some detail that is not captured by this alternate wording,
> then that detail needs to be explained in this first paragraph of this
> section.

OK, changed to:

   After the successful completion of the GSA_AUTH exchange, the GM
   registration for a group can reuse the established IKE SA.
   In this scenario the GM will use the GSA_REGISTRATION exchange.
   Payloads in the exchange are generated and processed as defined in
   Section 2.3.1.

> Section 2.3.3: The text mixes the terms "GM" and "initiator". It would
> be more clear to use one term throughout.

Cleaned up (s/initiator/GM).

> Section 2.3.3: It says:
> 
>    In the simplest case when no dependency between transforms exists,
>    all Proposals in SAg payload will have the same value in Proposal Num
>    field.
> 
> This is a contradiction to other text, especially this: "Proposals
> of different types having the same value in Proposal Num field are
> treated as a set.".  So, if there are no dependencies at all, I would
> expect each proposal to have a unique value in Proposal Num field.

There is no contradiction, there is poor wording :-(

If IKEv2 each Proposal substructure describes transforms for a particular protocol.
In the case of IKEv2 this would be GIKE_REKEY, AH or ESP. So, the GM will
send at least two proposals - one for GIKE_REKEY and the other for Data-Security SA,
let it be ESP. Then, the Proposal Num allows the GM to express, that there
is a dependency between the transforms for different protocols. For example,
if the GCKS selects algorithm X for GIKE_REKEY, then it must also select 
algorithm X for ESP (for example, in case these algorithms are implemented in 
HSM and it doesn't allow to export derived keys - so you are stick
to using it  for all protocols). To express this the GM will mark proposal
for GIKE_REKEY containing transform X and proposal for ESP containing
transform X with the same Proposal Num. The GM may include
another proposal for these protocols (say, containing transform Y)
with a different Proposal Num. 
Un the simplest case, when no dependency exists, proposals for all protocols 
will have the same Proposal Num field and each will contain all supported
transforms for a corresponding protocol. 

Please, let me know how the wording can be made more clear.
Currently I made some minor changes for the sake of accuracy:

   Proposal Num field in Proposal substructure is treated specially in
   SAg payload: it allows a GM to indicate that algorithms used in Rekey
   SA and in Data-Security (AH and/or ESP) SAs are dependent.  In
   particular, Proposals for different protocols having the same value
   in Proposal Num field are treated as a set, so that if GCKS uses
   transforms from one of such Proposal for one protocol, then it MUST
   only use transforms from one of the Proposals with the same value in
   Proposal Num field for other protocols.  For example, a GM may
   support algorithms X and Y for both Rekey and Data-Security SAs, but
   with a restriction that if X is used in Rekey SA, then only X can be
   used in Data-Security SAs, and the same for Y.  To indicate this the
   GM sends several Proposals marking those of them that must be used
   together by putting the same value in their Proposal Num field.  In
   the simplest case when no dependency between transforms exists, all
   Proposals in SAg payload will have the same value in Proposal Num
   field.

> Section 2.3.3, last paragraph: It is very hard to understand.  Perhaps:
> 
>    Once a GM has received GIKE_REKEY policy during a registration, the
>    IKE SA MAY be closed.  By convention, the GCKS closes IKE SA.  The
>    GKCS MAY choose to keep the IKE SA open for inband rekeying,
>    especially for small groups.  If inband rekeying is used, then the
>    initial IKE SA can be rekeyed with the standard IKEv2 mechanism
>    described in Section 1.3.2 of [RFC7296].  If for some reason the
>    IKE SA is closed before a GIKE_REKEY policy is received during the
>    registration process, the GM MUST consider itself excluded from the
>    group.  To continue participating in the group, the GM needs to
>    re-register.

OK, will use this text with one correction:

s/before a GIKE_REKEY policy is received/and no GIKE_REKEY policy is received

This cannot be "before", since it happens at the time of registration (GSA_AUTH).
But GCKS may omit sending GIKE_REKEY policy (including keys) to this particular
GM for some reason (e.g. the GCKS intends to use inband rekey).
In this case, if IKE SA is closed for any reason, the GM should re-register.

> Section 2.3.4: s/The GCKS may also respond with an INVALID_GROUP_ID/
>                 /The GCKS SHOULD respond with an INVALID_GROUP_ID/

I don't mind, but in this case in addition: 

s/it will respond with an AUTHORIZATION_FAILED/it SHOULD respond with an AUTHORIZATION_FAILED

> Section 2.4: Please add some punctuation between the type of group
> maintenance channel and the description of that channel.  Section 2.2
> uses "--" for this purpose.

See above about the tooling issue. I added an attribute that puts definition on the next line.

> Section 2.4.1: It says:
> 
>    Since this rekey does not require a response and it sends
>    to multiple GMs, G-IKEv2 rekeying MUST NOT use IKE SA windowing
>    mechanism, described in Section 2.3 of [RFC7296].
> 
> I was not able to get the meaning on first reading.  Perhaps:
> 
>    Since the Rekey message does not require a response and it is sent
>    to multiple GMs, the GCKS MUST NOT use the IKE SA windowing
>    mechanism described in Section 2.3 of [RFC7296] for the Rekey
>    message.

I prefer:

    Since the Rekey messages do not require responses and they are sent
    to multiple GMs, the windowing mechanism described in Section 2.3 
    of [RFC7296] MUST NOT be used for the Rekey messages.

Does it work for you?

> Section 2.4.1.4: It says:
> 
>    When a group member receives the Rekey Message from the GCKS it
>    decrypts the message using the current KEK, validates its
>    authenticity using the key retrieved in a previous G-IKEv2 exchange
>    if AUTH payload is present, verifies the Message ID, and processes
>    the GSA and KD payloads.
> 
> It is unclear which part of the processing depends on the presence of
> the AUTH payload.  Please reword to be clear which parts are always
> done and which parts are done only if the AUTH payload is present.

How about the following:

    When a group member receives the Rekey message from the GCKS it
    decrypts the message and verifies its integrity using the current KEK. If the AUTH payload is present
    in the decrypted message, then the GM validates authenticity of the message using the key retrieved 
    in a previous G-IKEv2 exchange. Then the GM verifies the Message ID, and processes
    the GSA and KD payloads.

> Section 3.3: It says:
> 
>    ...  In particular, the GM's task
>    is to find a way to decrypt at least one of the SA_KEY attributes
>    using either the GSK_w or the keys from the WRAP_KEY attributes that
>    are present in the same message or were receives in previous
>    messages.
> 
> This is really a transition sentence to prepare the reader for the
> following paragraphs. On my first reading, it sounds more like a
> challenge or goal.  I suggest:
> 
>    ...  The GM decrypts at least one of the SA_KEY attributes using
>    either the GSK_w or the keys from the WRAP_KEY attributes that
>    are present in the same message or were received in previous
>    messages as described below.

I propose:

    ...  The GM tries to decrypts at least one of the SA_KEY attributes using
    either the GSK_w or the keys from the WRAP_KEY attributes that
    are present in the same message or were received in previous
    messages as described below.

(if GM is being excluded from the group using LKH, then it cannot decrypt any SA_KEY,
but since it doesn't know this, it will try to do it).

Is it OK?

> Section 3.4: The text talks about "first bits", which is ambiguous.
> Perhaps "most significant bits" or "leftmost bits in Network Byte
> Order".

Changed to "leftmost bits" (I believe byte order is irrelevant here since we're talking about strings, not integers).

> Section 4: I find the structure of this section difficult.  The
> discussion mixes the payloads that are reused from IKEv2 and the new
> ones that are defined for G-IKEv2.  I think it would be easier to
> follow if the ones that are reused from IKEv2 were discussed in a
> subsection and then a separate subsection talked about the new
> payloads.  

Hmm, I'm a bit confused. All Section 4 is about new payloads,
specific to G-IKEv2, with the exception of the G-IKEv2 header,
which is identical to IKEv2 header. Two payloads (SAg and IDg)
have the same format as existing IKEv2 payloads, but different semantics.

The problem with subsections is that in this case the nesting level
for new payloads will be too deep: currently there exists 4.4.2.1.1.,
will be one level deeper.

Perhaps it is sufficient if I add the following text to the beginning of Section 4:

   G-IKEv2 header (Section 4.1),
   IDg payload and SAg payload reuse IKEv2 format for the IKEv2 header,
   IDi/IDr payloads and SA payload respectively.

Does it help or not?

> The identifiers for the new payloads have already been
> assigned by IANA, but the IANA registries point to draft-yeung-g-ikev2.

That's true, they were assigned a long ago.

> Section 4.5: I find the term "Key Packets" confusing.  I suggest a
> better term might be "Wrapped Keys".  Similarly, the use of "packet"
> should be changed in "Group Key Packet" and "Member Key Packet".

Perhaps "Key Bag"? Or "Key Bundle"?

By definition Key Packet contains several related keys, 
and is referred to some substructure, so "Wrapped Key"
seems to be not a good substitution.

No changes made so far, waiting for more discussion.

> Section 6.1: Its says: "Below are possible scenarios involving using
> PPK."  Some of them do not use PPK.  Please reword.

Changed to:

	This is illustrated below.

I also removed some unnecessary stuff (optional PPK).

> Section 7.2.1: I understand that implicit authentication doesn't
> provide source origin authentication, but there is a bit more that
> can be said here.  Can the GM determine that each message came from
> the same entity, even if the GM cannot be sure that entity is the
> GCKS?

I don't think it can. With implicit authentication any member of the group
can impersonate the GCKS. The source IP address can be set to any value,
since no reply is needed.

> Nits:
> 
> All: Some places the document uses "Data Security SAs" and other
> places it uses "Data-Security SAs".  Please pick one.

Changed to Data-Security SAs.

> Section 1: It says: "... that allows to perform a group key ...".
> What entity does it allow to perform group key management?  I think
> is should say: "... that allows the GCKS to perform a group key ...".

This text is already changed to (by other reviewer's request):

   This document presents an extension to IKEv2 [RFC7296] called
   G-IKEv2, which allows performing a group key management.

Is it OK?

> Section 1: It says: "... (see Appendix A of [RFC7296]."  Please add a
> closing parenthesis.

Done (also caught by other reviewer).

> Section 1: It says: "Large and small groups may use different sets of
> these protocols."  I think that "protocols" is the wrong word.  I think
> that "exchanges" is a better word.

I prefer "Large and small groups may use different sets of these mechanisms."

(since GSA_REKEY is not an exchange, it is one-way message).

Is it better?

> Section 1.2: s/[RFC4301], its extension/[RFC4301], and its extension/

OK.

> Section 1.2: s/good understanding/an understanding/

Don't mind.
 
> Section 2.1: s/However some/However, some/

Fixed.

> Section 2.1.1: s/As IKEv2 extension G-IKEv2/As IKEv2 extension, G-IKEv2/

Done.

> Table 1 caption: s/the protocol/G-IKEv2/

OK.

> Section 2.3: s/The registration protocol consists/Registration consists/

Well, let it be so.

> Section 2.3.3: s/Section 2.5)/Section 2.5/

Fixed.

> Section 2.3.3: s/AEAD and non-AEAD transforms must not be combined in/
>                 /AEAD and non-AEAD transforms not be combined in/

Well, this change is less obvious for my non-native ear, but I trust you here.

> Section 2.3.3: It says: "... those of them that must be used ...".
> Please reword.  The use of the word "must" here caused me to miss the
> meaning on first reading.  Perhaps:
> 
>    Use of the same value in the Proposal Num field of different
>    proposals indicates that the GM expects these proposals to be
>    used in conjunction with each other.

OK, will use this text.

> Section 2.3.3: s/SHOULD NOT close IKE SA/SHOULD NOT close the IKE SA/

I cannot find this particular text, probably it is gone in the process of editing.
Anyway, I got the idea with "the" and fixed whenever found.

> Section 2.3.4: s/Data-Securirt/Data-Security/

Cannot find, probably fixed earlier.
 
> Section 2.3.4: s/GCKS may have no need/GCKS may have no further need/

OK.

> Section 2.4.1: s/Rekey securely, usually using/Rekey, usually using/

I understand that the current wording is weird, but shouldn't we emphasis 
that rekey messages are sent over secure (one-way) channel?

Perhaps:

          The GCKS initiates the G-IKEv2 Rekey by sending a protected message to the GMs, 
          usually using IP multicast.

> Section 2.4.1.1: s/The chunk A lasts from/The chunk a starts with/

OK.

> Section 2.4.1.1: s/to the last octet/and continues to the last octet/

Ditto.

> Section 2.4.1.2: s/ messages, however when/ messages; however, when/

Done.

> Section 2.4.1.3: s/must have the Message ID 0/MUST use Message ID of 0/

Fixed.

> Section 3.2: s/individual GMs' keys/keys for each GM./

OK.

> Section 3.3: s/GCKS's key management method/
>               /key management method use by the GCKS/

Sorry, perhaps 

	key management method used by the GCKS

?

> Section 4: The punctuation needs corrections.  I suggest:
> 
>    ... However, the processing of some payloads are
>    different.  Several new payloads are defined: Group Identification
>    (IDg), Group Security Association (GSA), and Key Download (KD).

OK. This text is also addressed in Minor Issues.

> Section 4.4.2.1: s/Method (AUTH)and/Method (AUTH) and/

Already fixed.

> Section 4.5: s/a keys and/keys and/

Fixed.

> Section 7.1: s/in [RFC7296] section 5 Security Considerations/
>               /in the Section 5 of [RFC7296]/

Done.

> Note: I did not review the Appendix.
 
Thank you for the thorough review.

The changed made so far are available at: 
https://github.com/smyslov/G-IKEv2

Regards,
Valery.