I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Please resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-msec-gdoi-update-09.txt
Reviewer: Elwyn Davies
Review Date: 19 July 2011
IETF LC End Date: 19 July 2011
IESG Telechat date: (if known) -
Summary:
Not ready.
Major issues:
One has to ask: Why is an updated protocol being based on ISAKMP/RFC
2408 with references to RFC 2407 and RFC 2409 when all these are now
obsolete?
Minor issues:
General: A default statement about how integer fields are encoded is
needed.
s1: The removal of the list of extra payload types needed by the GDOI
protocol that was in RFC 3547 is not helpful. There is also one extra
one (GAP) in addition to those in RFC 3547 - and the POP payload has
disappeared (as mentioned). Without this summary, understanding s3 and
s4 is less easy, even with the abbreviations expanded s1.2. It would
also be helpful to note the 'standard' ISAKMP payloads that are used
(e.g. HASH is not referenced back to RFC 2408 at all). I know this is
now in s5 but it makes reading s3 and s4 much more difficult.
s1/s3/s4/s5: A number of the ISAKMP 'payloads' are technically not used
in the syntactic position of payloads as defined in RFC 2408, but are
used as subsidiary TLVs - SA Attributes carriers - linked to the SA
payload. This is somewhat confusing and should be explained better and
earlier in the document. Also the new GAP 'payload' is used in two
different syntactic contexts once as a SA Atrribute carrier linked to
the SA and alternatively as a genuine indpendent payload. Although the
formats are identical, I would prefer to see these two items with
different 'payload' identifiers to ensure that there is no mix up.
[Aside: It appears to me that the whole subsidiary SA attribute
mechanism is unnecessary. As far as I can see there is effectively a
'grammar' of SA attribute payloads that would work perfectly well if the
SA Attributes were just ordinary payloads and linked to the SA using the
next payload field. Whether or not this is correct, it is obviously far
too late to change the system that has been established through RFC
3547.]
s1.3, LKH: I believe this stands for Logical Key Hierarchy rather than
Lock Key Hierarchy.
s2.2: 'may move to port 4500' - This should probably be 'MAY move'.
s3.1: 'The Phase 1 identity SHOULD be used by a GCKS...': Are there
really any alternatives? Not using one is 'not recommended' a few lines
on. This looks as if it is an artefact of removing the POP alternative.
RFC 3547 said there were exactly two ways and one has been deleted by
this spec, so I think this is now a MUST.
s3.2, para after figure:
> HDR is an ISAKMP header payload that uses the Phase 1 cookies and a
> message identifier (M-ID) as in IKEv1.
>
Since IKEv1 is very obsolete it seems inappropriate to refer to it here.
The descriptions are now in RFC 2408. The Group Member MUST generate
M_ID before starting the exchange. This is also covered in s3.2.1.
s3.2.1/s4.2: There doesn't seem to be an IANA registry for ISAKMP
Exchange Types The values used for GROUPKEY-PULL (32) and GROUPKEY-PUSH
(33) appear to conflict with the values defined for Quick Mode and Base
Mode in Appendix A of RFC 2409. This is probably not a real world
problem since RFC 2409 (IKEv1) is obsolete but it is potentially
confusing. It may also be that these values are supposed to be 'per
DOI', but Quick Mode and Base Mode are not (i believe) related to a DOI.
s3.2.1: Having read the relevant section of RFC 2406, I am unclear when,
if at all, the Commit Flag bit should be set. Is more guidance needed?
Presumably using Authentication only wpould be a mistake for any of
these messages?
s3.3, para 3:
> Otherwise, the GM SHOULD tear down the Phase 1 session after first
> notifying the GCKS that it is doing so.
How is this done? I can't find any exactly suitable text in RFC 2408 etc.
s3.3 paras 3 and 4: Duplicated instructions about Sender-ID requests:
> If a Data-security SA
> describes the use of a counter mode cipher, the GM determines whether
> it requires more than one Sender-ID (SID) (see Section 3.5). If so,
> it includes a GAP payload indicating how many SID values it requires.
>
> When constructing the third GDOI message, it first reviews each Data-
> security SA given to it. If any include a cipher counter mode, it
> needs to request for one or more Sender-IDs for its exclusive use
> within the counter mode nonce. Do to this, the GM will include a GAP
> payload with its request, as described in the Section 5.4 section of
> this document.
s3.3, para 6:
> If the SEQ payload is present, the sequence number included in the
> SEQ payload MUST be greater than any previously received sequence
> number. If it is less than the previously received number, it MUST
> be considered stale and ignored.
^^^^^^^^^^^^^^^^
What must be 'considered stale'? Just the sequence number or the whole
exchange?
s5.1: What about use with IPv6?
s5.2:
> o Next Payload (1 octet) -- Identifies the next payload for the
> GROUPKEY-PULL or the GROUPKEY-PUSH message as defined above. The
> next payload MUST NOT be a SAK Payload or SAT Payload type; it MUST
> be the next non-Security Association type payload.
The term 'non-Security Association type payload' isn't properly defined.
Also as mentioned earlier the GAPpayload is used in two syntactic
context - one when it is valid after an SA and one where it is used as a
real payload. It would be cleaner to use two types as there isn't
really a shortage.
s5.2, SA Attribute Next Payload: The figure shows two octets whereas the
text says one octet. Which is right? Presumably implementers have
chosen which octet to use so we better get the right one!
s5.2.1: It would helpful to provide a grammar for the allowed
combinations of SA Attribute '(sub-)payloads'.
s5.3 (also s4.3):
> o Protocol (1 octet) -- Value describing an IP protocol ID (e.g.,
> UDP/TCP) for the rekey datagram.
^^^^^^^^^^^^^^
This term is not clear. In s4.4 GROUPKEY-PUSH datagram is used for the
same thing which is clearer. Probably good to reference the IANA
registry that has the right values.
(http://www.iana.org/assignments/protocol-numbers/protocol-numbers.txt)
s5.3/s5.5.1, SRC/DST ID Data Len fields: Need to specify unit of length
(presumably octets).
s5.3, KEK Attributes:
> In the table, attributes that are defined as TV are
> marked as Basic (B); attributes that are defined as TLV are marked as
> Variable (V).
The acronyms TV and TLV are defined in s3.3 of RFC 2408 but need to be
included in Section 1.3 here.
s5.4/s5.4.1/s5.4.2: The formats of the two attribute options are not
specified here but are only in the IANA considerations. This is
inconsistent with the other attribute value specs.
s5.4.1: The length of the number specifying each of ATD/DTD is not
specified here - it is implied by the specification in the IANA
considerations but not explicitly stated. Also can you get into a mess
by changing ATD and/or DTD and actioning the new values at the wrong
moment?
s5.5.1.1: None of the 'new' SA attributes listed in the sub-sections of
s5.5.1.1 are specified fully here but are only in the IANA
considerations. This is inconsistent with the other attribute value
specs.
s5.6.3.1: The Key Creation Date and Key Expiration Date fields are
supposed to contain 4 octet time values. What values are these supposed
to contain? The system will not be interoperable without knowing this.
Nits/editorial comments:
General: None of the figures have titles.
GeneraL: The term 'octet' is preferred to 'byte' (used in 4 places).
RFC 2408 uses octets and it is also used extensively in this document,
so there should be consistency.
s1.2, Logical Key Hierarchy): spurious right bracket.
s3.2, figure: It would help if this figure was titled indicating that
it represented the message exchanges and explicitly numbered the
messages since the messages are referred to by number elsewhere.
Stating that what we are seeing here is a grammar for the ISAKMP payload
types in each message wouuld make things immediately clearer. This also
harks back to the point that the SA payload is actually a combination of
the SA payload and some set of SA attribute '(sub-)payloads'.
s3.2, next to last para: s/and its knowledge ensure/and its knowledge
ensures/
s3.2, next to last para: s/received during the GROUPKEY-PULL/received
during the GROUPKEY-PULL exchange/
s3.2: There is some overloading of terminology here. On superficial
reading HASH looks like a 'function' but on closer inspection HASH is an
ISAKMP payload type (not explicitly made clear)
s3.5, para 4: s/When at least one Data-Security SAs/When at least one
Data-Security SA/
s3.5, second set of bullets: s/1. A GCKS maintains an SID-counter,
which records which SIDs that have been allocated./1. A GCKS maintains
an SID-counter, which records which SIDS have been allocated./
s4.3, para 4: s/distribute LKH update arrays sufficient/distributing LKH
update arrays sufficient/
s4.4, para 4: s/includes Data-Security SA including a
counter-modes/includes one or more Data-Security SAs using a
counter-mode/
s5: Useful to sya which ISAKMP payloads are used in the standard form
(HASH, SIG, I believe).
s5.2, SA Attribute Next Payload: The field value is a presumably a type
code rather than the field itself. s/MUST be either a SAK Payload
or a SAT Payload/MUST be either the code for a SAK Payload
or the code for a SAT Payload/
s5.2.1, last para: s/group-wise/groupwide/?
s5.x, Payload Length specifications: Should all contain statement that
they include the length of the genric payload header as per RFC 2408
(could be a general statement at the start).
s5.5.1
> o Protocol (1 octet) -- Value describing an IP protocol ID (e.g.,
> UDP/TCP). A value of zero means that the Protocol field MUST be
> ignored.
The IANA registry from which the values are taken should be specified.
(http://www.iana.org/assignments/protocol-numbers/protocol-numbers.txt)
s5.5.1.1.2, para 2:
> Note that unless Symmetric may be
> the only value that can be meaningfully described for an SA TEK
> distributed in an GROUPKEY-PUSH message.
This does not parse. I think it is trying to say that Symmetric is the
only meaningful value in a GROUPKEY-PUSH message.
s5.9, last para: s/spi of zero/SPI value of zero/
s7, para 2:
> so host security is of the utmost important once.
I don't know what is intended here.
s7.2.5, para 1:
> in order to mitigate avoid overloading its computational resources
Maybe s/mitigate avoid/avoid/
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
