[Home]

Summary:ASTERISK-17860: [patch] Auto detect NAT if the same device can be used in several natted/unnated scenarios
Reporter:Pedro Garcia (pedro-garcia)Labels:
Date Opened:2011-05-14 15:39:19Date Closed:2012-02-27 10:14:10.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Interoperability
Versions:1.8.4 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c.patch
( 1) sip.conf.sample.patch
( 2) sip.h.patch
Description:I have some devices in the following scenario:

- Asterisk server with public IP address

Mobile devices (clients):

- When in internal network, no NAT between the client and the server
- When in "roaming" (i.e. a Hotel with WiFi), the client is behing a NAT
- When in 3G, operator transparent sip proxy so it looks as no NAT, but does not support symmetric RTP.
- Sometime, the device gets a public IP with no NAT at all.

No NAT setting available in asterisk works for all these scenarios at the same time, and I can not request the user to activate different accounts depending on its location.

I have added a new NAT setting (nat=auto) to the current ones. When set, chan_sip auto detects from the Via header, the recv sockaddr, and the rport setting if the client is behind a NAT.

It also adds to cli interface results (sip show peer/s) info on this (so now you could see "N" for NAT and nothing for no NAT as before, "a" for auto detect no NAT, and "A" for autodetect NAT.

I include the two patches involved, one to sip.h to add the flag to "SIP_PAGE3", and the patch itself to chan_sip.c

They apply cleanly on 1.8.4 and work with TCP and UDP, but also on 1.8.3.2 with TCP, UDP and TLS (from 1.8.3.3 there is a reported bug in TLS that has nothing to do with this patch but that prevents me from using TLS)



****** ADDITIONAL INFORMATION ******

<inline patches removed by lmadsen>
Comments:By: Leif Madsen (lmadsen) 2011-05-17 07:31:11

Marked as Needs License until the license has been approved. Changing this to another status is a manual process.
By: Andrea Cristofanini (hydrolife) 2011-05-17 09:05:01

could we test the patch ?
By: Luke H (luckman212) 2011-05-19 16:11:40

Yes, this sounds really great for softphones on a 3g device -- would like to test the patch as well !!
By: Andrea Cristofanini (hydrolife) 2011-05-20 04:42:27

Hey Digium Guys, could you post back the patch ??? Reporter need testing  ...
By: Luke H (luckman212) 2011-05-25 17:19:28

So far this patch seems to work for me, I patched SVN-branch-1.8-r320650M  and have not seen any issues.  Would love to see this rolled into the trunk so it doesn't have to be manually patched each time, imo It's a fantastic patch
By: Luke H (luckman212) 2011-05-30 09:47:54

anyone else tested this patch?  it seems to work for me but I would be interested to hear from anyone else.
By: Luke H (luckman212) 2011-09-28 10:43:34.412-0500

Pedro, did you apply for a license so this can be merged into main?  I really like this patch but it's a bit of a drag to always have to apply it when updating from source.  I'd like to see this baked in to 1.8!

edit: guys I've been trying to contact the author of this patch (Pedro Garcia) does anyone know how to reach him?  If he has vanished, would there be any way this code could be rolled into the main branch without him applying for the license?
By: Pedro Garcia (pedro-garcia) 2011-09-30 14:00:25.650-0500

I am still here. I think I already applied (and got) the license, unless I missed something. If not, Digium guys would not have published the patch (as far as I know).

I though there was no interest in the patch, so I stopped updating it here, buy I have done some changes to it after 1.8 (it did not always detect correctly in the last version I got), and now I am fixing it so that it correctly detects NAT also for unregistered users.

It is OK for me to resubmit the patch and document it, as it is also a drag for me to update it in every new version. Is there something I could do to move in that direction? (the patch has been there with no news for months and nobody but you looked interested in getting it applied).
By: Luke H (luckman212) 2011-09-30 14:11:26.955-0500

Pedro, I'm glad to see you back!  I'm really surprised that there were few who showed interest in the patch -- it's very useful indeed.  Please do submit the updated patch if you don't mind sharing it.   Leif- what's the procedure for getting this looked at on  Review Board?
By: Luke H (luckman212) 2011-09-30 16:22:54.934-0500

Target release 11 - really?  I can sort of understand the logic of not introducing this new feature into 1.8 but why not target it for 10 at least?  10 is not even RC yet...
By: Malcolm Davenport (mdavenport) 2011-09-30 16:47:20.046-0500

Last call for new features ended in June, before the branching.  We're past the first beta, into the second, and we're less than a month from scheduled release.  Adding new features at this point is pretty risky.  You're welcome to take this up on the -dev list to see if there's consensus that an exception should be made for this case.
By: Luke H (luckman212) 2011-09-30 16:57:55.800-0500

Ok well let's see after Pedro uploads his updated patch, then I'll post on asterisk-dev and hopefully get myself & a handful of other testers to bang on it and see if it breaks anything.  If enough people give a thumbs up you'll consider baking this into 10?
By: Pedro Garcia (pedro-garcia) 2011-09-30 20:03:23.895-0500

Here the patch against today's SVN. Added patch for config.sample. There is a difference with the previous version: now nat=auto needs to be set up globally for it to work. I think it is cleaner this way, as now if you do not set it, there is no change on behaviour vs traditional nat settings.
By: Pedro Garcia (pedro-garcia) 2011-09-30 20:27:09.395-0500

Tested with UDP/TCP/TLS (+ anecdotically SRTP) on:
- NAT (3G)
- No NAT (LAN)
- No NAT (3G) + transparent proxy not handling symmetric RTP correctly

Need to test with unregistered user, probably tomorrow.
Update: it works also with unregistered users.
By: Pedro Garcia (pedro-garcia) 2011-12-04 04:21:24.677-0600

I resubmitted the patch (some files looked not to have the license).

I also modified chan_sip.c.patch, as there was a bug leading to some inestability sometimes:

Since addition of PAGE3 SIP flags, "handle_common_options" is called from two different functions with a two items and a three items "mask" array. This was leading my code to write out of the array in some cases. I think this needs to be fixed anyway as when in the future more PAGE3 SIP flags are added and masked from "handle_common_options" it will also happen. Now chan_sip.c.pach is fixing this also.

Is there some way to remove the old file patches to avoid confusion?
By: Luke H (luckman212) 2011-12-04 14:57:13.044-0600

Thank you for updating the patch Pedro!  I am going to try it when 1.8.8 gets released, I think which is going to be in a few days time.  I really hope they integrate this patch soon, my build process is getting too complicated with all of the out-of-band patching that needs to be done!  Digium:  please bake this in
By: Terry Wilson (twilson) 2012-01-18 14:51:29.302-0600

I can see where this patch would be useful, and I'm committed to making sure it gets in. I can see a couple of things that should probably be changed. The best place to have discussions about that would be at https://reviewboard.asterisk.org. Here are instructions for getting the post-review tool: https://wiki.asterisk.org/wiki/display/AST/Reviewboard+Usage and the login for reviewboard should be the same as your old mantis username/password.

Thanks for the patch. I look forward to working with you to get it in.
By: Leif Madsen (lmadsen) 2012-02-16 07:22:09.736-0600

Pinging reporter or developers. Was the requested information gleaned somewhere? :)
By: Pedro Garcia (pedro-garcia) 2012-02-20 08:59:32.954-0600

Sorry for the delay in answering. I have been busy these last weeks... I see a lot of activity on the review board: I will follow-up there.
By: Pedro Garcia (pedro-garcia) 2012-02-20 09:01:03.463-0600

Sorry for the delay in my answer. I have been busy these last weeks.
I see a lot of activity on the review board: I will follow-up there.

Update: no way to log-in into reviewboard. Maybe I am doing something wrong...

I answer here some of the topics on the reviewboard (if I manage to login I will move it there):

- The code in my patch relies on "nat=auto" to be set-up globally. It was not initially like this, but I did it to make sure that if you did not activate it, asterisk would be behaving exactly as if the patch was not applied.

- "nat=auto" must be set up globally also if you want to support nat detection for guest users, as there is no peer configration specifically for them (unless I missed something).

- As someone commented in the reviewboard, the aim of the patch is not to have to set up nat to "yes" ot "not", buy let asterisk decide between these two choices for each incoming connection.

- "nat=yes" should work in most scenarios, but in some cases, i.e. when the user is behind a transparent SIP proxy from Cisco, nat and rport will be borken toghether, and it is not a good idea to ask users to change the configuration of their sip phone depending on there location, if the PBX can detect it for them.

I will be glad to see the auto functionality in 11. I am not quite sure if I need to do something else, but in any case I am available to check / modify some things.

By: Terry Wilson (twilson) 2012-02-27 10:14:10.081-0600

We modified the original patch so that you could set auto on the individual force_rport and comedia settings. We also removed the requirement for global settings and reworked nat= to be a combinable list of attributes (nat=auto_force_rport,auto_force_comedia for the original nat=auto, for example). Thank you very much for the patch.