also inline.
On 12/9/24 7:34 AM, Valery Smyslov wrote:
Hi Robert,
thank you for your review. Please, see inline.
Reviewer: Robert Sparks
Review result: Ready with Nits
I am the assigned Gen-ART reviewer for this draft. The General Area Review
Team (Gen-ART) reviews all IETF documents being processed by the IESG for the
IETF Chair. Please treat these comments just like any other last call comments.
For more information, please see the FAQ at
<https://wiki.ietf.org/en/group/gen/GenArtFAQ>.
Document: draft-ietf-ipsecme-g-ikev2-17
Reviewer: Robert Sparks
Review Date: 2024-12-08
IETF LC End Date: 2024-12-10
IESG Telechat date: Not scheduled for a telechat
Summary: Essentially ready for publication as a proposed standard RFC but with
nits to address before publication.
This document is well written and easy to follow. (I agree with Russ, but won't
repeat the points in his last call review).
Thank you.
There are quite a few places where article and noun agreement should be
adjusted.
It was anticipated, alas :-)
I convinced myself that the recommendation for excluding a GM while preserving
forward access control works when the first of the two rekey messages never
arrives at one of the remaining group members, but would like those following this
closely to think through that case again to make sure there's not an issue.
Actually, the situation when some GMs don't receive rekey messages might be an issue with any
multicast rekey operation. The document contains some recommendation (see Section 2.4.1.3)
and also provides a way for GMs to reliably detect this situation (with use of GSA_NEXT_SPI attribute).
If this happens, the only thing these GMs can do - re-register to the GCKS.
Nits in document order.
There are places where there are bare SHOULDs that are not clarified by other
text. When would a member chose _NOT_ to follow the SHOULD in the last
sentence of 2.3.1? Should the document discuss what a GCKS would do if a GM
included transform types it SHOULD NOT (end of 3rd paragraph of 2.3.3) or if it
provided a non-zero SPI length field?
The SHOULD in 2.3.1 is just to encourage GMs to inform GCKS about the situation
(the GCKS has sent an unsupported policy). It is not an obligatory action for a GM,
thus the SHOULD. We can change the text:
OLD:
If the group member finds the policy sent by the GCKS is
unacceptable, the member SHOULD initiate GSA_REGISTRATION exchange
sending IDg and the Notify NO_PROPOSAL_CHOSEN (see Section 2.3.2)).
NEW:
If the group member finds the policy sent by the GCKS is
unacceptable, the member SHOULD inform the GCKS about this by
initiating the GSA_REGISTRATION exchange and sending IDg along with
the NO_PROPOSAL_CHOSEN notification in it (see Section 2.3.2)).
What about the text in 2.3.3, the GM generally should only include
transforms not listed in this document, because they will be ignored
by the GCKS (the GCKS just doesn't know what to do with them).
Some GM implementations may find it easier to include
all the transform types defined in IKEv2. This is discouraged
by the SHOULD, but not prohibited (as there is no harm).
We can clarify this a bit:
Other transform types
SHOULD NOT be included as they will be ignored by the GCKS.
Is it better?
Small touches like this are an improvement. Please skim through all the
bare SHOULDs (and SHOULD NOTs) to see if something similar should be said.
7th paragraph of 2.3.3 fails to parse at "by inclusion one".
Perhaps:
OLD:
A GM MAY also indicate the support for IPcomp by inclusion one or
more the IPCOMP_SUPPORTED notifications along with the SAg payload.
NEW:
A GM MAY also indicate the support for IPcomp by inclusion one or
more the IPCOMP_SUPPORTED notifications along with the SAg payload
into the request.
Is it better?
Perhaps:
A GM MAY also indicate support for IPcomp by including one or more of
the IPCOMP_SUPPORTED notifications notifications along with the SAg payload.
Consider "listens" instead of "passively listens".
OK, changed.
The 3rd paragraph of 2.4.1.4 may benefit from future-proofing - the timescales
called out here may not be the same a decade from now.
We can change the text to avoid any mention of timescale:
OLD:
GSA_REKEY messages are sent infrequently (typically one per several
hours or, in extreme cases, several minutes), which is much greater
than typical network packet reordering intervals.
NEW:
This specification assumes that the GSA_REKEY messages are sent with
intervals, that are significantly greater than typical network packet
reordering intervals.
Does this work for you?
yes
The last paragraph of 2.4.3 is harder to follow than the rest of the document.
Can it be simplified?
Perhaps:
If a group member receives a Delete payload with zero SPI and
protocol ID of GIKE_REKEY, it means that the group member is
excluded from the group. Such Delete payload may be received either
in the GSA_REKEY pseudo-exchange or in the GSA_INBAND_REKEY exchange.
In this situation the group member MUST re-register if it wants to
continue participating in this group. The registration is performed
as described in Section 2.3. Note, that if the unicast SA between
the group member and the GCKS exists, then the group member may use
the GSA_REGISTRATION exchange to re-register. However, after
excluding an GM from the group the GCKS MAY immediately delete the
unicast SA with this GM (if any) if the credentials of this GM are
revoked.
Let us know if it is still hard to follow.
It's better - thanks.
2nd paragraph of 3.2 - should "Besides" be "Additionally"?
Indeed. Thank you.
(And I believe you are referring to 2nd paragraph of 3.1 here...)
yes - apologies
The version of the Group Key Bag Attributes table in section 4.5.2 doesn't match
the version in section 9. I think you should go with what's in section 9, but please
remove the trailing comma after "GIKE_REKEY," - it makes it look like the next row
is a continuation of _this_ value in the column.
I think you mean that the content of "Multi-Valued" column differs. This is a good catch, fixed.
In fact, the "NO/YES" construction was chosen in 4.5.2. to emphasize, that
this attribute in most cases is single-valued, and only in case of LKH it can be multi-valued
(that clarified in the note below). But formally it is multi-valued.
And I removed the trailing comma, thanks for pointing out.
Consider calling out that several of the "new" values for registries were already
established by other documents. See AUTHORIZATION_FAILED as an example,
In fact, they were allocated by the earlier version of this document, as part
or early codepoint allocation process and some of them are renamed by this document.
I'm not sure this should be mentioned, the history of allocations for this draft is long and complex;
do we want to explain it in details to give readers more chances for confusion?
I think calling it out for _reviewers_ (especially the IESG) would be
helpful. It's not helpful for readers 10 years from now. If it were me,
I'd put it in a "Note to be deleted" in the IANA considerations section
or ask the shepherd to amend the shepherd writeup with a short
explanation. The point is to save all of those people the effort of
figuring out why _some_ of the new values already have codepoints and
others are still TBA.
Section 8.2.4 has a bare (and only) mention of protection from reflection attacks
(vs replay attack) - should more be said?
We can add the following para at the end of this section:
The strict role separation between the GCKS and the GMs and, as a
consequence, the limitation for Rekey SA to be outbound/inbound only,
helps to prevent reflection attack.
Does this help?
Sure - thanks.
Regards,
Valery.
--
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx
