<p style="white-space: pre-wrap; word-wrap: break-word;">I've prepared a next iteration on this but we need consensus on whether or not peer5 should be matched purely by IP or not.  I think on everything else we agree.</p><p><a href="https://gerrit.asterisk.org/9858">View Change</a></p><p>24 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@26">Patch Set #6, Line 26:</a> <code style="font-family:monospace,monospace">transports).</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/is/are/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@33">Patch Set #6, Line 33:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/the there/there/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@61">Patch Set #6, Line 61:</a> <code style="font-family:monospace,monospace">2 - Incoming UDP request from 1.1.1.1:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/TCP/UDP/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@95">Patch Set #6, Line 95:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@98">Patch Set #6, Line 98:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">- current: as per previous</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@103">Patch Set #6, Line 103:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@109">Patch Set #6, Line 109:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@114">Patch Set #6, Line 114:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">- new: as per previous</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@122">Patch Set #6, Line 122:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@131">Patch Set #6, Line 131:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@135">Patch Set #6, Line 135:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@140">Patch Set #6, Line 140:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">As I understand the purpose here we're looking for a peer that can be used to auth against based on IP:port.  Not based on name.  If we find peer5 here insecure=invite could apply else auth will be required.  Even if we don't find anything here it's still possible that peer5 will be found during the name search by name phase?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Finding peer5 here will regress again for https://issues.asterisk.org/jira/browse/ASTERISK-27457 - which is what introduced the patch specifically to eliminate this in the first place.</p><p style="white-space: pre-wrap; word-wrap: break-word;">My personal opinion is that we should not match peer5 here.  In my experience with SIP/TCP (majority Yealink and SNOM, but also some ) UACs will open a single TCP connection, issue REGISTER and keep the connection open, eliminating the problem you're referring to.  If the endpoint is behind NAT the administrator will already be setting options in asterisk to deal with NAT eliminating your concerns (as far as I understand).</p><p style="white-space: pre-wrap; word-wrap: break-word;">To make matters worse for Alexander, if he also happens to set insecure=invite, the "guest" user will automatically authenticate as the registered from the same IP peer.  This wasn't even raised in that issue.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@145">Patch Set #6, Line 145:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@148">Patch Set #6, Line 148:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@152">Patch Set #6, Line 152:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See above.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@157">Patch Set #6, Line 157:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@160">Patch Set #6, Line 160:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@163">Patch Set #6, Line 163:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">pass 1: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See above.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@166">Patch Set #6, Line 166:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/doesn't/don't/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34457">Patch Set #5, Line 34457:</a> <code style="font-family:monospace,monospace">           return peer2->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The best you can do for the second pass that matches by IP address only is to just return "match" he […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This will break the use case again for Alexander, and is what the original patch tried to address.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Based on what I've seen (obviously limited experience) TCP based UACs will open a TCP connection to the server, and utilize that single TCP connection for all further SIP communications.  For this reason I believe that this change is safe.  For TCP communications, especially when behind NAT, the REGISTER address means nothing and we won't be able to establish a new tcp connection to that anyway and we need to re-use the connection on which the REGISTER came in anyway.</p><p style="white-space: pre-wrap; word-wrap: break-word;">What's the risk if we miss a match?  I'm guessing we could reject connections that in the current iteration of the code is being rejected anyway.  Based on feedback from Alexander (for whom current apparently is working) he's seeing the same thing that I'm seeing.  Specifically he mentioned to me in offline email that the current code is working for him, but he doesn't have static hosts configured.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The above code is my best-effort attempt at both re-enabling the previous behaviour for static TCP hosts, as well as enabling guests over TCP for Alexander from hosts where there already exists registrations on.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also consider the case where there are multiple registrations coming from the same IP address.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34460">Patch Set #5, Line 34460:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static int peer_ipcmp_cb(void *obj, void *arg, int flags)<br>{<br>     return peer_ipcmp_cb_full(obj, arg, NULL, flags);<br>}<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is dead code and needs to be removed. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not dead.  Used in load_module function to create the peers_by_ip container.  This container is used in various places and I don't feel comfortable enough to eliminate those uses.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34446">Patch Set #6, Line 34446:</a> <code style="font-family:monospace,monospace">       * for non-UDP the source port can't typically be controlled, we only want</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"can't" was a better word before where you have "won't" now.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No.  Alexander pointed out that it is possible for a TCP connection to be source-port controlled (in fact, I realized FTP relied on that for using source port 21 for return TCP connections).</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I think won't is the better word choice here since technically it can be controlled.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34458">Patch Set #6, Line 34458:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">For the reasons I stated previously you need to just return CMP_MATCH here. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please refer my comments and explanation in the commit message.  The question really is what's the desired behaviour and I suspect the new behaviour is.  If IP matching fails then hopefully we'll name match and still request authentication.  Else it's down to guest land, and if guest is disabled, it's back to authentication.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34461">Patch Set #6, Line 34461:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">{<br>    return peer_ipcmp_cb_full(obj, arg, NULL, flags);<br>}<br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Delete this function for the reasons I stated previously.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's still in use in load_module() for the peers_by_ip container (which is used in various places).  I'm not comfortable making this modification.  From what I can tell this is merely a wrapper function that simply provides a default argument, so I also don't get why it's considered confusing (maybe that's the whole point).</p></li></ul></li></ul><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: comment </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: 5 </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>
<div style="display:none"> Gerrit-Comment-Date: Thu, 23 Aug 2018 11:15:49 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>