[SSSD] Support for subdomains - stage 1

Jan Zeleny jzeleny at redhat.com
Mon Apr 2 08:07:00 UTC 2012


Ok, all those changes took me a while but I'm finally done. Please note that 
patch #0003 has been split into two and the part with utility functions has 
been moved right after sysdb routines. There is a circular dependency between 
the two which I don't think is worth trying to solve, neither patch would work 
without the other. This patch split caused the numbering to be shifted by one.

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

Done

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

That was a leftover from times when the design was not clear.

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

Changed

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

I think we don't have to be that paranoid.

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

Hm, I didn't write the routine and I could read it quite well. Nevertheless I 
added some comments. Those routines functions you are missing would be rather 
simple (almost aliases), that's why they are missing. Do you think there is a 
need for them?

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

There is actually a circular dependency, that's why I left it this way. The 
only thing I could do is split the patch and move the part with tests. However 
compared to the original state, I have split the "Modified 
responder_get_domain()" patch and moved the part with utility functions.

> > #0002
> > Responder routines for asking provider for a list of supported subdomains
> 
> Nack.
> dp_get_domains_callback() should be static.

Done

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

Done

> 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()

Done

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

The behavior has changed completely

> This patch also depends on domain_info_utils.c

Done

> get_domains_done() needs comments on the logic.

Done

> The creation of new_sd_list should be its own function.

I disagree. The funcion would be called on this one place anyway so there is 
probably only a little gain in doing that.

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

Done

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

Not really, only renamed the entry procedure and everything works. Most of the 
changes were introduced in the updated patch #0002

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

Same as previous comment

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

Yes, that was fix that should have been part of the previous patch. Fixed now. 
The reason is obvious IMO - the same behavior was and still is in the original 
code of functions like nss_cmd_getgrnam().

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

IMO the logic is straightforward (for the record I didn't write the original 
code ;-)) but I still added some comments just to be sure.

> > #0007
> > Basic infrastructure for handling get_subdomains requests in providers.
> 
> Nack
> 
> Please fix debug levels to use the new format.

Done

> You initialized be_req to NULL twice.

Fixed

> 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'm not sure what do you mean here, how could it be null when it's not a 
pointer?

> One additional point about this patch. I don't like that you're forcing
> the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer
> that we add a subdomain_provider option, which defaults to being the
> same as the ID provider. I prefer to maintain the distinction in case we
> ever decide to have alternate mechanisms for determining the subdomains
> (in IPA or AD).

I discussed this with Sumit to be sure and we agreed that subdomains are 
supposed to be tightly bound to the ID provider. One of the reasons is that ID 
provider has to sort out all following requests so it will also needs to keep 
track of all subdomains.

> > #0008
> > Add the infrastructure in IPA provider which fetches a list of handled
> > subdomains from server and stores in in sysdb.
> 
> Nack.
> 
> Please don't use variables named 'state' if they're not the state for a
> tevent_req. It gets confusing. Please rename 'struct subdomains_state'
> to 'struct ipa_subdomains_ctx' and call the variable something like
> ipa_sd_ctx.

Actually the name you suggest is already used so I came up with another one: 
struct ipa_subdomains_req_ctx

> ipa_subdomains_get_conn_done() should follow the multiple search base
> pattern, not really because we expect to support multiple bases but
> because we *should* support filters if they've been added. It's possible
> that a deployment might choose to filter out certain domains from a
> particular client using this method.

Done

> You have the wrong debug message if new_domain_list[c] fails to be
> allocated in ipa_subdomains_parse_results(). It lists talloc_array.

Done

> I'd prefer if you added an ipa_subdomain_map and used that for
> determining the attribute names to use in
> ipa_subdomains_parse_results(). Mostly for consistency.

I don't think there would be much gain, the code almost wouldn't change, only 
names of attributes would be different.

> > #0009
> > Currently the connections to the data provider use the same name as the
> > domain. With sub-domains the name of the sub-domain cannot be used to
> > find the right data provider connection, hence we store the name of the
> > connection in a new member.
> 
> Ack
> 
> > #0010
> > Support for subdomain name given to provider when calling
> > get_account_info
> 
> Nack (minor)
> 
> Some whitespace issues in be_get_account_info (leading tabs) around the
> req->domain allocation.

Done

> > #0011
> > New ID-related config options for subdomains, these have to be present
> > because IPA provider doesn't provide these values and defaults need to
> > be implemented. Having defaults on the responder level didn't seem right
> > since the policy might differ for each domain.
> 
> Nack.
> 
> I don't think this really makes sense at all. In most cases, users will
> prefer to use the value on the LDAP server. If they choose to override
> it, they'll do so through the existing override options (in the case of
> override_homedir, it already has %d available anyway.
> 
> We definitely don't need separate handling for shells. I can *kind of*
> see a value if you wanted to have only subdomains have a non-default
> location. I'm not sure I like that though. I feel like it's probably
> more complexity than we need.

I think you possibly missed the point. The point is that this information is 
NOT on the server, therefore we need a value that will fill it in. Otherwise 
only a blank field will be stored in sysdb and returned to the client

> > #0012
> > The routine expand_homedir_template is now used on more place, therefore
> > it was convenient to move it to util code.
> 
> Ack
> 
> > #0013
> > s2n extended LDAP operation - this is used to ask IPA server for entities
> > from supported subdomains
> 
> Nack.
> 
> This code really needs comments. s2n_encode_request() and
> s2n_response_to_attrs() are incomprehensible.
> 
> Please make ipa_s2n_exop_send() and  ipa_s2n_exop_recv() static.

This should be all done, my thanks go to Sumit

> > #0014
> > Extend IPA ID provider toi support fetching information for entities
> > within subdomains
> 
> Ack
> 
> > #0015
> > Support for routing PAM request from reponder to the right provider.
> > Similar case as patches #3-#6 in NSS responder but much more simple.
> 
> Ack
> 
> > #0016
> > Basic support (or rather un-support) for subdomains in auth providers.
> > Basically it's designed to acknowledge that the request is not for the
> > main domain but for subdomain and in that case it supports only
> > SSS_PAM_ACCT_MGMT and returns error otherwise. Note that similar thing
> > as the IPA backend change in last hunk is probably not necessary for
> > krb5 backend since when using it, there will be no subdomains for that
> > domain.
> 
> Ack
> 
> > #0017
> > Support providing sysdb context and domain info per request, not per
> > backend context. This is necessary for the next patches.
> 
> Ack
> 
> > #0018
> > Accept be_req instead of be_ctx in access providers. Together with the
> > previous patch, this is needed for the next one.
> 
> Ack
> 
> > #0019
> > Detect if the authorization request came for a user in subdomain and if
> > yes, replace the original backend-wide sysdb context and domain info
> > with their subdomain specific replacements.
> 
> Ack
> 
> > #0020
> > Use request-wide sysdb context and domain info instead of their
> > backend-wide counterparts in HBAC.
> 
> Ack

Thank you
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subdomains.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 29969 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120402/48322137/attachment.bin>


More information about the sssd-devel mailing list