ptclient & ldap changes

[Igor Brezac]

Wesley Craig wrote:
> On 31 May 2008, at 00:06, Igor Brezac wrote:
>> sasl used to be required for ldap proxy authz, but I do not think 
>> this is the case any more.  I suggested that both ldap_sasl and 
>> ldap_proxy_authz do the same thing.
>
> Perhaps I misunderstand you.  Since SASL authN and proxy authZ are 
> more or less completely orthogonal, why would you have them do the 
> same thing?  I propose that ldap_sasl control the way bind is done.  
> And ldap_proxy_authz is used to control how user DNs are obtained.
Your patch breaks existing configurations, we usually try to preserve 
configuration compatibility when possible.  Otherwise I am fine with 
your approach.   Maybe automatically set ldap_proxy_authz to true when 
ldap_sasl is turned on and when ldap_proxy_authz is not explicitly 
specified in the config?
>
>> I suppose you could have both methods, but is that really needed?   
>> Each group should have one entry in DIT.
>
> If you're using the "attribute" method for group population, the group 
> may (should?) have zero entries in the DIT.  That's just exactly my 
> point.  The code I've added is a (clever) way to validate group names 
> when groups don't have their own DNs -- which is frequently the case 
> when membership is recorded in the user's entry.

OK.  This makes sense.
>
>>> ... fixing the current flaw was simpler and more compatible than 
>>> refactoring ptclient/ldap.c entirely.
>>
>> It'd be nice if there is a simpler way to configure group (membership 
>> and validation) in pt/ldap.  :)
>
> True.  Feel free to propose something.  I've fixed the problem I have, 
> and improved the code generally.  Further improvements are welcome, of 
> course :)

I was hoping you can make some improvement since I wrote the original 
code.  :)   Unfortunately I have not had time to mess with it in years.  :(

>
>> Now looking at the code both ldap_member methods are broken. :( - The 
>> 'attribute' method stores group DNs in authstate struct which is not 
>> correct.
>
> You're misunderstanding how ptsmodule_make_authstate_attribute() 
> works.  The "attribute" method retrieves the attribute named in 
> ldap_member_attribute from the user's DN.  The syntax of that 
> attribute is assumed to be in a format suitable for auth_state.  No 
> additional search is necessary.

True, although it is broken when values of ldap_member_attribute are 
represented with DN.  You are probably right, not worth messing with.

>
>> - The 'filter' method, as you pointed out, gets one group at most.
>
> Again, not exactly.  If you use the default size limit and are in more 
> than one group, you get no groups -- size limit is exceeded.

If you are a member of one group, it works.  We are saying the same 
thing.  Bottom line and I think you'll agree, the code needs improvement.

> LDAP_NO_LIMIT might be a useful way to handle this case, but you're 
> still left wondering how to handle the case where the server's size 
> limit is exceeded.  Do I populate what I got back?  Do I populate 
> nothing?
>

I suppose LDAP_NO_LIMIT is lesser of two evils.  It seems impractical 
for a user to be member of several hundred groups...

>> It'd be nice if all this complexity can be moved to the ldap 
>> configuration, similar to the way sasl simplified things for user 
>> stuff, Dynlist/dyngroups looks interesting...
>
> sasl simplifies what?  IMHO, using sasl makes almost everything more 
> complex.

I respectfully disagree, maybe it is just me.  :)  I suppose for 
openldap admins it may be more complex. 
In terms of cyrus imap admin, all they should need to know is ldap 
username/pass.  They should not need to have any knowledge of DIT, 
filters, bind dn, etc...

>
> Cyrus supports two schemes for using groups.  Rather than have more or 
> less every configurable LDAP knob visible in imapd.conf, I think it 
> would be better if it "just worked".  However, I don't think LDAP is 
> very close to that goal as a technology.  A more achievable goal for 
> Cyrus's use of LDAP would be posting recipes for the two schemes in 
> the wiki.
>

Agreed, but I do not think it is far off.

-Igor