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. 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: 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 Kind Regards, Jaco -- _____________________________________________________________________ -- 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
