Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Description • 9 years ago

      No description provided.

	Maire Reavy [:mreavy]
	Comment 1 • 9 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 2 • 9 years ago

Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 9 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 4 • 9 years ago

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?

	Eric Rescorla (:ekr)
	Comment 5 • 9 years ago

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.

	Byron Campen [:bwc]
	Comment 6 • 9 years ago

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.

	Byron Campen [:bwc]
	Comment 7 • 9 years ago

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.

	Byron Campen [:bwc]
	Comment 8 • 9 years ago

(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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 9 • 9 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 10 • 9 years ago

There's also gathering_state() now that I look harder. I'll inline it.

	Eric Rescorla (:ekr)
	Comment 11 • 9 years ago

(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()).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 12 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 13 • 9 years ago

Bug 1187775 - skip host and reflexive ICE candidates if relay-only

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 14 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 15 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 16 • 9 years ago

No tests since TURN server is not available on builders. Open to ideas.

Tested "none", "relay" and "all" manually here: http://jsfiddle.net/ha4zpa2g

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 17 • 9 years ago

Also note https://github.com/openpeer/ortc/issues/224 - unsure what use-case "nohost" is for.

	Byron Campen [:bwc]
	Comment 18 • 9 years ago

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.

	Byron Campen [:bwc]
	Comment 19 • 9 years ago

(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.

	Eric Rescorla (:ekr)
	Comment 20 • 9 years ago

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.

	Eric Rescorla (:ekr)
	Comment 21 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 22 • 9 years ago

(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.

	Byron Campen [:bwc]
	Comment 23 • 9 years ago

(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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 24 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 25 • 9 years ago

(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.

	Byron Campen [:bwc]
	Comment 26 • 9 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 27 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 28 • 9 years ago

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

	Byron Campen [:bwc]
	Comment 29 • 9 years ago

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?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 30 • 9 years ago

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?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 31 • 9 years ago

"Wanna to" is how I hear the cool kids say "Time to" and "want to" at the same time.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 32 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 33 • 9 years ago

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 ?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 34 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 35 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 36 • 9 years ago

Rebased.

	Pulsebot
	Comment 37 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6ed0e57113
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea1373a7a77

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1e6ed0e57113
https://hg.mozilla.org/mozilla-central/rev/4ea1373a7a77

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 39 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 40 • 9 years ago

Comment on attachment 8641072 [details]
MozReview Request: Bug 1187775 - Plumb RTCConfiguration.iceTransportPolicy down to NrIceCtx. r=smaug, r=bwc

Sorry I meant beta.

	Randell Jesup [:jesup] (needinfo me)
	Comment 41 • 9 years ago

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.