1680771 - Ensure WebRTC connections are partitioned

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Anne (:annevk)]

From discussion with Byron and Jan-Ivar it appears name lookups and connections might not always be suitably partitioned using a principal.

Byron was willing to provide a couple pointers to the relevant code and I was hoping Tim (or colleague) would be willing to take a look as to how it might have to be adjusted to take advantage of network partitioning.

Comment 1

[Byron Campen [:bwc]]

Here's the code that we use to open TCP sockets:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#148

We use a System principal for the proxy info lookup here:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#207

We use the loading principal for the channel (in the web proxy case) here:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#387-388

We do not seem to be using any principal when opening a TCP stream in the non-proxy case; this code just uses nsISocketTransport. I am not sure how one would tell this interface about principal stuff, but maybe SetOriginAttributes would be the thing to use?

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#332-355

I should note that I am working on delegating all DNS lookups for TCP to WebrtcTCPSocket (and therefore Necko) over in bug 1416220. I should also note that our mochitests exercise the localhost case on linux and OS X. Right now the patches for bug 1416220 are not working on the linux testers for some reason, although these tests do work on my own linux box. Not sure what is up with that, I may be tripping over some of the localhost-related policy.

Comment 2

[Anne (:annevk)]

Thanks Byron! I hope that Tim has time to take a look. We should probably also look at the UDP connections?

Comment 3

[Byron Campen [:bwc]]

Right now, the DNS resolution for both UDP and TCP is handled here:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/nriceresolver.cpp#174-177

Bug 1416220 will stop using this for TCP, but UDP will continue to use it.

The UDP socket code does different things depending on how processes are set up:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/nr_socket_prsock.cpp#1650-1665

For the socket process and parent process case, we use a plain NrSocket for UDP. Note that this is very low level API, and will certainly not be able to handle DNS on its own:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/nr_socket_prsock.cpp#576

For the content process case, we use NrUdpSocketIpc, which in turn relies on UDPSocketChild, which may be able to handle DNS on its own:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/nr_socket_prsock.cpp#1473

In order to delegate DNS lookups to Necko for our UDP sockets, we would want to unify these two cases, probably relying on UDPSocketChild for both (unless on the parent process, but it is unclear if we even want to continue supporting that).

Comment 4

[Tim Huang[:timhuang]]

From the code, it looks that we can partition the WebRTC connections. Right now, the partition key is from the cookieJarSettings. So, we can have the connection partitioned if we can get the cookieJarSettings from the content.

For TCP connections, I think we can get the cookieJarSettings from the loadInfo passed by the mProxyConfig. And set it to the channel here.

For UDP connections, we can set the originAttributes to the nsISocketTransport directly. But I am not sure if we can get the loadInfo from the content in this case.

I need more time to check the DNS resolution.

Byron, I have two questions here. Does the loadInfo in the mProxyConfig came from the content document? Does WebRTC have another way to pass information from the content document other than mProxyConfig? Thanks.

Comment 5

[Byron Campen [:bwc]]

(In reply to Tim Huang[:timhuang] from comment #4)

From the code, it looks that we can partition the WebRTC connections. Right now, the partition key is from the cookieJarSettings. So, we can have the connection partitioned if we can get the cookieJarSettings from the content.

For TCP connections, I think we can get the cookieJarSettings from the loadInfo passed by the mProxyConfig. And set it to the channel here.

For UDP connections, we can set the originAttributes to the nsISocketTransport directly. But I am not sure if we can get the loadInfo from the content in this case.

I need more time to check the DNS resolution.

Byron, I have two questions here. Does the loadInfo in the mProxyConfig came from the content document? Does WebRTC have another way to pass information from the content document other than mProxyConfig? Thanks.

Hmm, it seems not: https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/dom/media/webrtc/jsapi/PeerConnectionMedia.cpp#874

I think it would be possible to fix this, though. mProxyConfig is the only way we would pass this kind of information, although it has to go through the MediaTransportHandler ipdl as an NrSocketProxyConfig first.

Comment 6

[Byron Campen [:bwc]]

So would it be appropriate to use this LoadInfo c'tor with the Document's LoadPrincipal? Or is there some other way we should be doing this?

https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/netwerk/base/LoadInfo.h#84

I seem to recall having some difficulty using the "real" principal here back when I wrote this code, but let me try.

Comment 7

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcbe977c54a9b38a7e9b54da66225d8fe2ea0997

Comment 8

[Byron Campen [:bwc]]

Ok, the patch in comment 7 seems to work ok. In order to set the cookie jar stuff, I guess NewChannelFromURIWithProxyFlags would have to take another param, or maybe we could expose NewChannelFromURIWithProxyFlagsInternal since that takes a LoadInfo, which is what we already have.

Comment 9

[Tim Huang[:timhuang]]

(In reply to Byron Campen [:bwc] from comment #6)

So would it be appropriate to use this LoadInfo c'tor with the Document's LoadPrincipal? Or is there some other way we should be doing this?

https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/netwerk/base/LoadInfo.h#84

Yes, I think this is correct. And the CookieJarSettings will be inherited from the document here.

(In reply to Byron Campen [:bwc] from comment #8)

Ok, the patch in comment 7 seems to work ok. In order to set the cookie jar stuff, I guess NewChannelFromURIWithProxyFlags would have to take another param, or maybe we could expose NewChannelFromURIWithProxyFlagsInternal since that takes a LoadInfo, which is what we already have.

I think you can set the cookieJarSettings to the loadInfo of the channel before opening it. So, we don't need the NewChannelFromURIWithProxyFlagsInternal.

Comment 10

[Byron Campen [:bwc]]

(In reply to Tim Huang[:timhuang] from comment #9)

(In reply to Byron Campen [:bwc] from comment #6)

So would it be appropriate to use this LoadInfo c'tor with the Document's LoadPrincipal? Or is there some other way we should be doing this?

https://searchfox.org/mozilla-central/rev/1843375acbbca68127713e402be222350ac99301/netwerk/base/LoadInfo.h#84

Yes, I think this is correct. And the CookieJarSettings will be inherited from the document here.

(In reply to Byron Campen [:bwc] from comment #8)

Ok, the patch in comment 7 seems to work ok. In order to set the cookie jar stuff, I guess NewChannelFromURIWithProxyFlags would have to take another param, or maybe we could expose NewChannelFromURIWithProxyFlagsInternal since that takes a LoadInfo, which is what we already have.

I think you can set the cookieJarSettings to the loadInfo of the channel before opening it. So, we don't need the NewChannelFromURIWithProxyFlagsInternal.

So I guess we can stomp the LoadInfo with nsIChannel::SetLoadInfo? I'll need to double check that we don't lose any of the flags by doing that.

Comment 11

[Tim Huang[:timhuang]]

(In reply to Byron Campen [:bwc] from comment #10)

So I guess we can stomp the LoadInfo with nsIChannel::SetLoadInfo? I'll need to double check that we don't lose any of the flags by doing that.

You don't need to set the whole loadInfo. The loadInfo will also be created when creating the channel. All you need to do is to set the CookieJarSettings to the loadInfo of the channel.

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f5f641ec204e8dc31488466fcf97869e0aba40e

https://treeherder.mozilla.org/jobs?repo=try&revision=f79cf8bc2e8cb9a8334c18ce58975757ee70d99f

https://treeherder.mozilla.org/jobs?repo=try&revision=54acde68feb7b3d37a53954c8a6fd0f88695101e

https://treeherder.mozilla.org/jobs?repo=try&revision=b7a273d3a00c96390a7a0f4bc0a88a149e0a90b1

Comment 13

[Byron Campen [:bwc]]

Try looks good.

Comment 14

[Byron Campen [:bwc]]

Attachment: Bug 1680771: Use a real LoadInfo for WebrtcTCPSocket, ensure we use that LoadInfo's CookieJarSettings for our DNS/proxy lookup, and remove a flag that was breaking http proxy support for webrtc.

Comment 15

[Byron Campen [:bwc]]

Strawman patch up for review. What else needs to be done here besides the work over in bug 1416220?

Comment 16

[Tim Huang[:timhuang]]

I have a question regarding the non-proxy connections.

In which case the non-proxy connections could be established? If it could be established due to the script in the content, then I think we should also find a way to partition it. If not, and the content script cannot observe non-proxy channels, then I think it's fine.

Byron, could you answer this for me? Thanks.

Comment 17

[Byron Campen [:bwc]]

(In reply to Tim Huang[:timhuang] from comment #16)

I have a question regarding the non-proxy connections.

In which case the non-proxy connections could be established? If it could be established due to the script in the content, then I think we should also find a way to partition it. If not, and the content script cannot observe non-proxy channels, then I think it's fine.

Byron, could you answer this for me? Thanks.

Non-proxy connections would be established by JS in the same way proxied ones are, the thing that makes the difference is whether an http proxy is configured. The code that opens the non-proxied TCP socket is here:

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#332-355

How would one go about partitioning this? Bear in mind that at this stage, DNS lookup will be finished already. This is just establishing a raw TCP socket over which media data will flow, so there's not going to be any cacheing or anything, so maybe there's nothing to partition?

Comment 18

[Tim Huang[:timhuang]]

We have also partitioned the HTTP connection pool beside the HTTP cache. Could the non-proxy connections be reused? If so, I think we should also partition it.

Comment 19

[Byron Campen [:bwc]]

So in the non-proxy case, there is no http connection at all. It is a raw TCP socket.

Comment 20

[Tim Huang[:timhuang]]

Ah, I didn't get your point in the first place. I think you are right that there is no point to partition a raw TCP socket.

Comment 21

[Byron Campen [:bwc]]

Attachment: Bug 1680771: Use a real LoadInfo for WebrtcTCPSocket, ensure we use that LoadInfo's CookieJarSettings for our DNS/proxy lookup, and remove a flag that was breaking http proxy support for webrtc.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bwc, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b211266a7c14b03a590ff11023eaf62f2e141ebd

Comment 24

[Byron Campen [:bwc]]

Had to rebase, and re-test manually. Try looks good. Let's try landing again.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4c8a19d48c
Use a real LoadInfo for WebrtcTCPSocket, ensure we use that LoadInfo's CookieJarSettings for our DNS/proxy lookup, and remove a flag that was breaking http proxy support for webrtc. r=timhuang,mjf

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1f4c8a19d48c