Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309543004:
Create a new preference which disables UDP protocol for WebRTC (Closed)
|
Created:
5 years, 4 months ago by guoweis_left_chromium Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a new preference which disables UDP protocol for WebRTC. The preference will be used in an extension. The extension API CL will follow.
WebRTC by default uses UDP to connect to peer and TURN servers. This could lead to ip leak when using proxy server to hide the IP. Introduce a preference which disallows any usage of UDP protocol.
This is similar to how we added "enable_multiple_routes" (CL: https://codereview.chromium.org/916873004/) and is based on ongoing CL https://codereview.webrtc.org/1311153003/
BUG=webrtc:4784
Committed: https://crrev.com/a84ace48ac5c7c257913ce4f6ed9ab88d7eb9dca
Cr-Commit-Position: refs/heads/master@{#346294}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 1
Patch Set 7 : rebase #Patch Set 8 : #Patch Set 9 : fix bad rebase. #Patch Set 10 : #Patch Set 11 : #Messages
Total messages: 52 (21 generated)
Patchset #1 (id:1) has been deleted
guoweis@chromium.org changed reviewers: + pthatcher@chromium.org
PTAL.
guoweis@chromium.org changed reviewers: + juberti@chromium.org
https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:49: pref_service->GetBoolean(prefs::kWebRTCUdpTransportEnabled); I thought we were going to call this something like kWebRTCForceThroughProxy (default false), so that later if we do have UDP proxies (like RETURN), we could still have UDP traffic, as long as it went through the proxy. https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... File content/public/common/renderer_preferences.cc (right): https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... content/public/common/renderer_preferences.cc:45: caption_font_height(0), More weird formatting https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... File content/public/common/renderer_preferences.h (right): https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... content/public/common/renderer_preferences.h:104: // or TURN servers. Default is true. Weird formatting https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:126: bool enable_udp_transport) Can we make some kind of struct to contain the various options (origin+enable_multiple_routes+enable_udp_transport) instead of making more and more parameters? https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:132: enable_udp_transport_(enable_udp_transport) {} Even if we make the Chrome flag "ForceThroughProxy", I think we can keep this one as "disable_udp_transport". https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:154: config.disable_udp_transport = !enable_udp_transport_; Can we keep it consistent across both (either enable on both or disable on both)? I'd prefer "disable".
guoweis@chromium.org changed reviewers: + wfh@chromium.org
guoweis@chromium.org changed reviewers: + sergeyu@chromium.org, thakis@chromium.org
sergeyu@chromium.org: Please review changes in content/renderer thakis@chromium.org: Please review changes in chrome/ wfh@chromium.org: Please review changes in content/common https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:49: pref_service->GetBoolean(prefs::kWebRTCUdpTransportEnabled); On 2015/08/25 23:12:34, pthatcher2 wrote: > I thought we were going to call this something like kWebRTCForceThroughProxy > (default false), so that later if we do have UDP proxies (like RETURN), we could > still have UDP traffic, as long as it went through the proxy. Whether traffic goes through proxy or not is a decision that chrome owns and enforces. ForceThroughProxy doesn't seem a right term to me when there is no proxy configured. When there is RETURN, I would assume it's a configuration of chrome and WebRTC should be the same and UDP packets just get routed differently. https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... File content/public/common/renderer_preferences.cc (right): https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... content/public/common/renderer_preferences.cc:45: caption_font_height(0), On 2015/08/25 23:12:34, pthatcher2 wrote: > More weird formatting Done. https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... File content/public/common/renderer_preferences.h (right): https://codereview.chromium.org/1309543004/diff/20001/content/public/common/r... content/public/common/renderer_preferences.h:104: // or TURN servers. Default is true. On 2015/08/25 23:12:34, pthatcher2 wrote: > Weird formatting Done. https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:126: bool enable_udp_transport) On 2015/08/25 23:12:34, pthatcher2 wrote: > Can we make some kind of struct to contain the various options > (origin+enable_multiple_routes+enable_udp_transport) instead of making more and > more parameters? origin is used for completely different things. I don't think it should be put together. I could put enable_multiple_routes and enable_udp_transport together if that looks better. https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:154: config.disable_udp_transport = !enable_udp_transport_; On 2015/08/25 23:12:34, pthatcher2 wrote: > Can we keep it consistent across both (either enable on both or disable on > both)? I'd prefer "disable". I also like disable more but if we looked at the old code review on "enable_multiple_routes", it was a CL feedback to change it from disable to enable. For consistency, I'll keep everything to be enable_udp_transport.
adding a bool to content::RendererPreferences in content/common/view_messages.h lgtm from security
https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:49: pref_service->GetBoolean(prefs::kWebRTCUdpTransportEnabled); kWebRTCNonproxiedUdpTransportEnabled would also work https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:126: bool enable_udp_transport) On 2015/08/26 14:43:40, guoweis_chromium wrote: > On 2015/08/25 23:12:34, pthatcher2 wrote: > > Can we make some kind of struct to contain the various options > > (origin+enable_multiple_routes+enable_udp_transport) instead of making more > and > > more parameters? > > origin is used for completely different things. I don't think it should be put > together. I could put enable_multiple_routes and enable_udp_transport together > if that looks better. Yeah, that would be nice. Why not just use P2PPortAllocator::Config.
Patchset #3 (id:60001) has been deleted
On 2015/08/26 18:25:39, pthatcher2 wrote: > https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... > File chrome/browser/renderer_preferences_util.cc (right): > > https://codereview.chromium.org/1309543004/diff/20001/chrome/browser/renderer... > chrome/browser/renderer_preferences_util.cc:49: > pref_service->GetBoolean(prefs::kWebRTCUdpTransportEnabled); > kWebRTCNonproxiedUdpTransportEnabled would also work > > https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://codereview.chromium.org/1309543004/diff/20001/content/renderer/media/... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:126: bool > enable_udp_transport) > On 2015/08/26 14:43:40, guoweis_chromium wrote: > > On 2015/08/25 23:12:34, pthatcher2 wrote: > > > Can we make some kind of struct to contain the various options > > > (origin+enable_multiple_routes+enable_udp_transport) instead of making more > > and > > > more parameters? > > > > origin is used for completely different things. I don't think it should be put > > together. I could put enable_multiple_routes and enable_udp_transport together > > if that looks better. > > Yeah, that would be nice. Why not just use P2PPortAllocator::Config. PTAL.
lgtm, assuming you fix the comments https://codereview.chromium.org/1309543004/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1309543004/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:1203: const char kWebRTCNonProxiedUdpTransportEnabled[] = Might be more clear as "Whether WebRTC should use non-proxied UDP. If false, WebRTC will not send UDP unless it goes through a proxy. If no UDP proxy is configured, it will not send UDP. If true, WebRTC will send UDP regardless of whether or not a proxy is configured." https://codereview.chromium.org/1309543004/diff/80001/content/public/common/r... File content/public/common/renderer_preferences.h (right): https://codereview.chromium.org/1309543004/diff/80001/content/public/common/r... content/public/common/renderer_preferences.h:103: // connect to peer or TURN servers. Default is true. to peer or => to peers and https://codereview.chromium.org/1309543004/diff/80001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1309543004/diff/80001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:170: // Keep track options which impact PortAllocator, like whether to allow Keep track options => keep track of options https://codereview.chromium.org/1309543004/diff/80001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:172: // information is ignored from |config_|. Actually, the code doesn't ignore it, it only adds to it. Perhaps phrase this as "configuration common to all PortAllocators created by this factory; additional, per-allocator configuration is passed into CreatePortAllocator.". https://codereview.chromium.org/1309543004/diff/80001/content/renderer/p2p/po... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1309543004/diff/80001/content/renderer/p2p/po... content/renderer/p2p/port_allocator.h:41: // Disable nonproxied UPD-based transport when set to false. non-proxied
juberti@webrtc.org changed reviewers: + juberti@webrtc.org
https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.h:38: // Disable TCP-based transport when set to false. This is somewhat confusing because enable_tcp_transport is subtly different than enable_nonproxied_udp_transport. i.e., TCP transport set to false allows TCP transport to relay, but UDP transport set to false doesn't allow UDP transport to relay. I think the key issue is the use of the word |transport|, since disabling TcpPort is different than disabling *all UDP traffic*. https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.h:41: // Disable non-proxied UPD-based transport when set to false. UDP
https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.h:38: // Disable TCP-based transport when set to false. On 2015/08/27 00:21:20, juberti1 wrote: > This is somewhat confusing because enable_tcp_transport is subtly different than > enable_nonproxied_udp_transport. i.e., TCP transport set to false allows TCP > transport to relay, but UDP transport set to false doesn't allow UDP transport > to relay. > > I think the key issue is the use of the word |transport|, since disabling > TcpPort is different than disabling *all UDP traffic*. That's a good point. But since TCP to the relay would go through a proxy, we could name it "enable_nonproxied_tcp" to match "enable_unproxied_udp", couldn't we? Then they would be consistent. I agree with dropping the word "transport". I don't like it.
On 2015/08/27 16:59:09, pthatcher2 wrote: > https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... > File content/renderer/p2p/port_allocator.h (right): > > https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.h:38: // Disable TCP-based transport when > set to false. > On 2015/08/27 00:21:20, juberti1 wrote: > > This is somewhat confusing because enable_tcp_transport is subtly different > than > > enable_nonproxied_udp_transport. i.e., TCP transport set to false allows TCP > > transport to relay, but UDP transport set to false doesn't allow UDP transport > > to relay. > > > > I think the key issue is the use of the word |transport|, since disabling > > TcpPort is different than disabling *all UDP traffic*. > > That's a good point. But since TCP to the relay would go through a proxy, we > could name it "enable_nonproxied_tcp" to match "enable_unproxied_udp", couldn't > we? Then they would be consistent. > > I agree with dropping the word "transport". I don't like it. direct tcp could also go through a proxy. Whether tcp goes through a proxy or not is not something we could control. https://www.chromium.org/developers/design-documents/network-stack/socks-proxy One is transport level (tcpport), the other is protocol level. How about enable_tcp_port & enable_unproxied_udp_protocol
Guowei and I drew a diagram together. I was wrong about this. But he realized that disable_tcp_transpot is never used anyway, so we can just remove it. Problem solved :). On Thu, Aug 27, 2015 at 9:59 AM, <pthatcher@chromium.org> wrote: > > > https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... > File content/renderer/p2p/port_allocator.h (right): > > > https://codereview.chromium.org/1309543004/diff/100001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.h:38: // Disable TCP-based transport > when set to false. > On 2015/08/27 00:21:20, juberti1 wrote: > >> This is somewhat confusing because enable_tcp_transport is subtly >> > different than > >> enable_nonproxied_udp_transport. i.e., TCP transport set to false >> > allows TCP > >> transport to relay, but UDP transport set to false doesn't allow UDP >> > transport > >> to relay. >> > > I think the key issue is the use of the word |transport|, since >> > disabling > >> TcpPort is different than disabling *all UDP traffic*. >> > > That's a good point. But since TCP to the relay would go through a > proxy, we could name it "enable_nonproxied_tcp" to match > "enable_unproxied_udp", couldn't we? Then they would be consistent. > > I agree with dropping the word "transport". I don't like it. > > https://codereview.chromium.org/1309543004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/08/27 18:54:07, chromium-reviews wrote: > Guowei and I drew a diagram together. I was wrong about this. But he > realized that disable_tcp_transpot is never used anyway, so we can just > remove it. Problem solved :). \o/
Patchset #5 (id:120001) has been deleted
On 2015/08/27 18:57:30, juberti wrote: > On 2015/08/27 18:54:07, chromium-reviews wrote: > > Guowei and I drew a diagram together. I was wrong about this. But he > > realized that disable_tcp_transpot is never used anyway, so we can just > > remove it. Problem solved :). > > \o/ PTAL.
Patchset #5 (id:140001) has been deleted
chrome/ lgtm
guoweis@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/public/common/*
https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.cc (right): https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.cc:41: cricket::PORTALLOCATOR_DISABLE_UDP_RELAY; So these flags disable all UDP traffic. I'm confused by "non-proxied". https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.h:38: // Disable non-proxied UDP-based transport when set to false. nit: reword to avoid double negative: Enables non-proxied UDP-based transport. https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.h:38: // Disable non-proxied UDP-based transport when set to false. What is "non-proxied"? do we ever use proxy for UDP? it looks like this flag actually disables all UDP traffic, so should it be called enable_udp_transport?
On 2015/08/28 15:21:27, Sergey Ulanov wrote: > https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... > File content/renderer/p2p/port_allocator.cc (right): > > https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.cc:41: > cricket::PORTALLOCATOR_DISABLE_UDP_RELAY; > So these flags disable all UDP traffic. I'm confused by "non-proxied". > > https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... > File content/renderer/p2p/port_allocator.h (right): > > https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.h:38: // Disable non-proxied UDP-based > transport when set to false. > nit: reword to avoid double negative: Enables non-proxied UDP-based transport. > > https://codereview.chromium.org/1309543004/diff/160001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.h:38: // Disable non-proxied UDP-based > transport when set to false. > What is "non-proxied"? do we ever use proxy for UDP? it looks like this flag > actually disables all UDP traffic, so should it be called enable_udp_transport? Yes, it effectively disables all UDP for now. However, the thought is that once RETURN (UDP supporting proxy) is availabe, we don't want to bother user to turn it off or invent another option for that. I updated the comment.
lgtm
content/public/common LGTM with nit. https://codereview.chromium.org/1309543004/diff/180001/content/public/common/... File content/public/common/renderer_preferences.h (right): https://codereview.chromium.org/1309543004/diff/180001/content/public/common/... content/public/common/renderer_preferences.h:102: // Set to true to indicate that WebRTC is allowed to use non-proxied UDP to nit: This should probably be rephrased starting with "Set to false to...," since it defaults to true. (I find it confusing that we have enable_* preferences that default to both true and false, as well as disable_* preferences. The "convention" so far, if there is one, seems to be this "Set to true/false" comment to indicate how to override the default if desired. That said, I do appreciate you saying what the default is in the comment.)
On 2015/08/28 17:19:21, Charlie Reis wrote: > content/public/common LGTM with nit. > > https://codereview.chromium.org/1309543004/diff/180001/content/public/common/... > File content/public/common/renderer_preferences.h (right): > > https://codereview.chromium.org/1309543004/diff/180001/content/public/common/... > content/public/common/renderer_preferences.h:102: // Set to true to indicate > that WebRTC is allowed to use non-proxied UDP to > nit: This should probably be rephrased starting with "Set to false to...," since > it defaults to true. > > (I find it confusing that we have enable_* preferences that default to both true > and false, as well as disable_* preferences. The "convention" so far, if there > is one, seems to be this "Set to true/false" comment to indicate how to override > the default if desired. That said, I do appreciate you saying what the default > is in the comment.) Ok. I rephased it.
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, pthatcher@chromium.org, thakis@chromium.org, sergeyu@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1309543004/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309543004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309543004/220001
juberti@google.com changed reviewers: + juberti@google.com
lgtm for overall design
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juberti@google.com, sergeyu@chromium.org, pthatcher@chromium.org, thakis@chromium.org, creis@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1309543004/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309543004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309543004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juberti@google.com, sergeyu@chromium.org, pthatcher@chromium.org, thakis@chromium.org, creis@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1309543004/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309543004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309543004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309543004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309543004/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a84ace48ac5c7c257913ce4f6ed9ab88d7eb9dca Cr-Commit-Position: refs/heads/master@{#346294} |
