Subject: [MMUSIC] AD Review: draft-ietf-mmusic-ice-sip-sdp

From: Adam Roach <[email-masked]>
Date: Thu, 6 Jun 2019 20:10:57 -0500

This is my AD review for draft-ietf-mmusic-ice-sip-sdp.

Thanks to everyone for the huge amount of work invested in updating
these ICE procedures. My review did not turn up any technical issues, and
I will be progressing the document to IETF Last Call momentarily.

I have a fair number of minor changes that should be made to the
document.  Treat them as you would any other IETF last call comments.

---------------------------------------------------------------------------

§2:

 >  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
 >  "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
 >  "OPTIONAL" in this document are to be interpreted as described in RFC
 >  2119 [RFC2119].

Please update this paragraph to use the text from RFC 8174.

---------------------------------------------------------------------------

§3.1:

 >  corresponds to the [RFC3264] Offer/Answer protocol and the
 >  terminologies offerer and answerer correspond to the initiator and
 >  responder terminologies from [RFC8445] respectively.

Nit: this is a somewhat unconventional use of the word "terminologies," 
and is
slightly awkward in a couple of other ways as well.  Suggest updating as:

    corresponds to the [RFC3264] Offer/Answer protocol, and the
    terms "offerer" and "answerer" correspond to the initiator and
    responder roles from [RFC8445] respectively.

 >  Once the initiating agent has gathered, pruned and prioritized its

Nit: "...gathered, pruned, and prioritized..."
                          ^

---------------------------------------------------------------------------

§3.2.1.2:

 >  Within a "m=" section, each candidate (including the default

Nit: ...an "m=" section...
          ^
---------------------------------------------------------------------------

§3.2.1.6:

 >  bandwidth value of zero [RFC4566], the agent MUST still include ICE
 >  related SDP attributes.

Nit: "...ICE-related..."

---------------------------------------------------------------------------

§3.3.2:

 >  the answerer MUST NOT include any ICE related SDP attributes in the

Nit: "...ICE-related..."

 >  If the answerer detects a possibility of the ICE mismatch, procedures

Nit: "...possibility of an ICE mismatch..."
                         ^^

 >  Note:  <draft-holmberg-ice-pac>> provides guidance on finding working

Nit:  ...[draft-holmberg-ice-pac]...

---------------------------------------------------------------------------

§3.3.3:

 >  On the other hand, if the answer indicates the support for ICE but

Nit: "...indicates support for ice..."

 >  in the answer, as described in (Section 3.2.5), the offerer MUST

Nit: "...described in Section 3.2.5, the offerer MUST..."

 >  Note:  <draft-holmberg-ice-pac>> provides guidance on finding working

Nit:  ...[draft-holmberg-ice-pac]...

---------------------------------------------------------------------------

§3.4.1.1.1:

 >  An agent sets the rest of the ice related fields in the SDP for this

Nit: "...ICE-related..."

---------------------------------------------------------------------------

§3.4.1.1.3:

 >  the SDP for this data stream as if this was an initial offer for that

Nit: "...as if this were..."

---------------------------------------------------------------------------

§3.4.1.2.1:

 >  gathered since the last offer/ answer exchange, including peer

Nit: "...offer/answer..."

---------------------------------------------------------------------------

§3.4.1.2.1:

 >  connection address, port and transport protocol in each "c=" and "m="

Nit: "...connection address, port, and transport protocol..."
                                  ^
---------------------------------------------------------------------------

§3.4.1.3:

 >  If the ICE state is running, a lite implementation MUST include all

Nit: ...state is "running", a lite...

 >  formed identical to the procedures for initial offers.

Nit: "...identically..."

 >  it MUST restart ICE.  Similarly, the username fragments or passwords

Nit: "...fragments and passwords..."

---------------------------------------------------------------------------

§3.4.3.1:

 >  media server during call hold using 3rd party call-control
 >  procedures.  Omitting further details how this is done, this could

Nit: cite RFC 3725 here.

---------------------------------------------------------------------------

§3.4.3.1:

 >     If a pair on the new check list was
 >     also on the previous check list, and its state is not Frozen, its
 >     state is copied over.  Otherwise, its state is set to Frozen.

How is this procedure different than the simpler (and equivalent sounding):

      If a pair on the new check list was also on the previous check list,
      its state is copied over.  Otherwise, its state is set to Frozen.

---------------------------------------------------------------------------

§3.4.3.1.1:

 >  The agent MUST remember the nominated pair in the Valid list for each
 >  component of the data stream, called the previous selected pair prior
 >  to the restart.

This is hard to parse. I think it means to say:

    The agent MUST remember the nominated pair in the Valid list for each
    component of the data stream, called the "previous selected pair", prior
    to the restart.

(note the addition of quotation marks and a comma)

There's a similar confusing lack of quotation marks in section 3.4.3.2.

---------------------------------------------------------------------------

§4.1:

 >  <component-id>:  is a positive integer between 1 and 256 (inclusive)
 >     that identifies the specific component of the dta stream for which

Nit: "...data stream..."

 >  a reference to the document defining the usage of the extension

Nit: missing ending period.

---------------------------------------------------------------------------

§4.4:

 >  The ice-ufrag attribute MUST contain at least 24 bits of randomness,
 >  and the ice-pwd attribute MUST contain at least 128 bits of
 >  randomness.

This is reiterating normative requirements from RFC 8445 section 5.3.
In general, reiterating normative behavior from a related specification
should be avoided.  I would suggest rephrasing as:

    RFC 8445 requires the ice-ufrag attribute to contain at least 24 bits of
    randomness, and the ice-pwd attribute to contain at least 128 bits of
    randomness.

---------------------------------------------------------------------------

§4.6:

 >  Example shows 'rtp+ecn' ice-option SDP line from <<RFC6679>>:

Nit: [RFC6679]

Also, add RFC 6679 to the "Informative References" section.

---------------------------------------------------------------------------

§6.1.1:

 >  the session terminated.  For the ICE lite peers , the agent MUST

Nit: remove the space before the comma.

---------------------------------------------------------------------------

§8.2.1:

 >  Require Header Field in their offer.  UAs that rejects non-ICE offers

Nit: "...that reject non-ICE..."

 >  SHOULD use a 421 response code, together with an Option Tag "ice" in
 >  the Require Header Field in the response.

This normatively reiterates normative language in RFC 5768. Please
change the "SHOULD" to a "will generally" or something similar.

---------------------------------------------------------------------------

§11.1:

 >  [RFC8126]  Cotton, M., Leiba, B., and T. Narten, "Guidelines for
 >             Writing an IANA Considerations Section in RFCs", BCP 26,
 >             RFC 8126, DOI 10.17487/RFC8126, June 2017,
 >             <http://www.rfc-editor.org/info/rfc5226>.
                                                  ^^^^

Nit: the URL should point to RFC 8126.

---------------------------------------------------------------------------

§11.3:

This section provides no new information, and should be removed.

---------------------------------------------------------------------------

Appendix D:

 >  This begs the question -- why is the updated offer/answer exchange

Nit: "This raises the question..."

---

From: Suhas Nandakumar <[email-masked]>
Date: Thu, 6 Jun 2019 22:05:20 -0700

Thanks Adam for the review. I have submitted version-36 to address the
feedback.

Cheers
Suhas

On Thu, Jun 6, 2019 at 6:11 PM Adam Roach <[email-masked]> wrote:

> _______________________________________________
> mmusic mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mmusic
>