Closed Bug 1548841 Opened 7 months ago Closed 7 months ago

Handle incoming mDNS ICE candidates in webrtc signaling

Categories

(Core :: WebRTC: Networking, enhancement, P2)

68 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

We can use the existing DNS implementation to resolving mDNS addresses. Because of this, we can support handling incoming mDNS ICE candidates before moving on to generating and advertising our own mDNS candidates.

In both Safari and Chrome, mDNS ICE Candidates are only used if camera/microphone is not granted, so for testing interoperability, datachannels are necessary. Youenn on the Safari team has a test app here: https://evening-thicket-98446.herokuapp.com/src/content/peerconnection/filetransfer-b2b/.

Summary: Handling incoming mDNS ICE candidates in webrtc signaling → Handle incoming mDNS ICE candidates in webrtc signaling

Comment on attachment 9062534 [details]
Bug 1548841 - Handle incoming mDNS ICE candidates; r=bwc!

The main feedback I'm looking for is whether this should live in MediaTransportHandlerSTS or if I should push it down into nICEr. Any other suggestions you have would be appreciated. What I have so far works, but is not complete as far as the spec goes. Thanks!

Attachment #9062534 - Flags: feedback?(docfaraday)

My kneejerk reaction is to say it is fine to keep this at the MediaTransportHandler level.

Actually in interesting idea that the ICE stack itself doesn't need to be able to resolve anything.

I'm only wondering if things become more complicated once we start advertising mDNS ICE candidates ourself. Because then we need to ensure that the raw IP addresses are not leaked through getStats() and other API's. Not sure if it makes more sense to have a privacy filter for each API call before handing out the data, or if it makes more sense to centralize everything into nICEr and only let the ICE stack know about raw IP addresses. I'm not sure what makes more sense or is easier.

But I'm okay with landing as is and refactor later if it turns out this approach causes problems when we want to advertise mDNS candidates.

Attachment #9062534 - Flags: feedback?(docfaraday) → feedback+

(In reply to Nils Ohlmeier [:drno] from comment #4)

Actually in interesting idea that the ICE stack itself doesn't need to be able to resolve anything.

I'm only wondering if things become more complicated once we start advertising mDNS ICE candidates ourself. Because then we need to ensure that the raw IP addresses are not leaked through getStats() and other API's. Not sure if it makes more sense to have a privacy filter for each API call before handing out the data, or if it makes more sense to centralize everything into nICEr and only let the ICE stack know about raw IP addresses. I'm not sure what makes more sense or is easier.

But I'm okay with landing as is and refactor later if it turns out this approach causes problems when we want to advertise mDNS candidates.

That is a good point, although we also need to handle the stats carefully even when receiving candidates so we don't leak a remote mDNS candidate. The patch I attached here handles hiding the addresses in part.

What I missed the first time around was [1] which says we can't pair remote mDNS addresses to local relay addresses for TURN. I might be able to make things work by passing a flag into nICEr to avoid that situation, but first I'm going to see how much work is involved in modifying nICEr to work with mDNS addresses, that might end up being cleaner.

[1] https://tools.ietf.org/html/draft-ietf-rtcweb-mdns-ice-candidates-03#section-3.3.2

Attachment #9062534 - Attachment description: Bug 1548841 - Handle incoming mDNS ICE candidates → Bug 1548841 - Handle incoming mDNS ICE candidates; r=bwc

This adds a mdns_addr field to nICEr ICE candidates to track the mDNS address
associated with a candidate, if any. This is used to hide the real address
when generating ICE stats. This potentially could be handled at the
MediaTransportHandler level, but we need to know if a candidate is mDNS to
prevent pairing it with relay candidates, which is part of the next commit.

This adds a unit tests to check that the mDNS addresses are handled properly.
As part of doing this, TestBogusCandidate was fixed. As written, it was never
parsing the bogus candidate because it was in the wrong ICE state.

Depends on D29861

This can lead to leaking remote mDNS addresses to TURN servers.
See: https://tools.ietf.org/html/draft-ietf-rtcweb-mdns-ice-candidates-03#section-3.3.2

Depends on D30935

Peer reflexive candidates could leak local addresses otherwise hidden by using
mDNS. These must not be part of ICE statistics unless the same address has
been signaled separately.

See: https://tools.ietf.org/html/draft-ietf-rtcweb-mdns-ice-candidates-03#section-3.3.1

Depends on D30936

I tested this predominantly for using the site mentioned in comment 0. With these changes, I see interoperability with Chrome and Safari, mostly using peer reflex candidates, but I did get a successful connection with Chrome using a mDNS candidate. The test app unfortunately has site compatibility issues with Firefox, but it is possible to check that things are working in about:webrtc. I'm hoping that more unit testing will be possible once we start generating our own mDNS candidates.

In Chrome, when peer reflexive candidate addresses are hidden, nothing is displayed. I put in "(redacted)" to make it clear that we are intentionally hiding information rather than there being some other problem, but we might want to just hide the address to be compatible with Chrome.

Attachment #9062534 - Attachment description: Bug 1548841 - Handle incoming mDNS ICE candidates; r=bwc → Bug 1548841 - Handle incoming mDNS ICE candidates; r=bwc!
Status: NEW → ASSIGNED
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d909d196e7a9
Handle incoming mDNS ICE candidates; r=bwc
https://hg.mozilla.org/integration/autoland/rev/173b03319a46
Obfuscate mDNS ICE candidate addresses; r=bwc
https://hg.mozilla.org/integration/autoland/rev/15ec66f24d05
Do not pair local relay candidates to remote mDNS candidates; r=bwc
https://hg.mozilla.org/integration/autoland/rev/3c4fb056f40c
Do not reveal peer reflexive candidate addresses unless otherwise signaled; r=bwc
You need to log in before you can comment on or make changes to this bug.