httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Denis Gervalle <...@softec.st>
Subject Re: [util_ldap.c] Your recent correction in util_ldap_cache_checkuserid().
Date Wed, 12 May 2004 23:02:52 GMT
Brad,

Your patch fix the flaw I talk about in my two previous e-mails. (Do not 
forget the main trunk which has the same flaw.)

I agree with all you said, except that my patch (Bug 27134) do not make 
unnecessary rebind. The main difference between your solution and mine 
is that I delegate the rebinding of the connection for userid checking 
to the util_ldap_connection_open in place of simulating it directly with 
a direct call to ldap_simple_bind_s.

IMHO, I think that this is a better practice for future maintenance 
since it concentrate the binding of ldap connection in only one 
function. It also simplify the recovering on ldap server down situation, 
since util_ldap_connection_open takes care of that too. It would also 
had avoided the flaw we discuss for 3 weeks :P

Best regards,

Denis Gervalle
SOFTEC sa
http://www.softec.st

Brad Nicholes wrote:
>    I'm not sure that I understand what the problem is.  By updating the
> binddn/bindpw in the connection record, we always know exactly which
> userid the connection is bound to.  During the
> util_ldap_connection_find() routine there are two searches performed. 
> The first search looks for a connection within the cache that exactly
> matches the connection credentials that were passed in.  If a connection
> is found, it is used without the overhead of having to rebind the
> connection.  If an exact connection is not found, then a second search
> is performed looking for a connection that matches the connection
> attributes but not the credentials.  If a connection is found that
> matches the connection attributes then the l->bound flag is turned off
> which forces the connection to be rebound to the correct user during the
> call to util_ldap_conection_open() routine.  The advantage to this is
> avoiding unnecessary rebinds when the credential caching is turned off. 
> The patch you submitted rebinds the connection always whether it needs
> to be or not.
>    But as a result, I did find one problem with the current code that
> does have an affect when a bind fails during the
> util_ldap_cache_checkuserid() processing.  If the bind fails while
> validating the user credentials then the connection is left in an
> unbound state although the connection record still indicates that the
> connection is bound to a specific user.  This could cause problems for a
> subsequent ldap search.  This patch unbinds the connection which will
> force it to be reinitialized the next time that it is used.
> 
> Brad 
> 
> 
> Brad Nicholes
> Senior Software Engineer
> Novell, Inc., the leading provider of Net business solutions
> http://www.novell.com 
> 
> 
>>>>Denis Gervalle <dge@softec.st> Monday, May 10, 2004 3:33:08 PM >>>
> 
> Brad,
> 
> After a second look to util_ldap patches, I notice that my original
> mail 
> is already about r1.22 which only solve the problem half way. I have 
> tried to held your attention on that problem and you seems to have 
> missed the point. Patch 1.22 has still a serious flaw when a wrong 
> binddn/bindpw are provided. IMHO, my patch  proposal (Bug 27134), which
> 
> delegate all binding operation to the single util_ldap_connection_open
> 
> function, avoids all possible trap that could messed up the connection
> 
> cache now and in the future. Hopeless, it is now no more applicable 
> as-is to the cvs tree.
> 
> However, my patch has been test against version 1.24 from the cvs. 
> Albert Lunde who did these tests have provided his conclusions under
> the 
> same bug number (Bug 27134). These results (for which he send me a 
> detailled report) shown that version 1.24 (which obviously include 1.22
> 
> patch) is entirely unreliable. We are still in close relation about 
> these problems, and he recently confirm me that my proposal provide him
> 
> good results with a number of connections proportional (as expected) 
> with the number of process involved in prefork mode.
> 
> Hope that you will have a closer look to my patch this time.
> Do not hesitate too contact me if you need more information.
> Best regards,
> 
> Denis Gervalle
> SOFTEC sa
> http://www.softec.st 
> 
> Brad Nicholes wrote:
> 
> 
>>  There are still 2 more patches for util_ldap.c that are proposed
> 
> for
> 
>>backport to the 2.0 tree.  These are r1.22 and r1.24.  These 2
> 
> patches
> 
>>should fix the problem that you discribed.  Once the backport
> 
> proposal
> 
>>recieves a third +1 vote, I will apply them to the 2.0 tree.
>>
>>Brad
>>
>>Brad Nicholes
>>Senior Software Engineer
>>Novell, Inc., the leading provider of Net business solutions
>>http://www.novell.com 
>>
>> 
>>
>>
>>>>>Denis Gervalle <dge@softec.st> Tuesday, April 20, 2004 4:04:47 PM
>>>>>
>>>>>       
>>>>>
>>
>>Brad,
>>
>>May I suggest you to have a look at bug 27134 that provide a patch 
>>regarding the rebinding of the connection during 
>>util_ldap_cache_checkuserid().
>>The recent correction you provide in the CVS of apache 2.0 has a flaw
> 
> 
>>when the binding fails (because of wrong binddn or password). In that
> 
> 
>>case, the util_ldap_connection_open will not repair the connection
>>since 
>>it will appear to be still bound. In my patch proposal, I prefer to 
>>delegate the bind to util_ldap_connection_open which make it the only
> 
> 
>>place for connection binding. It also take care of retries in case of
> 
> 
>>server down.
>>Hope this helps,
>>Regards,
>>
>>Denis Gervalle
>>SOFTEC
>> 
>>

Mime
View raw message