Re: [Ice] Eric Rescorla's No Objection on draft-ietf-ice-rfc5245bis-17: (with COMMENT) (excluding Security Considerations)

[Christer Holmberg <[e-mail]>]

Hi Ekr,

Thank You for the review and comments! Please see inline.

Note that I will address your comments on the Security Considerations in a separate reply.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Review in context at:
https://mozphab-ietf.devsvcdev.mozaws.net/D3312

> IMPORTANT:
>
> *  If the agent's tie-breaker value is larger than or equal to the contents of the ICE-CONTROLLING attribute, the agent generates
> IMPORTANT: This algorithm seems like it's not going to work properly.
> Consider the case where A and B happen to have the same tie-breaker and both think they are controlling, and the 
> Binding Requests cross. Each now sends a 487 and then they switch to controlled. Ugh. Unless I'm missing something, 
> if the tie-breakers match, you are stuck. Given that the chance is 2^{-64} this seems to not be a critical failing, but the algorithm 
> still seems wrong.

The algorithm hasn't changed since RFC 5245, and nobody indicated that it doesn't work. I guess people assume that due to the random characteristics, the chance of both agents using the same value is extremely small.

Also note that it is only in special cases where both endpoints will indicate the same role to begin with.

---

REST:

>   single solution that is flexible enough to work well in all
>   situations.
> This section seems pretty dated. Aren't ICE and ICE-style solutions pretty much the de facto standard now.

I suggest to remove the sentence.

---

>   may not be aware of it.  The type of NAT and its properties are also
>  unknown.  L and R are capable of engaging in an candidate exchange
>   process, whose purpose is to set up a data session between L and R.
> Nit: a candidate exchange. This appears to be a result of changing offer/answer (where "an" was appropriate) to "candidate"

Correct. I will fix as suggested.

---

>   At least one viable candidate has a transport address obtained
>   directly from a local interface.  Such a candidate is called a host
> Nit: this is awkward. Perhaps "The first category of candidates are those with a transport ..."

I will fix as suggested.

---

>   When the agent sends the TURN Allocate request from IP address and
>   port X:x, the NAT (assuming there is one) will create a binding
>Nit: "a TURN Allocate"

I will fix as suggested.

---

>   the next candidate pair on the list periodically.  These are called
>   ordinary checks.  When a STUN transaction succeeds, one or more
>   candidate pairs will become so called valid pairs, and will be added
> Nit: I would quote "ordinary check" here and "triggered check" below.

I will fix as suggested.

---

>   provide means to exchange candidate information between the ICE
>   agents.  The specific details of (i.e how to encode candidate
>   information and the actual candidate exchange process) for different
> Nit: i.e -> i.e.,

I will fix as suggested.

---

>   Nomination, Regular Nomination:  The process of the controlling agent
>      indicating to the controlled agent which candidate pair the ICE Given that you have 
>      removed Aggressive Nomination, do you still need to refer to "Regular Nomination"

People asked to keep it in the Terminology section, to make it clear that "nomination" is 5245bis refers to what was called "regular nomination" in RFC 5245.

However, I suggest to s/regular nomination/nomination in section 13.

---

>   candidate gathering, (2) candidate prioritization, (3) redundant
>  candidate elimination, and (4) sending of the candidates to the peer.
> This is an odd diagram. There's no reason why these have to happen in sequence and in fact in Trickle ICE, they don't, so 
> this diagram seems misleading., as well as potentially contradicting the beginning of S 5.1.1.
>
> "An ICE agent gathers candidates when it believes that communication is imminent. "

I think the sequence applies to core ICE, and is also how it's done in 5245. 

The fact that trickle ICE does it differently I think shall be described in the trickle spec.

However, it if helps, I could add the following note:

"NOTE: This specification assumes that all candidates are gathered before they are sent to the peer. The trickle ICE extensions define procedures
where gathered candidates can be sent to the peer as soon as they have been gathered, while additional candidates are still gathered."

---

>   When candidates are obtained, unless the agent knows for sure that
>   RTP/RTCP multiplexing will be used (i.e. the agent knows that the
>   other agent also supports, and is willing to use, RTP/RTCP
> Nit: "i.e.,"

I will fix as suggested.

---

>      addresses that do allow location tracking, that are configured on
>      the same interface, and are part of the same network prefix MUST
>      NOT be gathered.
> You need to remove both of these commas, because they indicate a nonrestrictive clause, but this is a restrictive clause.

I will fix as suggested.

---

>   The gathering process is controlled using a timer, Ta.  Every time Ta
>   expires, the agent can generate another new STUN or TURN transaction.
> No comma here.

I will fix as suggested.

---

>   The agent will receive a Binding or Allocate response.  A successful
>   Allocate response will provide the agent with a server reflexive Or nothing or an error.
>
>              (2^8)*(IP precedence) +
>              (2^0)*(256 - component ID) 
>
> Isn't this the same formula as in S 5.1.2.1.

Section 5.1.2.1 defines the generic formula. This instance is when it is used by a Lite implementation.

Of course, in order to align, I could replace "IP precedence" with "local preference".

---

>      Foundation:  A sequence of up to 32 characters.
>
> The Foundation is never transmitted, AFAIK. So why does it have to be up to 32 characters? It certainly wasn't exchanged in 5245.

It is included in the candidate attribute defined in 5245 (Section 15.1):

    candidate-attribute   = "candidate" ":" foundation SP component-id SP
                               transport SP
                               priority SP
                               connection-address SP     ;from RFC 4566

    foundation            = 1*32ice-char

---

>   data stream, and for updating the peer with the ICE's selection, when
>   needed.  The controlled agent is told which candidate pairs to use
>   for each data stream, and does not require updating the peer to
>
> Told by who?

The text seems to be a leftover.

In fact, my suggestion would be to only keep the following:

   "For each session, each ICE agent (Initiating and Responding) takes on
   a role.  There are two roles -- controlling and controlled.  The
   controlling agent is responsible for the choice of the final
   candidate pairs used for communications. The sections below
   describe in detail the  actual procedures followed by controlling and 
   controlled agents."

What a full and lite implementation do is described in the text below.

---

>   pair priorities, orders pairs by priority, prunes pairs, removes
>   lower-priority pairs, and sets check list states.  If candidates are
>   added to a check list (e.g, due to detection of peer reflexive 
>
> Please fix your subject verb agreement here.

I will remove the s:es.

---

>      pair priority = 2^32*MIN(G,D) + 2*MAX(G,D) + (G>D?1:0) 
>
> This was kinda terrible in 5245. Given that you use it once, maybe just have + GT(G, D)
>
> And then say GT(G, D) == 1 if G>D and 0 otherwise.

I probably just don't see it, but what is the difference?

---

>                       Figure 8: Initial Pair State This figure caption is kind of a mess. I suggest just removing it.

I will remove it.

---

>   state in the check list set has been processed, the first check list
>   is picked again.  Etc.
> Nit: "again, etc."

I will fix as suggested.

---

>   pair to the remote candidate of the pair, as described in
>   Section 7.2.4.
> IMPORTANT: You don't just send a STUN request, you start a STUN transaction,

There are multiple instances in the spec where it talks about sending STUN requests or Binding requests.

Section 7.2 provides the STUN client details, including how to process the response etc.

---

>   lists.  On the other hand the responding agent either performs the
>   triggered or ordinary checks as described above.
>
> I don't understand this paragraph. What distinction are you trying to draw.

I don't understand the text either. I suggest to remove the whole paragraph.

---

>   o  The base is local candidate of the candidate pair from which the
>      Binding request was sent.
> Nit: "is the local"

I will fix as suggested.

---

>   The ICE agent constructs a candidate pair whose local candidate
>   equals the mapped address of the response, and whose remote candidate
>
> IMPORTANT: When does this happen?

The last sentence of the parent section (7.2.5.3)  says:

"If a check is considered a success, the ICE agent performs (in order) the actions described in the following sections."

---

>   a different check list than the one to which the pair that generated
>   the connectivity checks), or it may be a pair not currently in any
>   check list.
> IMPORTANT: How would a valid pair be on some other check list?

That seems like an error, but I will think a little more about it.

---

>      this specification.  There may be a conflict, but it cannot be
>      detected.
> What previous version? This was required in 5245. Maybe at this point we can just deprecate this?

I suggest to remove the whole bullet.

---

> 7.3.1.3.  Learning Peer Reflexive Candidates 
>
>This entire section seems to duplicate 7.2.5.3.1

Section 7.2.5.3.1 describes how a STUN client detects a peer reflexive candidate then it receives a STUN response.

Section 7.3.1.3 describes how a STUN server detects a peer reflexive candidate when it receives a STUN request.

Sure, the text is similar, but separate procedures.

---

>         in-progress transaction.  Cancellation means that the agent
>         will not retransmit the request, will not treat the lack of
>         response to be a failure, but will wait the duration of the 
>
> Why are you cancelling In-Progress checks when you receive a peer-reflective check? 
> If you receive two in a row, then it seems like this delays a successful check. 

True.

So, I will remove the cancel part.

Do you still think it should create a new binding request, or is the ongoing In-Progress check enough to check the pair?

---

> More generally, this document should explain how you end up in this
> situation: you only get here when "the source transport address of the request 
> does not match any existing remote candidate", so how can it be on a check list 
> unless this is the second observation of a peer reflexive.

I am not sure I understand. The source transport sentence you refer to is related to detection of peer reflexive candidates.

---

>   Prior to nominating, the controlling agent let connectivity checks
>   continue until some stopping criterion is met.  After that, based on lets.
>   The criterion details for stopping the connectivity checks and for
>   selecting a pair for nomination, are outside the scope of this "criterion details" seems ungrammatical. 
>
>   5245 had "criteria". What's wrong with that?

I have no idea. At some point someone has most likely requested that "criteria" is replaced with "criterion details". I can change back to "criteria".

---

>   have ceased using a given local candidate (a candidate may be used by
>   multiple ICE sessions, e.g. in forking scenarios), the agent can free
>   that candidate.  The three-second delay handles cases when aggressive
> Nit: "e.g.,"

I will fix as suggested.

---

>   Session Description Protocol (SDP) [RFC4566] is defined in
>   [I-D.ietf-mmusic-ice-sip-sdp].
> Presumably you want to cite 5245 S 14, which states:
>
>Consequently, when a controlling agent is communicating with a peer that supports options it doesn't know about, the agent MUST run a regular nomination algorithm.  When regular nomination is used, ICE will converge perfectly even when both agents use different pair prioritization algorithms.

I am not sure it is related to SDP.

I suggest to modify the first paragraph:

OLD:

 "For example, the agent will not use the aggressive nomination procedure defined in RFC 5245."

NEW:

 "For example, the agent will not use the aggressive nomination procedure defined in RFC 5245. In addition,
   It will ensure that an RFC 5245-compliant peer does not use aggressive nomination either, as described in
   Section 14 of RFC 5245."

---

>   15 seconds.  Agents MAY use a bigger value, but MUST NOT use a value
>   smaller than 15 seconds.
> This is a very old number. Is it supported by an modern measurement?

I don't know. The number is used in 5245, and there were never any suggestions to change it.

---

>   will converge perfectly even when both agents use different pair
>   prioritization algorithms.  One of the keys to such convergence is
>   triggered checks, which ensure that the nominated pair is validated 
>
> Given that you have removes aggressive, you presumably want to revise this section

I suggest the following modification:

OLD:

   "One of the complications in achieving interoperability is that ICE
   relies on a distributed algorithm running on both agents to converge
   on an agreed set of candidate pairs.  If the two agents run different
   algorithms, it can be difficult to guarantee convergence on the same
   candidate pairs.  The regular nomination procedure described in
   Section 8 eliminates some of the tight coordination by delegating the
   selection algorithm completely to the controlling agent.
   Consequently, when a controlling agent is communicating with a peer
   that supports options it doesn't know about, the agent MUST run a
   regular nomination algorithm.  When regular nomination is used, ICE
   will converge perfectly even when both agents use different pair
   prioritization algorithms.  One of the keys to such convergence is
   triggered checks, which ensure that the nominated pair is validated
   by both agents.  Consequently, any future ICE enhancements MUST
   preserve triggered checks."


NEW:

   One of the complications in achieving interoperability is that ICE
   relies on a distributed algorithm running on both agents to converge
   on an agreed set of candidate pairs.  If the two agents run different
   algorithms, it can be difficult to guarantee convergence on the same
   candidate pairs.  The nomination procedure described in
   Section 8 eliminates some of the tight coordination by delegating the
   selection algorithm completely to the controlling agent, and ICE
   will converge perfectly even when both agents use different pair
   prioritization algorithms. One of the keys to such convergence is
   triggered checks, which ensure that the nominated pair is validated
   by both agents.  Consequently, any future ICE enhancements MUST
   preserve triggered checks."

---

>16.  STUN Extensions
>None of the stuff here is "New" any longer, as it was allocated in RFC 5245.

I'll change it to "STUN Attributes".

---

>   First and foremost, ICE makes use of TURN and STUN servers, which
>   would typically be located in the data centers.  The STUN servers
>   require relatively little bandwidth.  For each component of each data
> Nit: this used to read "the network operators data centers" and when you removed "network operators" this became ungrammatical

I will remove "the" and say "in data centers".

---

>   there will be four transactions per call (one for RTP and one for
>   RTCP, for both caller and callee).  Each transaction is a single
>   request and a single response, the former being 20 bytes long, and Is this currently true? 
>
> How many people still don't support RTP-MUX?

I don't know. But, unless you use mux exclusive the offerer still has to gather and provide RTCP candidates.

Maybe we could add the following new paragraph to the end of the section:

"NOTE: As the usage of RTP/RTCP multiplexing is becoming more common in VoIP deployments, it will reduce the need for separate STUN transaction for RTP and RTCP."

---

>   can and will vary over time.  In a network with 100% behave-compliant
>   NAT, it is exactly zero.  At time of writing, large-scale consumer
>   deployments were seeing between 5 and 10 percent of calls requiring 
>
> This text dates to 5245. Is that still true?

I have no idea. I suggest to remove the sentence.

---

Regards,

Christer