samba4_ldap_controls

[Simo Sorce]

On Mon, 2005-12-12 at 12:43 +1100, tridge at samba.org wrote:
> Simo,
> 
> Congratulations on the progress you've made on the ldap controls code!
> 
> I think the general approach is good, and we should aim to get this
> into the main Samba4 branch as soon as possible.
> 
> Here are a few more specific comments:
> 
> 1) It looks like you have a bug in paged_search(), when looking for a
> matching cookie in the list of results_store structures. The variable
> 'current' is not updated in the loop, so it will spin forever if the
> cookie doesn't match. I'd suggest something like:
> 
>  for (current=private_data->store; current; current=current->next) 
> 
> instead of the while loop

wooops thanks.


> 2) I think much of the code could be broken up into smaller, simpler
>    functions. For example, in the paged search control, adding a
>    ldb_request_has_control() function would get rid of that awkward
>    loop, and as you suggested on irc, you could have functions to
>    add/remove controls from the lists. Also, pulling the if
>    (current==NULL) case out into a separate function would be good
>    (the code that handles a new request).

yeah I will build a set of helper functions in common/ldb_controls.c

> 3) the code is currently vulnerable to a DOS attack. I think we need a
>    limit on the number of paged searches active per client, and maybe
>    a timer to destoy inactive searches? I wonder how openldap handles
>    this?

Yeas I know, I was waiting to talk to you to find out the best way to
handle this kind of problems. The same may happen in the sorting module.

> 4) why was messageid changed from a uint32_t to an int?

In ldap the message is defined as signed integer, I had gcc complaining
about signedness in the (un)marshalling code.
We can put it back to uint32_t if needed.

> 5) in ldap_ildap.c you have:
> 
> 	if (res->controls) {
> 		*control_res = talloc_steal(conn, res->controls);
> 	}
> 
> better to remove the if() and just have this:
> 
> 	*control_res = talloc_steal(conn, res->controls);
> 
> as talloc_steal() knows how to deal with NULL. This also means the
> result is guaranteed to be initialised to NULL on no controls (which
> prevents confusion in callers).

ok, good to know.

> 6) the check for remaining critical extensions in each of the backends
>    should be broken out into a common function

yes

> 7) the ldb_dn_explode_or_special() function looks strange to me. It
>    looks like it doesn't explode the main part of the DN at all? What
>    is this function meant to handle? (a comment would be nice too!). 

It was more a hack than a definitive choice.
I'm still unsure which is the best approach.

>    I would have expected you to extend ldb_dn_explode() to look for a
>    leading '<' in any component, and mark the DN as having this
>    extended form for that component. Then extend the linearise
>    functions to do the opposite, if needed. The "<GUID" check looks
>    very ad-hoc to me. Are you sure its right?

No, it is not right, but a quick hack, I still need to ponder well which
options do we have.

> 8) looking at the 2nd stage init, I see you've done it exactly how I
>    suggested on IRC, but I think I must have had too much/little
>    coffee at the time, and my suggestion wasn't very good! It looks
>    rather strange having that return NULL. Maybe we should go back to
>    the old init, then have a new LDB_OP_INITIALISE separate ldb
>    operation, and guarantee that LDB_OP_INITIALISE will be the first
>    ldb_request given. That seems more a more natural approach, and it
>    allows modules that don't need the 2nd stage init to just ignore it
>    in their operation switch statement

I will think some more about that.

> 9) as discussed on irc, I think we need to extend
>    lib/ldb/tools/cmdline.c to allow for controls to be specified to
>    all the existing ldb tools, rather than having a new
>    ldbtest_controls.c tool.

Yes, the problem is finding out a decent syntax.

> 10) I get LDAP_UNAVAILABLE_CRITICAL_EXTENSION when trying
>     ldbtest_controls with the sort control against a w2k3 server. Does
>     that one work for you?

It worked, I will recheck I have not missed anything with latest
commits, have you provided an attribute? I'm not sure w2k3 like
distinguishedName (the default I used).

> More comments as I come across them ....

Thanks, I really needed some review at this stage.

Simo.

-- 
Simo Sorce    -  idra at samba.org
Samba Team    -  http://www.samba.org
Italian Site  -  http://samba.xsec.it