[MMUSIC] Review of draft-ietf-mmusic-ice-sip-sdp-10

[Adam Roach <[e-mail]>]

I've done a review of draft-ietf-mmusic-ice-sip-sdp-10, and have a 
handful of comments. I've broken these down into three sections: 
big-picture questions, technical comments, and editorial comments. I 
apologize for the lateness in posting this review; the comments are 
substantial, and converting them into email form took far longer than I 
anticipated.


Big Picture Questions

The text in section 9, read literally, deprecates RFCs 4091 and 4092. If 
that is the actual intention, the abstract and "obsoletes" metadata need 
to be updated to reflect this fact. If the deprecation is qualified 
(e.g., the RFCs are still okay in other contexts, but should not be used 
in conjunction with ICE), then the text in section 9 needs to be much 
clearer.

It's not clear to me what the relationship is between this document and 
RFC 5245 (and, for that matter, 6544). Is this supposed to obsolete RFC 
5245? That appears to be the intention. It's also not clear why we would 
obsolete 5245 without also incorporating 6544 -- it seems that doing so 
leaves the use of ICE TCP with SIP to be kind of in limbo.

Assuming the intention *is* to replace RFC 5245, the structure of this 
document seems to leave non-SIP uses of RFC3264 (e.g., WebRTC) stranded. 
If the intention is *not* to replace RFC 5245, the replication of all of 
the SDP syntax seems to be problematic: the the case of a conflict, 
which specification is intended to govern?

In short, I think the relationship between this document, the 
contemporary work (e.g., ICE-BIS), and the historical work on which it 
is based needs to be made much clearer in the document itself, and we 
need to have a clear picture about how to handle SDP for non-SIP uses of 
ICE (e.g., by splitting this document into an SDP part and a SIP part, 
similar to how we split 3264 from 3261).


Technical Comments

I believe that the RECOMMENDED-strength normative statement in the final 
paragraph of 4.1.1.1 overstates things a bit. While this guidance makes 
set in certain circumstances. For example, for devices that are known to 
typically known to be on a single network with mutual routability, using 
a relayed candidate as a default will lead to calls being unnecessarily 
sent out of network and back. I strongly suggest that this paragraph be 
reworked to be a considered discussion of the different trade-offs 
involved in using each kind of candidate, with a non-normative 
suggestion to make the behavior configurable in user agents. What I want 
to avoid here is guidance which leads to devices behind a single ALG 
necessarily going through two relays by default when talking to each 
other (which is what the current guidance can lead to -- I can sketch 
this out in more detail if it's not obvious how this happens).

Section 4.1.1.2 uses the word "media stream" in a way that's not 
consistent with the way that term is defined by RFC 7656, which could 
lead to confusion. In RFC 7656 terminology, "media stream" is an alias 
for "RTP stream," which is identified by a single SSRC. With 
technologies such as simulcast. A single m-line can contain multiple 
media streams. I think you want the term "source stream," with a 
citation of that term in RFC 7656.

Section 4.1.1.2 discusses ice-options attributes in the terms of 
"support", without any discussion of activation. Although this is 
treated (somewhat lightly) later in the document, this smells like the 
same kind of potential morass we ran into with SIP options tags: the 
patchwork means of indicating feature *support* versus feature 
*activation* made it very difficult to specify and implement things in a 
consistent fashion. I strongly recommend that this document spend a bit 
more text discussing this distinction, and maybe even consider a formal 
syntax for distinguishing between supported and activated features.

Section 4.1.3 should probably point out that, in the case of forking, 
the same local candidates are used for all branches of the fork.

Section 4.2, section paragraph: replace "be rejected" with "fail" -- 
there are more ways to fail than rejection, and the action should be the 
same for rejection as for other failures.

Section 4.2.1.2 should add motivation for the requirement imposed by its 
second paragraph, or the requirement should be removed. The need to do 
this is not obvious, and I suspect it may be unnecessary.

Section 4.2.2.2.3 specifies, during offer processing, that the 
implementation "SHOULD wait for [ICE] checks to complete" before sending 
an answer. Keeping in mind that these updates typically happen with the 
SIP UPDATE method, and given that RFC 3261 specifies "TUs SHOULD respond 
immediately to non-INVITE requests" (cf. RFC3261 section 17.1), I'm 
concerned about the timing implications here. We should minimally point 
out that we're explicitly telling UAs to ignore this normative statement 
in RFC3261, and make sure that we've done the analysis to ensure that 
we're not going to cause problems for the SIP state machine (I'm 
concerned, in particular, about unnecessary SIP retransmissions here).

Section 4.2.4.1.4: the final paragraph seems to reiterate behavior 
described in ICE-BIS. It's probably not ideal to describe this in two 
places, as any subtle difference can result in interop problems.

Given the state of ICE today, I think the definition of "candidate" in 
section 5.1 should also include the syntax for TCP transports.

Section 5.1, <connection-address>: the language in here that states "If 
the DNS query returns more than on IP address, on is chosen," implying 
that one is chosen at random. There are specific procedures for how this 
is done (especially with IPv6); the statement should point to the 
selection mechanisms.

Section 11.2.1, final paragraph: We should have text in here addressing 
the fact that user agents that are not willing to receive non-ICE 
answers need to include "Require: ice" in their offer; and that clients 
that reject non-ICE offers should use the 421 response code to do so, 
and should indicate "Require: ice" in their rejections.



Editorial Comments (of varying severity)

The final sentence of the definition of "Default Destination/Candidate" 
is a bit difficult to follow. Suggest: ' For the RTCP component, the 
address and port are indicated using the "a=rtcp" attribute defined in 
[RFC3605], if present; otherwise, the RTCP component address is the same 
as the address of the RTP component, and its port is one greater than 
the port of the RTP component.'

Section 3, final paragraph: remove "the" before "[ICE-BIS]".

Section 4.1.3, second paragraph: eliminate the space before the first 
comma. Place a comma between "its role" and "and processes"

Section 4.1.3, final paragraph: replace "complaint" with "compliant".

The contents of section 4.1.5 appear to be completely redundant with the 
contents of section 4.1.5.1.1. I would recommend eliminating section 
4.1.5. Also, the section numbering here is really baroque and 
unnecessarily deep. For example, section 4.1.5.1 contains no text and 
only one subsection; this is also true of section 4.1.5.2. The more I 
look at this, the more bizarre it appears. I think the correct thing to 
do here is:
(a) Remove all the text currently in section 4.1.5.
(b) Move the paragraph in section 4.1.5.1.1 and the two paragraphs in 
4.1.5.2.1 into section 4.1.5
(c) Eliminate the (now empty) sections 4.1.5.1, 4.1.5.1.1, 4.1.5.2, and 
4.1.5.2.1.

Section 4.2.1.2.1, second paragraph says "The agent MAY include 
additional candidates it did not offer previously" before we explain how 
this can happen. I recommend adding a "(see section 4.2.4.1.1)" after 
"previously".

Section 4.2.2.3, final paragraph: The second line has an "i" that has 
been separated from the rest of its word (which appears on the following 
line).

Section 4.2.3: add commas: "being ICE-unaware, a subsequent" ... "this 
is done, this could result" ... "ICE-unaware, that answer would not"

Section 4.2.3.1.1 should not rely on the section header exclusively for 
context here. The first sentence should read "If ICE support is 
indicated int he SIP answer and the offer was a restart..." Ditto for 
sections 4.2.3.1.3 (add "and ICE is running") and 4.2.3.1.3 ("and ICE is 
completed").

Section 5.1 indicates that ABNF is used for "candidate," but this is not 
repeated for (e.g.) sections 5.3. and 5.4. I would suggest moving the 
statement from 5.1 (and 5.2) into section 5 so that it applies to all 
syntax definitions.

In all of section 5, the attributes really should include examples.

Also, section 5 should indicate which exernally-defined ABNF constructs 
are being used in the document. This is done in some locations, but not 
others. Running the ABNF through a validator will highlight all 
externally-defined elements; it will also help uncover any other issues 
with the ABNF.

All attributes are defined with ABNF of the form: "attribute-name" ":" 
-- this is unnecessary, and should be shortened to "attribute-name:".

Section 5.1, under <connection-address>: change "using an A" to "using 
an A record."

Section 5.1, under <transport> refers to specification of TCP with ICE 
in the future tense rather than the past. This should be reworked.

Section 5.1, under <foundation>, "...in the Frozen algorithm" should be 
followed by "defined in" and then a citation for where the Frozen 
algorithm is defined.

Section 5.1, <priority> -- this just says it's an integer, without 
describing its meaning. Either explain what it means, or cite something 
that does so.

Section 5.1, <rel-addr> and <rel-port>: replace "<rel-addr> and 
<rel-port> is equal" with "<rel-addr> and <rel-port> are equal".

Section 5.2, in the ABNF for "remote-candidate", indicate that 
component-ID is defined in RFC 4566.

Section 5.4, final paragraph: this indicates that the grammar allows for 
"6 bits of randomness per character" -- this should indicate that it 
allows for "6 bits of information per character".

Section 6: rewrite the first sentence using active voice so as to 
positively identify which entities the requirements apply to.

Section 7.1: "Note that" is an awkward way to start a section; I suggest 
striking it.

Section 7.1, first line: replace "may not" with "might not." I had to 
read this several times to realize that the intended meaning here was 
"might not." In colloquial usage, "may not" is a prohibition (e.g., "you 
may not take more than two cookies.")

Section 8.1, first paragraph refers to "ringing the phone of the called 
party." Old-style telephones are only one use case for SIP; this should 
instead read "alerting the called user agent."

Section 8.1.1 second paragraph is HUGE. Consider how to split it up to 
be less "wall of text"y.

Section 8.1.1, fourth paragraph replace "each candidate" with "every 
candidate."

Section 8.1.1, final paragraph: "it's a localized decision" is extremely 
informal and doesn't match the general style for RFCs. Please rephrase.

Section 8.1.2, first paragraph describes a kind of difficult-to-follow 
situation in which an INVITE contains an offer, and the answer is 
provided in a PRACK response, resulting in an empty "200 OK." Two 
comments: (1) this would really benefit from a sequence diagram; and (2) 
are SBCs and ALGs likely to do the right thing with empty 200s?

Section 8.1.2, second paragraph indicates that placing an offer in an 
INVITE the 2xx can cause substantial PDD and clipping; this is true 
regardless of whether ICE is in use. I think you mean to say that ICE 
can exacerbate PDD and clipping in this case rather than causing it.

Sections 8.4, 9, and 11.1 -- remove the redundant RFC numbers (as in 
"defined in RFC 3312 [RFC3312]").

Section 10 refers to timer Ta without indicating where it is defined.

Section 11.1: Replace "SIPS" with "TLS". As Olle is fond of noting, 
there are circumstances in which TLS is feasible but SIPS is not, and 
specifying "TLS" here is sufficient.

Section 11.2: replace "stun" with "STUN."

Section 11.2.1, final paragraph: replace "if its not used" with "if it's 
not used."

Section 11.2.2 -- expand the acronym "NAT" on first use.

Section 11.2.2, first paragraph: replace "application layer SIP" with 
"application-layer SIP"

Section 12.1: Insert "The" at the very beginning of the first sentence.

Section 12.2: Make "RFC 5245" a reference.

Section 12.2, third paragraph should start "[RFC6679] defines the..." 
(add the word "the")

Section 13: make "RFC 5245" and "RFC 6336" references.


/a