Hey, Responses inline.
On Tue, Aug 14, 2018 at 6:16 AM, Jaco Kroon <j...@uls.co.za> wrote: > Hi All, > > The following bugs refers: > > ASTERISK-27457 - chan_sip: Guests disallowed via TCP (or TLS) if > existing peer from same IP > commit b2c4e8660a9c89d07041271371151779b7ec75f6 > > ASTERISK-27881 - PBX calls via chan_sip TCP trunk now get > authentification error > change set https://gerrit.asterisk.org/#/c/asterisk/+/9858/ > > The first change seems to be part of a potentially larger patch set, I > only looked at the specific commit I referenced. > > The original code assumed that TCP|TLS peers from the same IP was the > same peer. I don't think this applies to authenticated connections, but > I could be wrong. > > The peer_ipcmp_cb{,_full} is used to find peers by IP, so I'm *guessing* > to authenticate them based on IP address. > > Given that assumption I believe the old behaviour was correct in that > any TCP based protocol can't control the source port. The check should > probably also have included WS and WSS protocols since those are TCP > based too. > > The effect was (as I understand) that the original code, given a peer like: > > [peer1] > host=a.b.c.d > transport=udp,tcp > > Really could either accept a new "call" from IP a.b.c.d over a TCP > connection with any source port, or over UDP but with source port 5060 > (note, no insecure=port). > > After the change, the incoming tcp connection would need to originate > from port 5060 (not going to happen). So the logical workaround is > insecure=port but this now has the effect of loosening things for udp as > well, which may or may not be unwanted. And currently doesn't work. My > change set solves that particular problem, but I'm less and less > convinced it's the right solution. Seems like the new behavior is buggy. TCP connections (and other connection oriented protocols) could come from any source port. > Looking at the logic of peer_ipcmp_cb_full: > > if source address doesn't match > => no match > > originally, if tcp|tls > => match > > originally, elseif peer2 has port=insecure > now, if udp|ws|wss and peer2 has port=insecure > => match if peer also has port=insecure else no match > > if ports match > => match > > Right, so my thinking is that insecure=port only really makes sense for > udp based transports and should be implied for non-udp based protocols, > specifically with host!=dynamic. Based on that I'd say the code should > probably do the following: That would jive with how I understand the world as well. > > 1. if source addresses doesn't match => no match > > (2.) if peer2 transport isn't allowed (subset of?) peer transport => no > match > > 3. if transport is not udp (and peer2 is not dynamic?) => match > > 4. if peer2 transport is udp, and has insecure=port: > 4.1 if peer also has insecure=port, match, else no match. > > 5. if ports match, match, else, no match. > > Based on the comments a further optimization may apply: > > * This callback will be used twice when doing peer matching. There is > a first > * pass for full IP+port matching, and a second pass in case there is a > match > * that meets the insecure=port criteria. > > Based on the above we can skip the second pass for non-udp peers (In > sip_find_peer_full function), in which case point 4 can drop the test > for transport udp too. This second pass is also the reason why 4.1 can > assume no-match as the first iteration would already have done the port > comparison. > > With this when a transport!=udp client registers to us, both host and > port will be implicitly set to that connection, and so will match > specific, and since we only match non-udp host if it's not dynamic, this > should also cover the sip/!udp guest use-case from an IP from where > there is also a valid registration (which is what I believe tripped up > previously). > > Point three partially restores the previous behaviour. If we have a > peer with an explicit address (in other words, !peer->host_dynamic) we > can match here, which basically has the same effect as setting > insecure=port implicitly for all non-UDP transports (and by implication > negates the need for insecure=port for non-udp transports). > > Point two is to allow something like: > > [peer1] > host=a.b.c.d > transport=udp > > [peer2] > host=a.b.c.d > transport=tcp > > To function correctly, I suspect currently given the above order if the > tcp variant comes in, the udp peer will match, resulting in transport > not allowed. > > Interestingly, I'm not sure If you'd like to try to fix it, feel free to do so. Otherwise, we can revert the patch that originally broke things. -- Matthew Fredrickson Digium, Inc. | Asterisk Project Lead 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
