meetecho / janus-gateway

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

Sip plugin, Re-Invite from remote part and Chrome DTMF #1591

Closed StephaneG31 closed 5 years ago

StephaneG31 commented 5 years ago

Hi, I will try to explain a problem in our use case. We use Janus just as a SIP Gateway to join some conference call. So we need to use DTMF. With Firefox it works, DTMF also.

With Chrome, DTMF are ignored or not received by our audio conf call bridge.

UserAgent <=> Janus <=> SBC....SBC <=> AudioBridge Application Server

When negotation SDP, Chrome for DTMF send a=rtpmap:126 telephone-event/8000

Audio bridge accepts. But Audio Bridge workflow are configured for sending re-INVITE to "renegociate" some SDP, and audio bridges and send in the re-INVITE packet a=rtpmap:101 telephone-event/8000

It's the same SDP type but payload type ID is different (we don't know why the audio bridge of our partner work like this and we can't change this).

The re-INVITE is received by Janus and Janus send juste un ACCEPT message without forwarding the re-INVITE to browser to "renegociate".

So after that Chrome keeps initial INVITE, so DTMF are sent with payload type 126, and we think (but we didn't make a capture on our mutulized SBC) that SBC will drop this kind of packet which are not ACCEPT in any INVITE or RE-INVITE.

I think in the code the interesting lines are from here : https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L3498

Maybe if one of SDP in a re-Invite message is different from initial negociation, Janus must forward request to the browser ?

I think Janus detect SDP changed with the changedvariable https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L3481, but this one seems to not be use after

lminiero commented 5 years ago

I don't remember exactly how we integrated the renegotiation support in the SIP plugin, but we do forward the SDP if there are media changes IIRC (e.g., you add or remove audio/video), and don't otherwise (e.g., for onhold we don't). It's very likely that this change in payload type doesn't trigger that renegotiation towards the browser, and I honestly don't know if the browser would accept the renegotiation if we did.

I'll add to the TODO list the fact we have to check what's happening there. In the meanwhile, there are a couple of things you can do to mitigate the issue until this is solved:

  1. with the SIP plugin you can send DTMF tones via SIP INFO as well; if your bridge supports them, you can use those as a fallback for the time being;
  2. another solution is to force 101 as payload type in the SDP you generate; after the createOffer, and before the setLocalDescription, replace 126 with 101 in the SDP string, and that should do the trick (should hopefully prevent the renegotiation, if that's the cause).
StephaneG31 commented 5 years ago

@lminiero thanks. We try a brutal patch, before https://github.com/meetecho/janus-gateway/blob/master/html/janus.js#L2548 we add

 offer.sdp = offer.sdp.replace("a=rtpmap:126", "a=rtpmap:101");
 offer.sdp = offer.sdp.replace("113 126", "113 101");

and it works !!!!

lminiero commented 5 years ago

Good to know that small hack does the job! I'll keep note of the fact we need to fix this renegotiation issue in Janus itself, though.

lminiero commented 5 years ago

Mh, looks like we're not just answering ourselves instead of involving the browser when nothing changes, as I remembered: we always do that for re-INVITEs. Looking back at #1099 (which is where we added support for renegotiations) it's apparently because we implemented support for sending re-INVITEs, but kept the previous behaviour for incoming re-INVITEs instead (just send a 200 OK back and ignore).

While the fix should be relatively "easy", it will involve a new event for the SIP plugin, one that existing applications may not be able to support, at the risk of breaking media sessions (they'd never answer incoming re-INVITEs properly). As such, I'll probably do this in a pull request in a backwards compatible way, that is with a new property you can set when starting or accepting a new call (e.g., autoaccept_reinvites) that will default to TRUE: this way, the normal behaviour will be to still automatically send a 200 OK back; if that property is set to FALSE, we'll relay the SDP to the browser with a new event (maybe update or incomingupdate?), who can answer with the existing accept request.

I'll notify here when the PR is ready, so that you can test it and let me know if that works in your specific scenario (without the hack, that is).

StephaneG31 commented 5 years ago

Happy to read this, we use Janus for a very specific scenario and only you can be aware of backwards compatibility. Toggle option is a good way. We will test it as soon as it will be available and after merging to ours others diffs in the SIP plugin. We have made some security mechanisms for signing messages between clients and Janus, we hope one day we can make a pull request (for Sip plugin or maybe a derived plugin of sip plugin).

lminiero commented 5 years ago

Pull requests for that would be more than welcome! If you have anything SIP specific you changed, feel free to anticipate what you're planning to share: I'm going to present the current status of the SIP plugin at the upcoming OpenSIPS summit in a few days, so it might be nice to mention the possible improvements in the talk, especially if it tightens security. FYI, I'll also be at Kamailio World a few days after that, so in case you're going to attend either of those please let me know and we can chat there!

lminiero commented 5 years ago

@StephaneG31 just pushed the PR and it seems to do the trick, please let me know if it works in your application as well once you remove the "hack"!

GerardM22 commented 5 years ago

Hi Lorenzo, It works but after removing a test in janus_sip.c: details in #1597

GerardM22 commented 5 years ago

After the last commit, it works fine for me. Thank you very much !

StephaneG31 commented 5 years ago

Hi @lminero, @GerardM22 and me are in the same team, so we can consider your fix is good in our use case. Thank you. We just waiting you merge it in master to merge our signing message exchange commits. I will try to open an issue to explain ours needs and how we make "a solution".

Before make a pull request, I need to review with my company open source rules how we can make pull request (with company account or with our personal account).

Our use case and implementation are very specific (we sign messages with some shared secret) but maybe you must take the concept and generalize to a more open implementation.

lminiero commented 5 years ago

Is it #1598 you're talking about?

StephaneG31 commented 5 years ago

Great, it's exactly the same needs. We work for a Telco so maybe in future ours gateways maybe very expose to this kind of attacks. I'm happy to read that some others persons have the same security problem as us. We implement that in a little different way with a global shared secret and a "local/client/tenant" secret to available multi-tenant and available tenant "auth".

We add origin (tenant name) header and token in register and call messages. Tenant have a tenant private shared key which is used with the global secret to sign message).

More generalized, it will be great to have a message signing factory/connector (something like that) so you can have a local signing mechanism like in #1598 or delegate that to an other service (like an API with a Redis to store token associated with message).

That's why I write that we implement that Ina very specific way but I think the concept must be generalized with an implement more opened, but I don't think we have time and knowledge of Janus to implement this generalization.

lminiero commented 5 years ago

The problem is that I can't add proprietary stuff to the plugin, that works for some and doesn't for others. In case there's a more generic approach (possibly standard) then it would be better.

lminiero commented 5 years ago

Closing as #1597 has been merged :+1: