[pim] AD Review of draft-ietf-pim-igmp-mld-extension-04

[Alvaro Retana <[e-mail]>]

Dear authors:

Thank you for this document!

In general, I think this document is in good shape -- I do have
comments and suggestions below (inline) that I would like to see
addressed before publication.

My main concern is the formal Update of rfc3376 and rfc3810.  Do we
really need to formally Update those documents?  I didn't see a
specific discussion on the list about this topic.

In general (to me), Updating implies that implementations of
rfc3376/rfc3810 are expected to also support the specification in this
document.  OTOH, not formally Updating means that the extension is
optional.  The discussion in §5 about how implementations "rarely
change" makes we wonder whether Updating is the right thing to do --
or if it is enough to define the extension and let it be implemented
as needed (as new TLVs are defined in the future, for example).

The formal Update also concerns me as it is related to the recent
adoption of rfc3376bis/rfc3810bis.  Because the objective is to
elevate IGMPv3/MLDv2 to Internet Standard, the Update would represent
a change without proper experience.

In summary, I recommend not formally Updating rfc3376/rfc3810.

Mike (as Chair/Shepherd): Please poll the WG about whether it wants to
go ahead with a formal Update or not.  [If we go forward with a formal
Update then I'll need you to also poll the authors of rfc3376 and
rfc3810 about granting BCP78 rights [rfc5378] -- but let's cross that
bridge if we get to it.]


Thanks!

Alvaro.


[Line numbers from idnits.]

...
12	                     IGMPv3/MLDv2 Message Extension
13	                  draft-ietf-pim-igmp-mld-extension-04

[] Please extend the title; perhaps "Internet Group Management
Protocol (IGMP) and Multicast Listener Discovery (MLD) Message
Extension"


15	Abstract

17	   IGMP and MLD protocols are extensible, but no extensions have been
18	   defined so far.  This document provides a well-defined way of
19	   extending IGMP and MLD, using a list of TLVs (Type, Length and
20	   Value).

[major] "IGMP and MLD protocols are extensible, but no extensions have
been defined so far."

As I see it, IGMP/MLD can be extended by defining new types or by
using Additional Data...so extensions have already been defined. ;-)

Let's focus the Abstract on the TLVs; suggestion (borrowing a little
form the Introduction):

   This document specifies a generic mechanism to extend IGMPv3 and
   MLDv2 by using a list of TLVs (Type, Length and Value).


...
73	1.  Introduction

75	   In this document, we describe a generic method to extend IGMPv3
76	   [RFC3376] and MLDv2 [RFC3810] messages to accommodate information
77	   other than what is contained in the current message formats.  This is
78	   done by allowing a list of TLVs (Type, Length and Value) to be used
79	   in the Additional Data part of IGMPv3 and MLDv2 messages.  This
80	   document defines a registry for such TLVs, while other documents will
81	   define the specific types and their values, and their semantics.  The
82	   extension would only be used when at least one TLV is to be added to
83	   the message.  This extension also applies to the lightweight versions
84	   of IGMPv3 and MLDv2 as defined in [RFC5790].

[nit] s/In this document, we describe/This document describes


86	   When this extension mechanism is used, it replaces the Additional
87	   Data section defined in IGMPv3/MLDv2 for TLVs.

89	   Additional Data is defined for query messages in IGMPv3 [RFC3376]
90	   Section 4.1.10 and MLDv2 [RFC3810] Section 5.1.12, and for report
91	   messages in IGMPv3 [RFC3376] Section 4.2.11 and MLDv2 [RFC3810]
92	   Section 5.2.11.

[minor]  s/query/Query  s/report/Report
To keep consistent terminology with the existing RFCs.


[major] Can TLVs only be used with queries and reports?  Given that
the length of the data is determined by the length of the IP packet
(not the length of the message), it seems like Additional Data
(including TLVs) could be used in any message -- I don't think this
handling of a longer length is properly specified anywhere.  Maybe
this document would be a good place to tighten that up.


94	   One such TLV is being defined for use in BIER IGMP/MLD overlays
95	   [I-D.ietf-bier-mld].  This TLV provides BIER specific information
96	   that only will be processed by BIER routers.

[] This information is not needed in this document; please remove it.


...
106	3.  Extension Format

108	   A previously reserved bit in the IGMPv3 and MLDv2 headers is used to
109	   indicate whether this extension is used.  When this extension is
110	   used, the Additional Data of IGMPv3 and MLDv2 messages would be
111	   formatted as follows.  Note that this format contains a variable
112	   number of TLVs.  It MUST contain at least one TLV.

[nit] s/would be formatted/is formatted


[major] "MUST contain at least one TLV"  What should a receiver do if
the bit is set, but a TLV is not included?


...
138	      Extension Type: 2 octets.  This identifies a particular Extension
139	      Type as defined in the IGMP/MLD Extension Type Registry.  If this
140	      is not the first TLV, it will follow immediately after the end of
141	      the previous Extension Value field, or immediately after the
142	      previous Extension Length field if the previous Extension Length
143	      was zero.  There is no alignment or padding.

[minor]
OLD>
   If this is not the first TLV, it will follow immediately after the
   end of the previous Extension Value field, or immediately after the
   previous Extension Length field if the previous Extension Length
   was zero.  There is no alignment or padding.

NEW>
   If this is not the first TLV, it will follow immediately after the
   end of the previous one.  There is no alignment or padding.


...
149	      Extension Value: This field contains the value.  The length and
150	      the contents of this field is according to the specification of
151	      the Extension Type as defined in the IGMP/MLD Extension Type
152	      Registry.  The length MUST be as specified in the Extension Length
153	      field.

[minor] s/
   according to the specification of the Extension Type as defined in
   the IGMP/MLD Extension Type Registry.  The length MUST be as specified
   in the Extension Length field.
   /according to the specification of the Extension Type.


...
161	3.1.  Multicast Listener Query Extension

163	   The MLD query format with extension is shown below.  The E-bit MUST
164	   be set to 1 to indicate that the extension is present.  Otherwise it
165	   MUST be 0.

[minor] s/The MLD query format/The MLD Query Message format [RFC3810]


[nit] s/Otherwise/Otherwise,/g


...
217	3.2.  Version 2 Multicast Listener Report Extension

219	   The MLD report format with extension is shown below.  The E-bit MUST
220	   be set to 1 to indicate that the extension is present.  Otherwise it
221	   MUST be 0.

[minor] s/The MLD report format/The MLDv2 Report Message format [RFC3810]


...
258	3.3.  IGMP Membership Query Extension

260	   The IGMP query format with the extension is shown below.  The E-bit
261	   MUST be set to 1 to indicate that the extension is present.
262	   Otherwise it MUST be 0.

[minor] s/The IGMP query format/The IGMP Query Message format [RFC3376]


...
288	3.4.  IGMP Version 3 Membership Report Extension

290	   The IGMP report format with the extension is shown below.  The E-bit
291	   MUST be set to 1 to indicate that the extension is present.
292	   Otherwise it MUST be 0.

[minor] s/The IGMP report format/The IGMPv3 Report Message format [RFC3376]


...
329	4.  Processing the extension

331	   How to validate and process a specific type will be defined in the
332	   respective type specifications, but prior to validating and
333	   processing each of the types, the following general validation MUST
334	   be done.

336	   First one MUST check that the E-bit is set, otherwise this
337	   specification does not apply.  There MUST be no data in the IP
338	   payload after the last TLV.  To check this, one will need to walk
339	   through each of The TLVs until there are less than four octets left
340	   in the IP payload.  If there are any octets left, validation failed.

342	   The walk also stops and validation fails if a TLV has a length
343	   exceeding the remainder of the IP payload.  For this validation, one
344	   only examines the content of the Extension Length fields.

346	   If the validation failed, the entire Additional Data field MUST be
347	   ignored as specified in IGMPv3 [RFC3376] and MLDv2 [RFC3810].

349	   If the validation succeeded, one will proceed examining each of the
350	   specified types and perform validation and processing of the
351	   respective types.  Unsupported types MUST be ignored; type validation
352	   and processing proceeds as if they were not present.

[] Suggestion>

   The procedure specified in this document applies only when the E-bit
   is set.

   If the validation of the TLVs fails, the entire Additional Data field
   MUST be ignored as specified in IGMPv3 [RFC3376] and MLDv2 [RFC3810].
   The following checks must pass for the validation of the TLVs not to
   fail:

      There MUST NOT be any data in the IP payload after the last TLV.
      To check this, one will need to walk through each of The TLVs until
      there are less than four octets left in the IP payload.  If there are
      any octets left, validation fails.

      The total length of the Extension MUST NOT exceed the remainder of
      the IP payload length.  For this validation, one only examines the
      content of the Extension Length fields.

   Future documents defining a new type MUST specify any additional
   processing and validation.  These rules, if any, will be examined only
   after the general validation (above) succeeds.

   Unsupported types MUST be ignored.


354	5.  Applicability and backwards compatibility
...
363	   Implementations that do not support this extension mechanism will
364	   simply ignore the extension, provided they are compliant with IGMPv3
365	   and MLDv2 RFCs, which specify that additional data must be ignored,
366	   and behave as if the extension is not present.  Implementations that
367	   support this extension MUST behave as if it is not present if they
368	   support none of the extension types in an IGMP/MLD message.  If they
369	   support at least one of the types, they will process the supported
370	   types according to the respective type specifications, and ignore any
371	   unsupported types.

[minor] s/
   Implementations that do not support this extension mechanism will simply
   ignore the extension, provided they are compliant with IGMPv3 and MLDv2
   RFCs, which specify that additional data must be ignored, and behave as
   if the extension is not present.
   /Implementations that do not support this extension mechanism will ignore
   it, as specified in [rfc3376] and [rfc3810].


[] I think that all the text starting with "Implementations that
support this extension..." is both unnecessary in this section (as it
is not abut backwards compatibility) and redundant (with the previous
section).


373	   It is possible that a new extension type only applies to queries, or
374	   only to reports, or there may be other specific conditions for when
375	   it is to be used.  A document defining a new type MUST specify
376	   clearly under what conditions the new type should be used, including
377	   for which message types.  It MUST also be considered what the
378	   behavior should be if a message is not used in the defined manner,
379	   e.g., if it is present in a query message, when it was only expected
380	   to be used in reports.

[minor] s/MUST specify clearly/MUST specify


[mayor] s/It MUST also be considered/It MUST also be specified


382	   When defining new types, care must be taken to ensure that nodes that
383	   support the type can co-exist with nodes that don't, on the same
384	   subnet.  There could be multiple routers where only some support the
385	   extension, or multiple hosts where only some support the extension.
386	   Or a router may support it and none of the hosts, or all hosts may
387	   support it, but none of the routers.  With multiple types being used,
388	   it must also be considered that some hosts or routers may only
389	   support some of the types, and potentially one node might support
390	   only one type, and another node only another type.

[] This paragraph sounds rambling as it tries to cover too many cases...

Suggestion>
   When defining new types, care should be taken to consider the effect
   of partial support for the new TLV, by either the hosts or routers,
   on the same link.

[*] Note that I replace "subnet" with "link" -- this was recommended
on the list [1], but I saw no reply.

[1] https://mailarchive.ietf.org/arch/msg/pim/kIAgg3CgFpC2meQ191NxL0jBvjY


392	   Documents defining new types MUST have security considerations
393	   relevant to the new types.  They MUST also in addition to defining
394	   the behavior of hosts and routers supporting the new types, consider
395	   compatibility with hosts and routers on the same subnet that do not
396	   support the new types.  Further, they MUST consider whether there are
397	   any dependencies or restrictions on combinations between the new
398	   types and any pre-existing types.

[] The first sentence points to an already existing requirement for
all documents, so it is not needed.  The second sentence is redundant
with the last paragraph.  The third sentence can be appended to the
last paragraph.


400	   This document defines an extension mechanism only for IGMPv3 and
401	   MLDv2.  Hence this mechanism would not apply if hosts or routers send
402	   older version message.

[minor] s/would not apply if hosts or routers send older version
message./does not apply to older version messages.


404	6.  Security Considerations

[minor] Please indicate that the Security Considerations of rfc3376
and rfc3810 also apply here.


406	   This document extends IGMP and MLD message formats, allowing for a
407	   variable number of TLVs.  Implementations must take care when parsing
408	   the TLVs to not exceed the packet boundary, an attacker could
409	   intentionally specify a TLV with a length exceeding the boundary.

[nit] s/extends IGMP/extends the IGMP


411	   An implementation could add a large number of minimal TLVs in a
412	   message to increase the cost of processing the message to magnify a
413	   Denial of Service attack.

415	   The respective types defined using this extension may impact security
416	   and this MUST be considered as part of the respective specifications.

[] "MUST be considered"  This (to consider) is not an action that can
be enforced -- as I mentioned before, Security Considerations are
already required for all drafts.  Unless you have something very
specific, I think this paragraph is not needed.


418	7.  IANA Considerations

420	   A new registry called "IGMP/MLD Extension Types" should be created
421	   with registration procedure "IETF Review" as defined in [RFC8126]
422	   with this document as a reference.  The registry should be common for
423	   IGMP and MLD and can perhaps be added to the "Internet Group
424	   Management Protocol (IGMP) Type Numbers" section.  The initial
425	   content of the registry should be as below.

[minor] Suggestion>

   IANA is asked to create a new registry called "IGMP/MLD Extension Types"
   in the "Internet Group Management Protocol (IGMP) Type Numbers" section,
   with registration procedure "IETF Review" [RFC8126], and with this
   document as a reference.  The registry is common for IGMP and MLD.  The
   initial content of the registry should be as below (empty).

[End of Review -04]