[Home]

Summary:ASTERISK-04352: [patch] Add SIP domain support
Reporter:Olle Johansson (oej)Labels:
Date Opened:2005-06-05 08:05:32Date Closed:2008-01-15 15:49:21.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug4466_sip_trace_wrong_extension.txt
( 1) sipdomain0906.txt
( 2) sipdomain0916.txt
Description:This patch adds SIP domain support to the SIP channel. Much like a mail server, with this patch Asterisk is aware of local domains and will refuse connections to other domains (or IP addresses).

This patch is needed to implement SIP transfers properly, since we need to know whether a transfer is to another server or a local transfer.
Comments:By: Olle Johansson (oej) 2005-06-05 08:06:18

...And this patch is backwards compatible. If you do not add a domain or domain=yes, asterisk will continue to act as before happily.
By: Kevin P. Fleming (kpfleming) 2005-06-05 11:55:31

Code review notes:

1) This hunk appears to be from another patch:

@@ -8246,10 +8309,7 @@
r->expire=ast_sched_add(sched, expires_ms, sip_reregister, r);
ASTOBJ_UNREF(r, sip_registry_destroy);
} else {
- if (r->expire)
- ast_log(LOG_WARNING, "Got 200 OK on REGISTER that is already done\n");
- else
- ast_log(LOG_WARNING, "Got 200 OK on REGISTER that isn't a register\n");
+ ast_log(LOG_WARNING, "Got 200 OK on REGISTER that isn't a register\n");
ast_set_flag(p, SIP_NEEDDESTROY);
return 0;
}

2) It seems to be safe that you are not providing locking on the domain list, since it can't be modified except through addition or complete destruction. However, comments to that effect in the code at each place where the list is modified may be appropriate, in case someone decides to add other methods to modify the list in the future.

3) clear_sip_domains() should set the list pointer to NULL _before_ walking the list, otherwise a check_sip_domain() call might be walking the list at the same time (unless the 'reload' lock will absolutely guarantee that cannot happen). Actually, that's a good reason for a lock on the list itself, unless the reload lock will be adequate.

4) Addition of AUTO domains should be controllable from the config file; people who use this feature may want to very strictly control the domains that are accepted.

5) There are references in the comments to 'localdomain', but I believe you have changed the config option to 'domain'.

6) I can't help but think that people are going to want some sort of pattern-matching for this... but that can wait for another patch on another day :-)

I might suggest the following config options instead of a combined 'domain' option:

restrictdomains=no/yes/auto

'no' - current behavior
'yes' - new behavior, but only include explicitly configured domains
'auto' - new behavior, plus auto-generated domains

domain=<foo>

adds a domain to the list

Thoughts?
By: Olle Johansson (oej) 2005-06-05 16:44:56

Thanks for very good feedback, I'll check into locking, that is easy to do.
Yes, the 1) was a mismatch, there's been quite a few changes today :-)

I'll work on the rest during the week. Had two config options first, but tried to be clever and only use one :-)

/Olle
By: Olle Johansson (oej) 2005-06-08 15:27:12

New patch uploaded.
Now autoconfig is a separate option (yes/no)
If you turn this on, or add domain= entries, domain support will be turned on.

Added virtual SIP hosting support:

domain = digium.com,digium-incoming
domain = asterisk.org,asterisk-incoming

This config send incoming calls from unknown callers (no peers/users) to different contexts based on the domain in the URI.

Now using locks on the domain list.
By: Michael Jerris (mikej) 2005-07-12 17:02:23

Are we waiting for testing on this?
By: Olle Johansson (oej) 2005-07-18 03:41:47

I don't really know. Need feedback from kevin. If he's ready to commit, I'll spend time updating the patch to current cvs.
By: Olle Johansson (oej) 2005-07-19 15:39:11

...updated patch to current CVS head.
By: Olle Johansson (oej) 2005-07-21 08:35:21

Oops, disabled a line with // and forgot to remove it... New patch soon.
By: Olle Johansson (oej) 2005-07-21 08:52:45

Actually, this patch removes a // comment - sorry for misreading. I turned formatting review to N/A since I can't reset it... ;-)
By: Olle Johansson (oej) 2005-07-26 07:09:35

READY for cvs
By: Mark Spencer (markster) 2005-08-06 10:41:32

Actually, I think this patch could be updated with only a minor modification to allow us to map domains into contexts!
By: Olle Johansson (oej) 2005-08-10 14:27:00

Mark, it does.
By: Kevin P. Fleming (kpfleming) 2005-08-22 22:11:38

Let's get this one cleaned up for 1.2...

Review notes:

1) This patch uses url_decode() from another patch, if I'm understanding things properly, and that one is pending some changes.

2) add_sip_domain() calls memset() on sd before checking to see if it's non-NULL.

3) add_sip_domain() leaks memory if newdomain is zero-length.

4) Please use ast_strip() instead of your own whitespace-skipping loop.

5) Use ast_copy_string() instead of strncpy().

6) Please use the list macros instead yet-another-open-coded implementation :-)

7) + ast_log(LOG_NOTICE, "***OEJ Reading GENERAL\n");

8) This is unrelated:

+ if (!ntohs(externip.sin_port))
+ externip.sin_port = ntohs(DEFAULT_SIP_PORT);
By: Olle Johansson (oej) 2005-08-24 13:17:08

url_decode already exists in chan_sip
By: Olle Johansson (oej) 2005-08-24 13:22:56

1,2,3,4,5,7,8 fixed.
For 6 my reply is "I don't know how..."
By: Olle Johansson (oej) 2005-08-25 09:49:28

This patch does not have a relationship with the uri_encode patch, more than some instances of url_decode might have to change to ast_uri_decode if that patch is committed before this one.
By: Serge Vecher (serge-v) 2005-08-31 09:35:36

oej, can you please update this patch -- it does not apply clean. Would like to test domain functionality today.

Thank you for all your hard work!


By: Kevin P. Fleming (kpfleming) 2005-08-31 14:43:32

We will be working on this patch here in my office (OEJ and myself) and will get it into CVS HEAD some time today.
By: Olle Johansson (oej) 2005-09-06 02:07:22

Updated file that applies to current cvs
* Added checksipdomain() dialplan function
By: Michael Jerris (mikej) 2005-09-06 06:23:07

STOP ADDING FUNCTIONALITY!
By: Serge Vecher (serge-v) 2005-09-06 16:58:37

Perhaps the name of checksipdomain() could be changed to islocalsipdomain() or similar to reflect it's nature (for load-balancing purposes as far as I understand). The reason for this is that I need a dialplan function that returns an empty string if argument is local domain, and the argument itself
if it is an external one (reverse behavior of checksipdomain()). That function could be then called isexternalsipdomain(); it is needed to properly route calls to external domains -- right now this is done via not-very-pretty-macros.
By: Serge Vecher (serge-v) 2005-09-06 19:33:25

Ok, I've patched chan_sip.c revision 838 and successfully compiled it. Here are my comments:
1) Doesn't seem to break old functionality -- everything is working as usual.
2) The description for this bug states that "Asterisk ... will refuse connections to other domains ...)". To me this means that when we INVITE extensions at "other domains". What I observed is that this patch makes Asterisk domain-aware for registration purposes. Attempts to REGISTER with Asterisk specifying a domain other than what Asterisk now considers local will result in a 404. Well, this is probably just a discrepancy in bug description.
3) The only "bug" I've noticed is that when I try to register with Asterisk specifying a wrong extension, but a correct domain name, Asterisk will deny a registration with 404 NOTEing on console that registration failed, because "not a local SIP domain" -- which is not true (btw, it would be useful to see the actual name of attempted domain in the error message). I've attached a SIP trace with an excerpt of related console output. First, we try to register with a wrong extension (101), get denial and notice on console, then we change the to correct extension (151) and everything is peachy.
4) Great work, Olle, thanks a lot!
By: Olle Johansson (oej) 2005-09-07 01:06:32

Thanks for the review, will come back with changes.

How we acts to foreign domains depends on the settings in sip.conf. We can refuse everything or allow INVITEs, so that we can call out to foreign domains from internal phones. INVITEs need to be handled in the dial plan, that is why I created the dialplan function as well.
By: Serge Vecher (serge-v) 2005-09-19 17:30:21

Updated OEJ's patch (sipdomain0906.txt) to apply cleanly to 09/19 HEAD in case somebody else wants to test this. Has been running HEAD with this patch since 09/06 without problems. Only little buglet (see post 09-06-05 19:33) remains to be resolved.
By: Kevin P. Fleming (kpfleming) 2005-09-26 19:11:23

Committed to CVS HEAD with major mods to use list macros and to simplify some code. Thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:49:21.000-0600

Repository: asterisk
Revision: 6668

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r6668 | kpfleming | 2008-01-15 15:49:21 -0600 (Tue, 15 Jan 2008) | 2 lines

add basic SIP domain support (issue ASTERISK-4352, with major mods)

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

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