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

[Stig Venaas <[e-mail]>]

Hi Alvaro

Sorry for not replying earlier. Please see my comments inline.

As we discussed earlier in this thread, I'm fine with not making it a
formal update. I don't know if anyone in the WG thinks otherwise.

I did not get your comment about using TLVs in other message types
than queries or reports. I think I incorporated all the other changes.
Please see inline for details. I also attached a new draft version
where I incorporated the changes.

Appreciate the great review.

On Fri, Oct 22, 2021 at 3:53 AM Alvaro Retana <[e-mail]> wrote:
>
> Ping!
>
> On September 9, 2021 at 5:15:37 PM, Alvaro Retana ([e-mail]) wrote:
>
>
> 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.]

I think we still need Mike to do this. Although people also had a
chance to comment on this thread. Only Brian commented.

>
> 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"

Done
>
> 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).
>
ok.
I would argue that Additional Data is not an extension, it just makes
it extensible, which this document takes advantage of. But fine with
this.

> ...
> 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

Changed to "this document defines"

>
>
> 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.
ok

>
> [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.

This document extends IGMPv3 and MLDv2 and 3376/3810 only define 2
message types each. Query and Report. This document only applies to
those. Those documents define Additional Data for each of these
message types. See for instance RFC 3376 section 4.1.10.

This document defines the content of the Additional Data when a
previously reserved bit in the IGMPv3/MLDv2 messages is set.

I don't think this document should restrict the use of Additional Data
when that reserved bit is not set, and it should not apply to other
IGMP/MLD message types.

>
> 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.
>

I am happy to remove this, but there were members of the working group
suggesting a reference.

> ...
> 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
ok
>
> [major] "MUST contain at least one TLV"  What should a receiver do if the bit is set, but a TLV is not included?

Good point. I would say that the additional data MUST be ignored. It
should be treated as if the extension was not present. I added that to
the validation rules in section 4.

>
> ...
> 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.

Agree, thanks.
>
> ...
> 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.

Yes, that is better. Thanks.
>
> ...
> 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

Agree.
>
> ...
> 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]

Agree
>
> ...
> 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]
Agree

>
> ...
> 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]
Agree
>
> ...
> 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.

Thanks, the suggested text is great.

>
> 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].
ok

>
> [] 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).

Agree
>
> 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
Agree with these
>
> 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.

Yes, you're right. Thanks.

> [*] 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

I'm sorry I overlooked this email. I responded to it now and I agree,
link is correct.

>
> 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.

Agree. I changed the the last sentence to this:
Further, it must be considered whether there are any dependencies or
restrictions on combinations between the new types and any
pre-existing types.

Let me know if I should use normative language.

>
> 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.
Agree
>
> 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
Agree
>
> 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.

Yes, I suppose there isn't any point in saying that.

>
> 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).
Great, used this.
>
> [End of Review -04]
>
>
>
> _______________________________________________
> pim mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pim

Thanks for great comments,
Stig