Re: [MMUSIC] Benjamin Kaduk's Discuss on draft-ietf-mmusic-ice-sip-sdp-37: (with DISCUSS and COMMENT)

Benjamin Kaduk <kaduk@mit.edu> Tue, 06 August 2019 20:00 UTC

Return-Path: <kaduk@mit.edu>
X-Original-To: mmusic@ietfa.amsl.com
Delivered-To: mmusic@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id B168A12003F; Tue, 6 Aug 2019 13:00:32 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.2
X-Spam-Level:
X-Spam-Status: No, score=-4.2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
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 LRu3DGEtaWB7; Tue, 6 Aug 2019 13:00:29 -0700 (PDT)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id C809F1200D8; Tue, 6 Aug 2019 13:00:28 -0700 (PDT)
Received: from kduck.mit.edu ([24.16.140.251]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id x76K0JYW032715 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 6 Aug 2019 16:00:21 -0400
Date: Tue, 06 Aug 2019 15:00:19 -0500
From: Benjamin Kaduk <kaduk@mit.edu>
To: Christer Holmberg <christer.holmberg@ericsson.com>
Cc: The IESG <iesg@ietf.org>, "draft-ietf-mmusic-ice-sip-sdp@ietf.org" <draft-ietf-mmusic-ice-sip-sdp@ietf.org>, "mmusic-chairs@ietf.org" <mmusic-chairs@ietf.org>, "fandreas@cisco.com" <fandreas@cisco.com>, "mmusic@ietf.org" <mmusic@ietf.org>
Message-ID: <20190806200018.GL59807@kduck.mit.edu>
References: <156505044722.2011.681165665144624888.idtracker@ietfa.amsl.com> <HE1PR07MB3161ED365902E5643690CA4493D50@HE1PR07MB3161.eurprd07.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <HE1PR07MB3161ED365902E5643690CA4493D50@HE1PR07MB3161.eurprd07.prod.outlook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: <https://mailarchive.ietf.org/arch/msg/mmusic/Wg2bkWglx420PoeEiOweB9SNqyQ>
Subject: Re: [MMUSIC] Benjamin Kaduk's Discuss on draft-ietf-mmusic-ice-sip-sdp-37: (with DISCUSS and COMMENT)
X-BeenThere: mmusic@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Multiparty Multimedia Session Control Working Group <mmusic.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mmusic>, <mailto:mmusic-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mmusic/>
List-Post: <mailto:mmusic@ietf.org>
List-Help: <mailto:mmusic-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mmusic>, <mailto:mmusic-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 06 Aug 2019 20:00:33 -0000

On Tue, Aug 06, 2019 at 12:42:28PM +0000, Christer Holmberg wrote:
> Hi Benjamin,
> 
> Thank You for the review! Please see inline.
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> > A fairly minor point, but the example in Section 4.6 is not compliant with the rest of the document. 
> > Specifically, any implementation *of this document* must include the "ice2" ice-option in addition
> > to any other option being used, per Section 3.2.1.5.
> 
> Good catch. We'll add the 'ice2' ice-option to the example.
> 
> ---
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> > Do we have anywhere a definition of what it means to "indicate ICE support in an SDP offer/answer"? 
> > (As distinct from ice2 support.)  We refer to the concept in several places but there are many protocol 
> > fields that might be interpreted as such; is any one of them sufficient?
> 
> I don't there is a definition. It basically means when the offer or answer contains ICE-related attributes.
> 
> I guess we could add some text about that.
> 
> ---
> 
> Section 2
> 
> > The suggested text in RFC 8174 includes a full BCP 14 citation with both RFCs; please consider using that form.
> 
> Ok.
> 
> ---
> 
> Section 3.2.1.5
> 
> >   An agent compliant to this specification MUST include an SDP ice-
> >   options attribute with an "ice2" attribute value [RFC8445].  If an
> >   agent receives an SDP offer or answer that does not contain an SDP
> >   ice-options attribute with an "ice2" attribute value, the agent can
> >   assume that the peer is compliant to [RFC5245].
> >
> > I think this can only be assumed if there is some other indication of ICE support -- stock SDP O/A does not mandate ICE, IIRC.
> 
> Correct.
> 
> ---
> 
> Section 3.2.2
> 
> > Aren't "rtcp attribute SHOULD be included" and "rtcp attribute MAY be omitted" just duplicating existing normative requirements
> > from previous specifications (which thus would not need new normative language here)?
> 
> Correct. The SHOULD and MAY shall not be normative. The MUST will remain, though.
> 
> ---
> 
> Section 3.2.5
> 
> >       implementation dependent.  Informally, the responding agent MAY
> >       consider the mismatched transport address information as a
> >
> > Perhaps the capitalized "MAY" is not needed for an informatl description?
> 
> I agree. Will fix that.
> 
> >   2.  The transport address from the peer for the default destination
> >       correspond to IPv4/IPv6 address values "0.0.0.0"/"::" and port
> >
> > What does "correspond to" mean here (and later)?
> 
> Perhaps "corresponds" is the wrong wording. It basically means that the default destination IP address is set to 0.0.0.0 (IPv4) or :: (IPv6).
> 
> So, perhaps using "set" or "represents" instead:

That was my guess, so thank you for confirming.

>        "The transport address from the peer for the default destination
>        is set to/represents address value "0.0.0.0"/"::" and port value "9"."
> 
> ---
> 
> Section 3.3.1
> 
> > If the initial offer SHOULD contain an ice-pacing attribute, why do we not include one in the 
> > examples (both in Section 3.2.6 and Appendix A)?
> 
> We will add the attribute to the examples.
> 
> ---
> 
> > Section 3.3.2
> >
> > (ice-pacing in examples could be good for answers, too)
> 
> Will fix.
> 
> > To check my understanding, the requirement for transport protocol match beween m= offer/answer applies 
> > just to the *default* destination, i.e., the address from the c= line and the port from the m= line, and thus I 
> > can have a=candidate entries for both IPv4 and IPv6 for the same m= section? 
> 
> Correct.
> 
> > Or does "In each "m=" line, the answerer MUST use the same transport protocol as was used in the offer "m=" line." 
> > also restrict the a=candidate attributes?  (As Éric notes, IPv6 examples would go a long way.)
> 
> No, it only applies to the "m=" line.
> 
> (Unfortunately, for backward compatibility, we have to deal with some legacy offer/answer restrictions, including those related to the m= line values. If we would know that every device (including intermediaries) support ICE we wouldn't need the m= line for anything)
> 
> I think Adam earlier addressed Éric's comment on having IPv6 examples. However, if including IPv6 examples would make the spec more easy to understand then I think we should include them.

Ah, Adam's response had slipped my memory with the intervening IETF week :)
If there's not going to be a reason in practice to do something, we may not
need to provide an example of it -- do what you think is best.

> ---
> 
> Section 3.3.4
> 
> >   If there are one or more check lists with the state set to Failed,
> >   the controlling agent MUST generate a subsequent offer in order to
> >   remove the associated data streams by setting the port value of the
> >   data streams to zero (Section 3.4.1.1.2), even if the peer did
> >   indicate support for the 'ice2' ice-option.  If needed, such offer
> >   can also be used to align the connection address, port and transport
> >   protocol, as described above.
> >
> > It feels a little weird to me that we say "can also be used" instead of "is used", since it seems to be a MUST-level 
> > requirement for the next offer to normalize the address/port/protocol in the offer with those discovered via ICE.
> 
> I agree. Will modify to "is used".
> 
> ---
> 
> Section 3.4.1.1
> 
> >   The rules governing the ICE restart imply that setting the connection
> >   address in the "c=" line to 0.0.0.0 (for IPv4)/ :: (for IPv6) will
> >   cause an ICE restart.  Consequently, ICE implementations MUST NOT
> >   utilize this mechanism for call hold, and instead MUST use
> >   "a=inactive" and "a=sendonly" as described in [RFC3264].
> >
> > Is this really "ICE implementations" or "SDP O/A implementations supporting ICE"?
> 
> "ICE implementations" is more common, and the draft defines the procedures for ICE implementations using SDP O/A.
> 
> ---
> 
> Section 3.4.1.2.2
> 
> >   line associated with that data stream MUST match the data associated
> >   with the nominated pair for that data stream.  In addition, the
> >   offerer only includes SDP candidates representing the local
> >   candidates of the nominated candidate pair.  The offerer MUST NOT
> >   include any other SDP candidate attributes in the subsequent offer.
> >
> > Does this mean that exactly one a=candidate line must appear in the corresponding m= section?
> 
> Correct. Do you think that needs to be more clear?

It's probably okay as-is; this was mostly to double-check my understanding.

> ---
> 
> Section 3.4.1.3
> 
> >   A lite implementation MUST NOT add additional host candidates in a
> >   subsequent offer.  If an agent needs to offer additional candidates,
> >   it MUST restart ICE.  Similarly, the username fragments and passwords
> >   MUST remain the same as used previously.  If an agent needs to change
> >   one of these, it MUST restart ICE for that data stream.
> >
> > nit: This "MUST remain the same" is worded oddly, as the next sentence effectively contradicts it.
> 
> I agree. Maybe something like this:
> 
> "A lite implementation MUST NOT add additional host candidates in a
> subsequent offer, and it MUST NOT modify the username fragments
> and passwords.  If an agent needs to offer additional candidates, or
> modify the username fragments and password, it MUST restart ICE
> for that data stream."

Works for me, thanks.

> ---
> 
> >Section 3.4.3.1
> >
> >   o  If ICE state is completed and the SDP answer conforms to
> >      Section 3.4.2, the agent MUST remain in the ICE completed state.
> >
> > It's not entirely clear what "conforms to Section 3.4.2" means, given that some situations in Section 3.4.2 effectuate ICE restart.
> 
> Section 3.4.2.1. is about ICE restart.

Indeed, and it looks like I had confused myself a bit when I wrote this.
(3.4.2 toplevel does have "The agent SHOULD generate an answer for this
data stream as if the remote- candidates attribute had not been present,
and then restart ICE for this stream.", which is not quite the same.)

> Perhaps the following would be more clear:
> 
>         "If ICE state is completed and the SDP answer conforms to
>          Section 3.4.2, and the answer is not associated with an 
>          ICE restart (3.4.2.1),  the agent MUST remain in the ICE completed state."

I'm definitely not insisting on this change, but it might still be worth
doing.

> ---
> 
> Section 3.4.3.2
> 
> >   there as described in section 12 of [RFC8445].  The state of ICE
> >   processing for each data stream MUST change to Running, and the state
> >   of ICE processing MUST change to Running
> >
> > Did this sentence get cut off prematurely?
> 
> I think the sentence is correct (perhaps it should say "ICE state" instead of "state of ICE processing" - will look into that), but there needs to be a "." at the end.

Thanks for checking.  It seemed to make sense if I just added the '.', but
double-checking seemed prudent.

> --
> 
> Section 4.1
> 
> > I appreciate that IP address privacy is mentioned here.  (It might be good in the security considerations, too.)
> 
> Will look into that.
> 
> ---
> 
> Section 4.2
> 
> > I don't really understand why there can be more than one remote-candidate for a given component.  Isn't there only 
> > going to be one nominated pair, and thus the single remote part of the pair?
> 
> Where do you read that there can be more than one remote-candidate for a given component?
> 
> Note that component is not the same as m= line: a given m= line can contain 2 components - one for RTP and one for RTCP.

This was user error, sorry -- I mentally put the "component-ID" element in
remote-candidate-att and not remote-candidate.

> ---
> 
> Section 4.5
> 
> >   If absent in an offer and answer the default value of the attribute
> >   is 50 ms, which is the recommended value specified in [RFC8445].
> >
> > nit: is this "offer and answer" or "offer or answer"?
> 
> Correct. Will fix that.
> 
> ---
> 
> Section 6
> 
> >   Note that ICE is not intended for NAT traversal for SIP, which is
> >   assumed to be provided via another mechanism [RFC5626].
> >
> > This sentence reads a bit oddly when one recalls that the full title of RFC 8445 is "Interactive Connectivity Establishment (ICE): 
> > A Protocol for Network Address Translator (NAT) Traversal".  Perhaps the intended sentiment is more that the scheme described 
> > in this document for using SDP to provide an ICE usage is not the primary mechanism for NAT traversal for SIP, though if one chooses 
> > to use it as such, the procedures in the rest of the section are needed.
> 
> I guess the statement is misleading. What it means that ICE is not intended for NAT traversal of the *SIP signalling*. The procedures in section 6 still apply when using SIP to negotiate ICE for data plane NAT traversal.

Ah, that's a subtlety that I missed on first read.  It makes sense now :)

> ---
> 
> Section 6.1.1
> 
> >   described in [RFC3262].  Such retransmissions MUST cease on receipt
> >   of a STUN Binding request with transport address matching candidate
> >   address for one of the data streams signaled in that SDP or on
> >   transmission of the answer in a 2xx response.  If no Binding request
> >
> > nit: I think "candidate address" needs an article.
> 
> I guess "transport address" also needs one?

It can take one, yes, though my native-speaker filter would apparently be
willing to accept it without one, too.

> Something like:
> 
> "with a transport address matching the candidate address for..."
> 
> >   the session terminated.  For the ICE lite peers, the agent MUST cease
> >   retransmitting the 18x after sending it four times since there will
> >   be no Binding request sent and the number four is arbitrarily chosen
> >   to limit the number of 18x retransmits (poor man's version of
> >   [RFC3262] basically).  (ICE will actually work even if the peer never
> >
> > nit: the tone of the parenthetical is rather distinct from conventional RFC style.
> 
> Unless the other authors want to keep/modify it, I suggest to remove all text after "...limit the number of 18x retransmits.

I don't remember if we imply anywhere else that the 18x helps middleboxes,
but no real objection from here.

> The relation to RFC 3262 is already described earlier in the section.
> 
> ---
> 
> Section 6.4
> 
> >   Indeed, an agent SHOULD NOT indicate that QoS preconditions have been
> >   met until the checks have completed and selected the candidate pairs
> >   to be used for media.
> >
> > Does this include the updated offer/answer exchange having completed?
> 
> I am not sure I understand what you mean by "offer/answer exchange having completed".

The selection of candidate pairs is an ICE-level operation, and we can
start using them for media once ICE has completed.  There's also a partial
requirement in this document to do an updated offer/answer exchange that
records the result of ICE in the m= and c= lines (basically, for the
benefit of middleboxes).  With the current text, I don't know if there's
any interaction between this offer/answer update and the indicated checks.
(Reading it again, I'm not sure there actually is such an interaction --
sorry if I was just confused!)

> ---
> 
> Section 8
> 
> > I think this top-level section would be a great place to reiterate that the SDP and ICE security considerations apply, since 
> > we are using both of them in combination.  Specifically, the IP Address Privacy concerns are only briefly mentioned 
> > elsewhere in the document, and could be worth reiterating.
> 
> I am not sure whether we need to mention the SDP security considerations, because they are implicit via the SDP O/A security considerations that we reference in section 8.1. But, they also apply to other sub sections, so I agree we can mention them on the top-level.
> 
> But, maybe something like:
> 
> "The generic ICE security considerations are defined in (RFC8445), and the generic SDP offer/answer security considerations are defined in (RFC3264). The security considerations also apply to implementations of this document."

That works for me; thanks!

-Ben

> ---
> 
> Section 11.2
> 
> > draft-ietf-ice-pac has to be normative, since it is an OPTIONAL protocol feature (per https://www6.ietf.org/iesg/statement/normative-informative.html).
> 
> We will make it normative. 
> 
> (Previously ice-pac was only mentioned in a note, so I guess that is the reason why the reference was only informative.)
> 
> ---
> 
> Regards,
> 
> Christer
>