ptclient & ldap changes

[Igor Brezac]

Wesley Craig wrote:
> On 30 May 2008, at 13:50, Igor Brezac wrote:
>> 1) I suggest that you keep ldap_sasl for backward configuration 
>> compatibility.  ldap_sasl  name is used for sasl vs non sasl binds.   
>> Note that 'ldap_id' needs to have authorization to proxy.
>
> I left ldap_sasl in place, but removed it's ability to control the 
> proxy function.  Instead, it continues to control whether sasl bind is 
> used.  Are you suggesting that both ldap_sasl & ldap_proxy_authz would 
> be required for proxy to be enabled?  I'd be OK with that, presuming 
> that ldap_sasl is indeed required for proxy authz to work.  The code 
> has a dependency on the proxy authz ldap control, I don't recall that 
> it requires sasl.  I suspect not, but I haven't actually dug into it.

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.
>
>> 2)  I suppose you can have ldap_group_method: attribute, but this is 
>> not how ldap data is typically used for groups.  Also, I suggest that 
>> ldap_group_attribute be used instead of ldap_member_attribute.  As 
>> you correctly described ldap_group and ldap_member do two different 
>> things and your implementation would be a bit confusing.   You can 
>> possibly default ldap_group_attribute to the value of 
>> ldap_member_attribute.
>> I personally do not like ldap_group_method: none, mabe Kan can chime 
>> in.  This option basically allows for an arbitrary group identifier 
>> (potentially non existing one) to be assigned to a mailbox.
>
> There are two ways to obtain the list of groups a user is a member of, 
> corresponding to ldap_member_method "filter" and "attribute".
>
>     filter) A search is executed, typically (uid=%u) in an "ou=groups" 
> tree.  The list of DNs found correspond to the groups the user is in.
>     attribute) Rather than execute a search, the attributes in the 
> users entry are used to form the list of groups the user is in.
>
> The problem is that group name validation doesn't have both models.  

I suppose you could have both methods, but is that really needed?   Each 
group should have one entry in DIT.

> I'm not sure how this discrepancy jibes with your understanding of how 
> ldap data is typically used for groups.
> As you can see from the patch, I haven't added a 
> "ldap_group_attribute" and don't use "ldap_member_attribute".  Both 
> ldap_group_method's use ldap_group_filter.  The difference is in how 
> the result is treated.
> Perhaps you're suggesting that ldap_group_* just be dropped, since 
> having two distinct configurations for highly related group 
> information is confusing and probably unnecessary.  I'd agree with 
> that, but I 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.  :)

>
> I don't think the "none" method is any more worrisome than permitting 
> someone to assign rights to a group they are not in.
>
>> 3) This seems unnecessary, but can you explain a little more?
>
> It's probably not strictly necessary to change the default size limit, 
> but the previous error handling treats "size limit exceeded" the same 
> as "not found".  That is a problem, since in some cases the "size 
> limit exceeded" is not an error at all.  A very common result of the 
> current default & code is that when ldap_member_method is "filter", 
> the user gets exactly no groups and no error message indicating why.  
> A more sensible solution would be to make the limit 10 or 100.  Is 
> there a limit on the number of groups Cyrus allows?  For 
> ldap_member/group_method of "attribute", a limit of 1 is probably 
> smart, since it reduces extraneous traffic.
>

Now looking at the code both ldap_member methods are broken. :( 
- The 'attribute' method stores group DNs in authstate struct which is 
not correct.  Additional search is needed to get group names.
- The 'filter' method, as you pointed out, gets one group at most.  
Changing ldap sitezlimit would fix this issue.

Cyrus does not limit on the number of groups.  I suppose LDAP_NO_LIMIT 
can be used to match other mechanisms, or implement configurable limit 
that can be used in other mechanisms.

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...

-Igor

> :wes
>
>> Wesley Craig wrote:
>>> I have a number of ptclient & ldap bug fixes and improvements to make:
>>>
>>>     1) In 2.3.12p2, if ldap_sasl is enabled, user DNs are obtained 
>>> through SASL authN/Z proxying.  This assumes that the LDAP server 
>>> supports authN/Z proxying and that ptclient/ldap has authorization 
>>> to proxy for all users.  I've moved this option under a new 
>>> configuration option, ldap_proxy_authz, since the authZ proxying is 
>>> more or less orthogonal to using SASL for LDAP authN.
>>>
>>>     2) Groups have two LDAP configurations, one for populating the 
>>> groups a user belongs to and a second for validating a (new) group 
>>> name.  In 2.3.12p2, those two configurations suffer from 
>>> non-parallel construction.  In particular, ldap_member_method allows 
>>> both "attribute" and "filter", while the ldap_group_* configuration 
>>> has no "_method" configuration, implicitly assuming "filter" 
>>> instead.  I've added a ldap_group_method configuration, with three 
>>> options, "filter", "attribute" and "none".  "none" allows any string 
>>> that can be canonicalized to be used.  "filter" works just like 
>>> ldap_group_* was working -- exactly one DN may be returned.  
>>> "attribute" looks for at least one DN to be returned.  A correct 
>>> "attribute" configuration searches for the attribute used in 
>>> ldap_member_attribute.  The assumption is that if anyone has the 
>>> group attribute, it is a valid group name.
>>>
>>>     3) I changed the default ldap_size_limit to 2.  I also inserted 
>>> some additional checks in the code to specifically look for cases 
>>> where size limit is exceeded.  These may or may not be errors, 
>>> depending on what you're looking for.
>>>
>>>     4) I fixed two small bugs in ptloader.c, one where unused memory 
>>> to syslog'd and another where the error message returned from the 
>>> ptloader module isn't null terminated when being passed to auth_pts.c.