Subject: [secdir] secdir review of draft-ietf-rohc-ikev2-extensions-hcoipsec-10

From: "Glen Zorn" <[email-masked]>
Date: Fri, 11 Dec 2009 13:56:58 -0800

I have 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 editors and WG chairs should treat these comments just
like any other last call comments.

This is a re-review; my comments on the previous version are reproduced
below.  Most of the issues that I raised in my previous review have been
addressed; however, a few remain (see below).

COMMENTS
There is still no "Intended status" line in the first page header.

SECTION 3.1.1

Paragraph 1, last line says: "The ROHC Attribute is shown in Figure 2."
Suggest changing this to "The format of the ROHC Attribute is shown in
Figure 2."

I would still prefer bifurcating the ROHC Attribute number space over using
a 15-bit type and a flag, but I suppose that is mostly a matter of taste.

SECTION 3.1.2

The description of the MAX_CID attribute says in part "The range of values
for MAX_CID MUST be at least 0 and at most 16383 (the value 0 implies having
one context)."  This seems a little clumsy to me; suggest changing to
something like "The range of values for the MAX_CID attribute is
[0...16383], the value 0 signifying a single context)."  On the other hand,
the use of zero to mean one is less than intuitive, so maybe a better idea
would be "The range of values for the MAX_CID attribute is [1...16383]."
QUESTION: Is the maximum number of contexts 16383 or 16384?

SECTION %

This section has changed quite a bit, but I think that the results are not
altogether positive.  It says that the IANA allocation policy for ROHC
attributes is "Designated Expert", but fully half the attribute space is
given over to "Private Use"; this seems self-contradictory.  It also assumes
that there will be an IETF Last Call but I don't see that as being a
requirement of the "Designated Expert" policy as described in RFC 5226, nor
of RFC publication.  I would suggest that this section be reworked to give
explicit instructions to the IANA WRT the allocation of values (presupposing
that none of the "canned" policies in RFC 5226 are applicable).

SECTION 6

If the people mentioned in paragraph 1 actually contributed to the document,
maybe this paragraph should be moved to a new "Contributors" section.

What, no acknowledgement for me? ;-)

SECTION 7

Should RFC 5226 be a normative reference?

Hope this helps.

~gwz

> -----Original Message-----
> From: Glen Zorn [mailto:[email-masked]]
> Sent: Saturday, October 10, 2009 12:12 AM
> To: [email-masked]'; [email-masked]'; [email-masked]';
> [email-masked]'; [email-masked]'; 'kivinen@safenet-
> inc.com'; [email-masked]'; [email-masked]'
> Subject: secdir review of draft-ietf-rohc-ikev2-extensions-hcoipsec-09
> 
> I have 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 editors and WG chairs should treat these
> comments just like any other last call comments.
> 
> 
> 
> COMMENTS
> 
> General
> -------
> There are lots of occurrences of constructions similar to "The Notify
> Payload (defined in [IKEV2]) is illustrated in Figure 1."  Roughly
> translated into English, this says "The Notify Payload (defined in the
> reference to RFC 4306) is illustrated in Figure 1." which, of course is
> nonsense: the _reference_ to RFC 4306 doesn't define, the document does.
> Suggest changing all such instances to something like "The Notify
> Payload (defined in RFC 4302 [IKEV2]) is illustrated in Figure 1."
> 
> Abstract
> --------
> I can't tell what the intended status of this draft might be (i.e.,
> Standards Track, etc.).  The I-D Tracker says that the draft wants to be
> a Proposed Standard, but there is no reference to RFC 2119 nor any use
> of 2119 keywords.  It might be a good idea to fix this (under the
> assumption that the editor will do so, I'll not further comment upon
> 'must' that probably should be 'MUST', etc.
> 
> There shouldn't be any references in the Abstract.
> 
> The acronym "ROHC" should be expanded on first usage.
> 
> 
> Section 2.1
> -----------
> Paragraph 4 says:
> 
>    A new Notify Message Type value, denoted ROHC_SUPPORTED, indicates
>    that the Notify payload is conveying ROHC channel parameters.  The
>    value for the ROHC_SUPPORTED message is specified in Section 4.
> 
> However, that's not really true: section 4 just says that IANA needs to
> assign a value.  Suggest changing to:
> 
>    A new Notify Message Type value, denoted ROHC_SUPPORTED, indicates
>    that the Notify payload is conveying ROHC channel parameters (Section
> 4).
> 
> 
> The description of the Notify Payload fields doesn't include the SPI
> field or the Notification Data field.  Since the SPI Size field is
> specified to be zero, I would assume that the SPI field itself must be
> omitted.  Is that correct?  If so (RFC 4306 isn't crystalline on the
> subject, either) & since the diagram of the payload and the description
> are specific to this application, I think the this should be stated, if
> not illustrated in the diagram itself.  Also, I think that the contents
> of the Notification Data field should be described; maybe something like
> 
>    Notification Data (variable length) (2 octets)
>       This field contains three or more ROHC Attributes (section 2.1.1).
> 
> I find the headings for this section and the next misleading: this
> section is headed "ROHC Channel Parameters that are Signaled" when it
> actually seems to be talking about the "ROHC_SUPPORTED Notify Message",
> while the next is headed "ROHC_SUPPORTED Notify Message" when it is
> actually describing "ROHC Attributes".  Suggest changing the headings
> accordingly.
> 
> 
> Section 2.1.1
> -------------
> The format and description of the ROHC Attribute are quite confusing: on
> the one hand, the ROHC Attribute Type field is stated to be 2 octets in
> length, but on the other hand the actual value is only 15 bits (as
> reflected in the IANA Considerations section); further, since the length
> is not reflected in the registry value itself, an implementation would
> need to set the AF bit (claimed to be, but not, part of the Attribute
> Type) according to the Attribute type.  A different, perhaps more
> elegant, way to accomplish the same goal might be to dispense with the
> AF bit altogether and simply specify that fixed-length Attributes are
> numbered 0x8000-0xFFFF, while variable-length Attributes are allocated
> from the range 0x0000-0x7FFF.
> 
> 
> Section 2.1.2
> -------------
> The sub-headings on the attribute descriptions are disconcerting: since
> the first paragraph lists the attributes by name (MAX_CID, etc.), I
> scanned for those in the following paragraphs but was met with textual
> descriptions (like "Maximum Context Identifier") which might better be
> placed in the description itself.  Suggest changing them to list the
> name first; it might also be nice to put the actual Attribute number in
> the header for quick reference.  So the suggestion is to change, for
> example,
> 
>    Maximum Context Identifier (MAX_CID, AF = 1)
>       The MAX_CID attribute is a mandatory attribute.  Exactly one
> to
> 
>    MAX_CID (Maximum Context Identifier, AF = 1)
>       The MAX_CID attribute is a mandatory attribute.  Exactly one
> 
> or (if you accept my numbering suggestion above)
> 
>    MAX_CID (0x8001)
>       The MAX_CID (Maximum Context Identifier) attribute is a mandatory
> attribute.  Exactly one
> 
> Under ROHC_INTEG it says "The attribute contains an integrity
> algorithm".  I'm assuming that this is actually not true (unless some
> _really_ amazing advances in compression have occurred recently ;-).
> Suggest changing to "The attribute value contains the identifier of an
> integrity algorithm".
> 
> Under "ROHC_ICV_LEN":
>    The acronym "ICV" should be expanded on first usage.
>    Suggest changing "If ROHC_ICV_LEN length is zero" to "If the value of
> the ROHC_ICV_LEN attribute is zero"
> 
> Under "MRRU" it says "If present, the attribute value is two octets in
> length." but this doesn't seem to make sense.  Suggest changing to "The
> attribute value is two octets in length."
> 
>