Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTCIceTransport.getRemoteCandidates() does not return prflx candidates #2124

Closed
docfaraday opened this issue Mar 12, 2019 · 27 comments
Closed

Comments

@docfaraday
Copy link
Contributor

The spec says getRemoteCandidates() returns the set of remote candidates passed to addIceCandidate, but prflx candidates are never passed to that interface. They are discovered via ICE. It seems that they should make an appearance in getRemoteCandidates() also.

@youennf
Copy link
Contributor

youennf commented Mar 15, 2019

prflx candidates were not surfaced to JS in the past so this requires some thoughts.
In particular, prflx candidates are not discovered by the same ICE agent as for local/srflx candidates.

WebRTC stats do expose them somehow but it is mandated to not expose https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats-address for privacy reasons.

getSelectedCandidatePair() might expose prflx candidates.
In that case, we probably need to make sure the spec applies the above privacy rules as well.
For instance, https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate-candidate might be partially or totally obfuscated for prflx candidates.

With some work, we could probably return prflx candidates as part of getRemoteCandidates().
But I do not really see any benefit here.
The current definition of getRemoteCandidates is clear, easy to understand and implement.

What is the motivation/usecase for adding prflx candidates?

@alvestrand
Copy link
Contributor

Good questions. The most important part of a prflx candidate (the IP address) is readily observable to anyone with access to the network - but JS generally doesn't have that.

It seems obvious that seeing a candidate in stats or in the active candidate pair, and then not finding that candidate in getRemoteCandidates, is going to be puzzling to users who expect logical consistency between these candidate sets.

So we can either spend resources explaining the inconsistency, or find a consistent way of representing prflx candidates wherever candidates are listed.

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

We need to be careful to not expose IP addresses that have been hidden via the use of mDNS. Otherwise, one could create two peer connections A and B:

  1. Add all candidates from A to B.
  2. Filter out all mDNS candidates from B when adding them to A.
  3. B will resolve A's concealed IP addresses and start connectivity checks.
  4. A will pick them up as prflx candidates, exposing the IP address again.

Unless I've missed a mechanism in the mDNS draft (such as an additional STUN attribute).

I guess we could just always anonymise/remove IP addresses from prflx candidates before handing them out in stats or any other API surface.

@docfaraday
Copy link
Contributor Author

I'm trying to understand the threat model here. This might prevent JS from learning about a prflx, but does this really make a difference if you control the ICE agent, or the STUN/TURN servers, or the web server, or any other network element that the browser sends packets to? Am I missing something here?

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

With my example, you would not need to control any network element and could learn private IP addresses by just creating two RTCPeerConnection instances even if mDNS has been activated to conceal private addresses.

@docfaraday
Copy link
Contributor Author

Ok, so the threat model here is that you have a host candidate leaked to JS by having two PCs created on the same browser, and using mDNS. mDNS allows STUN transactions to succeed on host candidates without any host candidates being trickled, so we want to hide them from JS.

I can see the value of hiding this information in this case, but having non-obfuscated prflx candidates available in the stats API for the general case is probably important.

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

having non-obfuscated prflx candidates available in the stats API for the general case is probably important

I'm wondering: Is there a concrete use case that would require this?

@docfaraday
Copy link
Contributor Author

docfaraday commented Mar 21, 2019

having non-obfuscated prflx candidates available in the stats API for the general case is probably important

I'm wondering: Is there a concrete use case that would require this?

Did we have a concrete use-case in mind when we specified ICE candidate stats? If so, that's the use-case you're looking for. If not, well... shrug

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

Good question! I pass that on to the stats experts. 🙂 But to be honest, I have a feeling this might just be exposed because humans tend to like metrics.

@docfaraday
Copy link
Contributor Author

I wonder; what if we specified that when we learn about our own prflx candidates (ie; we get a STUN response that tells us about them), we trickle them as long as they don't expose any info we want to keep hidden. If we do trickle them, they end up being handled like a srflx.

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

I can't quite follow that. Can you elaborate? In particular, I don't understand what "trickle them" means.

@docfaraday
Copy link
Contributor Author

When the browser receives a response to an ICE check, we treat the XOR-MAPPED-ADDRESS as a newly-discovered local candidate for that component, just like we would do with a response from a STUN server. We check to see whether that IP:port is one we've already discovered, and whether we want to obfuscate it. If neither of those are true, we call the icecandidate event handler with it.

@lgrahl
Copy link
Contributor

lgrahl commented Mar 21, 2019

Thanks! How would we determine whether we want to obfuscate a prflx candidate?

@docfaraday
Copy link
Contributor Author

By looking at the IP address, and comparing it to your local addresses.

I wonder if the same checking should be performed for srflx, actually.

@youennf
Copy link
Contributor

youennf commented Mar 21, 2019

getRemoteCandidates definition is only about candidates added through addIceCandidate.
Given that there is no clear use cases for adding PRFLX candidates, it is suggested to keep the status quo.

Proposal is therefore to close this issue and do the following actions:

  • Make it clear in getRemoteCandidates description that PRFLX candidates will not be listed.
  • Ensure that getSelectedCandidatePair handles PRFLX candidates similarly to webrtc-stats to not leak any unnecessary information.

@youennf
Copy link
Contributor

youennf commented Mar 21, 2019

Filed #2134 for getSelectedCandidatePair.

@youennf
Copy link
Contributor

youennf commented Mar 21, 2019

Related PR: #2135

@docfaraday
Copy link
Contributor Author

I don't agree. If we have candidate stats, we should be exposing prflx candidates unless there's some good reason not to.

@docfaraday
Copy link
Contributor Author

To be clear, I think it is completely reasonable to expose only the remote candidates that have been passed to addIceCandidate, and it is also reasonable to expose only the local candidates that have been signaled via the icecandidate event handler. The change that makes the most sense to me is to signal prflx via icecandidate exactly like we do srflx.

@youennf
Copy link
Contributor

youennf commented Mar 21, 2019

The change that makes the most sense to me is to signal prflx via icecandidate exactly like we do srflx.

I agree that in some cases, a prflx candidate could have been a srflx candidate, should the web app provide a STUN server.
I do not see the advantage of trying to manage prflx as srflx (but I see some complexity in doing that).

We need to do signaling of srflx candidates through the web app.
On the other hand, signaling of prflx candidates through the web app is not needed since this is done through STUN requests/responses. And signaling of prflx candidates can actually leak private information.

@docfaraday
Copy link
Contributor Author

I would argue that an API that only gives us some of the ICE candidate statistics isn't very useful. In my mind, the question is how we make this API useful in the scenario where there are prflx candidates.

@youennf
Copy link
Contributor

youennf commented Mar 22, 2019

ICE candidate statistics are provided by WebRTC stats.
From https://w3c.github.io/webrtc-pc/#mandatory-to-implement-stats, stats are to be done "when the corresponding objects exist on a PeerConnection".
My interpretation of this rule is that:

  • Any ICE candidate that is exposed to JS/provided by JS should have corresponding stats.
  • The selected candidate pair should have corresponding stats.
  • A prflx candidate should have stats if it is part of the selected candidate pair.

@docfaraday
Copy link
Contributor Author

I don't think we ought to make exposure of a candidate to JS via stats (or stats-like stuff, eg getRemoteCandidates/getLocalCandidates) contingent on whether that candidate is part of a selected pair.

@youennf
Copy link
Contributor

youennf commented Mar 22, 2019

@alvestrand might know more about the intent of this rule, maybe I am misinterpreting what the minimum stats production requirements are.
An implementation is free to provide additional stats if desired.

I would separate this discussion from getRemoteCandidates/getLocalCandidates.
These are convenience APIs exposing data going through icecandidate events/addIceCandidate.
We might need to change the definition of these APIs to expose prflx candidates.
And changing icecandidate to expose prflx candidates might also require additional work.
This effort should be justified by some use cases.

@aboba aboba added the wontfix label Mar 24, 2019
@aboba
Copy link
Contributor

aboba commented Mar 24, 2019

Since getRemoteCandidates was defined in ORTC several years ago, it has been clear that it only returned candidates which were previously added by addCandidates. See: http://draft.ortc.org/#dom-rtcicetransport-getremotecandidates

@youennf
Copy link
Contributor

youennf commented Apr 24, 2019

Is it ok to close the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

7 participants
@dontcallmedom @alvestrand @aboba @lgrahl @docfaraday @youennf and others