[Home]

Summary:ASTERISK-03931: [patch] fix peer matching for multiple peers at the same IP address in 'insecure' mode
Reporter:Brian West (bkw918)Labels:
Date Opened:2005-04-13 13:21:24Date Closed:2008-01-15 15:40:47.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:To clarify, here's the scenario:

Insecure = very

Peers behind NAT

Peer-1 has IP 1.2.3.4 port 5060
Peer-2 has IP 1.2.3.4 port 5061

Call sent to * from the peer-2 ID. Call is processed and handled in * as Peer-1 or Peer-2 depending on match lookup in 'find_peer' which returns peer struct for the first addr_in match that is found.

Either find_peer needs to lookup by 'peername' if available, or should compare ports.

Right now, it is non-deterministic in its failure. If it were deterministic, it'd be a 'feature not a bug', but since it is non-deterministic, then surely its hard to claim * is doing the right thing.

-C
Comments:By: Brian West (bkw918) 2005-04-13 18:53:52

Gohead and commit this..

/b
By: Brian West (bkw918) 2005-04-14 15:39:12

Another small buglet

/b
By: coredump (coredump) 2005-04-14 15:55:10

I have tested this, and this fixes for me a problem I had with multiple peers behind NAT.  They now correctly are identified based on the sip header username, and not just on the ipaddress match.

We are using insecure=very as well.

-Chris
By: Mark Spencer (markster) 2005-04-15 01:03:03

You don't look up a peer on an *incoming* call based on its name, that's what users are for...
By: Brian West (bkw918) 2005-04-15 10:19:51

Mark did you even look at the code in question to make sure it is really doing what you think it is?  

/b
By: coredump (coredump) 2005-04-15 10:59:31

To clarify, here's the scenario:

Insecure = very

Peers behind NAT

Peer-1 has IP 1.2.3.4 port 5060
Peer-2 has IP 1.2.3.4 port 5061

Call sent to * from the peer-2 ID.  Call is processed and handled in * as Peer-1 or Peer-2 depending on match lookup in 'find_peer' which returns peer struct for the first addr_in match that is found.

Either find_peer needs to lookup by 'peername' if available, or should compare ports.

Right now, it is non-deterministic in its failure.  If it were deterministic, it'd be a 'feature not a bug', but since it is non-deterministic, then surely its hard to claim * is doing the right thing.

-C
By: Brian West (bkw918) 2005-04-18 11:49:17

Added compare for port.  It really should do this right?

/b
By: Brian West (bkw918) 2005-04-18 12:11:02

don't need the find_peer(of, now... can be NULL

/b
By: Olle Johansson (oej) 2005-04-19 11:39:02

bkw: Adding a good description to your bug report helps us to understand what you are doing and avoid the confusion...
By: Brian West (bkw918) 2005-04-19 14:28:51

Do people actually read the bug notes?

/b
By: coredump (coredump) 2005-04-19 15:17:16

Apparently not.  They take the time to add smarmy comments to them instead.  :)

The patch adds braces to ease readability of multiple nested if/else statements, and adds a port comparison to the address matching function in 'find_peer'.  The latter is the important thing, since that allows the correct peer struct to be returned.
By: Kevin P. Fleming (kpfleming) 2005-04-20 10:58:36

I'm not sure I understand why this is a problem...

If you use 'insecure=normal', Asterisk will do the match based on IP alone, not only IP _and_ port.

If you use 'insecure=very', Asterisk will do the match the same way, _and_ will ignore any secret you have specified in the sip.conf file.

Are these peers registering to you (thus necessitating the secret to be specified)?
By: Kevin P. Fleming (kpfleming) 2005-04-20 11:02:14

Your proposed patch is definitely not the right solution, because it reverts the behavior of the INSECURE flag not matching on port number when people don't want it to (in fact, the code you have supplied does the same thing as inaddrcmp(), which is already called in that function).

If there is a need to require authentication from these peers (but not for INVITEs) and support multiple peers from the same IP address, then we need to split the "insecure" setting so that you can specify either or both behaviors, not have them cumulative.

I would suggest something like:

insecure=port (don't match the port number, same as 'normal')

insecure=invite (don't require auth for INVITE)

insecure=port,invite (both, same as 'very')

Comments?
By: coredump (coredump) 2005-04-20 16:13:16

Need insecure=very, as some 'peers' do not register (locked to ip, dynamic=no).  Other peers do register, and these are the ones which are not recognized properly on outgoing calls.  Both types of peers originate and terminate calls through *.

Inaddrcmp doesn't get called when insecure=very is set as noted in the bugnotes *and* in the description.  Does anybody read????

Under what scenarios is *not* checking the port for trying to find a match a positive thing?  I understand it is current behavior, but so are most bugs. :)

BTW, this issue was not a problem, until we upgraded from STABLE ( 1.x ) to the HEAD, so behaviour *has* changed at some point between the two.

-C
By: Kevin P. Fleming (kpfleming) 2005-04-21 00:04:05

Why do you need insecure=very for peers that do not register to you? If they don't register, and don't need to authenticate, then use "host=" to set their IP address, "port=" to set their port number and don't specify a secret; you are done, they will be able to use your server as you would expect them to, no "insecure" setting is needed at all.

You also mention "outgoing" calls; none of this is related to outgoing calls (INVITE sent by Asterisk), but is all related to incoming calls only.

What do you mean "does anybody read"? Please point out _any_ instance of the text "inaddrcmp" in this bug prior to my mention of it; there are none. Every single call to find_peer in chan_sip that ends up looking up a peer by IP address calls inaddrcmp as the very first step of sip_addrcmp, before it even checks the SIP_INSECURE flag. What code are you reading?

I cannot describe why the option to not check the port number is present, it's been around since before I started working on Asterisk; however it is completely _optional_. If you don't put an "insecure" option on the peer, the peer will always be matched on their IP address _and_ port number.

I have a feeling you are having problems because you are using "insecure" settings improperly. Let me try to describe what they are supposed to be for:

"insecure=yes" is used for peers that register to you from one port number, but then send you INVITES from the same IP address but a different port number.

"insecure=very" is used for peers that register to you (performing normal authentication), but then send you INVITES and _will not_ authenticate on an INVITE (BroadVoice used to be an example of this, there may be others). It is a workaround to make the specified secret 'disappear' for INVITES but still be present for registrations.

I don't think there is any reason at all to use "insecure" settings for peers that don't register to you.
By: Olle Johansson (oej) 2005-04-21 13:35:39

A side note: We need to clean up the whole user/peer mess. Trying to teach people about this is so hard. It is messy and we really need a new concept.
By: Olle Johansson (oej) 2005-04-21 16:03:52

It makes a lot of sense to match on port number as well as IP.
By: Kevin P. Fleming (kpfleming) 2005-04-21 20:53:40

To date, nobody has been able to adequately describe to me what is actually happening. I have had reports that this problem is occurring with outbound calls (Asterisk sending INVITE to endpoint), and in that case there is never _any_ matching done in IP address, so the port number is irrelevant.

For inbound INVITES (from the endpoint), we _always_ match on the IP _and_ the port number unless one of the two 'insecure' modes has been enabled. If, and only if, they are turned on, then the match will succeed even if the port number is different. If someone can explain exactly what the situation is here, and provide a 'sip debug' showing why they need 'insecure' mode turned on, we can attempt to address the issue. Without that, there is no change to be made, because Asterisk is doing exactly what it has been asked to do.
By: coredump (coredump) 2005-04-23 08:41:10

Peers are loaded into * via Realtime, from an SQL database.

Current documentation (straight from the wiki) says:

  insecure=very ; To allow registered hosts to call without re-authenticating

Which is exactly what I want/need to do.  Allow 'registered' peers to initiate a call w/o reauthenticating.  I need to also select the *correct* peer, which it does not do currently, since the port is not used to match the peer.  The intent described in the config docs there seems to be different from what actually happens.

A solution I think:

Asterisk could perform a sequence of checks, looking for the most specific match first ( IP+Port, then IP ).  This won't break current behaviour, as it will still allow non-port matches.  Doubles worst case lookup time O(n) -> O(2n), since you might have to traverse peer-list twice though.  Best case lookup is unchanged.

Suggestions/alternatives, further edification on just what the heck difference is between a user/peer/friend, since there doesn't seem to even be a consensus as to just what that is anyway.

-C
By: Kevin P. Fleming (kpfleming) 2005-04-26 22:45:59

OK, now we are getting somewhere. As I suspected, this has nothing to do with outgoing calls, and is only related to incoming calls, from SIP peers.

I'll repeat my suggestion above: let's break 'insecure' into two separate values, which can be provided together, individually, or not at all. Anyone have any objections? If not, I'll code it up and commit it right away.
By: coredump (coredump) 2005-04-27 08:26:45

Sorry if there was confusion re: outgoing.  This is definitely calls 'incoming' to Asterisk from the remote peers.

The suggestion to split/add a flag to enable or disable port matching for peers would also 'fix' the problem that we were experiencing.  I definitely concur that would seem the best route to go, to add the desired port matching behaviour without breaking backward config/behaviour compatability.

-C
By: Kevin P. Fleming (kpfleming) 2005-04-27 12:14:48

OK, new 'insecure' settings have been committed to CVS HEAD.
By: Digium Subversion (svnbot) 2008-01-15 15:32:27.000-0600

Repository: asterisk
Revision: 5521

U   trunk/channels/chan_sip.c
U   trunk/configs/sip.conf.sample

------------------------------------------------------------------------
r5521 | kpfleming | 2008-01-15 15:32:27 -0600 (Tue, 15 Jan 2008) | 2 lines

allow fine-grained 'insecure' settings (bug ASTERISK-3931)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5521
By: Digium Subversion (svnbot) 2008-01-15 15:40:47.000-0600

Repository: asterisk
Revision: 6087

U   branches/v1-0/CHANGES
U   branches/v1-0/channels/chan_sip.c
U   branches/v1-0/configs/sip.conf.sample

------------------------------------------------------------------------
r6087 | russell | 2008-01-15 15:40:46 -0600 (Tue, 15 Jan 2008) | 3 lines

change insecure options to support 'port' and/or 'invite' instead of forcing
the 'port' option when using 'invite' (bug ASTERISK-3931)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6087