[Home]

Summary:ASTERISK-14829: [patch] When useragent was added to realtime_update_peer in chan_sip the necessary ldap changes were not made
Reporter:John Covert (jcovert)Labels:
Date Opened:2009-09-14 18:39:36Date Closed:2011-09-16 14:57:28
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Resources/res_config_ldap
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c.patch
( 1) res_config_ldap.c.patch
( 2) res_config_ldap.c.patch-bis
( 3) res_config_ldap.c.patch-ter
( 4) res_ldap.conf.sample.patch
Description:When useragent was added to realtime_update_peer in chan_sip the necessary ldap changes were not made

The call to realtime_update_peer was modified in chan_sip to store 'useragent' This causes constant error messages:

 [Sep 14 14:34:27] ERROR[30323]: res_config_ldap.c:1286 update_ldap: Couldn't modify dn:uid=johncovert.test,ou=people,dc=onstate,dc=net because Undefined attribute type

This problem last happened when "lastms" was added.  See issue ASTERISK-14166.

Whenever a new item is added to realtime, it is necessary to update the ldap definitions to support it. I propose making the following changes required to eliminate this error:

1. Modify contrib/scripts/asterisk.ldap-schema as follows:

a. update version number and list this bug number in the changes.
b. Add objectIdentifier AstAccountUserAgent AstAttrType:59
c. Add
 attributetype ( AstAccountUserAgent
   NAME 'AstAccountUserAgent'
   DESC 'Ast Account User Agent'
   EQUALITY caseIgnoreMatch
   SUBSTR caseIgnoreSubstringsMatch
   SYNTAX 1.3.6.1.4.1.1466.115.121.1.15)
d. Add AstAccountUserAgent to objectclass AsteriskSIPUser and for completeness to objectclass AsteriskIAXUser even though not yet used.

2. Modify configs/res_ldap.conf.sample to include
    useragent = AstAccountUserAgent
 in the Sip Users Table

3. This time also modify chan_sip.c to refer future developers to the need to update these two files whenever adding new realtime updates.

I expect to submit patches within the next two or three days once I have actually tested this; I cannot do this while production is running today.

Regards/john
Comments:By: John Covert (jcovert) 2009-09-14 19:21:08

This just became a serious regression.  Between 19406 and 19409 an error was introduced which prevents slapd from starting up.

See http://76.164.171.234/svn/asterisk/trunk/contrib/scripts/asterisk.ldap-schema?r1=197406&r2=197409&pathrev=197440

Note the extra "$" and blank line that was introduced after AstApplicationData at lines 565 and 566.

Another reason this is a regression is that since the new parameter was added to the main update, the IP address is never updated in the registration, and LDAP-based clients cannot be reached for incoming calls at all.


By: John Covert (jcovert) 2009-09-14 19:27:03

Ya know, it might be better to modify the ldap code so that if it doesn't find a translation (local = ldapname) in res_ldap.conf it just doesn't try the update.  However, I'm not doing that as part of this set of patches.


By: John Covert (jcovert) 2009-09-14 20:10:50

Hmmm.  A little more to do here.  Whatever is in useragent on a de-registration is causing the following to happen:

[Sep 14 16:26:06] ERROR[2893]: res_config_ldap.c:1286 update_ldap: Couldn't modify dn:uid=johncovert.test,ou=people,dc=onstate,dc=net because Invalid syntax

Might be null.  Need to handle the null case correctly, whether it's registration or de-registration.  Might need to define the field differently in the LDAP definition.  New stuff to learn about LDAP if I have to do that.

Will work on this later.
By: John Covert (jcovert) 2009-09-14 23:00:01

Here's the deal.  On de-registration, useragent is updated to null.  That should make the field go away.  LDAP doesn't support null-valued parameters.

In res/res_config_ldap.c the current code sets the mod_values field to a pointer to a null string.  The LDAP interface requires a null pointer.  I have a patch that fixes this, but I need to be sure that I make the section of the code which allows a parameter to be specified MULTIPLE times (and produces a semicolon separated list) works properly, i.e. creates "null" additions to the list and only deletes the entry if the ONLY time the parameter is specified is null.  If multiple null additions are specified, you'll get ";;;" (which is what you want in that case).

It's late.  That will be cleaned up tomorrow or Wednesday.

I'm also submitting a patch to chan_sip.c to add the following comment at the call to realtime_update_peer (only the "principal" call):

IMPORTANT: Anytime you add a new parameter to be updated, you must also add it to contrib/scripts/asterisk.ldap-schema and to configs/res_ldap.conf.sample as described in bugs 15156 and 15895


By: John Covert (jcovert) 2009-09-15 09:25:43

While working on this, I started to put in the following comment:

       /* The following line of code is troubling.  I'm not changing it yet, but I believe that it is incorrect.  mod_values is an array of pointers to character string values (ldap supports attributes with multiple values).  See ldap.h.  But we're allocating 2 x sizeof(char) when we should be allocating 1 x sizeof(char **).  We then put a pointer into this 2 x sizeof(char) space, and the only reason it doesn't blow up nastily is that ast_calloc gets chunks big enough that it doesn't matter. JRC. */

But after writing it up, I convinced myself that I need to make the change now.  It shouldn't change anything about what happens in execution (at least with the current allocation scheme in ast_calloc on current hardware architectures) but it's such an awful piece of code it needs to be fixed.


By: Gavin Henry (suretec) 2009-09-15 09:33:20

That regression typo was fixed quickly, but thanks for noticing. We're now at version 3.2.0.

Not only chan_sip.c but chan_iax2.c too.

Which ldap.h are you looking at? OpenLDAP 2.3 or 2.4?
By: Gavin Henry (suretec) 2009-09-15 09:33:39

Oh, and many many thanks for testing and the bug report!!!!
By: John Covert (jcovert) 2009-09-15 09:44:57

>That regression typo was fixed quickly, but thanks for noticing. We're now at version 3.2.0

Oh.  I thought I looked in trunk.  Well, then you'll have some work to do on the patches I'm uploading.  Sorry to not be final with them, but I've got to move on to other things.  They'll be ready (and tested on 1.6.1.6) very soon.

/john
By: Gavin Henry (suretec) 2009-09-15 09:48:51

Don't ever be sorry for contributing!!! I don't care how long it takes, you took the time to create patches that's all I care about!

Thanks John.

Gavin.
By: John Covert (jcovert) 2009-09-15 10:01:53

OK, just looked at 3.2.0.

Why are there both IpAddr (60) and IPaddress (46)?

Let's pull one of them (and leave its OID commented as available for next allocation).  I'd suggest keeping IPAddress, since it's the one in res_ldap.conf.sample.

Also the description of AstAccountUserAgent should be Asterisk Account User Agent, not Context.


By: John Covert (jcovert) 2009-09-15 10:34:32

Alright.  I'm uploading my patches just for your reference.  Note that you don't have to change LDAP_MOD_REPLACE to LDAP_MOD_DELETE.  The documentation says it does it if the pointer is null.

You might look at my patches and be sure you got everything I did, especially the multi-valued area.

I'm not uploading the schema.

chan_sip.c.patch -- this is against 1.6.1.6 -- it's just a comment that I think is important to put into chan_sip so that we don't have further regressions.  When the update fails, LDAP registered clients stop working for outgoing calls.

res_ldap.conf.sample.patch -- this is against trunk.  we need the mapping from useragent to AstAccountUserAgent.

res_config_ldap.c.patch -- this is against 1.6.1.6.  Please be sure you at least fix the (sizeof(char), 2) -> (sizeof(char **), 1) at the first occurence, and I see that at the second you have ast_calloc(sizeof(char *), 2), which will work, but should really only be one of them, and technically should be sizeof(char **) even though in any normal architecture they'll be the same.

/john
By: John Covert (jcovert) 2009-09-15 11:11:35

OK, it's a list terminated by a zero pointer (no n-elements field), so I agree with the ast_calloc(..., 2), but the sizeof does need to be char ** to be formally correct and portable to theoretical architectures which might store char * differently than char **.


By: Gavin Henry (suretec) 2009-11-10 06:15:18.000-0600

Hi,

I'm trying to apply these patches but they all fail. Can you test them via trunk?

Thanks.
By: John Covert (jcovert) 2010-05-22 23:27:09

This has been under my radar for a while, but I'm back to working on ldap issues for one of my clients (as you may have noticed from the unrelated ldap issue I submitted today).
By: John Covert (jcovert) 2011-01-19 11:54:09.000-0600

This seems to all be OK in 1.8, so this can be closed.