search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8k stars 2.45k forks source link

Wrong extension id for encrypted headers #1575

Closed fancycode closed 5 years ago

fancycode commented 5 years ago

In some cases, Janus is sending an answer with a wrong extension id if encrypted extension headers are used. You can test this by enabling the flag "Negotiation with encrypted header extensions for SRTP in WebRTC" in chrome://flags.

Offer from Chrome:

...
m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99 100 101 102 122 127 121 125 107 108 109 124 120 123
...
a=extmap:2 urn:ietf:params:rtp-hdrext:toffset
a=extmap:3 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:4 urn:3gpp:video-orientation
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:6 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a=extmap:7 http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
a=extmap:8 http://www.webrtc.org/experiments/rtp-hdrext/video-timing
a=extmap:10 http://tools.ietf.org/html/draft-ietf-avtext-framemarking-07
a=extmap:12 http://www.webrtc.org/experiments/rtp-hdrext/color-space
a=extmap:13 urn:ietf:params:rtp-hdrext:encrypt urn:ietf:params:rtp-hdrext:toffset
a=extmap:11 urn:ietf:params:rtp-hdrext:encrypt urn:3gpp:video-orientation
a=extmap:9 urn:ietf:params:rtp-hdrext:encrypt http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:0 urn:ietf:params:rtp-hdrext:encrypt http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a=extmap:0 urn:ietf:params:rtp-hdrext:encrypt http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
...

Answer from Janus:

...
m=video 9 UDP/TLS/RTP/SAVPF 96
...
a=extmap:11 urn:3gpp:video-orientation
a=extmap:0 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
...

Note that the extensions got mapped to the id of the encrypted version from the offer (that has the urn:ietf:params:rtp-hdrext:encrypt as prefix before the actual extension.

The problem is caused by the code that checks if the extension name is part of the m-line attribute in https://github.com/meetecho/janus-gateway/blob/ce8255e56b4a1cecc992cd55633bcdb1ef88cfe7/plugins/janus_videoroom.c#L5893

In this case this matches both the unencrypted and the encrypted extension and the last matching id (here the encrypted) is used. (On a side note, the id 0 means that an unused number should be assigned in the answer, so replying with extmap:0 is invalid, too).

When setting the answer, Chrome fails with "Failed to set remote answer sdp: Failed to set remote video description send parameters.".

lminiero commented 5 years ago

We don't support encrypted RTP extensions yet, and they require ad-hoc negotiation as you pointed out. I'll keep the issue open as a memo that we need to implement it, but I'm not sure when that will happen: AFAIK they're still hidden behind a flag anyway.

fancycode commented 5 years ago

It's fine to not support encrypted extensions, that's why Chrome is offering the same extensions both encrypted and unencrypted. However it's not allowed to answer with the unencrypted extension but with the id of the encrypted extension:

Offer:

a=extmap:4 urn:3gpp:video-orientation
a=extmap:11 urn:ietf:params:rtp-hdrext:encrypt urn:3gpp:video-orientation

Answer:

a=extmap:11 urn:3gpp:video-orientation

The solution for now would be to ignore all encrypted extensions (that contain urn:ietf:params:rtp-hdrext:encrypt) and not use their ids for the answer.

fancycode commented 5 years ago

Thinking more about this, re-assigning ids should be fine (i.e. using 11 or any other number for urn:3gpp:video-orientation in my example), but replying with an id of 0 is not. For now the fix would be to ignore the encrypted extensions (which luckily are all using the dynamic id 0). However if Chrome would change the order in the SDP and use assigned ids for the encrypted extensions and 0 for the unencrypted, this would no longer work and you would need to assign an id in Janus for the answer.

fancycode commented 5 years ago

Fix available in #1581.

lminiero commented 5 years ago

Closing as it should be fixed in the commit above.