Support relay-only ICE transport (RTCConfiguration.iceTransportPolicy = "relay")

RESOLVED FIXED in Firefox 42

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jib, Assigned: jib)

Tracking

(Blocks 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

()

Attachments

(2 attachments)

No description provided.
We don't support RTCConfiguration.iceTransportPolicy yet, and we need it for spec compliance.  See https://github.com/webrtc/adapter/pull/93#issue-97351439 for context.
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
Assignee: nobody → jib
Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx.
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

r+ for the webidl. Seems to follow the spec.
Attachment #8641072 - Flags: review?(bugs) → review+
Byron, for hooking up the remainder here, I figured you might know (and perhaps have opinions about) exactly where and how we should limit ice activity to relay only.

Some considerations: It seems like the spec expects this setting to be orthogonal to the list of ice servers, and that content should be able to alter this setting mid-connection (at least from "none" to something else, which might be useful I suppose, though I struggle to think of other use-cases).

Since we don't support pc.setConfiguration yet, we can largely ignore these complications for now, I just wanted to give a heads up, in case that bears on where we should put it now. Same goes for figuring out what "none" means, since without an ability to change it later, that setting is quite meaningless.

Unless you want to take it from here, could you point me in the right direction?
Flags: needinfo?(docfaraday)
https://reviewboard.mozilla.org/r/14423/#review13039

This really does seem to need some code to do the runtime re-set of the policy.

::: media/mtransport/nricectx.h:281
(Diff revision 1)
> +  // Set whether we're allowed to produce none, relay or all candidates.
> +  // XXX: Need to figure out what it means to change this mid-connection.
> +  nsresult SetPolicy(Policy policy);

Convention in this code is TODO(jib@mozilla.com)

::: media/mtransport/nricectx.h:283
(Diff revision 1)
> +  nsresult SetPolicy(Policy policy);
> +
> +  Policy GetPolicy();

Convention in this code is policy() and you should probably inline it.
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

https://reviewboard.mozilla.org/r/14423/#review13043

This all seems to be wired up correctly.
Attachment #8641072 - Flags: review?(docfaraday) → review+
https://reviewboard.mozilla.org/r/14423/#review13039

> Convention in this code is TODO(jib@mozilla.com)

There also needs to be a bug number here.
Blocks: 1189030
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> Byron, for hooking up the remainder here, I figured you might know (and
> perhaps have opinions about) exactly where and how we should limit ice
> activity to relay only.
> 
> Some considerations: It seems like the spec expects this setting to be
> orthogonal to the list of ice servers, and that content should be able to
> alter this setting mid-connection (at least from "none" to something else,
> which might be useful I suppose, though I struggle to think of other
> use-cases).
> 
> Since we don't support pc.setConfiguration yet, we can largely ignore these
> complications for now, I just wanted to give a heads up, in case that bears
> on where we should put it now. Same goes for figuring out what "none" means,
> since without an ability to change it later, that setting is quite
> meaningless.
> 
> Unless you want to take it from here, could you point me in the right
> direction?

   So, looking at JSEP 4.1.1 and 4.1.10, and the latest w3c editor's draft 4.2.4, this seems to be the state of things:

1) "None" implies that the ICE stack sends no packets whatsoever. This precludes the gathering of srflx and relay candidates. However, this conflicts with JSEP 4.1.10, since that says policy does not effect the candidate pool (pre-gathering). It also requires that no candidates be trickled (gathering/trickling host candidates does not require sending or receiving any traffic). So we have a couple of interpretations here; none means the ICE stack is totally dormant, or none means that the ICE stack can gather, but not trickle or send STUN checks. I suspect the former is more in line with the intent of the specs.

2) Relay implies that the ICE stack should not trickle anything but relay candidates. It also implies that STUN checks should only be sent from a relay candidate. It would be reasonable to take this to mean no gathering of srflx as well.

   So looking at these two, it seems that the proper approach is to have "none" suppress the start of gathering and the start of ICE checks entirely. "relay" should prevent the ICE stack from creating either host or srflx candidates, which will prevent the trickling of the same.

https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-ref%3A%22nr_ice_candidate_create%28nr_ice_ctx+*%2C+nr_ice_component+*%2C+nr_ice_socket+*%2C+nr_socket+*%2C+nr_ice_candidate_type%2C+nr_socket_tcp_type%2C+nr_ice_stun_server+*%2C+UCHAR%2C+nr_ice_candidate+**%29%22

This means that this would probably be a parameter on nr_ice_gather.

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#554
Flags: needinfo?(docfaraday)
https://reviewboard.mozilla.org/r/14423/#review13039

I've added a note to deal with this in Bug 1181768 comment 1.

> Convention in this code is policy() and you should probably inline it.

Are you sure? I copied this from GetControlling which was neither inlined nor controlling. Elsewhere in the file I see GetStreamCount, GetGlobalAttributes... Let me know, I'm fine either way.
There's also gathering_state() now that I look harder. I'll inline it.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> There's also gathering_state() now that I look harder. I'll inline it.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> https://reviewboard.mozilla.org/r/14423/#review13039
> 
> I've added a note to deal with this in Bug 1181768 comment 1.
> 
> > Convention in this code is policy() and you should probably inline it.
> 
> Are you sure? I copied this from GetControlling which was neither inlined
> nor controlling. Elsewhere in the file I see GetStreamCount,
> GetGlobalAttributes... Let me know, I'm fine either way.

The convention is more-or-less that things that are pure accessors to variable
foo_ are foo() whereas things that take work get real method names. However,
several of these are wrong and appear to have been missed by review
(e.g., GetControlling()).
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=bwc
Attachment #8641072 - Attachment description: MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. → MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=bwc
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review+
Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641497 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(docfaraday) → review+
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc
Attachment #8641072 - Attachment description: MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=bwc → MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
No tests since TURN server is not available on builders. Open to ideas.

Tested "none", "relay" and "all" manually here: http://jsfiddle.net/ha4zpa2g
Also note https://github.com/openpeer/ortc/issues/224 - unsure what use-case "nohost" is for.
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

https://reviewboard.mozilla.org/r/14561/#review13159

It would be good to have some test-cases for this. media/mtransport/test/ice_unittest.cpp is a good place to do this, even if we'd need to hand-run since a TURN server is required.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:137
(Diff revision 2)
> +    assert(!(ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY) || ctype == RELAYED);

ctype != RELAYED

::: media/mtransport/nricectx.cpp:726
(Diff revision 2)
> +  if (policy_ == ICE_POLICY_NONE) {
> +    return NS_OK;
> +  }

I wonder what happens if StartChecks is called before StartGathering... I'd be tempted to add a check to StartChecks. The specs are a little unclear on what is supposed to happen once offer/answer concludes when the ICE policy is none. Are we supposed to say ICE failed? Or are we supposed to wait for a SetConfig that re-enables ICE, and hope that we're in sync with the other end?

For now, let's just mark ICE failed if StartChecks is called with this policy. Once we get around to implementing SetConfig, we can revisit, and also do stuff like make sure we start gathering when the ICE policy changes.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c:263
(Diff revision 2)
> -          if(!cand){
> +          if(!cand && !(pair->pctx->ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY)) {

If we're going to suppress this (and I think it makes sense to, since I think the other end could send us a STUN response with a bogus reflexive address in it which would cause us to start sending through something other than a relay), we need to bail out because the code below assumes |cand| is created.
Attachment #8641497 - Flags: review?(docfaraday)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> No tests since TURN server is not available on builders. Open to ideas.
> 
> Tested "none", "relay" and "all" manually here: http://jsfiddle.net/ha4zpa2g

   If you want a mochitest, I suppose you could verify that using "relay" on one end causes ICE to fail.
https://reviewboard.mozilla.org/r/14561/#review13191

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:423
(Diff revision 2)
>        else if(suppress) {
>            continue;
>        }
> -
> +      if (!ice_tcp_disabled && !(ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY)) {
> -      if (!ice_tcp_disabled) {
>          /* passive host candidate */

Set ice_tcp_disabled to 1 if you are in RELAY_ONLY mode. That will save a test here andbelow.
https://reviewboard.mozilla.org/r/14561/#review13193

IMPORTANT: You need to suppress raddr/rport in the relay candidate. See my patch
for https://bugzilla.mozilla.org/show_bug.cgi?id=1189041
(In reply to Byron Campen [:bwc] from comment #18)
> > +    assert(!(ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY) || ctype == RELAYED);
> 
> ctype != RELAYED

If we're here (creating candidates) then RELAY_ONLY better be clear unless the type is RELAYED, right?

> I wonder what happens if StartChecks is called before StartGathering... I'd
> be tempted to add a check to StartChecks. The specs are a little unclear on
> what is supposed to happen once offer/answer concludes when the ICE policy
> is none. Are we supposed to say ICE failed? Or are we supposed to wait for a
> SetConfig that re-enables ICE, and hope that we're in sync with the other
> end?

A good question for the list. My guess would be the latter.

> For now, let's just mark ICE failed if StartChecks is called with this
> policy. Once we get around to implementing SetConfig, we can revisit, and
> also do stuff like make sure we start gathering when the ICE policy changes.

Sounds good.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> (In reply to Byron Campen [:bwc] from comment #18)
> > > +    assert(!(ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY) || ctype == RELAYED);
> > 
> > ctype != RELAYED
> 
> If we're here (creating candidates) then RELAY_ONLY better be clear unless
> the type is RELAYED, right?

Whoops.
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641497 - Flags: review?(docfaraday)
(In reply to Eric Rescorla (:ekr) from comment #21)
> IMPORTANT: You need to suppress raddr/rport in the relay candidate. See my
> patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1189041

I see https://tools.ietf.org/html/rfc5245#section-15.1 says they must be present, so I've set them to the same as the candidate's own ip and port, like in your patch. Hope that's what you meant.
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

https://reviewboard.mozilla.org/r/14561/#review13397

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:10
(Diff revision 3)
> +  bug: "796890",

Update bug number

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:14
(Diff revision 3)
> +function PC_LOCAL_SETUP_NO_ICE_HANDLER(test) {
> +  test.pcLocal.setupIceCandidateHandler(test, function() {}, function () {});
> +}
> +
> +function PC_REMOTE_SETUP_NO_ICE_HANDLER(test) {
> +  test.pcRemote.setupIceCandidateHandler(test, function() {}, function () {});
> +}

Doesn't this disable trickle? We definitely don't want that. I think what we want is to override the handler for pcLocal to fail the test if a candidate is gathered, and leave the handler on pcRemote alone.

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:15
(Diff revision 3)
> +  test.pcLocal.setupIceCandidateHandler(test, function() {}, function () {});

This only takes two params.

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:22
(Diff revision 3)
> +var iceEnded, candidates = [];

We won't need |candidates| if we override pcLocal's ICE candidate handler to fail the test.

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:25
(Diff revision 3)
> +  iceEnded = new Promise((resolve, reject) =>
> +      test.pcLocal._pc.addEventListener("iceconnectionstatechange", e => {

Maybe it would be useful to modify waitForIceConnected to reject if the ICE state goes to "failed", and use that? 

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1224

Also I think it would probably be a good idea to ensure that ICE fails on pcRemote.
Attachment #8641497 - Flags: review?(docfaraday)
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
Attachment #8641497 - Flags: review?(docfaraday)
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

https://reviewboard.mozilla.org/r/14561/#review13697

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:28
(Diff revisions 3 - 4)
> -    switch (test.pcLocal._pc.iceConnectionState) {
> +  .then(() => ok(true, "No candidates."));

The stuff above does not imply that there were no candidates, just that ICE failed. This should probably read "ICE on both sides failed"

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:50
(Diff revisions 3 - 4)
> -  test.chain.remove("PC_LOCAL_SETUP_ICE_LOGGER");
> -  test.chain.remove("PC_REMOTE_SETUP_ICE_LOGGER");
> +  test.chain.remove("PC_LOCAL_SETUP_ICE_LOGGER");  // Needed to suppress failing
> +  test.chain.remove("PC_REMOTE_SETUP_ICE_LOGGER"); // on ICE-failure.

What failure were these causing? I would not expect them to choke if ICE failed. Maybe another bug needs to be filed?
Attachment #8641497 - Flags: review?(docfaraday) → review+
https://reviewboard.mozilla.org/r/14561/#review13697

> What failure were these causing? I would not expect them to choke if ICE failed. Maybe another bug needs to be filed?

> 180 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_relayOnly.html | PeerConnectionWrapper (pcLocal): legal ICE state transition from new to failed 
>     PeerConnectionWrapper.prototype.logIceConnectionState/this.ice_connection_callbacks.logIceStatus@dom/media/tests/mochitest/pc.js:1174:11
>     PeerConnectionWrapper/this._pc.oniceconnectionstatechange/<@dom/media/tests/mochitest/pc.js:699:7
>     PeerConnectionWrapper/this._pc.oniceconnectionstatechange@dom/media/tests/mochitest/pc.js:698:5

The comment here is revealing: http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js?rev=79d3b8d91250&mark=10-10#9

Wanna to file a spec issue?
"Wanna to" is how I hear the cool kids say "Time to" and "want to" at the same time.
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641497 - Flags: review+ → review?(docfaraday)
Attachment #8641497 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
the second patch failed to apply:

applying 7bdc2a04af70
7bdc2a04af70 transplanted to b25204d3a5d3
applying 1319ed97e26d
patching file media/mtransport/third_party/nICEr/src/ice/ice_component.c
Hunk #1 FAILED at 220
1 out of 2 hunks FAILED -- saving rejects to file media/mtransport/third_party/nICEr/src/ice/ice_component.c.rej

maybe need rebase ?
Flags: needinfo?(jib)
Keywords: checkin-needed
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
Comment on attachment 8641497 [details]
MozReview Request: Bug 1187775 - skip host and reflexive ICE candidates if relay-only

Bug 1187775 - skip host and reflexive ICE candidates if relay-only
Attachment #8641497 - Flags: review+ → review?(docfaraday)
Flags: needinfo?(jib)
Attachment #8641072 - Flags: review?(docfaraday)
Attachment #8641072 - Flags: review?(bugs)
Attachment #8641072 - Flags: review+
Attachment #8641497 - Flags: review?(docfaraday) → review+
Rebased.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1e6ed0e57113
https://hg.mozilla.org/mozilla-central/rev/4ea1373a7a77
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

This request is for both patches.

Approval Request Comment
[Feature/regressing bug #]: Block IP leakage from WebRTC (1189030)
[User impact if declined]:
See Bug 1189030 comment 22.

[Describe test coverage new/current, TreeHerder]:
Landed on m-c in 42.

2nd patch comes with new test to prove that blocking of at least the host ICE candidates works in automated tests. Existing tests prove that at least host candidates continue to work as before. (Server-reflexive candidates aren't tested in the tree AFAIK, but these have been verified to work through manual tests locally).

[Risks and why]:

Relatively low risk.

1st patch doesn't really modify existing code so much as add plumbing of one new setting from the PeerConnection chrome JS level down to the C code.

2nd patch does touch the ICE code by introducing if()-statements to filter out generation of ICE candidates of type host and server-reflexive, leaving only relay (aka through-TURN-server candidates) candidates.

The API surface added (one setting) has been in the WebRTC spec for a very long time.

[String/UUID change made/needed]: none
Flags: needinfo?(rjesup)
Attachment #8641072 - Flags: approval-mozilla-aurora?
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Sorry I meant beta.
Attachment #8641072 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

cancelling request per comment in bug 1189030.  We'll let this ride the trains in 42.
Flags: needinfo?(rjesup)
Attachment #8641072 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.