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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
