Subject: Re: [Ice] [rtcweb] WGLC Review of draft-ietf-ice-rfc5245bis-12 - Harald's PDF comments

From: Christer Holmberg <[email-masked]>
Date: Mon, 16 Oct 2017 13:32:56 +0000

(RTCWEB list removed, to avoid cross-posting)

Hi Harald,

Below are my replies to the comments you provided in the PDF document:

---

Abstract
========

>Is   it   really   correct   to   cite   this   protocol   as   being
>only   for   multimedia?   Suggestion:   replace
>³multimedia²   with   ³communication².   This   is   in   line   with
>bullet   1   of   section   21   -   ³generalized².

---

Section   1   -   Introduction
========================

>Similar   to   the   above:   In   order   to   reduce   the   scope   of
>  changes,   suggest   adding   to   the   end   of
>the   introduction:
>³This   specification   may   be   used   both   for   multimedia
>sessions   and   other   types   of
>communication.²

I suggest to replace ³multimedia² with ³communication² (there are only 5
instances of ³multimedia²), and to add the sentence above to the end of
the Introduction.

---

Section   3   -   Terminology
=======================

>Terminology:   ³Component²:   ³A   component   is   a   piece   of   a
>media   stream   requiring   a   single
>transport   address²   -   I   think   this   should   be   ³a   single
>pair   of   transport   addresses²,   or   perhaps
>even   use   the   defined   term   ³candidate   pair².
>

			

>Suggested   reformulation:   ³A   component   is   a   piece   of   a
>media   stream   that   has   to   be
>assigned   a   single   candidate   pair,   which   can   be   used   by
> no   other   component,   before   being
>used   for   transmitting   part   of   a   media   stream².

Looks ok. Of course, if we make the ³media²/³data² change that you
suggested, it would say ³a piece of a data stream².

>³User²,   as   used   in   section   4.1.1   ³prior   to   alerting   the
>  user²,   is   not   defined.

I suggest that we say ³prior to alerting the user of the application
associated with the ICE session².

	

	
---

	

						Section   4   -   ICE   candidate   gathering   and   exchange
================================================

>4.1   -   part   of   this   section   title   is   missing.   It   may
>have   been   intended   to   be   ³Full
>Implementation²,   since   4.2   is   ³Lite².

It shall be ³Full². Will be fixed.

>4.1.1   -   nit:   ³Every   candidate   is   a   transport   address²
>->   ³has   a   transport   address².

Will be fixed as suggested.

>4.1.1   -   should   one   mention   that   candidates   don¹t   live
>forever?   (NAT   timeout,   interfaces   going
>up   or   down,   network   mobility)

Does it belong here, or could we add something to section 10?

>4.1.1.1   -   should   ³Host   candidates   corresponding   to   IPv6
>link-local   addresses   MUST   NOT   be
>gathered²   be   its   own   bullet,   possibly   placed   last   with
>a   condition   that   these   are   only   gathered
>if   there   is   no   global   IPv6   address   on   the   same
>interface?   Or   is   this   strictly   in   order   to   prevent
>location   tracking?

I will place it in its own bullet.

I am not an expert on IP addressing, but I don¹t see how a local-link
address could be used for location tracking more than other types of
addresses. My understanding is that we don¹t want to include them because
they are not globally unique.

>4.1.1.1   -   temporary   and   permanent   IPv6   addresses   are   not
> mentioned.
>Draft-ietf-rtcweb-transports   section   3.3   recommends   discarding
>permanent   addresses,   as
>per   RFC   6724.   Should   this   advice   be   added   to   this
>section?

I guess we could copy the following sentence to the end of the section,
saying:

 "The IPv6 default address selection specification [RFC6724] specifies
  that temporary addresses [RFC4941] are to be preferred over permanent
  addresses."

>4.1.1.2   -   paragraph   3   ends   ³Allocate   requests   SHOULD   be
>authenticated   using   a²   -
>something¹s   missing.   RFC   5245   had   ³ long-term   credential
>obtained   by   the   client   through   some
>other   means.²

I can¹t remember that we would have decided to remove that, so I¹ll put
the 5245 text back.

>Paragraph   4   -   first   occurence   of   ³Ta².   Please   expand.
>(Suggest   adding   a   paragraph:   The
>gathering   process   is   controlled   using   a   timer   called
>³Ta²).

I will fixed as suggested.

>4.1.1.4   -   host   candidates   die   too.   Suggest   adding   a
>paragraph:   ³Host   candidates   don¹t   time
>out,   but   the   addresses   may   change   or   disappear   for   a
>number   of   reasons.   The   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 will add the suggested text.

>4.1.2   grammar   nit   -   ³since   both   agents   will   not   be
>coordinated   in   their   checks²   ->   ³since   the
>agents   will   not   be   coordinated   in   their   checks².

I will fix as suggested.

>4.1.2.1   obsolete   reference   to   RFC   3484   for   ³precedence
>value   for   IP   addresses².   Note   that
>the   default   precedence   table   in   6724   section   2.1   is
>quite   different   from   the   one   in   3484,   so

			

>switching   the   reference   will   give   different   candidate
>orderings.   (Also,   6724   is   referenced   by
>section   4.2,   so   this   is   likely   an   oversight.)

I don¹t find the references to RFC 3483.

>4.1.2.2   should   the   paragraph   mention   that   VPN   candidates
>and   local   LAN   candidates   can
>have   different   local   preferences   too?   I   think   their   order
>  is   application   (or   even   user)
>dependent,   but   the   user¹s   preference   is   likely   to   be
>quite   strong   (when   using   VPN   for
>corp-LAN   connectivity:   don¹t   use   VPN   when   LAN   connection
>is   available   -   when   using   VPN
>for   location   hiding:   don¹t   use   host   at   all   when   VPN
>is   available   -   but   details   are   out   of   scope
>for   this   specification)

I have no input on this. Feel free to suggest text, and if no-one else has
an issue I will add it.

>4.3   nit:   Since   the   section   says   that   encoding   is   out
>of   scope,   the   title   should   be   changed   to
>³Exchanging   the   Candidate   Information².

I will fix as suggested.

>4.3   ³Related   Address²   is   not   defined   previously.   A
>pointer   to   section   B.3   would   be
>appropriate.   (It   seems   equally   undefined   there   what   is
>supposed   to   be   sent.   This   seems
>doubtful   for   interoperability.   Should   it   be   punted   to   a
>higher   layer,   as   in   ³Use   or   non-use   of
>the   Related   Address   is   defined   by   the   ICE   usage²?)

I don¹t think we should reference an appendix for a definition.

>4.4   Grammar   nit   -   ³may   alter   the   ICE   candidate
>information   that   breaks   ICE²   ->   ³may   alter
>the   ICE   candidate   information   in   ways   that   break   ICE².

It was suggested that we should remove the whole section. However, it has
later been suggested off-line that we should keep the section, but rename
the title.

	
But, if we keep the section, I will fix the text as suggested.

---

	

					
						Section   5.   ICE   Candidate   Processing

>5.1.1   determining   role   -   the   ³Both   lite²   bullet   is   the
> only   one   that   mentions   that   both   agents
>will   believe   they   are   controlled   or   controlling.   I   think
> this   is   true   for   both   case   1   and   3.
>
>I   believe   this   paragraph   belongs   at   a   higher   level,   and
>  should   be   reformulated   as:
>
>³The   signaling   protocol   enabling   the   candidate   exchange
>MUST   allow   the   agents   to
>determine   which   of   the   agents   is   the   Initiating   Agent,
>and   whether   or   not   the   peer   agent   is
>Full   or   Lite².   This   can   also   replace   the   second   NOTE,
>and   avoids   reference   to   ³glare²   (that¹s
>the   signalling   protocol¹s   problem).
>(I   don¹t   see   why   one   shouldn¹t   allow   the   6.3.1.1
>procedure   to   apply   in   this   case   too,   in   which
>case   the   signalling   protocol   requirement   can   be   a   SHOULD,
>  with   fallback   to   6.3.1.1,   which
>removes   the   need   to   call   out   3pcc   as   a   separate   case.
>  But   that   may   be   a   too   large   change   at
>this   stage.)

I want to separate the high-level procedures and the signalling protocol
requirements.

My suggestion would be to move the text out of the bullet list. We can
then reference the section that talks about role conflicts for more
information.

>5.1.2.1   grammar   nit:   ³is   neither   Completed   yet   nor   Failed
>  yet²   -   remove   the   first   ³yet².

I will fix as suggested.

>5.1.2.2   spell:   para   1   -   educed   ->   reduced

I will fix as suggested.

>5.1.2.6   state   procedure,   bullet   5:   ³in   the   first   list
>(ordered   by   the   check   list   set)²   doesn¹t   make
>sense.   ³In   the   first   list   according   to   the   usage-defined
> check   list   set   order²   would   make
>sense.   The   order   is   ICE   usage   controlled   per   bullet   2
>of   the   same   list.

I will fix as suggested.

>5.1.4.1   This   paragraph   would   be   clearer   if   it   added   the
>  words   ³The   queue   is   initially   empty,
>and   will   have   candidates   entered   as   part   of   the
>procedures   in   section   6.²

I will fix as suggested.

		

					
>5.2   Grammar   nit   ³On   determining   ....   means   ...²   is
>ungrammatical.   Suggested   rephrasing:   ³If
>the   lite   implementation   is   the   controlling   agent   (which
>will   only   happen   if   the   peer   agent   is
>also   a   lite   implementation),   it   selects   .....   and   then
>updates   Š.²

I will fix as suggested.

---

	

					
						Section   6.   Performing   connectivity   checks
=========================================

>Nit:   Intro   paragraph:   doubled   ³to   to².

Will remove.

>6.2.3.   Diffserv   treatment   -   doesn¹t   address   the   case   of
>media   wishing   to   utilize   multiple
>DSCP   markings   in   its   media   stream
>(draft-ietf-tsvwg-rtcweb-qos,   table   1).   Suggest   adding   ³If
>multiple   DSCP   markings   are   used   on   the   media   packets,
>the   agent   SHOULD   choose   one   of
>them   for   use   with   the   connectivity   check.²

I will add the suggested sentence.

> 6.2.5.2.   Failure   -   nit:   ³as   a   result   a   connectivity
>check²   -   missing   ³of²

Will fix as suggested.

>Sections   6.3.1.3   ³Learning   Peer   Reflexive   Candidates²   and
>section   6.3.1.4   ³Triggered
>checks²   don¹t   form   separate   procedures,   the   way   the   other
>  subsections   of   6.3.1   do.   I   suggest
>they   be   merged.

I THINK I see your point, but I am not sure I fully get it. What would you
call the merged section?

>6.2.5.2.2   describes   processing   ICMP   errors   for   connectivity
>checks.   ICMP   errors   are
>unauthenticated   and   easily   spoofable;   using   them   for
>shutting   down   communication   is   a   well
>known   vulnerability.

So, what do you suggest? That we remote the section, or that we keep the
section and say that ICMP errors shall not be processed for connectivity
check purposed?

>Timeout   would   seem   to   be   the   normal   outcome   of   a
>connectivity   check;   it   seems   strange   to
>hide   this   most   common   case   under   ³Unrecoverable   STUN
>response².   Suggest   raising   it   to   its
>own   section.

So, we should have the sections: ³Unrecoverable STUN response² and ³STUN
request timeout².

>6.2.5.3   Success   doesn¹t   say   ³The   following   sections
>(6.2.5.3.1   to   6.2.5.3.4)   are   performed   in
>order².   I   think   it   should,   since   other   sections   have
>subsections   (6.2.5   is   an   example)   where
>only   one   is   chosen   based   on   some   criteria.

I will add text.

>Section   6.2.5.3.2   introduces   the   ³valid   list²,   which   is
>one   list   for   the   whole   check   list   set.

The intention is to say that there is one valid list for each media stream.

>Section   6.3.2   introduces   the   ³valid   list²   as   if   it   was
> a   new   concept.   There   should   be   only   one,
>and   its   definition   probably   needs   to   be   lifted   higher   -
>  it¹s   also   referenced   from   section   7.

	
I agree with you. I see what I can do.

---

	

					
						Section   7   -   Concluding   ICE   processing
=====================================

>Section   7.1.1   starts   using   CHECK   LIST   and   VALID   LIST   in
>  uppercase,   but   inconsistently.
>
>Inconsistency   is   bad;   consider   lowercasing   them   all.

Peter T did a PR that was supposed to lowercase all, but the fix was not
complete. Will fix it.

>Nit:   nomiation   ->   nomination   (para   4).   Need   a   comma
>before   ³and   for   selecting   a   pair².

Will fix as suggested.

		

					
						³Ice2²   ice   option   needs   a   forward   reference,   since
this   is   the   first   mention,   and   definition
comes   later.

Will add a reference.

---

	

					
						Section   10   -   Keepalives
=======================

>The   text   is   unclear   about   whether   ³An   agent   MAY   use
>another   value   for   Tr²   is   allowed   to
>override   the   ³MUST   NOT   use   a   Tr   value   smaller   than   15
>  seconds².   I   think   it   doesn¹t   (has   to
>be   more   than   15),   but   I¹m   not   100%   that¹s   the
>intended   meaning.

Would the following be more clear:

Agents SHOULD use a Tr value of 15 seconds. Agents MAY use a bigger value,
but MUST NOT use a value smaller than 15 seconds.

>This   spec   says   that   all   endpoints   MUST   send   keepalives,
>but   doesn¹t   say   which   ones.

The text does say:

"ICE endpoints allow for STUN-based keepalives for UDP streams, and as
such, STUN keepalives MUST be used when an
   agent is a full ICE implementation and is communicating with a peer
that supports ICE (lite or full).²

>Draft-ietf-rtcweb-stun-consent-freshness specifies a 30-second usage of
>STUN bind
>requests, for instance, but it¹s not clear whether it uses STUN
>keepalives or not. Should this
>document say explicitly that the ICE Usage specifies which keepalives are
>used?

Well, I don¹t think that would clarify whether consent-freshness uses STUN
keepalives or not, would it? :)

Perhaps we could say something like:

³ICE agents MUST by default use STUN keepalives. Individual ICE usages and
ICE extensions MAY specify usage/extension-specific keepalives.²

---

	

					
						Section   12   -   Receiving   media
============================

>³....   if   there   is   a   change   in   source   transport   address,
>  but   the   media   packets   come   from   the
>same   peer   agent,   this   SHOULD   NOT   be   treated   as   an
>SSRC   collision².   Why   not   MUST
>NOT?

I have no idea. This is Magnus/Colin/Jonathan/Roni territory :)

---

	

			

Section   14   setting   Ta   and   RTO
=============================

>14.2   Ta   -   ³the   agent   MUST   indicate   the   proposed   value
>to   its   peer   during   the   ICE   exchange².
>The   term   ³ICE   exchange²   is   never   defined;   I   think   it
>refers   to   the   exchange   of   candidates   via
>the   signalling   protocol.   Please   define   the   term.

I think ³ICE exchange² sounds strange, so I think we should use another
term. Perhaps we could say:

³during the establishment of the ICE session.²

---

	

			

						Section   16   -   Security   Considerations:
=====================================

>There   is   no   privacy   consideration   -   in   particular,   the
>privacy   impact   of   exposing   host-specific
>addresses   together   with   VPN   addresses   is   not   mentioned   or
>  addressed.
>
>Suggestion:   Add   a   section   saying   ³The   process   of   probing
> for   candidates   reveals   the   source
>addresses   of   the   client   and   its   peer   to   any   on-network
> listening   attacker,   and   the   process   of
>exchanging   candidates   reveals   the   addresses   to   any   attacker
>  that   is   able   to   see   the
>negotiation.   Some   addresses,   such   as   the   server   reflexive
>addresses   gathered   through   the
>local   interface   of   VPN   users,   may   be   sensitive
>information.   If   these   potential   attacks   can¹t   be
>mitigated,   the   implementation   may   want   to   institute
>controls   for   which   addresses   are
>revealed   to   the   negotiation   and/or   probing   process.   Such
>controls   need   to   be   specified   as
>part   of   the   ICE   usage.²

I will add the suggested text.

Again, Thank You very much for the review! :)

Regards,

Christer