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

Allow SIP plugin to bind media to a specific address (see #3248) #3263

Closed pawnnail closed 10 months ago

pawnnail commented 1 year ago

The multi-stream version of the SIP plugin no longer binds RTP/RTCP sockets to a specific address just like the single-stream version used to. Instead, the multi-stream SIP plugin binds to "any" address for IPv4 and IPv6 (if supported). This highlights a potential security risk if deployed on a multihomed host in a hostile environment. Perhaps, is also wasteful as UDP ports are allocated on multiple addresses and a change of behaviour when migrating from a single-stream to multi. The change was added by PR #3159.

I would like the SIP plugin to use a specific address, IPv4 or IPV6, for media (RTP/RTCP) while preserving the option of binding to any interface (PR #3159). The binding address changes driven by the value of configuration parameter _local_mediaip are summarized below.

_local_mediaip single-stream multi-stream multi-stream proposed
\<empty string> not supported not supported any (IPv4 and IPv6)
\<unspecified> _localip (IPv4) any (IPv4 and IPv6) any (IPv4 and IPv6)
"0.0.0.0" any (IPv4) any (IPv4 and IPv6) any (IPv4 and IPv6)
"::" not supported any (IPv4 and IPv6) any (IPv4 and IPv6)
IPv4, e.g. "192.168.171.8" 192.168.171.8 any (IPv4 and IPv6) 192.168.171.8
IPv4-mapped IPv6, e.g. "::ffff:192.168.171.8" not supported any (IPv4 and IPv6) 192.168.171.8
IPVv6, e.g. "2406:da1c:aa9:9401:2825:674b:3e05:fca2" not supported any (IPv4 and IPv6) 2406:da1c:aa9:9401:2825:674b:3e05:fca2

So there is an impact on PR #3159: if _local_mediaip happens to be specified then RTP/RTCP sockets will bind to that address only rather than to all. There is nothing I can do about it other than to warn. I hope the proposal is flexible enough to address most desired configuration scenarios going forward.

Have tested the above scenarios on Linux and Windows WSL.

Have made _sdpip used for the SDP connection address header (c=) to fallback to a valid value of either _local_mediaip or _localip.

Also fixed an issue where bind() intermittently fails due to _audio_rtpaddress and _audio_rtcpaddress not initialized to 0.

pawnnail commented 1 year ago

Thanks for the review, Lorenzo. Will get it sorted within the next few days.

pawnnail commented 1 year ago

There are few more things I would like to do to tidy up port binding but not in the same PR. Arbitrary int attempts = 100 tends to fail often on a busy system, especially in a limited port range especially considering RTCP port needs to be adjacent to RTP port. And then it only makes sense to retry bind() on transient errors like hitting a busy port. But one thing at a time.

Retested again. Haven't done the rebase yet, will do once all the changes are approved. Thanks.

lminiero commented 1 year ago

Thanks, looks good now! I just added a tiny note on the comment format.

lminiero commented 12 months ago

Apologies for the lack of feedback, I've been pretty busy lately and this is an effort I need to study well, especially considering that, if it works, I'll need to port it to the NoSIP plugin as well, besides backporting it to 0.x for both plugins. I'll try to reserve some time later this week to have a deeper look and make some tests. Scream at me if it doesn't happen :laughing:

pawnnail commented 12 months ago

No problems and thanks. I am not in a hurry. Let me know if I can help.

lminiero commented 10 months ago

I'm adapting the patch to 0.x, and then I'll port the changes to NoSIP too. If all goes well, I'll try to merge later today. Apologies again for the delay!

lminiero commented 10 months ago

I'll merge and fix the remaining nits myself, since this has been open quite some time. Thanks again for the contribution and the patience!

pawnnail commented 10 months ago

Thanks!