w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
438 stars 115 forks source link

Should we remove getDefaultIceServers? #2023

Closed youennf closed 5 years ago

youennf commented 6 years ago

Running http://w3c-test.org/webrtc/RTCPeerConnection-getDefaultIceServers.html in Chrome, Firefox and Safari, it seems none of these browsers supports getDefaultIceServers. Not sure about edge. Should we just think of removing this method?

lgrahl commented 6 years ago

What was the rationale to add it?

aboba commented 6 years ago

The original rationale was that a browser might have ICE servers supported by default that an application might want to retrieve. However, if a browser doesn't support default ICE servers, it does not make sense to implement a method to retrieve the (non-existent) defaults. If only a single browser is intending to support it (Firefox?) it can be marked as "at risk".

lgrahl commented 6 years ago

It would be interesting to hear why browser implementations have chosen not to provide default ICE servers.

misi commented 6 years ago

FF implemented the variable, so in my understanding it partially implemented. See in FF about:config media.peerconnection.default_iceservers: []

I use it with in pair with media.peerconnection.use_document_iceservers: false to enforce to use my pre-configured trusted TURN servers instead of to use any application configured stun/turn servers/services, (that have unknown, random quality)

Please consider that there are many uncertainty, with application configured TURN!

So I think if someone is taking care about trusted TURN servers, privacy and about the TURN server selection control, then there are still reasons to have such browser side trusted default TURN Server.

lgrahl commented 6 years ago

I think this is a valid point: Pre-configured TURN servers in browsers can be useful to enforce a company policy.

/cc @nils-ohlmeier since it apparently is in Firefox but just not exposed? (Which might make sense btw.)

alvestrand commented 6 years ago

The deployment scenario envisioned in the Sapporo TPAC by @fluffy was that a corporate firewall would deploy a TURN server on the internal network edge, and refuse to allow connections to globally accessible TURN servers (and perhaps even all peer-to-peer connections). If the browser could be configured with the corporate TURN servers, off-the-shelf WebRTC apps would Just Work using the corporate TURN server, while if the browser had no such configuration, those apps would just fail. @fluffy please comment.

youennf commented 6 years ago

The idea to have a mechanism within browsers to configure default or mandatory STUN/TURN servers makes sense. I am unclear of the need to surface that kind of information to the web application (except maybe easing fingerprinting). What will the web application do with this information?

In particular, it is unclear what the getDefaultIceServers list refers in terms of UA behavior:

fluffy commented 6 years ago

Yes, pretty much what alvestrand said up a few messages ago. I think we also decided that the browser just returns this list in the call but it does not by default use any of them. The application would have to call this function to get the list, then add the things that were returned into whatever list of TURN/STUN servers that the JS app passes into the browser. This way the control of if theses are used or not is 100% with the JS app.

youennf commented 6 years ago

Does it make sense to let this in full control of the JS app? For instance, it might be difficult for the JS app to understand whether these servers are mandatory to use or not, except by actually trying to not use them and if failing use them. I would guess most (if not all apps) should then add whatever default servers are provided. If so, it seems the UA should do that for them (added bonus, no change required on the JS app).

If JS app wants some more control, an alternative would be to provide a boolean in the configuration stating "UA, please do not use any default STUN/TURN server".

This might reduce a bit the fingerprinting as well: we would be moving from a stable TURN server URL to an unstable relay candidate.

lgrahl commented 6 years ago

If JS app wants some more control, an alternative would be to provide a boolean in the configuration stating "UA, please do not use any default STUN/TURN server".

I like that idea but we need to ensure that the servers cannot leak in other portions of the API (stats for example). Perhaps they could be marked as opaque.

misi commented 6 years ago

According rtweb Security Architecture draft the Trust base is the Browser and not the App.. This way I think we should keep this decision for the user and UA/browser. I believe it should be allowed for the user (or enterprise)to choose which TURN relay service to use.

I fully agree Mozilla implementation that gives the freedom to choose. media.peerconnection.ice.default_address_only

I see benefits of using internal/well-know/trusted TURN service instead App configured:

@youennf

youennf commented 5 years ago

I am not clear whether there is consensus on removing/updating getDefaultIceServers. Currently, the two red flags I see around this API are:

  1. It is a source of fingerprinting
  2. It is not implemented, maybe because of 1, maybe because of lack of interest in the feature.

I would be particularly interested in Mozilla opinion here since Firefox seems to be the most advanced in that area. @jan-ivar?

juberti commented 5 years ago

Originally discussed in https://github.com/w3c/webrtc-pc/issues/263, and then discussed live in https://www.w3.org/2015/10/29-webrtc-minutes, where we mostly just wanted to close this issue with something simple and move on.

Since we have now gone 3 years and there hasn't been a clear need for this API, plus we have identified a real-world downside, I suggest we remove this API point.

lgrahl commented 5 years ago

@youennf @juberti Are you suggesting to...

  1. ... remove this API and the mechanism?
  2. ... update the mechanism to merge application-specific ICE servers with default ICE servers (perhaps explicitly bound to origins) but don't expose them via the API?

The latter could of course be left to the browser implementation but it might be useful to keep this as an optional feature or a note in the spec.

misi commented 5 years ago

I support the idea to add a note about it in the spec.

AFAIU of course we can remove this method, but I think it is not ultimate solution, because just alone it will not avoid totally an app to fingerprint ICE servers. e.g. During ICE candidate discovery TURN server relay IP could be used the same way to fingerprint ice servers. Maybe it is little bit more complicated, but in most cases the relay IP and turn server IP is the same. To hide from App the default ICE servers, and relay candidates and pair selections could be complicated task.

youennf commented 5 years ago

Given where we are in the spec dev cycle, I would tend to remove the API from the WebRTC 1.0 spec. That does not prevent to think of alternative APIs like the boolean one. That does not prevent Firefox to keep doing what they are doing and improving on it.

juberti commented 5 years ago

@lgrahl, I think we should do 1) and remove the API from WebRTC 1.0.

I am opposed to 2) at this point in time, as defining such interaction is likely be complex at best. (See RETURN as an example.)

jan-ivar commented 5 years ago

I suppose we could address fingerprinting somewhat the way Chrome does for deviceInfo.getCapabilities(), which is to return {} unless gUM permission has been granted. But as @misi points out, this seems futile if onicecandidate exposes the same info.

I'm not super opposed to removing it. Its absence wouldn't prevent UAs from implementing pre-provisioned ICE servers as I understand it, just remove a convenience method for JS to find out early if this special routing will happen?

I agree this is orthogonal to and separate from [[Configuration]].

juberti commented 5 years ago

The was getDefaultIceServers is supposed to work is that it doesn't cause any special routing on its own, it's something the app can query to add preconfigured servers to its RTCConfiguration. As noted in the minutes above,

harald: so we rejected the idea of letting the browser to inject ICE servers just because they feel like it.

So yeah, removal would prevent pre-provisioning.

juberti commented 5 years ago

Note that RETURN-type functionality could be added without this API, since those servers are treated like proxies instead of application TURN servers, and do not increase the fingerprinting surface.

jan-ivar commented 5 years ago

Thanks @juberti for clarifying. Didn't seem super clear from spec prose alone. cc @nils-ohlmeier.

misi commented 5 years ago

@juberti I understand your point that we are about to finish 1.0.

I can not see RETURN as a solution for the issue:

The trust base is the browser not the App, and in my understanding the user should have given the control to set which TURN service to use, the App provisioned or a hardcocded/pre-provisioned user contracted TURN service has SLA, distributed optimally around globe and so more Trusted.

IMHO pre-provsioning instead of App iceservers is a required feature, and spec should consider it.

juberti commented 5 years ago

The scenarios that you mention are not part of the WebRTC 1.0 use cases; the only use case that is even similar is 2.3.5, and even that is focused on enterprise aspects and specifically indicates that the enterprise servers are used in addition to the app servers.

Accordingly, these scenarios are not discussed at all in the WebRTC 1.0 security architecture document.

misi commented 5 years ago

I think it is an edge case that is not clarified enough. The mentioned documents gives only guide lines, but they are not specific enough that we could say so hard that such case are not included there. Like mozilla understood it correctly. So, I don't share your strict view. I think it could be also understand as included in enterprise use case.

Removing pre-provisioning functionality is also in conflict with 1.0 enterprise use case.

The WebRTC functionality will need to utilize both network specific STUN and TURN resources and STUN and TURN servers provisioned by the web application.

I don't want to waste more of your time on this. As mentioned I understand your point, that finishing 1.0 is the goal here.

I am fine to add my scenario to wbertc-nv use cases, and cover it there. But you should then consider also say no the same way removing this 1.0 functionality.

jan-ivar commented 5 years ago

@juberti's interpretation of getDefaultIceServers doesn't actually seem to support pre-provisioning at all, except for the tiny minority of web applications that opt into it on purpose.

That's very different from what Firefox supports:

I wonder if getDefaultIceServers is in the spec because both sides thought they'd gotten what they wanted, in which case it doesn't actually represent a consensus.

juberti commented 5 years ago

Yeah, I think there are two different questions here: 1) should it be possible to configure 'enterprise' TURN (or perhaps STUN) servers, used in addition to any application servers. This is what RETURN aimed to solve, but we never got around to fully deciding on this. 2) should it be possible to configure one's own TURN servers, to be used instead of the application servers. I don't think this is likely to be workable, as this will violate the assumptions of some applications (who are forcing data onto their own relays for traffic optimization, anonymization, etc)

I think @jan-ivar is right in that getDefaultIceServers was an attempt at a cheap compromise, and while I think the minutes are pretty clear that it requires application participation, I would not be surprised if not everyone had the same interpretation.

Anyway, to make a long story short, my position is that getDefaultIceServers doesn't provide much value, and while I think we should solve 1) in the future, we should be able to do so without a JS-exposed API.

alvestrand commented 5 years ago

Assigning to Jan-Ivar to prepare a PR for removing.

jan-ivar commented 5 years ago

violate the assumptions of some applications (who are forcing data onto their own relays for traffic optimization, anonymization

If "forcing" = app already filters out other ice candidates, this should work fine when media.peerconnection.use_document_iceservers is true, and when it's false, the conflict seems inherent (app and user both insisting on their TURN server), hence works as expected.

aboba commented 5 years ago

hbos to mark this as a "feature at risk".

henbos commented 5 years ago

PR exists: for marking it "feature at risk", not for removal.

henbos commented 5 years ago

For the sake of the changelist I filed #2185 for the feature at risk thing. Consider this issue blocked on that one instead.

a2sheppy commented 5 years ago

FWIW I like the idea of having a flag you can set when creating an RTCPeerConnection (or anytime STUN/TURN servers come into play) that indicates "also allow any default servers to be used".

In fact, I would expand that from being a pure binary, to include the following options using an enum:

This gives tons of flexibility and should not be hard to implement. It also lets browsers choose whether or not to support default ICE servers, whether or not to include any automatically, whether or not to allow them to be user-configured, whether or not to allow them to be set by an enterprise policy, etc. And it does so without exposing those servers' identities to the client through a method like getDefaultIceServers(), which I would then remove.

henbos commented 5 years ago

I think it's worth considering if this is a use case we want to solve for webrtc-nv-use-cases, but I don't see getDefaultIceServer() happening before WebRTC 1.0 reaches PR, and - as juberti points out - if we accurately capture the use cases of interest, the way forward should probably not be a JS exposed API.

There is a lack in implementer interest for this API as it stands and I don't think it's mature, I support removing it.

a2sheppy commented 5 years ago

I would concur that removing getDefaultIceServers() makes sense. It needs to be done regardless, I think; if we want to solve the concept of default servers, it should be done without exposing those to the client directly, so this method is unneeded regardless.

henbos commented 5 years ago

This feature has been marked at risk since April 25. Chairs @alvestrand @aboba @jan-ivar, can we add the Ready for PR label?

henbos commented 5 years ago

According to https://www.w3.org/2019/09/18-webrtc-minutes.html the decision was to move this to an extension spec pending the possibility of future improvements to it

henbos commented 5 years ago

Removing from this spec here: https://github.com/w3c/webrtc-pc/pull/2354 Adding to extension spec here: https://github.com/henbos/webrtc-extensions/pull/14

henbos commented 5 years ago

Both PRs have now merged. Closing.