[Home]

Summary:ASTERISK-27248: [patch]external_media_address and external_signaling_address don't always honor localnet
Reporter:Walter Doekes (wdoekes)Labels:patch pjsip
Date Opened:2017-09-04 10:37:30Date Closed:2017-09-06 09:45:40
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_pjsip
Versions:13.17.1 Frequency of
Occurrence
Related
Issues:
is caused byASTERISK-27024 nat/external_media settings ignored in 14.4.1
Environment:Attachments:( 0) ASTERISK-27248_undo.patch
( 1) ASTERISK-27248.patch
Description:Let's say I have this pjsip config:
{noformat}
external_signaling_address=127.1.1.1
external_media_address=127.2.2.2
{noformat}
Then I want my outgoing invites to look like this:
{noformat}
INVITE sip:bob@DEST:9284;transport=UDP SIP/2.0
Via: SIP/2.0/UDP 127.1.1.1:5060;rport;branch=z9hG4bKPjce5a5266-b624-4a52-b420-3648f073ec6d
From: <sip:alice@SOURCE>;tag=904cc2dd-1f73-4d2c-b712-78c99761bc0f
To: <sip:bob@SOURCE>
Contact: <sip:asterisk@127.1.1.1:5060>
...
o=- 1018431938 1018431938 IN IP4 SOURCE
s=Asterisk
c=IN IP4 127.2.2.2
{noformat}
If I add an unrelated localnet setting, then it should not affect those values. For example:
{noformat}
local_net=127.255.255.255/32
local_net=255.255.255.255/32
{noformat}
However, in Asterisk 13.17.1 it does differ, because of this code:
{noformat}
               if (!transport_state->localnet
                       || ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
                       ast_debug(5, "Setting external media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
                       pj_strdup2(tdata->pool, &sdp->conn->addr, ast_sockaddr_stringify_host(&transport_state->external_media_address));
               }
{noformat}
The ha struct stores the values in (default) "deny" order: if it's *not* found, then it's ALLOWed. If it *is* found, it returns DENY.

Thus:
{noformat}
ast_apply_ha(transport_state->localnet, &addr) == AST_SENSE_ALLOW)
{noformat}
means: it's NOT in the local net

and:
{noformat}
ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW)
{noformat}
means: it IS in the local net.

Logically, you would have it return DENY if it's NOT in the list, and ALLOW if it's in the list, but that's not how ast_apply_ha() works.

If we check the latest 13.x, we see this:
{noformat}
$ wgrep . -B1 -A3 localnet.*SENSE
{noformat}
{noformat}
./res/res_pjsip_session.c- if (!transport_state->localnet
./res/res_pjsip_session.c: || ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
./res/res_pjsip_session.c- ast_debug(5, "Setting external media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
./res/res_pjsip_session.c- pj_strdup2(tdata->pool, &sdp->conn->addr, ast_sockaddr_stringify_host(&transport_state->external_media_address));
./res/res_pjsip_session.c- }
{noformat}
DENY -> is local -> setting media to external because local??
{noformat}
./res/res_pjsip_nat.c- /* See if where we are sending this request is local or not, and if not that we can get a Contact URI to modify */
./res/res_pjsip_nat.c: if (ast_apply_ha(transport_state->localnet, &addr) != AST_SENSE_ALLOW) {
./res/res_pjsip_nat.c- ast_debug(5, "Request is being sent to local address, skipping NAT manipulation\n");
./res/res_pjsip_nat.c- return PJ_SUCCESS;
./res/res_pjsip_nat.c- }
{noformat}
DENY -> is local -> OK
{noformat}
./res/res_pjsip_sdp_rtp.c- if (transport_state->localnet
./res/res_pjsip_sdp_rtp.c: && ast_apply_ha(transport_state->localnet, &addr) == AST_SENSE_ALLOW) {
./res/res_pjsip_sdp_rtp.c- return;
./res/res_pjsip_sdp_rtp.c- }
./res/res_pjsip_sdp_rtp.c- ast_debug(5, "Setting media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
{noformat}
ALLOW -> is not local -> return -> not setting external IP because non-local??
{noformat}
./res/res_pjsip_t38.c- if (transport_state->localnet
./res/res_pjsip_t38.c: && ast_apply_ha(transport_state->localnet, &addr) == AST_SENSE_ALLOW) {
./res/res_pjsip_t38.c- return;
./res/res_pjsip_t38.c- }
./res/res_pjsip_t38.c- ast_debug(5, "Setting media address to %s\n", ast_sockaddr_stringify_host(&transport_state->external_media_address));
{noformat}
ALLOW -> is not local -> return -> not setting external IP because non-local??

It appears to me that 3/4 checks are wrong.

I'd check the regression box, because a customer noticed this after 13.13.1, but I'm not sure how the changes interact. It appears that some of this was already broken before that change.
Comments:By: Asterisk Team (asteriskteam) 2017-09-04 10:37:31.601-0500

Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution.

A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report.

Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].
By: Friendly Automation (friendly-automation) 2017-09-06 09:45:41.587-0500

Change 6395 merged by Jenkins2:
res/res_pjsip: Standardize/fix localnet checks across pjsip.

[https://gerrit.asterisk.org/6395|https://gerrit.asterisk.org/6395]
By: Friendly Automation (friendly-automation) 2017-09-06 10:10:33.744-0500

Change 6396 merged by Jenkins2:
res/res_pjsip: Standardize/fix localnet checks across pjsip.

[https://gerrit.asterisk.org/6396|https://gerrit.asterisk.org/6396]
By: Friendly Automation (friendly-automation) 2017-09-06 10:15:38.656-0500

Change 6397 merged by Joshua Colp:
res/res_pjsip: Standardize/fix localnet checks across pjsip.

[https://gerrit.asterisk.org/6397|https://gerrit.asterisk.org/6397]
By: Friendly Automation (friendly-automation) 2017-09-06 10:19:13.373-0500

Change 6398 merged by Jenkins2:
res/res_pjsip: Standardize/fix localnet checks across pjsip.

[https://gerrit.asterisk.org/6398|https://gerrit.asterisk.org/6398]
By: Walter Doekes (wdoekes) 2017-09-09 07:07:50.533-0500

So, mea culpa, my fix did not work in the real world.

This bug was only apparent in my test case, and instead broke every one elses configuration.

**Testcase**

The checks had me confused because we were not checking the *remote address* but the *local SDP address*.

My test case ran on the local machine, like this:

- external_signaling_address=127.1.1.1
- external_media_address=127.2.2.2
- local_net=127.0.0.0/8
- call from ETH0IP to ETH0IP
- signaling IP would become 127.1.1.1 (OK)
- media IP would become ETH0IP (wrong, expected 127.2.2.2)

If I removed the local_net setting, the signaling and media IP would start to co-behave:

- signaling IP would be 127.1.1.1 (OK)
- media IP would be 127.2.2.2 (OK)

This, along with the weird semantics of DENY for "is in the list" confused me enough to oversee the fact that the local connection address was checked (correctly) in the case I altered.

Changing my testcase to:

- local_net=127.0.0.0/24 (instead of 127.0.0.0/8)
- call from 127.128.0.1 (isntead of from ETH0IP)

Now the SDP gets populated with 127.0.0.1 -- which falls in localnet -- and replaced like expected.

**Conclusion**

The immediate fix is to revert my fix. Although I'd prefer to document things a bit and amend the fix instead.

I'm not entirely convinced we should check our own supplied SDP IP to make the decision about whether to rewrite or not, but it's the best heuristic I see available. And like it is, it makes directmedia possible for Asterisken behind NAT.
By: Walter Doekes (wdoekes) 2017-09-09 07:53:14.161-0500

(Satisfying my own curiosity: the heuristic does not break locally sent SIP, because the SDP-rewriting code is never reached in those cases. nat_on_tx_message (2nd check in original post) in res_pjsip_nat.c only adds the SDP-rewriting nat_hook if the transport-addr is non-local.)
By: Friendly Automation (friendly-automation) 2017-09-10 07:48:29.993-0500

Change 6468 merged by Joshua Colp:
res/res_pjsip: Fix localnet checks in pjsip, part 2.

[https://gerrit.asterisk.org/6468|https://gerrit.asterisk.org/6468]
By: Friendly Automation (friendly-automation) 2017-09-10 07:48:38.843-0500

Change 6469 merged by Joshua Colp:
res/res_pjsip: Fix localnet checks in pjsip, part 2.

[https://gerrit.asterisk.org/6469|https://gerrit.asterisk.org/6469]
By: Friendly Automation (friendly-automation) 2017-09-10 07:48:45.813-0500

Change 6470 merged by Joshua Colp:
res/res_pjsip: Fix localnet checks in pjsip, part 2.

[https://gerrit.asterisk.org/6470|https://gerrit.asterisk.org/6470]
By: Friendly Automation (friendly-automation) 2017-09-10 07:48:52.164-0500

Change 6471 merged by Joshua Colp:
res/res_pjsip: Fix localnet checks in pjsip, part 2.

[https://gerrit.asterisk.org/6471|https://gerrit.asterisk.org/6471]
By: Kevin Harwell (kharwell) 2020-03-04 09:57:46.384-0600

[~MarcReset] Please create a new issue for this, and if need be we can link it back to this one. Thanks!
By: Marc Ketel (MarcReset) 2020-03-05 02:23:32.298-0600

https://issues.asterisk.org/jira/secure/ViewProfile.jspa?name=kharwell Thanks. Created ASTERISK-28768