Subject: [APPS-REVIEW] Review of draft-ietf-mmusic-ice-15.txt

From: Alexey Melnikov <[email-masked]>
Date: Sat, 28 Apr 2007 13:06:13 +0100

Hi Jonathan,
Eric Burger asked me as a member of Apps-review team to have a look at 
your document.

Overall I think the document is well written, has reasonable examples 
and detailed explanation for different design choices (I like that). It 
has proper IANA registrations and detailed Security Considerations.
Also, I don't think the document has any IDN related considerations.

Some comments are below, most of them are either nits or missing forward 
references.
=======

 >2.6.  Concluding ICE
 >
 >   Once all of the media streams are completed, the controlling endpoint
 >   sends an updated offer if the candidates in the m and c lines for the

Section 1 is quite clear that that this document talks about SDP, but 
not being familiar with SDP the first mentioning of "m" and "c" 
attributes caught me by surprise.
Can a reference to [10] be inserted here?

 >   media stream (called the DEFAULT CANDIDATES) don't match ICE's
 >   SELECTED CANDIDATES.

 >4.2.  Lite Implementation
 >
 >   For each media stream, the agent allocates a single candidate for
 >   each component of the media stream from one of its interfaces.  If an
 >   agent has multiple interfaces, it MUST choose one for each component
 >   of a particular media stream.  With the lite implementation, ICE
 >   cannot be used to dynamically choose amongst candidates.  Each
 >   component has an ID assigned to it, called the component ID.  For
 >   RTP-based media streams the RTP itself has a component ID of 1, and
 >   RTCP a component ID of 2.  If an agent is using RTCP it MUST obtain a
 >   candidate for it.

 [...]

 >   If an agent has included two candidates for a component, the v4
 >   candidate SHOULD be selected as the default.

This sentence is confusing.
Is this sentence talking about candidates received from the peer?

If not: this is not possible, as the agent was supposed to select
a single candidate, as per the first quoted paragraph.

 >5.7.2.  Computing Pair Priority and Ordering Pairs
 >
 >   Once the pairs are formed, a candidate pair priority is computed.
 >   Let O be the priority for the candidate provided by the offerer.  Let
 >   A be the priority for the candidate provided by the answerer.  The
 >   priority for a pair is computed as:
 >
 >      pair priority = 2^32*MIN(O,A) + 2*MAX(O,A) + (O>A?1:0)
 >
 >   Where O-P>A-P?1:0 is an expression whose value is 1 if O-P is greater
 >   than A-P, and 0 otherwise.

The formula uses O>A, so the explanation should use the same for 
consistency.
"P" is not defined anyway.

 >5.7.4.  Computing States

 [...]

 >   2.  The agent examines the check list for the first media stream (a
 >       media stream is the first media stream when it is described by
 >       the first m-line in the SDP offer and answer).  For that media
 >       stream, it:
 >
 >       *  Groups together all of the pairs with the same foundation,

Example here would be helpful.

 >       *  For each group, sets the state of the pair with the lowest
 >          component ID to Waiting.  If there is more than one such pair,
 >          the one with the highest priority is used.

 >7.1.1.1.  PRIORITY and USE-CANDIDATE
 >
 >   An agent MUST include the PRIORITY attribute in its Binding Request.
 >   The attribute MUST be set equal to the priority that would be
 >   assigned, based on the algorithm in Section 4.1.2, to a peer
 >   reflexive candidate, should one be learned as a consequence of this
 >   check (see Section 7.1.2.2.1 for how peer reflexive candidates are
 >   learned).

The last sentence is difficult to read. Should it be split into 2 sentences?

 >7.1.1.2.  ICE-CONTROLLED and ICE-CONTROLLING
 >
 >   The agent MUST include the ICE-CONTROLLED attribute in the request if
 >   it is in the controlled role, and MUST include the ICE-CONTROLLING
 >   attribute in the request if it is in the controlling role.  The
 >   content of either attribute MUST be the tie breaker that was
 >   determined in Section 5.2.

This is not detailed enough to understand what happens if both ends
think they are controlled or controlling.
In section 7.2.1.1 I found answer to my questions, so I suggest you add 
a forward reference to section 7.2.1.1 here.

 >7.1.2.1.  Failure Cases
 >
 >   If the STUN transaction generates a 487 (Role Conflict) error
 >   response, the agent checks whether it had included the ICE-CONTROLLED
 >   or ICE-CONTROLLING attribute in the Binding Request.  If the request
 >   had contained the ICE-CONTROLLED attribute, the agent MUST switch to
 >   the controlling role if it has not already done so.  If the request
 >   had contained the ICE-CONTROLLING attribute, the agent MUST switch to
 >   the controlled role if it has not already done so.

Is it possible that both ends switch to the opposite mode, i.e. in the 
unlikely case
that both ends have selected the same tie-breaker value?

 >   Once it has
 >   switched, the agent MUST immediately retry the request with the ICE-
 >   CONTROLLING or ICE-CONTROLLED attribute reflecting its new role.
 >   Note, however, that the tie-breaker value MUST NOT be reselected.

 >7.1.2.2.1.  Discovering Peer Reflexive Candidates

 [...]

 >   This peer reflexive candidate is then added to the list of local
 >   candidates for the media stream.  Its username fragment and password
 >   are the same as all other local candidates for that media stream.

This is the first time this requirement on password is mentioned. I 
suggest you mention
it earlier in the document, maybe the first time you mention password.

 >9.1.2.2.  Existing Media Streams with ICE Completed

 [...]

 >   The agent MUST include a candidate attributes for candidates matching
 >   the default destination for each component of the media stream, and
 >   MUST NOT include any other candidates.

Is this sentence trying to say that each component has only a single 
candidate?

 >15.1.  "candidate" Attribute
 >
 >   <connection-address>:  is taken from RFC 4566 [10].  It is the IP
 >      address of the candidate, allowing for IPv4 addresses, IPv6
 >      addresses and FQDNs.  An IP address SHOULD be used, but an FQDN
 >      MAY be used in place of an IP address.  In that case, when
 >      receiving an offer or answer containing an FQDN in an a=candidate
 >      attribute, the FQDN is looked up in the DNS using an A or AAAA

Both A *and* AAAA must be checked, if the agent supports both IPv4 and 
IPv6. Otherwise different
agents capable of using both IPv4 and IPv6 may end up selecting 
different addresses.

 >      record, and the resulting IP address is used for the remainder of
 >      ICE processing.

 >15.1.  "candidate" Attribute

I haven't check the ABNF with Bill Fenner's ABNF validator. Some minor 
comments below:

 >   component-id          = 1*DIGIT

One can be more strict with disallowing leading zeros ;-).
But more importantly, this doesn't specify the upper limit. I suggest 
you at least add an ABNF comment describing the highest value, there is 
no need to need to rewrite the ABNF to show it.

 >   priority              = 1*DIGIT

Same comment as above.

 >   candidate-types       = "host" / "srflx" / "prflx" / "relay" / token

I suggest you add a comment describing which "token" is referenced here.
Both  RFC 3261 and  RFC 4566 define "token" and definitions are not 
compatible.
(RFC 4566 allows for extra characters: "#" / "$" / "&" / "^" / "{" / "|" 
/ "}")

Some other general comments:
 I suggest using symbolic references in the document. There was a 
wonderful thread on this on the IETF mailing list recently.

 Should the Figure 1 be updated to show message flow? This will help in 
understanding the order of event.

_______________________________________________
APPS-REVIEW mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/apps-review

---

From: Dave Cridland <[email-masked]>
Date: Mon, 30 Apr 2007 11:31:38 +0000

On Sat Apr 28 13:06:13 2007, Alexey Melnikov wrote:
> Overall I think the document is well written, has reasonable 
> examples and detailed explanation for different design choices (I 
> like that). It has proper IANA registrations and detailed Security 
> Considerations.
> Some comments are below, most of them are either nits or missing 
> forward references.

I did note on a quick skim that this document has several normative 
references related to SIP (including SIP itself).

This rings alarm bells in my mind because XMPP/Jingle is also using 
ICE, yet does not use SIP - therefore, it seems logical that SIP 
should not be a normative reference.

A quick examination of where SIP is referenced suggest that SIP, at 
least, should be an informative reference here.

I've copied Robert McQueen, a Jingle implementor, since his 
understanding of Jingle is way better than mine, and he may be able 
to suggest which normative references should be moved to informative 
ones.

Dave.
-- 
Dave Cridland - mailto:[email-masked] - xmpp:[email-masked]
  - acap://acap.dave.cridland.net/byowner/user/dwd/bookmarks/
  - http://dave.cridland.net/
Infotrope Polymer - ACAP, IMAP, ESMTP, and Lemonade

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 30 Apr 2007 17:39:11 +0100

Dave Cridland wrote:

>> and detailed explanation for different design choices (I like that). 
>> It has proper IANA registrations and detailed Security Considerations.

I think draft-ietf-mmusic-ice-15.txt has a normative section describing 
SIP-specific behaviour.

> understanding of Jingle is way better than mine, and he may be able to 
> suggest which normative references should be moved to informative ones.

---

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 30 Apr 2007 10:48:47 -0600

> I think draft-ietf-mmusic-ice-15.txt has a normative section describing 

Ideally I think this document would define the ICE methodology in a 
generic fashion and other documents would describe how to apply that 
methodology in the context of using protocols such as SIP and XMPP. But 
I know it's more work to specify things that way, so personally I find 
it acceptable to define the SIP usage in this document and the XMPP 
usage in XEP-0176 [1].

Peter

[1] http://www.xmpp.org/extensions/xep-0176.html