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

[Eric Rescorla <[e-mail]>]

On Mon, Feb 19, 2018 at 6:18 AM, Christer Holmberg <
[e-mail]> wrote:

> 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 just explained why it doesn't work. Do you think this analysis is wrong?


I guess people assume that due to the random characteristics, the chance of
> both agents using the same value is extremely small.


Yes, I agree that the chance of experiencing the problem in the field is
very low. But that doesn't make the algorithm right, and it's not sensible
to (a) specify the edge case behavior (b) specify it so that it doesn't
work correctly. Better would be to return a *different* error that says you
have unresolvable role conflict.




> >   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.
>

OK, but then you should explain why you are using "regular" here.


>   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."
>

The issue here isn't that all candidates are gathered before they are sent
to the peer but rather  that the diagram implies that no gathering happens
before the peer's candidates are received. That's not a requirement of
5245, and, as I said, contradicts the beginning of S 5.1.1.



> >   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".
>

My suggestion is to have one copy.


> >      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


My mistake.




> >   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.
>

Fine.



>      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?
>

It has the same semantics but it's easier to read. I mean, either you think
the C syntax is self-explanatory or you
don't. If you do, then you don't need to explain it. If you don't, it's
opaque in the expression,



> >   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.


I agree, but this statement is still confusing. Is there a reason not to
change it?




> >   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."
>

Please add a reverse reference.


>   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.
>

Thanks.



>
> ---
>
> >      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.
>

This would benefit from consolidation.



>
> >         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?
>

My initial reaction is you don't need to do anything extra, but I could
easily be wrong here.


> 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.
>

My question is what sequence of events can lead to this situation, as it is
surprising to the readers.



> >   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."
>

Perhaps:
"as required by Section 14 of RFC 5245 for peers which receive unknown ICE
options"



>
> >   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,


Right, that's my point.


and there were never any suggestions to change it.
>

OK.


> 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
>

some of the need for tight



>    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."
>
>


> >   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.
>

Right, but then there will be three transactions per call in the common
case.



> >   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.
>

WFM.

-Ekr


>
> ---
>
> Regards,
>
> Christer
>