RE: [PATCH]Re: Adding a new user DN cache to authnz_ldap...

2004-11-04 Thread Brad Nicholes
Good point.  I will get the patch committed

Brad

>>> "Jari Ahonen" <[EMAIL PROTECTED]> Thursday, November 04, 2004
7:39:01 AM >>>
Brad,

I think this patch should be applied to the current HEAD
util_ldap.c code. It prevents util_ldap_cache_getuserdn()
timestamping cache entries with bindpw.

- Jari

- cut here - cut here -
--- util_ldap.c.orig2004-11-02 00:43:24.0 +0100
+++ util_ldap.c 2004-11-04 15:34:23.0 +0100
@@ -1096,7 +1096,12 @@
 /* Nothing in cache, insert new entry */
 util_ald_cache_insert(curl->search_cache,
&the_search_node);
 }
-else {
+/*
+ * Don't update lastbind on entries with bindpw because
+ * we haven't verified that password. It's OK to update
+ * the entry if there is no password in it.
+ */
+else if (!search_nodep->bindpw) {
 /* Cache entry is valid, update lastbind */
 search_nodep->lastbind = the_search_node.lastbind;
 }
- cut here - cut here -
> 


Re: Adding a new user DN cache to authnz_ldap...

2004-10-29 Thread Brad Nicholes
  Since the checkuserid cache does not allow NULL or blank passwords
today and its purpose is for authentication, I'm just not completely
comfortable allowing another function whose purpose it is to simply
lookup user DN's to insert NULL or blank password entries.  It seems
like it would potentially open up holes where there wasn't a problem
before.  Adding a separate cache would just eliminate any potential
problems.

Brad

>>> Jari Ahonen <[EMAIL PROTECTED]> Friday, October 29, 2004 2:51:47 PM
>>>
Brad Nicholes wrote:
> I see your point about the req structure being tied to closely to 
> authentication phase so that authorization will not work without it.

> Now I understand the need for util_ldap_cache_getuserdn() which is 
> the real patch that needs to be added.  In other words, the relevant

> bug is actually 28253 not 31898.

In a way they are both relevant because applying the changes
needed for 28253 will cause the behaviour in 31898.

> The question now is should the same search cache used by
> util_ldap_cache_checkuserid() also be referenced in
> util_ldap_cache_getuserdn().  I guess it could be, but I am thinking
> that even though some user DN's might be stored in two different
> caches, it would be safer to create a new cache just for user DN's.

I think the same search cache should be used. To me it makes no
sense to have two caches with more or less the same data in them.
Having a record in search cache with bindpw=NULL should be a
good enough indicator that the particular record has not
been through password verification (In fact the commets in
util_ldap_cache.h suggest this might have been planned at some
point). It is easy enough to check if bindpw exists in a cache
record in util_ldap_cache_checkuserid() to prevent access with
blank passwords. And a cache record created by
util_ldap_cache_checkuserid() can be used with
util_ldap_cache_getuserdn() since that function does not need
to care about passwords.

I have had a server running with this kind of setup for quite
a while now, it was just that the shared cache changes done
in 2.0.52 added the extra check that started to crash my server
when util_ldap_cache_checkuserid() tries to strcmp() that NULL
password.

- Jari

-- 
--
| Jari Ahonen   | Progress Software Europe   |
| [EMAIL PROTECTED]  | Schorpioenstraat 67|
| Tel: +31-10-2865 700  | 3067 GG Rotterdam  |
| Fax: +31-10-2865 225  | The Netherlands|
||
|   "Once you've made the commitment to recreation,  |
| there is no turning back." |
--


Adding a new user DN cache to authnz_ldap...

2004-10-29 Thread Brad Nicholes
   I see your point about the req structure being tied to closely to
authentication phase so that authorization will not work without it. 
Now I understand the need for util_ldap_cache_getuserdn() which is the
real patch that needs to be added.  In other words, the relevant bug is
actually 28253 not 31898.  The question now is should the same search
cache used by util_ldap_cache_checkuserid() also be referenced in
util_ldap_cache_getuserdn().  I guess it could be, but I am thinking
that even though some user DN's might be stored in two different caches,
it would be safer to create a new cache just for user DN's.  This way,
util_ldap_cache_checkuserid() can continue to enforce the no NULL or
blank passwords as it was originally designed, as well as avoiding a
potential false positive if util_ldap_cache_getuserdn() were to insert a
NULL password entry thus allowing a user without a password to pass
authentication when the cache is checked by
util_ldap_cache_checkuserid().  

Brad

>>> Jari Ahonen <[EMAIL PROTECTED]> Friday, October 29, 2004 8:53:41 AM
>>>
Brad,

What I've written below might sound that I'm just arguing that
things should be done my way. Don't take it like that, I'm just
trying to make it easier for you to understand my viewpoint on
this issue.

Brad Nicholes wrote:
> So basically what you did was write a function that puts bad data 
> into the cache that the cache was never designed to handle or allow.

I'm not sure I would agree with that. But then, I haven't seen
the design specifications for the cache. I based my assumptions
on comments in util_ldap_cache.h which I thought implied that
NULL bindpw is allowed in a cache record.

> I would suggest closing the first bug that you wrote and opening 
> another one that states that you would like to add new functionality
>  to util_ldap through a functioned call util_ldap_cache_getuserdn().

That's what I tried some time back with bug 28253 but it didn't
go very far...

> No, there are two other caches that are implemented by util_ldap. 
> They are the compare cache and the comparedn cache.  These caches
are
>  primarily used during the authorization phase rather than the 
> authentication phase.

True. And I understand that these caches exist and what they
are used for. But what is missing in the current util_ldap
implementation is a function that allows you to get the user DN
out of the directory without having to supply the correct user
password. None of the functions defined in include/util_ldap.h
do that, yet this is what is needed in order to do LDAP group
authorizations.

Even the Apache 2.1 mod_authnz_ldap will not be able to authorize
a request if that request is authenticated with some other module.
This is because the user DN (In authn_ldap_request_t *req structure)
is attached to the request in authentication phase and recalled
later in authorization phase. If the request is not authenticated
with mod_authnz_ldap, the req structrure does not exist and with
current util_ldap there is no way to get the DN without knowing
the user password. The end result of all this is that it is
impossible to do authorization based on LDAP groups if you are
authenticating with some other module.

So we need some way of creating that authn_ldap_request_t structure
in the authorization phase of the request. I thought the easiest
way of doing this would be making a copy of
util_ldap_cache_checkuserid() without password checking that would
store the results in the cache with bindpw=NULL. But doing so
caused the problems I reported in bug 31898. Storing bindpw=""
would cause slightly different problem because then the current
util_ldap_cache_checkuserid would allow anyone in with blank
password as long as the user had authenticated once and the cache
record hasn't expired.

Alternative fix for my problem is for myself to store the cache
entries with bindpw="" and move the blank password check that
is now somewhere in the middle of util_ldap_cache_checkuserid()
into the beginning of the function, before the cache check. In
fact maybe that blank password check should be one of the first
checks anyway since it makes no sense to do all that cache and
LDAP searching if blank passwords aren't allowed at all.

- Jari

-- 
--
| Jari Ahonen   | Progress Software Europe   |
| [EMAIL PROTECTED]  | Schorpioenstraat 67|
| Tel: +31-10-2865 700  | 3067 GG Rotterdam  |
| Fax: +31-10-2865 225  | The Netherlands|
||
|   "Once you've made the commitment to recreation,  |
| there is no turning back." |
--