[SSSD] Support for subdomains - stage 1

Stephen Gallagher sgallagh at redhat.com
Fri Mar 23 16:25:06 UTC 2012


On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote:
> Hi guys,
> it took me and Sumit some time but we finally have completed the first stage 
> of support for subdomains. I'm sending all patches in attachment.
> 
> This stage has basic support for subdomains but some pieces like PAC support 
> are left out. We agreed that those can wait for the second stage which depends 
> on some server side changes that are not ready yet. We'll keep you posted.
> 
> Please note that patches depend on Sumit's previous idmap library patch. I'll 
> get that reviewed either today or tomorrow.
> 
> #0001
> All sysdby routines that are used in later patches

Nack.

Please fix debug levels to use the new format.

Why are you creating a NULL-terminated array *and* returning the number
of elements? Usually you would do only one or the other. This isn't the
nack, just a curiosity.

I don't care for the inner loop through the elements in
sysdb_get_subdomains(). I'd rather see you use
ldb_msg_find_attr_as_string() three times. The benefit here is that you
can easily detect if one or more the elements are missing. In the
current approach, you'll just iterate through, but if the DN existed but
had no attributes associated with it, you would be left with a 'struct
subdomain_info' that contained only NULL strings... a recipe for
disaster.

Though, if you want to be *really* paranoid, you might even want to use
ldb_message_find_element() so you can test that they are in fact
single-valued as well.


sysdb_update_subdomains() desperately needs comments. It took me ten
minutes to figure out the logic there. I also question why you didn'd
include sysdb_[add|remove]_subdomain(). An explanation of this lack
should be in the source code, if there's a good reason.

The tests added by this patch depend on routines added in Patch 0003
(domain_info_utils.c). Probably this should be reordered.

> 
> #0002
> Responder routines for asking provider for a list of supported subdomains
> 

Nack.
dp_get_domains_callback() should be static.

Please add comments explaining the purpose of the "hint" argument.

Please redesign this routine to use the common sss_dp_issue_request().
This will give us a few things: 1) Bundling of concurrent 'force'
lookups, 2) processing closer to the tevent_req style.
get_domains_send() must have a matching get_domains_recv() function for
consumers to retrieve, if only to be able to find out if there were
errors.

Take a look at how sss_dp_get_account_send() and
sss_dp_get_sudoers_send() works now with sss_dp_issue_request()


If parsing fails in dp_get_domains_callback(), you're jumping to done:
rather than just treating it like other errors.

This patch also depends on domain_info_utils.c

get_domains_done() needs comments on the logic.

The creation of new_sd_list should be its own function.

> #0003
> The routine responder_get_domain is modified to look for subdomains as well 
> when looking for the correct domain descriptor
> 

Nack

As mentioned above, domain_info_utils.c should appear earlier in the
patch series. Probably it should have its own patch.

The logic in this patch looks good to me.

> #0004
> If a request for fully qualified entity (user/group) comes to the responder 
> and its domain has not been found, it updates its list of domains and 
> supported subdomains in each domain. To update the list, a get_domains call is 
> sent to each provider.
> 

Nack
This is going to need quite a bit of modification to handle the changes
required for Patch 0002. With that in mind, the logic looks fine.

> #0005
> Create a list of all domains and subdomains when the first request after start 
> comes in. To create the list, a get_domains call is sent to each provider.
> 

Nack

Similar to 0004, this is going to need to be reworked to account for
changes to 0002.

I don't understand why the addition of "if (dctx->domname)" in the
callbacks wasn't part of patch 0004. Also, a comment would got far
towards explaining why that's being done (I assume it's to handle
explicit domain requests)


> #0006
> Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
> 

Nack
Please add comments to get_next_dom_or_subdom() to explain the logic.
The code itself looks good.

> #0007
> Basic infrastructure for handling get_subdomains requests in providers.
> 

Nack

Please fix debug levels to use the new format.

You initialized be_req to NULL twice.

You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is
NULL, or else you'll crash on domains that don't support subdomain
lookups.



I'll get to the rest of the patches a bit later.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120323/62d3744a/attachment.sig>


More information about the sssd-devel mailing list