<p>Jaco Kroon <strong>uploaded patch set #8</strong> to this change.</p><p><a href="https://gerrit.asterisk.org/9858">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">chan_sip: improved ip:port finding of peers for non-UDP transports.<br><br>Also remove function peer_ipcmp_cb since it's not used (according to<br>rmudgett).<br><br>Prior to b2c4e8660a9c89d07041271371151779b7ec75f6 (ASTERISK_27457)<br>insecure=port was the defacto standard. That commit also prevented<br>insecure=port from being applied for sip/tcp or sip/tls.<br><br>Into consideration there are three sets of behaviour:<br><br>1. "previous" - before the above commit.<br>2. "current" - post above commit, pre this one.<br>3. "new" - post this commit.<br><br>The problem that the above commit tried to address was guests over TCP.<br>It succeeded in doing that but broke transport!=udp with host!=dynamic.<br><br>This commit attempts to restore sane behaviour with respect to<br>transport!=udp for host!=dynamic whilst still retaining the guest users<br>over tcp.<br><br>It should be noted that when looking for a peer, two passes are made, the<br>first pass doesn't have SIP_INSECURE_PORT set for the searched-for peer,<br>thus looking for full matches (IP + Port), the second pass sets<br>SIP_INSECURE_PORT, thus expecting matches on IP only where the matched<br>peer allows for that (in the author's opinion: UDP with insecure=port,<br>or any TCP based, non-dynamic host).<br><br>In previous behaviour there was special handling for transport=tcp|tls<br>whereby a peer would match during the first pass if the utilized<br>transport was TCP|TLS (and the peer allowed that specific transport).<br><br>This behaviour was wrong, or dubious at best. Consider two dynamic tcp<br>peers, both registering from the same IP (NAT), in this case either peer<br>could match for connections from an IP. It's also this behaviour that<br>prevented SIP guests over tcp.<br><br>The above referenced commit removed this behaviour, but kept applying<br>the SIP_INSECURE_PORT only to WS|WSS|UDP. Since WS and WSS is also TCP<br>based, the logic here should fall into the TCP category.<br><br>This patch updates things such that the previously non-explicit (TCP<br>behaviour) transport test gets performed explicitly (ie, matched peer<br>must allow for the used transport), as well as the indeterministic<br>source-port nature of the TCP protocol is taken into account. The new<br>match algorithm now looks like:<br><br>1. As per previous behaviour, IP address is matched first.<br><br>2. Explicit filter with respect to transport protocol, previous<br> behaviour was semi-implied in the test for TCP pure IP match - this now<br> made explicit.<br><br>3. During first pass (without SIP_INSECURE_PORT), always match on port.<br><br>4. If doing UDP, match if matched against peer also has<br> SIP_INSECURE_PORT, else don't match.<br><br>5. Match if not a dynamic host (for non-UDP protocols)<br><br>6. Don't match if this is WS|WSS, or we can't trust the Contact address<br> (presumably due to NAT)<br><br>7. Match (we have a valid Contact thus if the IP matches we have no<br> choice, this will likely only apply to non-NAT).<br><br>To logic-test this we need a few different scenarios. Towards this end,<br>I work with a set number of peers defined in sip.conf:<br><br>[peer1]<br>host=1.1.1.1<br>transport=tcp<br><br>[peer2]<br>host=1.1.1.1<br>transport=udp<br><br>[peer3]<br>host=1.1.1.1<br>port=5061<br>insecure=port<br>transport=udp<br><br>[peer4]<br>host=1.1.1.2<br>transport=udp,tcp<br><br>[peer5]<br>host=dynamic<br>transport=udp,tcp<br><br>Test cases for UDP:<br><br>1 - incoming UDP request from 1.1.1.1:<br> - previous:<br> - pass 1:<br> * peer1 or peer2 if from port 5060 (indeterminate, depends on peer<br> ordering)<br> * peer3 if from port 5061<br> * peer5 if registered from 1.1.1.1 and source port matches<br> - pass 2:<br> * peer3<br> - current: as per previous.<br> - new:<br> - pass 1:<br> * peer2 if from port 5060<br> * peer3 if from port 5061<br> * peer5 if registered from 1.1.1.1 and source port matches<br> - pass 2:<br> * peer3<br><br>2 - incoming UDP request from 1.1.1.2:<br> - previous:<br> - pass 1:<br> * peer5 if registered from 1.1.1.2 and port matches<br> * peer4 if source port is 5060<br> - pass 2:<br> * no match (guest)<br> - current: as previous.<br> - new as previous (with the variation that if peer5 didn't have udp as<br> allowed transport it would not match peer5 whereas previous<br> and current code could).<br><br>3 - incoming UDP request from anywhere else:<br> - previous:<br> - pass 1:<br> * peer5 if registered from that address and source port matches.<br> - pass 2:<br> * peer5 if insecure=port is additionally set.<br> * no match (guest)<br> - current - as per previous<br> - new - as per previous<br><br>Test cases for TCP based transports:<br><br>4 - incoming TCP request from 1.1.1.1<br> - previous:<br> - pass 1 (indeterministic, depends on ordering of peers in memory):<br> * peer1; or<br> * peer5 if peer5 registered from 1.1.1.1 (irrespective of source port); or<br> * peer2 if the source port happens to be 5060; or<br> * peer3 if the source port happens to be 5061.<br> - pass 2: cannot happen since pass 1 will always find a peer.<br> - current:<br> - pass 1:<br> * peer1 or peer2 if from source port 5060<br> * peer3 if from source port 5060<br> * peer5 if registered as 1.1.1.1 and source port matches<br> - pass 2:<br> * no match (guest)<br> - new:<br> - pass 1:<br> * peer 1 if from port 5060<br> * peer 5 if registered and source port matches<br> - pass 2:<br> * peer 1<br><br>5 - incoming TCP request from 1.1.1.2<br> - previous (indeterminate, depends on ordering):<br> - pass 1:<br> * peer4; or<br> * peer5 if peer5 registered from 1.1.1.2<br> - pass 2: cannot happen since pass 1 will always find a peer.<br> - current:<br> - pass 1:<br> * peer4 if source port is 5060<br> * peer5 if peer5 registered as 1.1.1.2 and source port matches<br> - pass 2:<br> * no match (guest).<br> - new:<br> - pass 1:<br> * peer4 if source port is 5060<br> * peer5 if peer5 registered as 1.1.1.2 and source port matches<br> - pass 2:<br> * peer4<br><br>6 - incoming TCP request from anywhere else:<br> - previous:<br> - pass 1:<br> * peer5 if registered from that address<br> - pass 2: cannot happen since pass 1 will always find a peer.<br> - current:<br> - pass 1:<br> * peer5 if registered from that address and port matches.<br> - pass 2:<br> * no match (guest)<br> - new: as per current.<br><br>It should be noted the test cases don't make explicit mention of TLS, WS<br>or WSS. WS and WSS previously followed UDP semantics, they will now<br>enforce source port matching. TLS follow TCP semantics.<br><br>The previous commit specifically tried to address test-case 6, but broke<br>test-cases 4 and 5 in the process.<br><br>ASTERISK-27881 #close<br><br>Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2<br>---<br>M channels/chan_sip.c<br>1 file changed, 44 insertions(+), 22 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/58/9858/8</pre><p>To view, visit <a href="https://gerrit.asterisk.org/9858">change 9858</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9858"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newpatchset </div>
<div style="display:none"> Gerrit-Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2 </div>
<div style="display:none"> Gerrit-Change-Number: 9858 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>