1548841 - Handle incoming mDNS ICE candidates in webrtc signaling

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

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/.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1548841 - Handle incoming mDNS ICE candidates; r=bwc!

Comment 2

[Dan Minor [:dminor]]

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!

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Nils Ohlmeier [:drno]]

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.

Comment 5

[Dan Minor [:dminor]]

(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

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1548841 - Obfuscate mDNS ICE candidate addresses; 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

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1548841 - Do not pair local relay candidates to remote mDNS candidates; r=bwc!

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

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1548841 - Do not reveal peer reflexive candidate addresses unless otherwise signaled; r=bwc!

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

Comment 9

[Dan Minor [:dminor]]

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.

Comment 10

[Dan Minor [:dminor]]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d909d196e7a9
https://hg.mozilla.org/mozilla-central/rev/173b03319a46
https://hg.mozilla.org/mozilla-central/rev/15ec66f24d05
https://hg.mozilla.org/mozilla-central/rev/3c4fb056f40c