Closed Bug 1056934 Opened 10 years ago Closed 7 years ago

Support TURN TLS in WebRTC

Categories

(Core :: WebRTC: Networking, defect, P1)

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- verified
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: thomas, Assigned: bwc, NeedInfo)

References

Details

Attachments

(6 files)

Bug 1056934 - Part 1: TLS support in the test ICE server.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1056934 - Part 2: Test-case for TURN TLS.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1056934 - Part 3: Make it possible to configure TURN TLS servers in nICEr.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1056934 - Part 4: Handle turns URLs when configuring nICEr.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1056934 - Part 5: Open TLS sockets when communicating with a TLS endpoint.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.
59 bytes, text/x-review-board-request
mcmanus
: review+
Details 
Support TURN TLS in WebRTC.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Excuse my confusion, bug 949703 is for proxying webrtc tcp traffic, not turn/tls per se.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
FWIW, you'll probably have to adapt the local candidate priority as part of this. 0 for TURN/TCP doesn't leave much space.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
I would highly appreciate this feature! In many companies TURN over TCP/TLS would make WebRTC traffic to go through the firewall (on port 443) without additional configuration. Already working with Chrome.
My demo of WebRTC at the Cambridge mini-DebConf failed to work on the wifi in the venue because of this.  Many corporate networks, meeting venues, hotels, etc, have problems with TURN over UDP and they need the user to do TURN over TLS.

I tried explicitly configuring an ICE server using "turns:turn-server.example.org:443" and Firefox just doesn't try to use it at all.  The version I tested was 41.0.1
Ick, the hotel was interfering with TURN TCP?
(Is it possible you were running your demo in an e10s window? TURN TCP is still not working in e10s.)
I only had the option of TURN over TLS or TURN over UDP.  I haven't been trying TURN over TCP.

The wifi wasn't operated by the hotel, it was the guest wifi of the tech firm hosting the event.

I worked around it by tethering my laptop to a mobile phone, where TURN over UDP was successful.

I have tried to check TURN over TLS status again today (both in Firefox and Chrome and also in ice4j) and didn't experience great results.  In Chrome it does work, but Chrome has other problems on my Debian system
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=770659
hi,
This feature would be great! working for a big company with webrtc using turn over tls in port 443 trough firewall. Chrome work without any problem, last FF46: TURNS is not yet supported.
Rank: 35 → 25
Priority: P3 → P2
Assignee: nobody → docfaraday
Rank: 25 → 15
Priority: P2 → P1
When can expect this issue to be resolved ? Would be great to have this feature to support webrtc call on Firefox for the networks which only supports TLS over port 443.
When can expect this issue to be resolved ? Would be great to have this feature to support webrtc call on Firefox for the networks which only supports TLS over port 443.
We're working on it now (Byron is the lead developer), and Firefox 53 is our target.
Depends on: 1323439
Attachment #8823470 - Flags: review?(drno) → review?(mcmanus)
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review102572

::: dom/network/TCPSocketParent.cpp:153
(Diff revision 1)
>    nsCOMPtr<nsISocketTransport> socketTransport;
> -  rv = sts->CreateTransport(nullptr, 0,
> +  const char* socketTypes[1];
> +  if (aUseSSL) {
> +    socketTypes[0] = "ssl";
> +  } else {
> +    socketTypes[0] = "starttls";

do you want starttls here, or (nullptr, 0) ?
Comment on attachment 8819455 [details]
Bug 1056934 - Part 1: TLS support in the test ICE server.

https://reviewboard.mozilla.org/r/99218/#review102768
Attachment #8819455 - Flags: review?(drno) → review+
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

LGTM

I would like the issues to get resolved. I'll leave it up to you if you want to address comments without issues.

::: media/mtransport/nricectx.cpp:622
(Diff revision 1)
>      MOZ_MTLOG(ML_DEBUG, "NAT mapping type: " << mapping_type.get());
>      TestNat* test_nat = new TestNat;
>      test_nat->filtering_type_ = TestNat::ToNatBehavior(filtering_type.get());
>      test_nat->mapping_type_ = TestNat::ToNatBehavior(mapping_type.get());
>      test_nat->block_udp_ = block_udp;
> +    test_nat->block_stun_ = false;

Is there a reason for over writing the constructor default value here with the same value?

::: media/mtransport/test_nr_socket.cpp:478
(Diff revision 1)
>    if (connect_invoked_ || !port_mappings_.empty()) {
>      MOZ_CRASH("TestNrSocket::connect() called more than once!");
>      return R_INTERNAL;
>    }
>  
> +  if (strlen(addr->tls_host)) {

strlen() appears to me like a waste of time here. How about something like: 
  if (addr->tls_host[0] != '\0') {

::: media/mtransport/test_nr_socket.cpp:520
(Diff revision 1)
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because STUN",
> +          my_addr().as_string);

"... dropping outgoing STUN TCP message because blocking STUN is requested" ?

::: media/mtransport/test_nr_socket.cpp:527
(Diff revision 1)
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because TCP",
> +          my_addr().as_string);

"... dropping outgoing TCP connection because TCP blocking is requested"?

::: media/mtransport/test_nr_socket.cpp:541
(Diff revision 1)
> -      return -1;
> +      r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because timeout",
> +            my_addr().as_string);

"... dropping outgoing TCP connection because port mappings timed out"?

::: media/mtransport/test_nr_socket.cpp:579
(Diff revision 1)
> +  UCHAR *cbuf = static_cast<UCHAR*>(const_cast<void*>(buf));
> +  if (nat_->block_stun_ && nr_is_stun_message(cbuf, *len)) {

How can this if() ever be true, with the 'if (r) {' early exit a couple of lines above?
If I'm not mistaken we should get here only if r is zero.
Attachment #8823466 - Flags: review?(drno) → review+
Comment on attachment 8823467 [details]
Bug 1056934 - Part 3: Make it possible to configure TURN TLS servers in nICEr.

https://reviewboard.mozilla.org/r/101990/#review102776

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:671
(Diff revision 1)
> +      strncpy(cand->stun_server_addr.tls_host,
> +              cand->stun_server->u.dnsname.host,
> +              sizeof(cand->stun_server_addr.tls_host));

Add zero termination at the end of |tls_host| to avoid problems when |u.dnsname.host| is bigger then |tls_host| and string in |tls_host| therefore might not be zero terminated.
Attachment #8823467 - Flags: review?(drno) → review+
Comment on attachment 8823468 [details]
Bug 1056934 - Part 4: Handle turns URLs when configuring nICEr.

https://reviewboard.mozilla.org/r/101992/#review102780
Attachment #8823468 - Flags: review?(drno) → review+
Comment on attachment 8823469 [details]
Bug 1056934 - Part 5: Open TLS sockets when communicating with a TLS endpoint.

https://reviewboard.mozilla.org/r/101994/#review102782

::: media/mtransport/nr_socket_prsock.cpp:878
(Diff revision 1)
>    int r,_status;
>    PRNetAddr naddr;
>    int32_t connect_status, getsockname_status;
>  
> +  // TODO: Add TLS layer with nsISocketProviderService?
> +  if (strlen(addr->tls_host))

Again I think strlen() is not the best way to detect if tls_host is set.
Attachment #8823469 - Flags: review?(drno) → review+
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> Is there a reason for over writing the constructor default value here with the same value?

I think this is leftover from earlier work. Will remove.

> strlen() appears to me like a waste of time here. How about something like: 
>   if (addr->tls_host[0] != '\0') {

I'm fine with doing it this way too.

> How can this if() ever be true, with the 'if (r) {' early exit a couple of lines above?
> If I'm not mistaken we should get here only if r is zero.

If we get this far in, yes r==0, but I'm not sure how it follows that this will never be true...
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review103020
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review102572

> do you want starttls here, or (nullptr, 0) ?

I was just doing what code elsewhere does, although I guess we'll never be switching protocol.
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> If we get this far in, yes r==0, but I'm not sure how it follows that this will never be true...

So r holds the amount of bytes read I assume.
If any bytes got read the "if (r)" in 570 will return.
So from line 573 on r is zero. How can nr_is_stun_message() on an empty buffer (I assume bytes from a previous partial read are not stored in this buffer) ever be true?
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review103540

I think changing it be null is the conservative and right thing.. r+ with that
Attachment #8823470 - Flags: review?(mcmanus) → review+
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> So r holds the amount of bytes read I assume.
> If any bytes got read the "if (r)" in 570 will return.
> So from line 573 on r is zero. How can nr_is_stun_message() on an empty buffer (I assume bytes from a previous partial read are not stored in this buffer) ever be true?

r holds the error code. The previous code was totally wrong. (yikes)
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> r holds the error code. The previous code was totally wrong. (yikes)

I should have known better. This make more sense now.
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9a5828b9c89
Part 1: TLS support in the test ICE server. r=drno
https://hg.mozilla.org/integration/autoland/rev/341ba823f532
Part 2: Test-case for TURN TLS. r=drno
https://hg.mozilla.org/integration/autoland/rev/0eb5a5b7809b
Part 3: Make it possible to configure TURN TLS servers in nICEr. r=drno
https://hg.mozilla.org/integration/autoland/rev/8689021c7d76
Part 4: Handle turns URLs when configuring nICEr. r=drno
https://hg.mozilla.org/integration/autoland/rev/c100d0ad4d4a
Part 5: Open TLS sockets when communicating with a TLS endpoint. r=drno
https://hg.mozilla.org/integration/autoland/rev/a78433dca4bb
Part 6: Fix bug where TCPSocketParent would not use TLS when requested. r=mcmanus
Hey,

would it be possible to schedule this release for mozilla52 instead? We made the experience that in Enterprise Environments (FF ESR & MacAffee WebGateway Proxies & Firewall on Symmetric NAT) FireFox behaves very unstable. We suspect that TURN over TLS would help a lot in these Situations (at lease A/B tests with Chrome seem to support this). Almost all of our customers do deploy FF ESR in their ecosystem. If the feature could be scheduled one release earlier, we could see it in the upcoming major ESR Release. Later releases would probably delay the feature for restrictive enterprises for a complete year ....

Thanks a lot
Byron, Maire -- we noticed this feature on the Fx53 tracking board. Is manual testing required for it?
Flags: qe-verify?
Flags: needinfo?(mreavy)
Flags: needinfo?(docfaraday)
Yes, this will require manual testing (unfortunately). I'll send you mail with a test case.
Flags: needinfo?(docfaraday)
Flags: needinfo?(mreavy)
(In reply to pantherdesign from comment #54)
> would it be possible to schedule this release for mozilla52 instead? We made
> the experience that in Enterprise Environments (FF ESR & MacAffee WebGateway
> Proxies & Firewall on Symmetric NAT) FireFox behaves very unstable. We
> suspect that TURN over TLS would help a lot in these Situations (at lease
> A/B tests with Chrome seem to support this). Almost all of our customers do
> deploy FF ESR in their ecosystem. If the feature could be scheduled one
> release earlier, we could see it in the upcoming major ESR Release. Later
> releases would probably delay the feature for restrictive enterprises for a
> complete year ....

Chromes implementation is different in lots of other ways, so just the fact that Chrome is more successful in connecting is not very convincing to me that this particular feature makes the difference. It could be, but could also be something else.
Could you please take Firefox 53 (or 54) and test if it helps at all and report back?
Flags: needinfo?(pantherdesign)
(In reply to Byron Campen [:bwc] from comment #56)
> Yes, this will require manual testing (unfortunately). I'll send you mail
> with a test case.

Any update on this?
Flags: needinfo?(docfaraday)
Sent mail just now.
Flags: needinfo?(docfaraday)
Flags: qe-verify? → qe-verify+
QA Contact: alexandru.simonca
I used Firefox Beta 53.0b6 with the pref browser.tabs.remote.autostart set to true (to be sure that the browser has e10s enabled) on Windows 10 x64, Mac OS X 10.12.4 & Ubuntu 16.04 LTS x64. 
This issue is VERIFIED FIXED.
You can check the steps I followed and the results at the following link:
 - https://public.etherpad-mozilla.org/p/TURN-TLS
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
Guess that this patch introduced an issue: in Firefox version 53 and above create RTCPeerConnection with *turns* scheme and IP address throws exception. From Developer Tools Console:
---
new RTCPeerConnection({iceServers:[{urls:["turns:127.0.0.1:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
"[Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/components/PeerConnection.js :: __init :: line 457"  data: no]"
TURNS is not yet supported.  PeerConnection.js:719
---

*turns* scheme works with hostname:
---
new RTCPeerConnection({iceServers:[{urls:["turns:localhost:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
RTCPeerConnection { _innerObject: Object, localDescription: null, remoteDescription: null, signalingState: "stable", canTrickleIceCandidates: null, iceGatheringState: "new", iceConnectionState: "new", peerIdentity: Promise, idpLoginUrl: null, onnegotiationneeded: null }
TURNS is not yet supported.  PeerConnection.js:719
---

and *turn* scheme works with IP address and hostname as well:
---
new RTCPeerConnection({iceServers:[{urls:["turn:127.0.0.1:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
RTCPeerConnection { _innerObject: Object, localDescription: null, remoteDescription: null, signalingState: "stable", canTrickleIceCandidates: null, iceGatheringState: "new", iceConnectionState: "new", peerIdentity: Promise, idpLoginUrl: null, onnegotiationneeded: null }
---

The issue appears to be only if *turns* scheme is combined with IP address.
Got following logs from the latest nightly - 55.0a1 (2017-04-13) (64-bit):

---
2017-04-13 14:42:41.043947 UTC - [Main Thread]: I/signaling [main|PeerConnectionImpl] PeerConnectionImpl.cpp:331: PeerConnectionImpl: PeerConnectionImpl constructor for 
2017-04-13 14:42:41.043972 UTC - [Main Thread]: D/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:184: Created PeerConnection: 0x7fc306ed8260
2017-04-13 14:42:41.046490 UTC - [Main Thread]: D/signaling [main|PeerConnectionCtx] PeerConnectionCtx.cpp:142: Creating PeerConnectionCtx
2017-04-13 14:42:41.046897 UTC - [Main Thread]: D/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:296: 13e89bbe6b9ca257: Get stun addresses via IPC
2017-04-13 14:42:41.047035 UTC - [Socket Thread]: D/mtransport NrIceCtx static call to find local stun addresses

2017-04-13 14:42:41.047243 UTC - [Main Thread]: E/mtransport Couldn't set TURN server for 'PC:1492094561046446 (id=6442450947 url=about:home)'
2017-04-13 14:42:41.047259 UTC - [Main Thread]: E/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:414: Init: Failed to set turn servers
2017-04-13 14:42:41.047265 UTC - [Main Thread]: E/signaling [main|PeerConnectionImpl] PeerConnectionImpl.cpp:676: Initialize: Couldn't initialize media object

2017-04-13 14:42:41.058659 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:209: OnProxyAvailable: Proxy Available: 0
2017-04-13 14:42:41.058672 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:226: SetProxyOnPcm: Had proxyinfo
2017-04-13 14:42:41.058717 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:267: OnStunAddrsAvailable: receiving (5) stun addrs
---

hope this helps. Do you want me to create a new bug for this?
turns with an ip address is very very unlikely to work unless you have a signed certificate for an ip address...
That's right. Its a rare case, but still valid. And even if it's not implemented Firefox should not throw an exception, should it?
Before version 53 there was just a warning that TURNS is not supported. Now TURNS is supported, but Firefox throws an exception for URL with IP address. I think it will be more consistent to keep the warning instead.
Moreover in my case there are 3 URLs in configuration - for all transports UDP, TCP and TLS:
---
{iceServers:[{
  urls:[
    "turn:127.0.0.1:3478?transport=udp",
    "turn:127.0.0.1:443?transport=tcp",
    "turns:127.0.0.1:443?transport=tcp"],
  credential:"ABC",
  username:"test@example.com:12345"
}]}
---
and because the last one is not supported Firefox throws exception and cannot connect.

My point is, it's great that TURNS is implemented now. I really like it, but it should not break applications that used to work without TURNS support prior version 53.
With any new feature comes new potential error conditions related to that feature, and using an IP address with turns is just broken.
Not exactly. TURNS didn't worked in previous version, so it cannot be broken.
What is broken (in example above) is TURN UDP and TCP with IP address. TURNS with IP address is just not allowed, but brakes the other "acceptable" TURN transports, which worked with version 52.

Your statement sounds like you agree with me that this is a bug. Right? So since this bug is closed I guess the best option is to create a new one. What do you think?
Trying to use TLS with an IP address is terrible enough that it warrants a full stop, IMHO. You need to stop doing this.
When I go to https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with authentication, everything looks good on the candidates report (there is a relay candidate with the server's IP), but on the Javascript debug console, I get:

TURNS is not yet supported.  main.js:119

This is with Firefox 54.  I'm not sure why the error comes up on the Trickle ICE page, as the WebRTC application I am using is definitely doing relaying over TLS with the same ICE configuration, with no errors.

Thanks.
Blocks: 1383575
(In reply to michael from comment #68)
> When I go to
> https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
> and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with
> authentication, everything looks good on the candidates report (there is a
> relay candidate with the server's IP), but on the Javascript debug console,
> I get:
> 
> TURNS is not yet supported.  main.js:119
> 
> This is with Firefox 54.  I'm not sure why the error comes up on the Trickle
> ICE page, as the WebRTC application I am using is definitely doing relaying
> over TLS with the same ICE configuration, with no errors.
> 
> Thanks.

The JS console log warning is a left over from this bug. I opened bug 1383575 for you to remove the warning.
(In reply to michael from comment #68)
> When I go to
> https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
> and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with
> authentication, everything looks good on the candidates report (there is a
> relay candidate with the server's IP), but on the Javascript debug console,
> I get:
> 
> TURNS is not yet supported.  main.js:119
> 
> This is with Firefox 54.  I'm not sure why the error comes up on the Trickle
> ICE page, as the WebRTC application I am using is definitely doing relaying
> over TLS with the same ICE configuration, with no errors.

The reason it is not working is that turns.fig.org doesn't even establish a TCP connection on port 443.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: