Add check to prevent TURN usage if the protocol is not allowed.

Add check to prevent TURN usage if the protocol is not allowed.

There are 2 ways to design this.

1. TCP Only mode: this means that we disable all UDP protocols across board.
2. disallow TURN over UDP. Along with DISABLE_UDP, DISABLE_STUN, we should achieve the same result.

I'm going with #2.

BUG=webrtc:4784
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/13d35f6ffc71c1a649a902369a6fa5637d05d9b0

[guoweis_webrtc, 2015-08-25 21:02:36]

Patchset #1 (id:1) has been deleted

[guoweis_webrtc, 2015-08-25 21:02:40]

Patchset #1 (id:10002) has been deleted

[guoweis_webrtc, 2015-08-25 21:03:28]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[guoweis_webrtc, 2015-08-25 21:03:29]

PTAL. I'm adding the test case.

[pthatcher1, 2015-08-25 22:59:30]

Needs a unit test.

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:50: // IP address.
Disallow to use => Disallow use of 
to relay server => to a relay server
IP address => an IP address that would be behind a proxy when using TCP

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:51: PORTALLOCATOR_DISABLE_TURN_OVER_UDP =
0x1000,
Would PORTALLOCATOR_DISABLE_UDP_TURN be equally clear, but shorter?

Also, we use "RELAY" in PORTALLOCATOR_DISABLE_RELAY, so perhaps
"PORTALLOCATOR_DISABLE_UDP_RELAY".

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:987: // Skip the turn server that uses
disallowed protocol.
Might be more clear as "Skip UDP connections to relay servers if it's
disallowed".

[pthatcher1, 2015-08-25 23:12:00]

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:31: PORTALLOCATOR_DISABLE_UDP = 0x01,
Can you add a comment here that this *doesn't* disable UDP to TURN, and to use
DIABLE_UDP_RELAY for that?

[guoweis_webrtc, 2015-08-26 06:10:14]

On 2015/08/25 23:12:00, pthatcher1 wrote:
>
https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
> File webrtc/p2p/base/portallocator.h (right):
> 
>
https://codereview.webrtc.org/1311153003/diff/30001/webrtc/p2p/base/portalloc...
> webrtc/p2p/base/portallocator.h:31: PORTALLOCATOR_DISABLE_UDP = 0x01,
> Can you add a comment here that this *doesn't* disable UDP to TURN, and to use
> DIABLE_UDP_RELAY for that?

PTAL.

[pthatcher1, 2015-08-26 17:27:53]

lgtm, assuming you fix the comments

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:31: // Disable local UDP port. This doesn't
impact how we connect relay servers.
UDP port => UDP ports  
connect relay servers => connect to relay servers.

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:35: // Disable local TCP port. This doesn't
impact how we connect relay servers.
Same here

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:609: // Test that we disable relay
over UDP, only TCP is used when connecting to the
UDP, only => UDP, and only

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:620:
cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET);
Did you run "git cl format" on this?

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:626: // candidates.
2 ports => 2 ports and 2 candidates
TURN/TCP and TCP Port => TURN/TCP and TCP Ports

[guoweis_webrtc, 2015-08-26 20:30:20]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-08-26 20:30:20]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1311153003/#ps90001 (title:
" ")

[commit-bot: I haz the power, 2015-08-26 20:30:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311153003/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311153003/90001

[guoweis_webrtc, 2015-08-26 20:31:37]

fixed all comments. landing it now.

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1311153003/diff/70001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:620:
cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET);
On 2015/08/26 17:27:53, pthatcher1 wrote:
> Did you run "git cl format" on this?

yeah, it was git cl format did that.

[commit-bot: I haz the power, 2015-08-26 22:30:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 22:30:42]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[guoweis_webrtc, 2015-08-26 22:33:06]

Committed patchset #4 (id:90001) manually as
13d35f6ffc71c1a649a902369a6fa5637d05d9b0 (presubmit successful).

[juberti1, 2015-08-26 22:48:44]

I'm not sure I understand this. Why doesn't the existing DISABLE_UDP flag work
here?

[pthatcher2, 2015-08-26 22:53:15]

On 2015/08/26 22:48:44, juberti1 wrote:
> I'm not sure I understand this. Why doesn't the existing DISABLE_UDP flag work
> here?

DISABLE_UDP is used to control the allocation phases, here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...


If we change the meaning to include UDP to TURN servers, and then used it for
this purpose, I believe that code would break.

I suppose we could refactor that code and then use DISABLE_UDP to mean "disable
all outgoing UDP", but then we'd have to change DISABLE_TCP to be consistent as
well.  I think it would end up being a bit of a rat hole.