Hi Russ,
thank you for your review. Please, see inline.
> 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-17
> Reviewer: Russ Housley
> Review Date: 2024-11-27
> IETF LC End Date: 2024-12-10
> IESG Telechat date: Unknown
>
> Summary: Not Ready
>
>
> Major Concerns:
>
> IKEv2 implementers that have no need for group security associations are not
> likely to read this document. For this reason, I think it is unwise to
> include the
> updates to RFC 7296 here that:
>
> (1) Rename transform type 5 from "Extended Sequence Numbers (ESN)" to
> "Anti-Replay Protection (ARP)"; and
>
> (2) Rename IKEv2 authentication method 0 from "Reserved" to "NONE".
These actions don't change bits on the wire and the semantics of the currently
defined values -
implementers who only read RFC 7296 and implement IKEv2 accordingly will get
compliant implementations.
The only possible way the confusion may happen is:
a) implementers don't look at the RFC 7296 metadata on the datatracker or on
the RFC Editor web site
or
implementers don't care about the RFC 7296 metadata (whether it is updated
and by which RFCs)
and
b) implementers are curious to look at the current IANA registries for IKEv2
In this case they could be confused by the fact that some registry and codepont
names
on the IANA page don't match those in RFC 7296.
To help those carelessly-curious implementers we can add the following
text to the IANA Considerations section (similar to the text in RFC 9370):
IANA has also completed the following changes. It is assumed that
RFCXXXX refers to this specification.
* Added a reference to [RFCXXXX] in what was the "Transform Type 5 -
Extended Sequence Numbers Transform IDs" registry.
* Added this note to what was the "Transform Type 5 - Extended
Sequence Numbers Transform IDs" registry:
This registry was originally named "Transform Type 5 - Extended
Sequence Numbers Transform IDs" and was referenced using that name
in a number of RFCs published prior to [RFCXXXX], which gave it
the current title.
* Added this note to the "Transform Type Values" registry:
"Anti-Replay Protection (ARP)" transform type was originally named
"Extended Sequence Numbers (ESN)" and was referenced by that name
in a number of RFCs published prior to [RFCXXXX], which gave it
the current title.
* Appended [RFCXXXX] to the Reference column of Transform Type 5 in
the Transform Type Values registry.
* Added this note to the "IKEv2 Authentication Method" registry:
"NONE" Authentication Method was originally named "Reserved" and
was referenced by that name in a number of RFCs published prior to
[RFCXXXX], which gave it the current title.
* Appended [RFCXXXX] to the Reference column of Authentication
Method 0 in the IKEv2 Authentication Method registry.
Will this resolve your concern? Or am I missing your point?
Please, see also the discussion here:
https://mailarchive.ietf.org/arch/msg/ipsec/iUU1ezD9NemjOLp6xXO4YWGxfG4/
> I find the use of GIKE_REKEY and GSA_REKEY a little bit confusing.
> I think it would help the reader if these were discussed a bit in the
> Introduction.
GSA_REKEY is an type of G-IKEv2 (pseudo) exchange, it appears in the G-IKEv2
Header (Exchange Type field).
The GIKE_REKEY is a protocol name, it appears in the GSA (and also in SAg)
payload.
> Figure 1 introduces GSA_REKEY, so perhaps, shortly after that a brife
> explanation
> og GIKE_REKEY can be included.
I don't think this is the right place, as Figure 1 only depicts exchanges.
Protocols are used in the SAg and GSA payloads
to indicate for which protocol the proposal/policy is.
I'm not sure where to put more text about GIKE_REKEY protocol.
Perhaps, the text in 2.3.3 (first use of GIKE_REKEY) can be changed:
OLD:
Proposals for Rekey SA
(with protocol GIKE_REKEY) and for Data-Security (AH [RFC4302] and/or
ESP [RFC4303]) SAs may be included into SAg.
NEW:
Proposals for Rekey SA and
for Data-Security (AH [RFC4302] and/or ESP [RFC4303]) SAs may be
included into SAg. Proposals for Rekey SA are identified by a new
Protocol ID GIKE_REKEY <TBA by IANA>.
Does this help? Are more clarifications needed?
> Section 1.2: To my ears, KEK and KWK are the same thing. Please add more
> words sto make the distinction more clear.
I think we can make the definition of KEK more accurate:
OLD:
Key Encryption Key (KEK)
The symmetric cipher key used in a Rekey SA to protect
distribution of new keys.
NEW:
Key Encryption Key (KEK)
The symmetric key (or a set of keys) used in a Rekey SA to protect
its messages.
Is this better?
> Section 2: Please move the last paragraph of Section 2.1 into this empty
> section.
> Also, a few words about what this section is going to to cover would be
> helpful.
Done. Also moved there the first paragraph of Section 2.1, which serves
as a brief description of G-IKEv2.
> Section 2.1: It says: "G-IKEv2 is compatible with most IKEv2 extensions
> defined so
> far." This begs for a list of the ones that are not compatible. The pointer
> to
> Section 6 should appear right after this statement.
OK.
> Section 2.4.1.1: I am surprised that the "ICV is computed using the current
> KEK
> keys". Two things surprise me. First, I think there is only one "current
> KEK" at a
> time. Second, I expected the current authentication key to be used for the
> ICV
> computation.
I changed:
s/current KEK keys/current KEK
First, "KEK keys" is a tautology, then the updated definition of KEK
says now that it is a key or a set of keys (e.g. one for encryption, one for
authentication),
thus I think it is OK to use KEK for ICV computation.
> Section 2.4.2.1: The text talks about "GSA, KD, N and D" payloads.
> However, Figure 11 does not show the use of a D payload. Further, Sections
> 2.3.3
> and 2.3.4 do not talk about the D payload.
> Section 2.4.2.2: The text talks about "GSA, KD, N and D" payloads.
> However, Figure 11 does not show the use of a D payload. Further, Sections
> 2.3.3
> and 2.3.4 do not talk about the D payload.
Deletion of group SA is covered later in Section 2.4.3.
To eliminate confusion I deleted mention of D payloads in registration or rekey
exchanges.
> Section 4.5.3.2: DSS public keys should be phased out, so I discourage the
> inclusion of DSS in this document.
G-IKEv2 follows the same recommendations on using crypto algorithms as IKEv2,
and this document is not the right place to change these recommendations.
The current recommendations (RFC 8247) mark DSS as "SHOULD NOT".
Until it is marked "MUST NOT" its use is still allowed and we have to
define format for DSS signatures.
> Minor Concerns:
>
> Section 4.5.4: Can the wrapped key be 94 bits? It appears that the payload
> is a
> number of octets, but no padding mechanism is described.
> The text is unclear because it says: "These bits..."
I see no problem if it is 94 bits (although it would be weird).
The payload is a whole number of octets, the padding mechanism doesn't matter.
Thus, the GCKS can fill the key bits beyond the needed length up to the whole
(any, but most naturally - nearest) number of octets with any value (zeroes,
ones, random).
GMs will consume exactly the needed number of bits, the rest (padding) will be
thrown away.
> Section 2.3.4: The 3third paragraph ends with: "Sender-ID values (see Section
> 2.5)." I think that it should be pointing to Section 2.5.1.
OK.
> Section 2.7: There is a missing ")". After reading several times, I am not
> sure
> where it belongs.
Fixed. It belongs to "(see Section 2.6)".
> Nits:
>
> Section 2.3.4: s/otherwise - in tunnel/otherwise, SAs are created in tunnel/
> Section 2.4.1.1: s/Messages Authentication/Message Authentication/
> Section 2.6: s/this transform/the Anti-Replay Protection transform/
> Section 3.3: s/the wrapped keys are sent over/over which wrapped keys are
> sent/
> Section 4.3: I suggest a rewording of the last sentence:
>
> The Payload Type for SAg payloads is thirty-three (33), which is
> identical to the SA Payload Type.
> Section 4.4.2: s/section 3.13.1/Section 3.13.1/
> Section 4.4.2.2.2: s/at the time of GMs' registration/when the GM is
> registered/
> Section 4.5.1: s/the keys in the bag are for/associated with the keys in the
> bag/
> Section 4.5.3.2: s/section 4.1.2.7/Section 4.1.2.7/
> Section 8.2.1: s/unless in/unless used with/
All fixed, thanks.
The proposed changes can be reviewed here:
https://github.com/smyslov/G-IKEv2/pull/26
> Note:
>
> I did not review the Appendix.
>
> Optional payloads (for example: "[CERTREQ]") really confuse IDnits.
> They appear to be references that are missing. I'm not sure there is
> anything to be
> done at this point, but you might be able to think of a way to handle it.
The presentation language for ISAKMP/IKEv1/IKEv2 exchanges is used since 1997,
and IDnits keeps complaining about missing references.
I think that the proper way to handle this issue is to update IDnits so,
that it can take xml as a source (as this is currently the only authoritative
RFC source)
and then it would be easy for IDnits to ignore the content of figures.
Regards,
Valery.
_______________________________________________
IPsec mailing list -- ipsec@ietf.org
To unsubscribe send an email to ipsec-le...@ietf.org
