Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STUN checks via TURN servers really should not have peer-reflexive priority (and should have unique priority as well) #400

Closed
jnoring opened this issue Nov 23, 2015 · 4 comments

Comments

@jnoring
Copy link

jnoring commented Nov 23, 2015

Currently per RFC 5245, all early STUN checks are interpreted as peer-reflexive candidates; the priority of these candidates is supplied by the controlling agent via the PRIORITY attribute on the STUN packet. These packets are supposed to be supplied peer-reflexive priority per RFC5245, section 7.1.2.1:

   An agent MUST include the PRIORITY attribute in its Binding request.
   The attribute MUST be set equal to the priority that would be
   assigned, based on the algorithm in Section 4.1.2, to a peer
   reflexive candidate, should one be learned as a consequence of this
   check (see Section 7.1.3.2.1 for how peer reflexive candidates are
   learned).  This priority value will be computed identically to how
   the priority for the local candidate of the pair was computed, except
   that the type preference is set to the value for peer reflexive
   candidate types.

   The controlling agent MAY include the USE-CANDIDATE attribute in the
   Binding request.  The controlled agent MUST NOT include it in its
   Binding request.  This attribute signals that the controlling agent
   wishes to cease checks for this component, and use the candidate pair
   resulting from the check for this component.  Section 8.1.1 provides
   guidance on determining when to include it.

...however, these stun checks could just as easily originate from a TURN server, which means the priority should be dramatically less than peer reflexive.

My question is: why does RFC 5245 have this behavior, and should this be the behavior for WebRTC? it makes sense to me for STUN checks that are direct to the controlled agent, but the controlling agent knows full well that STUN checks through a TURN server should not have peer-reflexive priority, but relay priority. Ultimately when candidates are sent via the signaling channel, in theory candidates (and candidate pairs) should have their priority updated, but in the mean time it seems like the list ordering is potentially wrong.

@juberti
Copy link
Contributor

juberti commented Nov 24, 2015

a) I think this is the wrong tracker; this should be handled in the ICE WG tracker (@pthatcherg, has that been set up yet?)

b) I think we really want to move away from static PRIORITY values and allow the controlling side to make dynamic decisions based on RTT, etc.

c) I didn't really follow the argument as to why STUN checks through a TURN server shouldn't have prflx priority. Can you spell out a more specific example?

@pthatcherg
Copy link
Contributor

I agree, this is something for the ICE WG. Discussion/questions should go
on the mailing list (ice@ietf.org) rather than starting at the Github issue
tracker. But there is a general issue tracker here:
https://github.com/ice-wg/ice. It's brand new, though, so there isn't
anything in it yet :).

On Mon, Nov 23, 2015 at 5:19 PM, Justin Uberti notifications@github.com
wrote:

a) I think this is the wrong tracker; this should be handled in the ICE WG
tracker (@pthatcherg https://github.com/pthatcherg, has that been set
up yet?)

b) I think we really want to move away from static PRIORITY values and
allow the controlling side to make dynamic decisions based on RTT, etc.

c) I didn't really follow the argument as to why STUN checks through a
TURN server shouldn't have prflx priority. Can you spell out a more
specific example?


Reply to this email directly or view it on GitHub
#400 (comment).

@dontcallmedom
Copy link
Member

@alvestrand @stefhak @elagerway Based on the comments above, I think this issue can be closed here.

@alvestrand
Copy link
Contributor

Closing per comments. Feel free to add a link to the ICE tracker (for archival purposes) once it's filed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants