Subject: [MMUSIC] Review of draft-ietf-mmusic-ice-13

From: Eric Rescorla <[email-masked]>
Date: Tue, 16 Jan 2007 20:47:01 -0800

$Id: draft-ietf-mmusic-ice-13-rev.txt,v 1.1 2007/01/16 23:38:58 ekr Exp $

GLOBAL COMMENTS
There is a *lot* of new terminology in this document. In my experience
glossaries at the beginning really don't help much. Each term should
be capitalized on first introduction and come with a definition in
context. Wherever possible, we should avoiding new terms that are
only used a few times.

In S 1-12 there are lots of forward references to stuff that
is only defined in S 12. I understand the urge to keep the
formal grammar towards the end, but you need to provide some
sort of abstract sketch of what's in an offer or answer in
order to make this understandable before the definition.

Somewhere there needs to be a diagram of the relationship of the
various pieces of the system. I.e., addresses, candidates, candidate
pairs, checks, check lists, valid lists, etc.

Many sections are just run-on descriptions of
protocol elements or operational steps. These each need to be broken
out with some sort of textual marker (bullets, whitespace, heds,
depending on the length of the relevant subsection). For instance,
S 4.1.1 reads (in part)

   An agent gathers candidates when it believes that communications is
   imminent.  An offerer can do this based on a user interface cue, or
   based on an explicit request to initiate a session.  Every candidate
   is a transport address.  It also has a type and a base.  Three types
   are defined and gathered by this specification - host candidates,
   server reflexive candidates, and relayed candidates.  The base of a
   candidate is the candidate that an agent must send from when using
   that candidate.

   The first step is to gather host candidates.  Host candidates are
   obtained by binding to ports (typically ephemeral) on an interface
   (physical or virtual, including VPN interfaces) on the host.  The
   process for gathering host candidates depends on the transport
   protocol.  Procedures are specified here for UDP.

   For each UDP media stream the agent wishes to use, the agent SHOULD
   obtain a candidate for each component of the media stream on each
   interface that the host has.  It obtains each candidate by binding to
   a UDP port on the specific interface.  A host candidate (and indeed
   every candidate) is always associated with a specific component for
   which it is a candidate.  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 is
   using both RTP and RTCP, it would end up with 2*K host candidates if
   an agent has K interfaces.

   The base for each host candidate is set to the candidate itself.

   Agents SHOULD obtain relayed candidates and MUST obtain server
   reflexive candidates.  The requirement to obtain relayed candidates
   is at SHOULD strength to allow for provider variation.  If they are
   not used, it is RECOMMENDED that it be implemented and just disabled
   through configuration, so that it can re-enabled through
   configuration if conditions change in the future.

   The agent next pairs each host candidate with the STUN server with

Each candidate type should be set off with its own hed.

Another example would be S 13:

   The foundation is composed of one or more ice-char.  The component-id
   is a positive integer, which identifies the specific component for
   which the transport address is a candidate.  It MUST start at 1 and
   MUST increment by 1 for each component of a particular candidate.
   The connect-address production is taken from RFC 4566 [10], allowing
   for IPv4 addresses, IPv6 addresses and FQDNs.  The port production is
   also taken from RFC 4566 [10].  The token production is taken from
   RFC 3261 [3].  The transport production indicates the transport
   protocol for the candidate.  This specification only defines UDP.
   However, extensibility is provided to allow for future transport
   protocols to be used with ICE, such as TCP or the Datagram Congestion
   Control Protocol (DCCP) [29].

This defines the following productions:

- foundation
- component-id
- connect-address
- port
- token
- transport

Each of these should be set off in its own bulleted list.

DETAILED COMMENTS
S 2.
There is a bad break around Figure 1.

   o  It's directly attached network interface (or interfaces in the
      case of a multihomed machine
s/It's/Its/

S 2.1.
s/net10/net 10/

   the STUN server is configured, or learned in some way.)  When the
                                ^
   the STUN server is manually configured or learned in some other
   unspecified way.)  When the

The formatting around the Note is screwd up.

   The final type of candidate is a RELAYED candidate.  The STUN Relay

s/RELAYED candidate/RELAYED CANDIDATE/

   Once L has gathered all of its candidates, it orders them highest to

s/them highest/them in highest/

S 2.4.
You need to introduce the concept of foundation here.

S 2.6.
"controlling agent" and "controlled agent" should be capitalized on
first use.

Figure 4 has the third arrow in the wrong relation to its label.

You need diagrams of both the mode where the flag is sent after
the initial ICE exchange and the aggressive one where it's sent
with some message subset.

I don't see what the term "favored" is bringing to the party here.
In any case, "selected" might be a better state. 

S 2.7.
I'm not sure that this section adds much to the overview.

S 3.
The definition of "Foundation" seems less than helpful. I would
rewrite:

   Foundation: An arbitrary string that is the same for two candidates
      that have the same type, base IP address, and STUN server. 
      If any of these are different then the foundation will be
      different. Two candidate pairs with the same foundation
      pairs are likely to have similar network characteristics.
      Foundations are used in the frozen algorithm.

The way "check" is used to mean "candidate pair" seems confusing.
A check is an action, not an object.

S 4.1.1.
Each type of candidate should have its own subsection.

   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 is
   using both RTP and RTCP, it would end up with 2*K host candidates if
   an agent has K interfaces.

Doesn't this 2*K rule apply to every kind of candidate?

Can a TURN server refuse to allocate a port? If so, does that
mean you get denied the server-reflexive address as well?

In the definition of foundation you should have a list of the
criteria, not a para.

S 4.1.2.
At minimum the rules for each component of the priority formula
should go in their own graf. Arguably under their own heds.

   These rules guarantee that there is a unique priority for each
   candidate.  This priority will be used by ICE to determine the order
   of the connectivity checks and the relative preference for
   candidates.  Consequently, what follows are some guidelines for
   selection of these values.

This section should go earlier.

The region from "One criteria for selection of the type..."
to the end of the section is informative. I wonder if it
belongs here at all. It certainyl should be its own section.

   important, the type preference for relayed candidates can be set
   lower than the type preference for reflexive and host candidates.
   Indeed, it is RECOMMENDED that in this case,

This can be compressed to be "SHOULD be set lower. The value
of 126 is recommended..."

S 4.1.3.
   A candidate is said to be "in-use" if it appears in the m/c-line of
   an offer or answer.  When communicating with an ICE peer, being in-

That should be a "a current offer or answer", right? If it appears
in the m/c line of a previous o/a, then it's not necessarily in-use
I would htink.

   each active media stream, to be in-use.  A media stream is active if
   it does not contain the a=inactive SDP attribute.

A reference for a=inactive seems appropriate.

   Unfortunately ... needed

This seems superfluous.

Isn't the last graf redundant?

S 4.3.
This whole section seems problematic. It's full of stuff that
only makes sense if you have seen the SDP grammar yet. I 
would replace it with an abstract discussion of the contents
of a candidate in an offer or answer. This will provide enough
context for the reader while avoiding the confusing forward
references to the grammer.

I would also include an example with a brief explanation of
how each value was being represented.

Why are types optional? When should I include them? Why wouldn't
I?

S 5.1.
I find the first full graf (starting with "If...") very
confusing. I would make the following changes to this
section: 

- Make this list a numbered list.
- Add the ice=lite criteria from the last para to the list.

Have the rest of the section be separate directions about
what to do if each constraint is violated... I would make
these separate grafs.

S 5.2.
   Based on this definition, once roles are determined for a session,
   they persist unless ICE is restarted, as discussed below.  A restart
   causes a new selection of roles.

"discussed below" should point to somehting.

S 5.7.
   There is one check list...

This graf should be moved to S 4 somewhere... It's a concept issue.

The remainder of this section is in dire need of heds.

You should have a state machine diagram for the check states.

I'm not sure it helps to introduce states for a Check List. They're
not used that often. I would just replace them with the relevant
conditions inline in the text.

S 5.8.
   When the timer fires, the agent MUST find the highest priority check
   in that check list that is in the Waiting state.  The agent then
   sends a STUN check from the local candidate of that check to the
   remote candidate of that check.  The procedures for forming the STUN
   request for this purpose are described in Section 7.1.1.  If none of
   the checks in that check list are in the Waiting state, but there are
   checks in the Frozen state, the highest priority check in the Frozen
   state is moved into the Waiting state, and that check is performed.
   When a check is performed, its state is set to In-Progress.  If there
   are no checks in either the Waiting or Frozen state, then the timer
   for that check list is stopped.

Breaking this into bullets would help readability.

   Performing the connectivity check requires the agent to know the
   username fragment for the local and remote candidates, and the
   password for the remote candidate.  For periodic checks, the remote
   username fragment and password are learned directly from the SDP
   received from the peer, and the local username fragment is known by
   the agent.

And for triggered checks they are...?

S 6.
Most of these sections say "do what S 5 says". Why do we need
this section at all?

S 7.1.1.
   A Binding Request ... a password of APASS.

This at least should be in a subsections since it's a detail
and doesn't flow well with the rest of the section. I'd put it
at the end of 7.1.1. too.

   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 learned from this check.

peer reflexive candidates are never defined adequately outside
the glossary. This is where they're really introduced, so you need
a narrative explanation.

S 7.1.2.
I think it would pay off to explain how you correlate the STUN
responses here.

Each of the cases described here needs to be broken out in its
own subsection.

   The agent MUST check that the source IP address and port of the
   response equals the destination IP address and port that the Binding
   Request was sent to, and that the destination IP address and port of
   the response match the source IP address and port that the Binding
   Request was sent from.  If these do not match, the processing
   described in the remainder of this section MUST NOT be performed.  In
   addition, an agent sets the state of the check to Failed.

   If the check succeeds, processing continues.  The agent creates a
   candidate pair whose local candidate equals the mapped address of the
   response, and whose remote candidate equals the destination address
   to which the request was sent.  This is called a validated pair,
   since it has been validated by a STUN connectivity check.  It is very
   important to note that this validated pair will often not be
   identical to the check itself; in many cases, the local candidate
   (learned through the mapped address in the response) will be
   different than the local candidate the request was sent from.

OK. This is a really important section but it just sort of flies
by. The key points here are:

1. The transport addresses in the UDP must match those you expect.
2. The XOR-MAPPED-ADDRESS attribute may not match.

I find this bit about creating a new candidate pair very confusing, since in
at least *some* cases this candidate pair will match the one you're
doing the check on. Is the notion here that you're creating a new
pair and then putting it on the valid list, so you need a copy even
if it's the same? If so, that seems unnecessarily prescriptive about
implementation. An alternative would be to not maintain a separate valid list and
just mark these checks with their own state....

Am I missing something?

   Once the priority of the candidate pair has been computed, the pair
   is added to the valid list for that media stream.  If the agent was a
   controlling agent, and the check had included a USE-CANDIDATE
   attribute, the candidate pair is marked as "favored".  If the agent
   was a controlled agent, and the check was a triggered check, and the
   request which caused the triggered check included the USE-CANDIDATE
   attribute, the candidate pair is marked as "favored".

Isn't there a race condition here if the periodic check for the candidate
precedes the triggered check?

   PRIORITY attribute of the request.  Its foundation is selected as
   described in Section 4.1.1.  The peer reflexive candidate is then
   added to the list of local candidates known by the agent (though it
   is not paired with other remote candidates at this time).

When is it paired with other remote candidates?

   unfrozen.  If the check had a component ID of one, the agent MUST

please be consistent about whether you use "one" or "1" here.

Also, please break out the cases in this para....

S 7.2.1.
This section needs heds again...

Why would something be encapsulated in a Data Indication? If it's relayed,
right? Please explain.

What's a tentative check? I think it would help to explain upfront
why this is tentative. Maybe something to the effect of 
"In some cases, the receipt of a check requires that the implementation
send an immediate "triggered check" in the remote direction..."

The list of cases here is flat but the first four cases are one
branch and the last case is a separate branch. Some hierarchy
here would help.

   If the tentative check is to be performed, it is constructed and
   processed as described in Section 7.1.1.

But sent immediately, right?

S 8.
More heds needed again...

I don't think the terms "introspective selection" and "proactive
selection" add much value here. It's not like they're used a
lot later. If you add heds you can refer to the section numbers
and give them sort of informal names...

You should have a diagram of each of the cases here (or have
it in the intro...)

S 9.1.
   An agent MAY change the ice-pwd and/or ice-ufrag for a media stream
   in an offer.  Doing so is a signal to restart ICE processing for that
   media stream.  When an agent restarts ICE for a media stream, it MUST
   NOT include the a=remote-candidates attribute, since the state of the

Why would it occur to me to include this attribute since this is the
first the reader has heard of it?

I'm trying to wrap my head around the cases here. It seems to me 
that you would do some kind of ICE restart if:

1. There is a new media session.
2. One of the media sessions has changed, right?
3. You want to change your implementation level so you do a full
   restart.
	 
Maybe you should enumerate these?

So the bit about not setting c=0.0.0.0 is sort of a parenthetical,
right? Maybe make it a note?

S 9.1.1.
Need heds again.

   If a candidate was sent in a previous offer/answer exchange, it
   SHOULD have the same priority.  For a peer reflexive candidate, the
   priority SHOULD be the same as determined by the processing in
   Section 7.1.2.  The foundation SHOULD be the same.  The username
   fragments and passwords for a media stream SHOULD remain the same as
   the previous offer or answer.

Is the basic concept here that you should replicate previous offers?
Maybe say that first....

Again, what does the remote-candidates attribute *do*...? Can
you explain the concept somewhere before it's introduced?

S 9.2.
   If the offer contained a change in the a=ice-ufrag or a=ice-pwd
   attributes compared to the previous SDP from the peer, it is a signal
   that ICE is restarting for this media stream.

"a signal"? Is it the only signal? Is this iff?

Can you reduce the last graf to "do whatever the offerer would do..."?

S 9.3.
   If ICE is restarting for a media stream, the agent MUST start a new
   Valid list for that media stream.  However, it retains the old Valid
   list for the purposes of sending media until ICE processing
   completes, at which point the old Valid list is discarded and the new
   one is utilized to determine media and keepalive targets.

I'm trying to figure out what the "Valid list" is doing here.
Isn't what I need to remember the address pairs I'm sending media
on, not the address pairs I might have sent on but decide not
to? The valid list seems like too-much information...

S 9.3.1.

The various subcases need to be broken out with bullets or heds
here...

S 10.
   All endpoints MUST send keepalives for each media session.

What is the scope of this mandate? Does this update all RTP stacks
whether or not they implement ICE? Is HTTP media?

   used [11].  The Binding Indication SHOULD NOT contain integrity
   checks; since the messages are simply discarded on receipt regardless
   of contents.

s/; since/as/

   media.  An agent receipt a Binding Indication MUST discard it
s/receipt/receiving/

S 11.1.1.

   If the state of a media stream is Running, there is no old Valid list

s/,/ and/ ?

S 12.
I wonder if this could all be made an appendix...? It seems kind of orthogonal
to ICE per-se.

S 13.
Each production here needs at least its own graf and should be set off
in the text.

E.g.,

    foundation -
      The foundation is composed of...

Each attribute should probably hve its own section.

S 14.
   Firstly, ICE provides the a=ice-options SDP attribute.  Each
s/Firstly/First/

S 15.
This example is fairly monolithic and hard to parse. It would be
better if the ladder diagram were broken up into pieces
(and maybe scattered throughout the text). Then you can
present a complete ladder diagram as a worked example.

   Agents L and R both pair up the candidates.  They both initially have
   two. 

two pairs, right?

   However, agent L will prune the pair containing its server
   reflexive candidate, resulting in just one.  At agent L, this pair
   (the check) has a local candidate of $L_PRIV_1 and remote candidate
   of $R_PUB_1, and has a candidate pair priority of 4.57566E+18 (note
   that an implementation would represent this as a 64 bit integer so as
   not to lose precision).  At agent R, there are two checks.  The
   highest priority has a local candidate of $R_PUB_1 and remote
   candidate of $L_PRIV_1 and has a priority of 4.57566E+18, and the
   second has a local candidate of $R_PUB_1 and remote candidate of
   $NAT_PUB_1 and priority 3.63891E+18.

This seems like listing each side's perspective would help. Also, I would
render the priorities in hex or decimal, not scientific notation,
so it's easy to see what's going on....

S 16.2.
   force a false valid on a false candidate.  This attack is very hard
   to launch if the false address identifies a third party, and is
   prevented by SRTP if it identifies the attacker themself.

I don't understand this claim.

S B.4.
The point that the username/password is a disambiguator should be 
made in the main text.

S B.6.
This section seems redundant to me.

S B.7.
Some of this should be promoted to the main text along with
an example of how it works.

-Ekr

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