Re: [pim] draft-ietf-pim-igmp-mld-extension WGLC

Stig Venaas <stig@venaas.com> Tue, 19 January 2021 18:20 UTC

Return-Path: <stig@venaas.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id AD9A93A1699 for <pim@ietfa.amsl.com>; Tue, 19 Jan 2021 10:20:23 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 3.103
X-Spam-Level: ***
X-Spam-Status: No, score=3.103 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, GB_SUMOF=5, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=venaas-com.20150623.gappssmtp.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Q_c-wq_zxpFj for <pim@ietfa.amsl.com>; Tue, 19 Jan 2021 10:20:21 -0800 (PST)
Received: from mail-io1-xd36.google.com (mail-io1-xd36.google.com [IPv6:2607:f8b0:4864:20::d36]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 6C3A93A1690 for <pim@ietf.org>; Tue, 19 Jan 2021 10:20:21 -0800 (PST)
Received: by mail-io1-xd36.google.com with SMTP id p72so16767571iod.12 for <pim@ietf.org>; Tue, 19 Jan 2021 10:20:21 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=venaas-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=822vJ4qpO4WL0bLCVNZk+i8bTwpgDc5PQMoTjH9KUv0=; b=hXVOCrbZkxw2/tPYdqGcAodr9TNac1BzAJAoPmS7/2+X1+SBfCsqAQNxiWm+3/XDvD +HFZXJUyqmF8Sa7pZYUPIgMEc638981xsgPHhDWPl1R6KNd8G5dRe7IvqBZJ6mtHzh/Y GDM32vFMAVwsiQTnYeSOkvaOLAM3LqdQNsZAKDyDCeKv2xBbQDL+O42KRU3Ab29rw7C4 SbjIeMLRxWQjSPCF+HrjCoyOEC8HkTe7kabd9bP2sq33NEu2m3u2QOMmIOsAlfC749Su OZzmiSe92YZPsvMse4bWB/DCceVAkFMcpwYq/IqqayGSJ+yPuuhWbQVPFOW14hNprLud 4wuw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=822vJ4qpO4WL0bLCVNZk+i8bTwpgDc5PQMoTjH9KUv0=; b=EPhMbWL4u7ZV5qHslQn2RCH+ueWDICdJycKRF4Pt+ET8yUrgQHUrNch3KexXkN/8b0 QSsN6dDWieVIsxhjzgYv5zVKAyoT5hBvDj5sFwTR2Z6v8zzpOxGCKE0MVkKVrW3gv27a Lhr/92gu5zvgUZ389tLdTlk5jVR/hTRcXI/nDyTDbVB6aU5IRBFji/bdt3Z6mmUxpMYZ 9TTy2U7s8dAV9HGHBhd6Xw7wTk8tGxlScfW3A4GUFJ0H6xvnkWqOCwcKugVkDqOCqm9p Za89SMYzrNwV3ocSS07XGt8NiaNFZauTvPNgXYReGI2Yjv06wrW3pm92rEVX80Gv3Xy2 CXRA==
X-Gm-Message-State: AOAM5300GUdKuSfeTUzmJNLDIk6w2XN9I9SC7v/23sNFrlTVPs+Kw/7V CVDLKQkQVQDi9Rh4qzI8ylJuZA/fXh3pB29s15xo/A==
X-Google-Smtp-Source: ABdhPJz2QixVnsbM8UFPW6HclrfyCAuAr53I8gQOv4OFlg1xDw2bjGAh061UqBBBSRsdS7mlow8Z8c8qrWV+9ZWhjSk=
X-Received: by 2002:a92:9f15:: with SMTP id u21mr4860613ili.120.1611080420385; Tue, 19 Jan 2021 10:20:20 -0800 (PST)
MIME-Version: 1.0
References: <BYAPR13MB25823AD8346977BC1FDE03BAF4CD0@BYAPR13MB2582.namprd13.prod.outlook.com> <MN2PR05MB598123DADDC8916389A69111D4C50@MN2PR05MB5981.namprd05.prod.outlook.com> <CAHANBtL7FXijAajqmz-vU+vZ_OUZ7XS7wynZgHF-x9TSukRhDA@mail.gmail.com> <CAHANBtLsasH1ZTT4tmLwvLaQ3NE4KuOzrmb_fqOcH3fcapUWkg@mail.gmail.com> <A725AB8B-6F24-45C1-BF44-854DD115EAE4@akamai.com>
In-Reply-To: <A725AB8B-6F24-45C1-BF44-854DD115EAE4@akamai.com>
From: Stig Venaas <stig@venaas.com>
Date: Tue, 19 Jan 2021 10:20:09 -0800
Message-ID: <CAHANBtJEHOKKYJER7wPPTAjpAb90LC0GiaU1QhE+4-wFeuNupA@mail.gmail.com>
To: "Holland, Jake" <jholland@akamai.com>
Cc: "Jeffrey (Zhaohui) Zhang" <zzhang=40juniper.net@dmarc.ietf.org>, Leonard Giuliano <lenny@juniper.net>, "jakeholland.net@gmail.com" <jakeholland.net@gmail.com>, "pim@ietf.org" <pim@ietf.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/347wUhIHBvkT-RBoxVsugCHZqVQ>
Subject: Re: [pim] draft-ietf-pim-igmp-mld-extension WGLC
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 19 Jan 2021 18:20:24 -0000

Hi Jake

Thanks for reviewing this, please see my comments inline.

On Sat, Jan 16, 2021 at 5:49 PM Holland, Jake <jholland@akamai.com> wrote:
>
> Hi Stig,
>
> Thanks for the update, on the whole I think this improved the text
> with respect to my previous comments and made several things better,
> thanks.
>
> However, while looking through the diffs, to me there seem some new
> points of confusion raised by the new edits, so here's a few new
> comments:
>
> 1.
> This new paragraph in the intro seems like it might be over-reaching:
>
>    When this extension mechanism is used, it will make use of the entire
>    Additional Data section defined in IGMPv3/MLDv2 for TLVs.  The TLV
>    scheme is flexible enough to provide for any future extensions.
>
> I think this spec doesn't say anything about possible alternative uses
> of other reserved bits indicating alternate semantics for the Auxiliary
> Data sections (and probably shouldn't make claims about being flexible
> enough for all future extensions).
>
> However, it does specify that when the E bit is set, no other data
> besides the Extension TLVs described here can appear in the Auxiliary
> Data part of the packet.  This is narrower than what's written, I think.

I do believe TLVs really is flexible enough for any future extension
and I mentioned this since people might question why we don't need
Additional Data when the extension is used. But I can certainly remove
this. The main point is that there is no data besides the Extension
TLVs. I'll try to improve this text. Of course we are also able to
extend IGMP/MLD in other ways when not setting the E-bit if we want.

> 1.a.
> Sorry, I should have noticed this in my first review.  But while revisiting
> this, I noticed that the MLDv2 and IGMPv3 specs call the extra data field
> "Auxiliary Data", but this spec calls it "Additional Data".
>
> This seems like a point of possible confusion, is there a reason not to call
> it "Auxiliary Data" in this doc as well?

The specs define both Additional Data and Auxiliary Data. The former
is at the end of the IGMP/MLD messages, which we are using. The latter
is potentially auxiliary data at the end of a group record. We are not
using that.
>
> 2.
> In section 3 the 2nd to last paragraph says there MUST be no data
> in the message after the last TLV:
>
>    There MUST be no data in the message after the last TLV.  The TLVs
>    are processed until the end of the message is reached.  When
>    processing the TLVs an implementation MUST keep track of how many
>    octets are remaining in the message and stop TLV processing when
>    there is no room for any further TLVs.  That is, TLV processing stops
>    if there are less than 4 octets remaining in the message after a TLV
>    is processed since there is not enough room for an additional minimal
>    TLV.  ...
>
>
> This wording I think is confusing on a few points:
> - "in the message" is sort of unclear (e.g. it could refer to the IP
>   payload length, at least for the last group record).  Although the doc
>   does refer briefly to the relevant sections of the IGMP and MLD specs
>   in the 2nd to last paragraph of the intro, it's not clear here, near
>   the normative requirements about parsing, what "in the message" means.
>   I think these requirements mean to say that the TLVs must fit entirely
>   within the Auxiliary Data field, with a minimum of leftover space.

Yes (except additional not auxiliary). Perhaps it is best if I refer
to bytes remaining in the IP payload.

> - the requirement "no data following the last TLV in the Auxiliary Data
>   field" might not be satisfiable, because the TLV length is given in
>   octets but the Auxiliary Data length is given in 32-bit words, so there
>   would have to be some leftover data when the sum of TLV lengths is not
>   a multiple of 4.  So I think this requirement needs to be articulated
>   better.

The additional data does not have to be a multiple of 4.

> - Nothing is stated about leftover data if any (since there might be up
>   to 3 octets unless there should be an alignment requirement?).  Maybe
>   it's worth stating it should be set to 0 and ignored, if there are
>   any extra octets at the end?
>
> 3.
> The second of these paragraphs in the "Extension Length" explanation in
> section 3 seems to be trying to clarify the parsing, but to me it raises
> more questions than it answers:
>       Extension Length: 2 octets.  This specifies the length in octets
>       of the following Extension Value field.  Note that this value may
>       be zero, in which case there is no Extension Value field present.
>
>       The next type field, if any, will come immediately after this
>       length field.
>
> I think this is trying to say that in the event of a 0-length Extension
> Value, the Extension Type of the next TLV (if any) follows the Extension
> Length immediately.
>
> But it made me realize nothing seems to state explicitly that the next
> extension type field (if any) immediately follows the extension value
> field (as opposed to padding for alignment or something, as Figure 1,
> right above this, seems to imply it might).
>
> It's a minor point, but it might make sense to tweak this to explain the
> whole picture with something like:
>       Extension Length: 2 octets.  This specifies the length in octets
>       of the following Extension Value field.
>
>       The next Extension Type field, if any, will come immediately after
>       the following Extension Value field (or immediately after the
>       Extension Length field, for cases with a 0-length Extension Value).

This sounds good. Thanks, I'll use this. It is pretty common to draw
the figure like this even when no alignment is expected, but I think
I'll point out that there is no alignment or padding.

> (And perhaps the 2nd paragraph or something like it should move into the
> explanation of the Extension Value, or into a paragraph explaining about
> the parsing overview, rather than being in the Extension Length explanation).

Agree, that is better.

> 4.
> I find this design a bit troubling:
>    Also if a TLV has a length exceeding the remainder of the
>    message, that TLV is ignored, and further TLV processing stops.
>
> As an implementor, if I see this scenario I would tend to assume that
> I'm talking to an implementation that is using a different non-standard
> meaning for the E bit and the Auxiliary Data (or perhaps an attack
> packet), and I would think the better choice is to discard the entire
> Auxiliary Data and treat it as though the E bit was not set, ignoring
> all the TLVs.  (I assume this should be OK since the Auxiliary Data can
> be ignored anyway by the devices that don't know about it.)

When the E-bit is set, one should be following this document, but of
course there may be bugs or attacks. I'm fine with being strict as you
suggest, the only downside is that one will have to validate this
before applying any of the TLVs. Since the length does not have to be
a multiple of 4, I think it might be best then to require that the
last TLV ends at the end of the IP payload (no trailing bytes).

Thanks,
Stig

>
> Anyway, I hope that's helpful, and thanks for the updates addressing my
> prior comments.
>
> Best regards,
> Jake
>
>
> On 1/15/21, 3:35 PM, "Stig Venaas" <stig@venaas.com> wrote:
>
> Thanks a lot for all the WGLC responses, in particular to Ian, Lenny,
> Jeffrey and Jake for great comments. Hope I didn't miss anyone.
>
> I just posted revision 03 that I think should address your comments.
> Great if you can have a look. I realize I forgot to update the
> acknowledgements though, so I will do that shortly.
>
> Regards,
> Stig
>
> On Thu, Dec 17, 2020 at 3:25 PM Stig Venaas <stig@venaas.com> wrote:
> >
> > Hi Jeffrey
> >
> > I appreciate your comments, please see responses inline.
> >
> > On Tue, Dec 15, 2020 at 6:21 PM Jeffrey (Zhaohui) Zhang
> > <zzhang=40juniper.net@dmarc.ietf.org> wrote:
> > >
> > > Support progressing this work.
> > >
> > >
> > >
> > > I have a couple of curiosity questions on the following:
> > >
> > >
> > >
> > >    Note that there may be additional data following this extension.  The
> > >
> > >    Total Extension Length field would indicate where this extension
> > >
> > >    ends, and the additional data starts.
> > >
> > >
> > >
> > > How would you have additional data following this extension? The TLV mechanism seems to be universal/generic to handle all possible extensions, so why would you have another extension blob after this, and how would you indicate so? By another bit in the reserved field?
> > >
> > > Just wondering if this additional data needs to be mentioned at all.
> >
> > I agree that there should be no need for additional data for future
> > extensions as TLVs can provide any potential extension. If the WG and
> > co-authors agree, I'm happy to remove the additional additional data.
> > We can then also simplify the length check and say that the extension
> > length specified must be equal to the remaining payload (subtracting
> > from packet payload length).
> >
> > >
> > >   Also, there is a possibility
> > >
> > >    that an implementation uses the Additional Data part of IGMP/MLD
> > >
> > >    messages, but not according to this extension scheme.
> > >
> > >
> > >
> > > Do you mean a non-standard existing implementation? Now that you mention that, are we supposed to define the interop procedures? Are they supposed to interop with the implementations following this draft at all?
> >
> > The IGMP/MLD RFCs specify how additional data may be present and how
> > it should be ignored. I've seen cases where implementations
> > intentionally (or maybe by accident) have sent packets with additional
> > data. I think it is safest to use a reserved bit to clearly indicate
> > that this extension is in place, rather than trying to parse whatever
> > additional data may be present.
> >
> > If we receive data from an existing implementation, the reserved bit
> > would not be set, and we would process it exactly as before.
> > If an existing implementation receives the new IGMP/MLD message, it
> > would both ignore the reserved bit, and the additional data according
> > to the RFCs (although if implementations tried to use additional data
> > in some way, then they might break, but I think we have to live with
> > that). They were not supposed to use it.
> >
> > When using the extension, one must consider interoperability with
> > implementations that do not support the extension mechanism, as well
> > as implementations that do not support the specified types. Not
> > supporting the mechanism, should be equivalent to not supporting any
> > of the types.
> >
> > I can try to add a few more details in the draft.
> > >
> > >
> > > Also, for the following:
> > >
> > >
> > >
> > >    The extension will be part of additional data as mentioned in
> > >
> > >    [RFC3810] Section 5.1.12 (resp.  [RFC3376] Section 4.1.10) for query
> > >
> > >    messages and [RFC3810] Section 5.2.12 (resp.  [RFC3376]
> > >
> > >    Section 4.2.11) for report messages.
> > >
> > >
> > >
> > > 5.2.12 should be 5.2.11.
> >
> > Good catch!
> >
> > Let me know if you have any thoughts on what I wrote above, or anything else.
> >
> > Thanks,
> > Stig
> >
> > >
> > >
> > > Jeffrey
> > >
> > >
> > >
> > > From: pim <pim-bounces@ietf.org> On Behalf Of Michael McBride
> > > Sent: Monday, December 7, 2020 7:24 PM
> > > To: pim@ietf.org
> > > Subject: [pim] draft-ietf-pim-igmp-mld-extension WGLC
> > >
> > >
> > >
> > > [External Email. Be cautious of content]
> > >
> > >
> > >
> > > Hello all,
> > >
> > >
> > >
> > > Today begins a two week wglc for https://urldefense.com/v3/__https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-extension-02__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFvcqNJMw$
> > >
> > >
> > >
> > > Please review (it’s a quick read) and let us know your opinion on it’s readiness to progress to the iesg.
> > >
> > >
> > >
> > > Thanks,
> > >
> > > mike
> > >
> > >
> > > Juniper Business Use Only
> > >
> > > _______________________________________________
> > > pim mailing list
> > > pim@ietf.org
> > > https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$
>
> _______________________________________________
> pim mailing list
> pim@ietf.org
> https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$
>