Subject: Re: [Ice] AD Evaluation of draft-ietf-ice-rfc5245bis-15 - PULL REQUEST

From: Christer Holmberg <[email-masked]>
Date: Tue, 9 Jan 2018 14:43:59 +0000

Hi,

Based on Ben’s review, I have created a PR:

https://github.com/ice-wg/rfc5245bis/pull/55

In order to maintain the PR readable, it does *not* yet include any
check-must-and-should-lover-case fixes, or valid-pair-alignment fixes.

Regards,

Christer

On 09/01/18 14:58, "Christer Holmberg" <[email-masked]>
wrote:

>Hi Ben,
>
>Thank You for the review! Please see inline.
>
>>Substantive Comments:
>>
>>- 2.1, 2nd to last: Does ICE really prioritize a path with fewer NATs
>>over a path with more NATs, or is it more a matter of selecting a path
>>with no NATs over a path with ³some² NATs? (If the former, how does it
>>figure out how many NATs are between an agent and it¹s servers and peers?
>>Is this just a matter of timing?)
>
>ICE agents can¹t determine how many NATs there are.
>
>I suggest to say:
>
>   "In general, the priority algorithm is designed so that candidates of
>    similar type get similar priorities and so that more direct routes
>    (that is, routes without data relays or NATs) are preferred over
>indirect routes
>    (routes with data relays or NATs)."
>
>
>----
>
>>- 7.2.5.2.3 and 7.2.5.2.4: Why are these SHOULDs not MUSTs? Do you
>>envision circumstances where it might make sense _not_ to set the state
>>to Failed?
>
>I can¹t think of any such cases.
>
>I suggest to s/SHOULD/MUST.
>
>----
>
>>-17, first paragraph: ³ If these
>>  potential attacks can not 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.²
>>
>>This seems ambiguous. Is it the implementation¹s decision to institute
>>such controls, or is each ICE usage spec expected to make a decision that
>>applies to all implementations of that usage?
>
>I think we could allow both ICE usage-specified and
>implementation-specific mechanisms. Maybe re-write as following:
>
>      "If these potential attacks can not be mitigates, ICE usages can
>define mechanisms for controlling which addresses are revealed
>       to the negotiation and/or probing process. Individual
>implementations may also have implementation-specific rules for
>controlling 
>       which addresses are revealed."
>
>>Should ³secure signaling techniques² distinguish between HBH and E2E
>>techniques, in light of the later discussion about insider attacks?
>
>Later down in the reply I suggest to remove the bullet list.
>
>---
>
>>-17,1, false peer reflexive candidate: Should this mention SRTP (or
>>DTLS-SRTP)  as a mitigating countermeasure?
>
>The list itself does not describe the countermeasures. SRTP is mentioned
>in the last paragraph of the section.
>
>====
>
>>Editorial Comments and Nits:
>>
>>Is there a reason that the Security Considerations and IANA
>>considerations are not in the conventional locations (i.e. the last two
>>sections before the references)? (I see that is also true in 5245, so
>>this is mostly curiosity.)
>
>No reason.
>
>I suggest to move the sections.
>
>---
>
>>-1, last paragraph: Would it make sense to change ³exchanged via
>>mechanisms² (which seems a tautology) to ³exchanged via various
>>mechanisms²,  ³exchanged by application-specific mechanisms², or
>>something similar?
>
>
>The document introduces the ³ICE usage² concept, so I suggest to say
>³exchanged with ICE usage-specific mechanisms².
>
>---
>
>>‹ Didn't 5245 already deprecate RFC 4091 and 4092? That is, _this_
>>document does not do that.
>
>I suggest to remove the sentence, and the RFC references.
>
>---
>
>>-2.1, first paragraph: "Naturally, one viable candidate has a transport
>>address obtained directly from a
>>  local interface²
>>
>>Shouldn¹t that say ³one or more viable candidates haveŠ.²? or ³At least
>>oneŠ²?
>
>I suggest to say ³At least one².
>
>---
>
>>-2.1, 4th paragraph from end: It would be helpful to describe what ³valid
>>pair² means earlier in the document.
>
>I assume you mean section 2.2?
>
>I suggest to following modification:
>
>    "The agent works through the check list by sending a STUN request for
>     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
>     to a candidate pair list called the valid list.
> 
>
>     As an optimization, as soon as R gets L's check message, R schedules
>     a connectivity check message to be sent to L on the same candidate
>     pair. This is called a triggered check, and accelerates the process
>     of finding valid pairs."
>
>
>I would not like to add more details to section 2 (I think it¹s too
>detailed already), but I suggest to add
>the following to section 4:
>
>    "Valid Pair: A candidate pair whose local candidate equals the mapped
>address of the connectivity check response,
>     and whose remote candidate equals the destination address to which
>the connectivity check request was sent."
>
>
>Related to this, I note that the document uses both ³valid pair² and
>³valid candidate pair² terminology. Perhaps it¹s enough to say ³valid
>pair² everywhere.
>
>Related to this, there is a bug in section 2.3. The text shall say:
>
>    "For each component of a data stream, the controlling agent nominates
>a valid pair from
>     the valid list to be used for data."
>
>
>---
>
>>-2.3, figure 4: The figure still says ³flag², while the text has been
>>changed in several place to speak of the nominated ³attribute². Please
>>use one or the other consistently.
>
>I will s/flag/attribute in the figure. Elsewhere the usage of ³flag² is
>correct.
>
>---
>
>>-4: There are at least a few uses of lower case ³must² and ³should² that
>>do not appear to have been intended as keywords. If that is the intent,
>>please update this to use the boilerplate from 8174.  (The 2119
>>boilerplate is ambiguous on whether lower case instances count as
>>keywords.)
>
>
>I suggest we keep the 2119 boilerplate, but I will go through the lower
>case instances and see whether they should be capitalized.
>
>---
>
>>s/³Š defined in the STUNŠ² / ³Š defined in STUNŠ ³ or ³Š defined in the
>>STUN protocol Š²
>
>In order to be aligned with the rest of the document, I suggest to simply
>say ³defined in [RFC5389]².
>
>---
>
>>-5, last paragraph: The figure does not actually mention ³default
>>candidate selection².
>
>I suggest to remove that part, and say:
>
>"As shown, the agents involved in the candidate exchange perform (1)
>   candidate gathering, (2) candidate prioritization, (3) redundant
>   candidate elimination and (4) sending of the candidates to the peer."
>
>
>As shown, I also removed the last sentence regarding lite implementations.
>I don¹t think it is needed, since the picture is a high-level overview.
>
>---
>
>>-5.1.1, last paragraph, last sentence: Is the MAY applicable just to the
>>responding agent, or both agents?
>
>I suggest to remove the last sentence. The text already says that the
>gathering can start before the user is alerted.
>
>---
>
>>-5.1.1.2, first paragraph : ³Those requirements are at SHOULD strengthŠ²
>>I suggest putting ³SHOULD² in quotes when talking about a requirement
>>stated elsewhere. (Even if that ³elsewhere² is in the same sentence :-)
>>)
>
>I suggest to remove the sentence, and begin the following sentence with:
>
>"However, use of STUN and TURN servers may be unnecessary inŠ"
>
>---
>
>
>>-5.1.1.3, first paragraph: "Two candidates MUST have the same foundation
>>when all of the following are true²
>>That seems like a statement of fact. (Same for the last paragraph talking
>>about different foundations.)
>
>I suggest to remove ³MUST².
>
>---
>
>>- 5.1.2.1: Missing article before ³IP Address for which the candidateŠ²
>>(This section also seems to have a lot of 2119 keywords that are really
>>statements of fact or definition. But I recognize those come from the
>>original 5245 text.)
>
>I suggest to say ³the IP Address².
>
>I suggest to keep the 2119 keywords.
>
>---
>
>>-6.1.2, 2nd sentence: ³ To form a check list, an ICE agent (initiating
>>and responding) forms candidate pairs"
>>Consider : ³To form check lists, initiating and responding ICE agents
>>form candidate pairsŠ²
>
>I will fix as suggested.
>
>---
>
>>-7.2.5.2.1: ³MUST be equal the destination IP address²
>>
>>Consider ³MUST equal² or ³MUST be equal to²
>
>I will change to ³MUST be equal to².
>
>---
>
>>-12.3: ³ As discussed below, ICE agents are encouraged to re-adjust
>>jitter buffersŠ²
>>Please cite the specific section(s).
>
>I actually suggest to remove section 12.3. It is about receiving data, and
>the content is covered in section 13.
>
>I also suggest to change section 12.3 to 12.2.1, as the text is only about
>sending media.
>
>I also suggest to change section 13 to 12.3. It was always intended to be
>a subsection to section 12.
>
>---
>
>>- 17, last 3 paragraphs: With the expansion of the security
>>considerations, this list does not include all the countermeasures
>>mentioned in the later sections.
>
>I suggest to remove the list, and say:
>
>    "There are several types of attacks possible in an ICE system.  This
>     section considers these attacks and their countermeasures."
>
>
>---
>
>>-17.2, last bullet: Why call out viruses specifically? It seems like
>>there are a lot of ways a server might be compromised that do not involve
>>viruses per se. (Same in 17.3).
>
> My only answer is that this is text from RFC 5245.
>
>I can remove ³virus², and simply say:
>
>"An attacker can compromise a STUN server, and cause it to send responses
>with incorrect mapped addresses.² (17.2)
>
>
>Šand:
>
>"However, TURN servers are susceptible to DNS attacks for purposes of
>turning it into a zombie or rogue server."
>
>
>---
>
>>-17.4.1: Since the description of the voice hammer attack has been
>>removed, can you cite something that does describe it? (Appendix A still
>>refers to section 17 to describe the voice hammer attack.)
>
>I didn¹t really find a good citation, but maybe something like:
>
>"The STUN amplification attack is similar to a voice hammer attack, where
>the attacker causes other agents to direct media traffic towards the
>attack target."
>
>
>I also realised that the following text needs to be removed from Appendix
>A:
>
>"in particular, the voice hammer attack described in Section 17 is
>   prevented only for full implementations, not lite."
>
>
>---
>
>>-17.4.1, 2nd paragraph: s/they¹ll/³they will²
>
>I will fix as suggested.
>
>---
>
>>-22: Did you consider making this section an appendix?
>
>I don¹t remember. I have no problem doing that, if you think an appendix
>would be better.
>
>Regards,
>
>Christer
>

---

From: Ben Campbell <[email-masked]>
Date: Wed, 31 Jan 2018 18:22:06 -0600

Hi

I just noticed something in my AD review of ice-trickle that I think also bears on 5245bis. Both documents make normative statements about the use of the ice-options attribute. (Section 13 in 5245bis and section 2 in ice-trickle). Given the loosening of the coupling to SDP since RFC5245, does this still make sense?

Thanks!

Ben.

---

From: Ben Campbell <[email-masked]>
Date: Wed, 31 Jan 2018 18:28:17 -0600

(Oops, resending. I mistyped the alias for draft-ietf-ice-trickle.all as “drat-ietf…”. I have comment about hiddin meanings there :-)  )

Hi

Thanks!

Ben.

---

From: Ben Campbell <[email-masked]>
Date: Wed, 31 Jan 2018 18:53:22 -0600

> On Jan 31, 2018, at 6:28 PM, Ben Campbell <[email-masked]> wrote:
> 

By “section 2 in ice-trickle”, I meant section 3.

---

From: Peter Saint-Andre <[email-masked]>
Date: Wed, 31 Jan 2018 19:25:25 -0700

Hi Ben, thanks for the careful review. Comments inline.

> I just noticed something in my AD review of ice-trickle that I think also bears on 5245bis. Both documents make normative statements about the use of the ice-options attribute. (Section 13 in 5245b…

Section 9 of 5245bis says:

   This section defines a new ICE option, 'ice2'.  The ICE option
   indicates that the ICE agent that includes it (in an ice-options
   attribute) is compliant to this specification.  For example, the ICE
   agent will not use the aggressive nomination procedure defined in
   [RFC5245].

   An ICE agent compliant to this specification MUST inform the peer
   about the compliance using the 'ice2' ICE option.

   NOTE: The encoding of the 'ice2' ICE option, and the message(s) used
   to carry it to the peer, are protocol specific.  The encoding for the
   Session Description Protocol (SDP) [RFC4566] is defined in
   [I-D.ietf-mmusic-ice-sip-sdp].

The intent is clear, but the text could be cleaned up a bit to enforce
the separation between the ICE protocol and the SDP encoding. I suggest
the following for the first paragraph:

   This section defines a new ICE option, 'ice2'.  The ICE option
   indicates that the ICE agent that includes it is compliant to this
   specification.  For example, the ICE agent will not use the
   aggressive nomination procedure defined in [RFC5245].

We need to do something similar in the trickle spec:

OLD
   Even if a signaling protocol does not include a capabilities
   discovery method, a user agent can provide an indication within the
   ICE description that it supports Trickle ICE using a token of
   "trickle" in the ice-options attribute.  This token MUST be provided
   either at the session level or, if at the media stream level, for
   every media stream (an agent MUST NOT specify Trickle ICE support for
   some media streams but not others).

NEW
   Even if a signaling protocol does not include a capabilities
   discovery method, a user agent can provide an indication within the
   ICE description that it supports Trickle ICE by communicating an ICE
   option of 'trickle'.  This token MUST be provided either at the
   session level or, if at the media stream level, for every media
   stream (an agent MUST NOT specify Trickle ICE support for some media
   streams but not others).

   NOTE: The encoding of the 'trickle' ICE option, and the message(s)
   used to carry it to the peer, are protocol specific.  The encoding
   for the Session Description Protocol (SDP) [RFC4566] is defined in
   [I-D.ietf-mmusic-trickle-ice-sip].

Would that address your concern?

Peter

---

From: Ben Campbell <[email-masked]>
Date: Wed, 31 Jan 2018 20:46:51 -0600

> On Jan 31, 2018, at 8:25 PM, Peter Saint-Andre <[email-masked]> wrote:
> 

For the record, this was not my complete review of ice-trickle :-)

I sent the issue separately since it also impacts 5245bis, which is past IETF LC.

>   aggressive nomination procedure defined in [RFC5245].

I think you are looking at an old version. That section (now section 10 in version 16) does not mention ice-options.  The text that triggered my concern is in section 13:

"  First, ICE provides the ice-options attribute.  Each extension or
   change to ICE is associated with a token.  When an agent supporting
   such an extension or change triggers candidate exchange, it MUST
   include the token for that extension in this attribute.  This allows
   each side to know what the other side is doing.  This attribute MUST
   NOT be present if the agent doesn't support any ICE extensions or
   changes.”

It’s pretty clear that’s just some old (probably pre-bis) text. But it does need to be fixed prior to publication.

> Would that address your concern?

I think that would address my concern for ice-trickle.

Thanks!

Ben.

---

From: Peter Saint-Andre <[email-masked]>
Date: Wed, 31 Jan 2018 19:52:20 -0700

> For the record, this was not my complete review of ice-trickle :-)

Duly noted! :-)

> I think you are looking at an old version. 

Datatracker done me wrong.

> That section (now section 10 in version 16) does not mention ice-options.  The text that triggered my concern is in section 13:
> 

Yep, I'll leave that to Christer.

> I think that would address my concern for ice-trickle.

Excellent. Awaiting further feedback on that one...

Peter

---

From: Ben Campbell <[email-masked]>
Date: Wed, 31 Jan 2018 22:47:02 -0600

Thanks for your quick response, Peter; this discussion was helpful—partially by bringing up a separate but related question (more for 5245bis than for ice-trickle):

Section 13 of 5245bis talks about a series of tokens for “ice-options”. But since the ice-options  SDP attribute in no longer defined in 5245bis, did we lose the text that says that the ICE option needs to be able to carry multiple tags, whatever the signaling protocol is?

Am I missing text somewhere in 5245bis that says what to do if you get some tags you understand and some you do not? (e.g. a 5245bis implementation getting “ice2 trickle”.) I think I know what we expect but am not sure where (or if) it is written down, especially for non-SDP applications.

(I’m sure I’m missing something obvious.)

Thanks!

Ben.

> On Jan 31, 2018, at 8:52 PM, Peter Saint-Andre <[email-masked]> wrote:
>