<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1698/">https://reviewboard.asterisk.org/r/1698/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 27th, 2012, 9:53 a.m., <b>Kevin Fleming</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't think that this patch as written is adequate; it turns on *both* force_rport and comedia when NAT is detected, and there could definitely be situations where that results in broken calls. If we're going to provide an 'auto' mode, we should also provide 'force_rport_auto' and 'comedia_auto', so that those can be individually automatically enabled in cases where the UA (or mid-path network element) in use doesn't work with both of them enabled.
However, I think there is a potentially larger problem here: whether 'force_rport' and/or 'comedia' will work properly for any given path that the remote UA is using is dependent on *both* the UA's behavior and the behavior of the network elements (NATs, SBCs, proxies, etc.) in between that UA and Asterisk. To go back to the reporter's statements in JIRA... what if the 3G operator's edge device (presumably an SBC) doesn't hide itself (so Asterisk detects that NAT is in use) but still doesn't support symmetric RTP (comedia)? The risk here is that users will just expect 'auto' mode to make everything work smoothly all the time, and that won't always be the case. To some extent, the current situation of requiring the Asterisk admin to take some sort of action (learning what is required for that configuration) is potentially better, because the admin will have to learn how to deal with these network complexities. Having an 'automatic' mode leads to users believing that they don't need to understand or care about these topics, and then we have to help them later when their calls "don't work".</pre>
</blockquote>
<p>On January 27th, 2012, 10:40 a.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> it turns on *both* force_rport and comedia when NAT is detected, and there could definitely be situations where that results in broken calls.
Yes. This is a feature for nat=auto, so whether to set nat=yes or nat=no automatically--it isn't a way to magically figure out what crazy set of circumstances might possibly be in play between the endpoint and Asterisk (something that is pretty much impossible to determine without some more help from the client). In my opinion, this is a documentation issue. It should be thoroughly documented that this will just pick between those two options.
The nat settings aren't bitfields/combinable, so we would end up with auto_force_rport, auto_comedia, and auto_everystuff for, what I would imagine to be a vanishingly small percentage of users who would use them. Searching with google, I get the impression that the vast majority of people use nat=yes and the greatest minority uses nat=no. If the idea is to create a type that just "tries to do the right thing" for clients that move between many different networks, I don't see how adding the more specialized options help. You still aren't going to be able to support networks that look natted and require comedia and ones that look natted and don't support comedia at the same time. The only way I can think of it being used is if a client moves *only* between some very specific networks (always stays at the same hotel?), with very specific requirements for each.
We've made it a decade without anyone submitting this patch. I bet that if we accepted it, it would be another decade before anyone said "What I really need is the ability to move between only these two very specific networks."
As far as users being confused that automatic doesn't work, well, users are confused when the default (whatever it may be) doesn't work. This patch makes things work for situations that wouldn't have worked without it. If it doesn't work for a situation, users still have to learn how things work just like they do now--this is no change from how things currently are. But, things also might "just work" for a larger number of cases. And that is a win for everybody, IMHO.</pre>
</blockquote>
<p>On January 27th, 2012, 11:31 a.m., <b>Kevin Fleming</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, I didn't express my point clearly. The need for 'nat=auto' was probably driven mostly by the fact that the default setting for 'nat' *used to be* 'no'. Now that the default in all widely-deployed and supported branches is no longer 'no', the percentage of systems that will 'just work' out of the box will increase, because for the overwhelming majority of endpoints, having 'nat' mode enabled is not harmful, even when it is not necessary.
Unless I am misunderstanding what this patch does, here is the gist of it: if we receive an out-of-dialog (or dialog-forming) request with an address/port in the top-most Via header (an address, not a domain name... as the Cisco docs that Simon linked to say, you can't do 'auto' mode if the Via header doesn't have an IP address) that does not match the address/port we received the request from, then we turn on force_rport and comedia. If the request included 'rport', then enabling force_rport has no effect. If the device would have worked with force_rport enabled in the first place, then enabling it later has no effect. If the device (and the network elements in the path) will work with comedia mode on, then enabling it later has no effect. If the device (or the network elements in the path) won't work with force_rport or comedia enabled (or both), then enabling either of them will break the communication.
So, what this patch does is allow Asterisk to support a group of endpoints that would not work with force_rport enabled when they are not behind a NAT but will work with it enabled when they are behind a NAT. In addition, these devices must also support comedia mode when located behind a NAT. To date, the only devices that we know of that do not work with force_rport enabled *also* don't benefit from it being enabled when they are behind a NAT (because the only NAT that they work behind has a SIP ALG that accommodates their unusual requirements), and they don't benefit from comedia mode being enabled either when they are behind a NAT (because said NAT handles that via its SIP ALG as well).
If there is *any* risk of regression, confusion or other problems caused by this patch being merged, then I'm unsure we should proceed with doing so. I just can't see that there is really any substantial demonstrated advantage to having it in place, that we haven't already achieved by making 'force_rport' the default. It's probably time to take this thread directly to asterisk-dev to get some additional feedback.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I didn't look at this issue as one to support specific devices so much as one that aims to work around broken ALGs. nat=force_rport *should* work for almost any device/NAT. If there was an ALG that changed the address and port in the Via, listened for responses on that port but sent from another, and it also didn't remove the ;rport parameter inserted by the device...then, I suppose, this patch would make things work better than nat=force_rport...as long as the ALG supported comedia. So, yeah, that isn't the best argument in the world, obviously (though broken ALGs are certainly the norm, I'm not sure that this is exactly how they are broken) :-/
I *think* that a Cisco phone with nat_enable=1 and a nat_address set would actually work with nat=auto where they wouldn't with nat=force_rport as we wouldn't detect it as natted and therefor not force rport on it. Although reading the Cisco docs, it looks like if both nat_enable is and voip_control_port are set, then it should be using the voip_control_port as the UDP source port as well and force_rport would work.
So, yes, I suppose we should verify that there are situations in *recent* versions of Asterisk where this is actually provably beneficial to large numbers of people. I'll play around with some known broken devices and see what I get. I hate NAT.</pre>
<br />
<p>- Terry</p>
<br />
<p>On January 26th, 2012, 8:28 p.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated Jan. 26, 2012, 8:28 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">First, this patch as originally posted is from JIRA user pedro-garcia. It has been in JIRA for a long while, and has finally come up to be considered. There were some changes that I wanted to made to the original patch, so I tried contacting the author to get them to put the patch on reviewboard, but after a week I got no response. So, I'm putting the original patch up, then will immediately add a review with my changes. Many thanks to pedro-garcia for his contribution.
>From the JIRA issue:
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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Lots of reloads with changing values, and registry natted and un-natted phones. I also set the nat_supertest in testsuite to run with nat=auto to make sure that it responded the same with existing and non-existing peers.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17860">ASTERISK-17860</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/chan_sip.c <span style="color: grey">(352610)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(352610)</span></li>
<li>/trunk/configs/sip.conf.sample <span style="color: grey">(352610)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1698/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>