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.
