Re: [Gen-art] Gen-art last call review of draft-ietf-msec-gdoi-update-09

[Brian Weis <[e-mail]>]

Hi Elwyn,

I believe I've addressed each of your comments in <http://tools.ietf.org/html/draft-ietf-msec-gdoi-update-10> with one exception:

>> s5.2.1: It would helpful to provide a grammar for the allowed
>> combinations of SA Attribute '(sub-)payloads'.

I agree that this would be useful, but at this late date I'm leery of creating and adding it this late in the document cycle.

Thanks much,
Brian

On Jul 29, 2011, at 8:40 AM, Brian Weis wrote:

> Hi Elwyn,
> 
> Thanks much for your detailed review. We'll handle your minor issues ASAP, but this email will address the major issue below.
> 
> On Jul 19, 2011, at 3:10 PM, Elwyn Davies wrote:
> 
>> 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?
> 
> This is a reasonable question to ask. The rationale stated by the document shepherd addresses this question:
> 
> "Among the normative references are 3 documents that have been obsoleted by the IPsec-v3 RFCs (RFC 4301, etc.) These RFCs were made obsolete the publication of IKEv2, without regard for the fact that although IKEv1 was directly obsoleted by IKEv2, other RFCs relying on those protocol definitions were not directly obsoleted by the publishing of IKEv2. WG chairs believe that updating GDOI as defined in RFC 3547 (and thus continuing to rely on these references) is necessary for interoperability."
> 
> Some additional thoughts:
> - There are multiple implementations of the GDOI specification. Inconsistencies have been noted in the standard that should be resolved to ensure their interoperability. As a related matter, some of the IANA definitions need to be clarified.
> - GDOI was published quite early in MSEC's history. Since that time the working group published documents describing how group key management systems should interact with IPsec, and also describing how to deal with cipher counter modes. The update document brings GDOI into conformance with those later published documents.
> - Because RFC 3547 was published so long ago, the required ciphers need updating to match current cryptographic guidance.
> 
> Thanks,
> Brian 
> 
>> 
>> 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/
>> 
>> 
>> 
> 
> 
> -- 
> Brian Weis
> Security Standards and Technology, SRTG, Cisco Systems
> Telephone: [phone]
> Email: [e-mail]
> 
> 
> 
> 
> 


-- 
Brian Weis
Security Standards and Technology, SRTG, Cisco Systems
Telephone: [phone]
Email: [e-mail]