[Home]

Summary:ASTERISK-13481: [patch] Registration expiry not compatible with some ITSP
Reporter:nick_lewis (nick_lewis)Labels:
Date Opened:2009-01-29 11:49:43.000-0600Date Closed:2009-08-12 17:40:21
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c-reqexpiry.patch
( 1) sip-expiry-fix1.diff
Description:The configured registration expiry value is overwritten by the initial expiry value from UAS. Subsequent registrations use the new value. This prevents the UAS from changing its registration expiry value.

Some ITSP use changing registration periods to discover the NAT binding period at the UAC. Their UAS will set it initially very low and keep increasing it until they see that the src port number of the registration has changed. This indicates the nat binding period. The ITSP will then use an expiry period a little less than this so that the UAC is reregistering at the minimum required to keep alive the nat binding. However the UAS will never increase the expiry beyond that requested by the UAC. If the UAC uses the initial UAS expiry instead of the configured value then the registration expiry gets stuck at a ludicrously short period.

Issue 14185 contained a patch chan_sip.c-expiryrequest6.patch that dealt with this in addition to getting the expiry parameter parsed but the revision r172234 does not implement all of the patch
Comments:By: Tilghman Lesher (tilghman) 2009-02-20 17:54:50.000-0600

oej:  since you committed the part of the previous patch, could you take a look at Nick's concern, please?
By: Leif Madsen (lmadsen) 2009-07-27 13:41:08

Assigned back to new for someone else to take a look at this.
By: Matthew Nicholson (mnicholson) 2009-07-30 16:31:17

I attempted to reproduce this issue by registering asterisk to an openser server with an expiry value configured in the 'register=>' line in sip.conf.  I tested by changing the max_expiry value on the openser server.  In all cases, the asterisk server sends the value configured in sip.conf in the 'Expires' field of the REGISTER message.  Also the asterisk server properly honors the expiry value returned from openser (returnd in the 'Contact' header).

I tested with both r172234 and trunk (r209554) and was unable to reproduce this bug.  Am I testing the right things?  Do my tests look valid to you?  Are you still able to reproduce this issue?
By: nick_lewis (nick_lewis) 2009-08-03 04:04:59

Thanks for looking into this. I will need to refamiliarize myself with the issue and r209554 before I can say for sure. (As I recall r->expiry was being altered from its configured value in some cases.)
By: nick_lewis (nick_lewis) 2009-08-05 09:45:12

Sorry the problem is now a bit more subtle than previously described and only occurs if a 423 response is received to the outgoing register request.

RFC3261 states that the current register request may be retried with a revised expiry value if it is rejected with a 423 code. It does not state that the configuration of the expiry value may be changed for subsequent registration requests.

Asterisk overwrites the r->expiry not only for the retry but also for subsequent registrations until there is a reload. The value of r->expiry varies unpredictably because reloads may happen at any time to modify unrelated peers and registrations. Registration should behave in a consistent way.

Another related problem is when the Min-Expires header contains a value that is greater than max_expiry. In this case it is overwriting the r->expiry with the Min-Expires value then overwriting it again with the default_expiry value. This overwriting is done even though there is no registration retry attempted. The correct r->expiry value is destroyed for no purpose.

Since the problem is localized to function handle_response_register switch case 423, I suggest that a variable be locally defined and used to restore r->expiry e.g.

case 423:
int configexpiry = r->expiry;
r->expiry = ...
...
...
...
r->expiry = configexpiry;
break;
By: Matthew Nicholson (mnicholson) 2009-08-05 09:47:23

Ok.  I'll take a look at that.  Shouldn't be difficult to fix.
By: Matthew Nicholson (mnicholson) 2009-08-05 10:49:57

Test the patch I just uploaded (sip-expiry-fix1.diff).  Let me know if it resolves your issue.
By: nick_lewis (nick_lewis) 2009-08-05 11:51:45

Thanks for resolving this issue
By: Matthew Nicholson (mnicholson) 2009-08-05 11:55:54

Does that mean the patch works for you?
By: nick_lewis (nick_lewis) 2009-08-07 05:09:11

We are using a different internet telephony service provider now so it is not easy for me to test it.

With your asterisk plus openser server setup would it be possible to change the min_expiry value on the openser and see what results in the sip packet capture? (The asterisk registration string would need to be set to a shorter expiry that min_expiry on the openser.)

The problem (ignoring any authentication) will look like this:

asterisk <-> openser
-> REGISTER
<- 423
-> REGISTER
<- 200
(later)
-> REGISTER
<- 200

With a fix it will look like this:

asterisk <-> openser
-> REGISTER
<- 423
-> REGISTER
<- 200
(later)
-> REGISTER
<- 423
-> REGISTER
<- 200
By: Matthew Nicholson (mnicholson) 2009-08-11 16:46:50

I tested the patch with a version of asterisk that I modified to send a 423 response.  The patch works, but there was a problem.  For the second registration after the 423, the nonce from the initial 401 response is reused.  If the host we are registering to does not like this (asterisk does not), it will respond with 401, we will then re-register with the original expires value which will prompt a 423 and this will continue until the end of time.  I am working on making this work for any re-registration requests.
By: nick_lewis (nick_lewis) 2009-08-12 03:15:02

good catch

It looks as if the structure needs a r->request_expiry in addition to r->expiry where r->request_expiry always contains the configured value and r->expiry is allowed to be negotiated and overwritten. The r->expiry can be reset to the r->request_expiry in the sip_reregister function e.g.

 r->expire = -1;
+ r->expiry = r->request_expiry;
 __sip_do_register(r);
By: nick_lewis (nick_lewis) 2009-08-12 03:50:14

Patch attached as per previous note
By: Matthew Nicholson (mnicholson) 2009-08-12 13:31:44

Excellent work, this patch works.  I am going to make a few tweaks to it and commit it.
By: Digium Subversion (svnbot) 2009-08-12 14:53:39

Repository: asterisk
Revision: 211876

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r211876 | mnicholson | 2009-08-12 14:53:38 -0500 (Wed, 12 Aug 2009) | 11 lines

Make asterisk handle 423 Interval Too Short messages better.

This change uses separate values for the acceptable minimum expiry provided by the 423 error and the expiry value stored in the configuration file.  Previously, the value pulled from the configuration file would be overwritten.

(closes issue ASTERISK-13481)
Reported by: Nick_Lewis
Patches:
     sip-expiry-fix1.diff uploaded by mnicholson (license 96)
     chan_sip.c-reqexpiry.patch uploaded by Nick (license 657)
Tested by: mnicholson

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

http://svn.digium.com/view/asterisk?view=rev&revision=211876
By: Digium Subversion (svnbot) 2009-08-12 17:38:54

Repository: asterisk
Revision: 211950

_U  branches/1.6.1/
U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r211950 | mnicholson | 2009-08-12 17:38:54 -0500 (Wed, 12 Aug 2009) | 18 lines

Merged revisions 211876 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r211876 | mnicholson | 2009-08-12 14:53:14 -0500 (Wed, 12 Aug 2009) | 11 lines
 
 Make asterisk handle 423 Interval Too Short messages better.
 
 This change uses separate values for the acceptable minimum expiry provided by the 423 error and the expiry value stored in the configuration file.  Previously, the value pulled from the configuration file would be overwritten.
 
 (closes issue ASTERISK-13481)
 Reported by: Nick_Lewis
 Patches:
       sip-expiry-fix1.diff uploaded by mnicholson (license 96)
       chan_sip.c-reqexpiry.patch uploaded by Nick (license 657)
 Tested by: mnicholson
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=211950
By: Digium Subversion (svnbot) 2009-08-12 17:39:02

Repository: asterisk
Revision: 211951

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r211951 | mnicholson | 2009-08-12 17:39:02 -0500 (Wed, 12 Aug 2009) | 18 lines

Merged revisions 211876 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r211876 | mnicholson | 2009-08-12 14:53:14 -0500 (Wed, 12 Aug 2009) | 11 lines
 
 Make asterisk handle 423 Interval Too Short messages better.
 
 This change uses separate values for the acceptable minimum expiry provided by the 423 error and the expiry value stored in the configuration file.  Previously, the value pulled from the configuration file would be overwritten.
 
 (closes issue ASTERISK-13481)
 Reported by: Nick_Lewis
 Patches:
       sip-expiry-fix1.diff uploaded by mnicholson (license 96)
       chan_sip.c-reqexpiry.patch uploaded by Nick (license 657)
 Tested by: mnicholson
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=211951
By: Digium Subversion (svnbot) 2009-08-12 17:40:20

Repository: asterisk
Revision: 211952

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_sip.c

------------------------------------------------------------------------
r211952 | mnicholson | 2009-08-12 17:40:20 -0500 (Wed, 12 Aug 2009) | 18 lines

Merged revisions 211876 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r211876 | mnicholson | 2009-08-12 14:53:14 -0500 (Wed, 12 Aug 2009) | 11 lines
 
 Make asterisk handle 423 Interval Too Short messages better.
 
 This change uses separate values for the acceptable minimum expiry provided by the 423 error and the expiry value stored in the configuration file.  Previously, the value pulled from the configuration file would be overwritten.
 
 (closes issue ASTERISK-13481)
 Reported by: Nick_Lewis
 Patches:
       sip-expiry-fix1.diff uploaded by mnicholson (license 96)
       chan_sip.c-reqexpiry.patch uploaded by Nick (license 657)
 Tested by: mnicholson
........

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

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