2011209 - Following an issue encountered when using TURNS servers behind a proxy

Status: UNCONFIRMED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Samuel Bélondrade [:sbelondr]]

Attachment: webrtc.zip

Steps to reproduce:

• Set up Proxy in Firefox
• Try to connect in the Turns server with ICE (snapcall)
• In the network panel, I've this error : SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID

Actual results:

Following an issue encountered when using TURN servers behind a proxy, we discovered that Snapcalls via TCP are no longer possible starting with Firefox version 128.

After investigation, we observed that since this version, http/1.1 is sent in the ALPN field of the Client Hello. This causes the error shown in the screenshot:
SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID.

Previously, the http/1.1 value was not set in the code and did not trigger this error.

Expected results:

In accordance with RFC 8833, I therefore propose adding a condition so that when the request is a WebRTC request, we set webrtc,c-webrtc instead of http/1.1 in the file TlsHandshaker.cpp, in the SetupNPNList function.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::WebRTC: Audio/Video' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Samuel Bélondrade [:sbelondr]]

Attachment: WIP: Bug 2011209 - Fix ALPN field

Comment 3

[Byron Campen [:bwc]]

Thoughts?

Comment 4

[Kershaw Chang [:kershaw]]

Thanks for the patch.

I was wondering why this only happens when a proxy is used. When no proxy is involved, I believe the TLS handshake is handled by the WebRTC code, since I don’t see alpn included in the TLS ClientHello. When a proxy is used, we rely on TLSHandshaker to do handshake, and we always include http/1.1 in the ALPN list.

In any case, the patch looks good to me.
Reporter, would you like to start the review process of your patch?

Comment 5

[Samuel Bélondrade [:sbelondr]]

Thanks for you reply.

Yes, I would like start the review process for my patch.

Comment 6

[Samuel Bélondrade [:sbelondr]]

Thanks for you reply.

Yes, I would like start the review process for my patch.

Comment 7

[Byron Campen [:bwc]]

So RFC 8833 is solely about webrtc DTLS handshakes, which is not what this bug relates to. I don't see anything about using webrtc/c-webrtc in TLS connections to TURN servers. The only thing I'm seeing related to ALPN and TURN is RFC 7443.

https://datatracker.ietf.org/doc/html/rfc7443

That specifies stun.turn and stun.nat-discovery for ALPN.

Are you aware of any IETF document that specifies the use of webrtc/c-webrtc on TLS connections (as opposed to DTLS)?

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

bwc, kershaw, given the conversation above I have marked this bug as P1, S3. Please feel free to change those values if you disagree with them.

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Byron Campen [:bwc] from comment #7)

So RFC 8833 is solely about webrtc DTLS handshakes, which is not what this bug relates to. I don't see anything about using webrtc/c-webrtc in TLS connections to TURN servers. The only thing I'm seeing related to ALPN and TURN is RFC 7443.

https://datatracker.ietf.org/doc/html/rfc7443

That specifies stun.turn and stun.nat-discovery for ALPN.

Are you aware of any IETF document that specifies the use of webrtc/c-webrtc on TLS connections (as opposed to DTLS)?

Thanks for pointing this out. I looked into this again and couldn’t find any documentation stating that we should include webrtc,c-webrtc in TLS connections.

I then compared the ClientHello messages between Firefox and Chrome when a proxy is used. One difference I noticed is that Chrome does not include http/1.1 in the ALPN field, while Firefox does. After removing http/1.1, Firefox also works correctly.

So it seems that including neither http/1.1 nor webrtc,c-webrtc may be the correct approach. Note that when a proxy is not used, Firefox also sends a ClientHello without any ALPN, so I think we should send the same ClientHello to the server regardless of whether a proxy is used.

Comment 10

[Samuel Bélondrade [:sbelondr]]

(In reply to Kershaw Chang [:kershaw] from comment #9)

(In reply to Byron Campen [:bwc] from comment #7)

So RFC 8833 is solely about webrtc DTLS handshakes, which is not what this bug relates to. I don't see anything about using webrtc/c-webrtc in TLS connections to TURN servers. The only thing I'm seeing related to ALPN and TURN is RFC 7443.

https://datatracker.ietf.org/doc/html/rfc7443

That specifies stun.turn and stun.nat-discovery for ALPN.

Are you aware of any IETF document that specifies the use of webrtc/c-webrtc on TLS connections (as opposed to DTLS)?

Thanks for pointing this out. I looked into this again and couldn’t find any documentation stating that we should include webrtc,c-webrtc in TLS connections.

I then compared the ClientHello messages between Firefox and Chrome when a proxy is used. One difference I noticed is that Chrome does not include http/1.1 in the ALPN field, while Firefox does. After removing http/1.1, Firefox also works correctly.

So it seems that including neither http/1.1 nor webrtc,c-webrtc may be the correct approach. Note that when a proxy is not used, Firefox also sends a ClientHello without any ALPN, so I think we should send the same ClientHello to the server regardless of whether a proxy is used.

I confirm that it works when removing the line protocolArray.AppendElement("http/1.1"_ns);.

Indeed, the RFC only specifies this for DTLS, but wouldn't it be consistent to keep the same logic for TLS ?

Comment 11

[Byron Campen [:bwc]]

The purpose of alpn is to signal the protocol that will be used in a not-yet-setup TLS/DTLS session. So, if the DTLS/TLS client is a webrtc endpoint, and is attempting to handshake with another webrtc endpoint, then using webrtc/c-webrtc is appropriate. However, if the DTLS/TLS client is trying to do TURN, and attempting to handshake with a TURN server, the wire protocol between the two is TURN, not webrtc; while webrtc packets may be routed through that association, the TURN server does not know or care about that, it just relays packets according to the TURN protocol. So, the appropriate alpn would be "stun.turn" in that case.

Comment 12

[Samuel Bélondrade [:sbelondr]]

Attachment: WIP: Bug 2011209 - add the WebRTCTURN flag to HTTP channels to modify ALPN negotiation to advertise stun.turn for WebRTC TURN server connections

Comment 13

[Samuel Bélondrade [:sbelondr]]

Hi,
I sent a patch a month ago following Byron's feedback. Is it possible to get some feedback on it?

Comment 14

[Byron Campen [:bwc]]

I'm a little unclear on why this is no longer having any effect:

https://searchfox.org/firefox-main/source/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp#4782

We're still asking for "webrtc,c-webrtc", but the TLS handshake is now using "http/1.1". Mind you, neither is right, but we've switched from one wrong thing to another. If we can fix whatever caused this config to stop working, and restore our old behavior, that should undo this regression. I think the first patch does this?

Then, it may be appropriate to attempt using "stun.turn" instead, maybe Nightly-only pref-controlled in order to mitigate interop problems (I don't know whether TURN TLS implementations will break with "stun.turn"). I suppose that would involve adding a val.Equals("stun.turn"_ns) check in there?

At a higher level, it seems like we need a function that handles all of the alpns necko is willing to honor. I do not know what that set is, or should be.

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Byron Campen [:bwc] from comment #14)

I'm a little unclear on why this is no longer having any effect:

https://searchfox.org/firefox-main/source/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp#4782

That’s because necko does not use that value in the TLS handshake. The value is only used in the Upgrade and ALPN headers.

We're still asking for "webrtc,c-webrtc", but the TLS handshake is now using "http/1.1". Mind you, neither is right, but we've switched from one wrong thing to another. If we can fix whatever caused this config to stop working, and restore our old behavior, that should undo this regression. I think the first patch does this?

Yeah, the first patch should work. However, please note that webrtc,c-webrtc is not specified in any spec, unless I’m mistaken. I’d prefer to align our behavior with Chrome, which uses an empty string in this case.

Then, it may be appropriate to attempt using "stun.turn" instead, maybe Nightly-only pref-controlled in order to mitigate interop problems (I don't know whether TURN TLS implementations will break with "stun.turn"). I suppose that would involve adding a val.Equals("stun.turn"_ns) check in there?

Agreed. We could try this.

At a higher level, it seems like we need a function that handles all of the alpns necko is willing to honor. I do not know what that set is, or should be.

We currently do not have that set, since the logic here is quite simple. For TCP, we only allow http/1.1 or h2.

Comment 16

[Byron Campen [:bwc]]

An empty string is more reasonable than the webrtc alpns. If that's what other browsers do, that seems like the best first step.