Subject: [Ice] TSV-ART Review of draft-ietf-ice-rfc5245bis-16 (Not complete yet)

From: Magnus Westerlund <[email-masked]>
Date: Mon, 29 Jan 2018 16:38:12 +0100

I've reviewed this document as part of the transport area directorate's 
ongoing effort to review key IETF documents. These comments were written 
primarily for the transport area directors, but are copied to the 
document's authors for their information and to allow them to address 
any issues raised. When done at the time of IETF Last Call, the authors 
should consider this review together with any other last-call comments 
they receive. Please always CC tsv-dir@… if you reply to or forward this 
review.

Unfortunately there was an error in the datatracker that resulted in 
that review requests was not timely sent. Thus, I got this request the 
day before the deadline. Therefore I am late with the review. Also, I 
have not yet completed it. I have only reviewed the document to end of 
section 7. So I send this out now for heads up, and intended to update 
it the next few days.

I know that this is an update to an published RFC. But, I have reviewed 
it on a general level without considering what is changed or not.

This draft is on the right track but has open issues, described in the 
review.

Significant Issues:

A. Section 5.2:

    Lite implementations only utilize host candidates.  A lite
    implementation MUST, for each component of each data stream, allocate
    zero or one IPv4 candidates.  It MAY allocate zero or more IPv6
    candidates, but no more than one per each IPv6 address utilized by
    the host.  Since there can be no more than one IPv4 candidate per
    component of each data stream, if an ICE agent has multiple IPv4
    addresses, it MUST choose one for allocating the candidate.  If a
    host is dual-stack, it is RECOMMENDED that it allocate one IPv4
    candidate and one global IPv6 address.  With the lite implementation,
    ICE cannot be used to dynamically choose amongst candidates.
    Therefore, including more than one candidate from a particular scope
    is NOT RECOMMENDED, since only a connectivity check can truly
    determine whether to use one address or the other.

I find it quite strange that the above text says there can only be 
single IPv4 based candidate, while for IPv6 a LITE implementation may 
have one candidate per IPv6 address. Isn't the LITE implication of 
having multiple candidates for the same address family similar? Yes, 
IPv6 kind of forces the need for dealing with multiple IPv6 addresses on 
any host. However, I can see that certain servers will actually be 
multi-homed in IPv4 and thus can in a sensible way actually have 
multiple IPv4 candidates, and let the clients select which interface has 
the best reachability.

Can you please be explicit on what in ICE prevents things to work for 
IPv4 but the same case works for IPv6?

B. Section 6.1.1:

    An agent MUST be prepared that the peer might re-determine the roles
    as part of any ICE restart, even if the criteria for doing so are not
    fulfilled.  This can happen if the peer is compliant with an older
    version of this specification.

What does it mean to be prepared for a peer that re-determine the roles? 
What is it one MUST do? If the peer changes its role upon an ICE 
restart, isn't that going to result in a role mismatch? Thus causing yet 
another ICE restart, where also this peer will re-evalute? Isn't that 
good enough? Or is it something else it can do?

C. Section 6.1.3:

    The ICE agent has a state determined by the state of the check lists.
    The state is Completed if all check lists are Completed, Failed if
    all check lists are Failed, and Running otherwise.

Does failed really require all the checklists to fail, or simply any to 
fail if the others are completed?

D. Section 6.1.4.2:

I don't know if I misunderstand the algorithm here in the bullet list. 
To me it appears that it will terminate prior to have initiated all 
possible tests, as it appears that it will not unfreeze some of the 
candidate pairs. If one have tests running for a foundation, but all 
other candidate checks have been started, then the steps are aborted. Is 
the bullet list rechecked every Ta?

E. Section 7.2.5.2.2.  ICMP Error

    An ICE agent MAY support processing of ICMP errors for connectivity
    checks.  If the agent supports processing of ICMP errors, and if a
    Binging request generates an ICMP error, the agent SHOULD set the
    state of the candidate pair to Failed.

I am a bit worried by this blanket statement on ICMP errors. I think it 
should be clarified which ICMP message types that are relevant to 
consider as errors? I assume Type 3 (Destination Unreachable) but maybe 
not all responde codes as Codes 4, 11,12 may be addressable in other 
ways, and likely Type 11 (Time exceeded) with response code 0, response 
code 1 is not a clear indication of a non working path.

F. Section 7.2.5.2.3.  Timeout

    If the Binding request times out, the ICE agent MUST set the
    candidate pair state to Failed.

Isn't this erroneous? Timeout for the connectivity check is happening 
when all the (re-)transmissions have timed out, isn't it? or as simple 
as missing the word "transaction"?

Minor/Editorial Issues:

1.  Section 5.1.2:

This section doesn't make it clear that higher priority values are more 
prioritized over lower values. That really should be defined here. Now 
that information only becomes evident implicitly in section 5.1.2.1.

2. Section 2.1.

    In order to execute ICE, an ICE agent has to identify all of its
    address candidates.

I think this sentence is raising a too high requirement. An ICE agent 
has attempt to identify as many of the address candidates as possible. 
The better coverage of the potential candidates the more likely it is to 
function. I would also argue that there are multiple cases where you 
will not figure out that there are candidates that you don't know about. 
An obvious example is in cases of two NATs between the local address 
realm and the STUN server, the agent can't figure out that there was an 
address given to the flow in the middle address realm between the two 
NATs. That can be only learn if the agent has a STUN server in that 
address realm. Secondly, there are cases where policy may be applied to 
exclude certain interfaces and their related candidates.

I also noted that this first paragraph and the second has a strange 
relation. The first part of first paragraph is general, then there is 
the part of the host candidate. Then the second part starts with STUN 
and TURN derived candidates. Maybe the first paragraph should be split 
between the general and the host part, or some other bridging is needed.

3. Section 2.3:

If the transactions above succeed, the agents will set
    the nominated flag for the pairs, and will cancel any future checks
    for that component of the data stream.

Although what is stated is normal, it is not guaranteed to happen, I 
know this is intended as a simplified overview, but ignoring that there 
can occur some shuffling back and forth if high priority checks complete 
after low priority ones should at least be hinted or at least allowed by 
the use of words.

4. Section 3.

As RFC 7825 do describe a significant enough different usage of ICE from 
SIP, I think it would be good to actually included an informational 
reference to this usage.

5. Section 5.1.1.4:

An ICE agent SHOULD
    monitor the interfaces it uses, invalidate candidates whose base has
    gone away, and acquire new candidates as appropriate when new
    interfaces appear.

I am missing discussion of new addresses here. If the base disappears, 
it might be that there is a new IP address that one should use. That 
doesn't necessary imply a new interface.

6. Section 7.2.5.1:

    If the Binding request generates a 487 (Role Conflict) error
    response, and if the ICE agent included an ICE-CONTROLLED attribute
    in the request, the agent MUST switch to the controlling role. If
    the agent included an ICE-CONTROLLING attribute in the request, the
    agent MUST switch to the controlled role.

I think the first sentence should have a forward reference to Section 
7.3.1.1 where the rest of the solution is described.

7. Section 7.3:

If the agent is using Diffserv Codepoint markings [RFC2475] in its
    data packets, it SHOULD apply the same markings to Binding responses.

I find this sentence a bit unclear. Is it intended to say:

If the agent receiving the binding request, intended to use DSCP 
markings !=0 for the data, it SHOULD set, the same marking to binding 
responses.

or

If the agent receives a binding request with DSCP markings, then it 
should apply to corresponding code point when forming the binding response?

There are unclarity of which agent is referenced and whom "it" is in the 
sentence.

Cheers

Magnus Westerlund

----------------------------------------------------------------------
Media Technologies, Ericsson Research
----------------------------------------------------------------------
Ericsson AB                 | Phone  +46 10 7148287
Torshamnsgatan 23           | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden | mailto: [email-masked]
----------------------------------------------------------------------

---

From: Christer Holmberg <[email-masked]>
Date: Tue, 30 Jan 2018 08:40:35 +0000

Hi Magnus,

Thank you for the review! Please see inline.

>they receive. Please always CC tsv-dir@Š if you reply to or forward this
>review.
>

==========

>IPv4 but the same case works for IPv6?

This is text from RFC 5245. I agree it is confusing, and unfortunately I
don¹t have a good answer.

I guess my approach would be to suggest that we simply remove the
restriction. There is generic text about dual-stack etc elsewhere, and I
don¹t see anything ICE lite specific.

OLD:

"Lite implementations only utilize host candidates.  A lite
   determine whether to use one address or the other."

NEW:

"Lite implementations only utilize host candidates.
With the lite implementation, ICE cannot be used to dynamically choose
amongst candidates. Therefore, including more than one candidate from a
particular IP address family is NOT RECOMMENDED, since only a connectivity
check can Truly determine whether to use one address or the other.²

---

>good enough? Or is it something else it can do?

The roles are re-negotiated during the ICE restart: it may, or may not,
result in a role mismatch.

³Prepared² means that the peer might change its role even though it does
not fulfil the 5245bis criteria for being allowed to do so.

---

>fail if the others are completed?

If there are one or more completed, the session can still continue. As
described in section 8.1.2:

   "If at least one of the check lists for other data streams is
    Completed, the controlling agent SHOULD remove the failed data
    stream from the session while sending updated candidate list to
    its peer."

---

>the bullet list rechecked every Ta?

No. Whenever Ta fires, one check list in Running state is checked. When
all check lists have been checked, it will start over from the top of the
list.

Or, did I misunderstand your issue?

---

>code 1 is not a clear indication of a non working path.

This is from RFC 5245.

I don¹t think the ICE WG should go through all different codes and
combinations, and determine what should be considered an error, and what
not.

If you can provide something (table, guidance etc), we are happy to
include it. Otherwise I¹d like to keep it as it is, and let
implementations deal with it.

---

>as missing the word "transaction"?

Correct. I will change to ³Binding request transaction²

========

>that information only becomes evident implicitly in section 5.1.2.1.

I suggest the following:

"This priority will be used by ICE to determine the order of the
   connectivity checks and the relative preference for candidates.
Higher priority values give more priority over lower values."

---

>exclude certain interfaces and their related candidates.

I suggest to replace with first sentence and simply say:

"In order to execute ICE, an ICE agent identifies and gathers one or more
address candidates."

>between the general and the host part, or some other bridging is needed.

I suggest to split the first paragraph into two paragraphs, where the
second paragraph begins with the "At least one viable candidateŠ²
sentence. 

---

>after low priority ones should at least be hinted or at least allowed by

One of the tasks have been to simplify section 2, because it was too
detailed for people who just wanted to get an overview of ICE. For
example, we removed the text about role conflicts.

So, I would prefer to not cover your case in section 2. I don¹t think it
is essential for people who want to get an overview. Implementers
obviously will need to read the whole spec.

---

>SIP, I think it would be good to actually included an informational

I can do that. However, note that RFC 7825 references RFC 5245. It even
references specific sections, which may not be the same in 5245bis.

In addition, RFC 7825 uses ³aggressive nomination² terminology, which has
been removed from 5245bis.

What about:

"RFC 7825 defines an ICE usage for the Real-Time Streaming Protocol
(RTSP). Note, however, that the ICE usage is based on RFC 5245."

---

>doesn't necessary imply a new interface.

What about:

"Host candidates do not time out, but the candidate addresses may
change or disappear for a number of reasons. An ICE agent SHOULD
monitor the interfaces it uses, invalidate candidates whose base has
gone away, and acquire new candidates as appropriate when new
<new>IP addresses (on new or currently used interfaces)</new> appear."

---

>7.3.1.1 where the rest of the solution is described.

I will add a reference.

---

>should apply to corresponding code point when forming the binding

It means that it will use the same markings in Binding responses that it
uses in data packets (audio, video, etc).

>There are unclarity of which agent is referenced and whom "it" is in the

It is the STUN server.

Would the following be more clear?

"If the agent is using Diffserv Codepoint markings [RFC2475] in data
packets that it sends, the agent SHOULD apply the same markings to Binding
responses."

Regards,

Christer

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 30 Jan 2018 14:28:51 +0100

Hi,

As I have now posted the complete review with some additional issues, I 
would request that you post these comments on that complete review and I 
can reply.

Cheers

Magnus

-- 

Magnus Westerlund