[SSSD] Support for subdomains - stage 1

Jan Zelený jzeleny at redhat.com
Thu Apr 5 08:46:55 UTC 2012


> On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
> > Once more, this time rebased on top of current master.
> > 
> > Thanks
> > Jan
> 
> Patch 0001: Sysdb routines for subdomains
> Ack, can you just add a DEBUG message in case sysdb_transaction_commit()
> fails?

Done

> Patch 0002: Add some utility functions for subdomains
> Ack
> 
> Patch 0003: Add conn_name to allow different names for domains and
> connections
> Ack
> 
> Patch 0004: Responder part of the subdomain retrieval work
> "get_domains_timeout" is not mentioned in neither configAPI nor manpages
> 
> +        if (ret == EOK) {
> +            DEBUG(SSSDBG_TRACE_FUNC,
> +                  ("Last call was too recent, nothing to do!\n"));
> +            ret = EOK;
>         ^^ this is redundant
> +            goto done;
> +        } else if (ret != EAGAIN) {

Fixed

> Please stick to either goto or return in get_domains_next()

Done

> Two review items from the original review are not addressed yet:
> There is no explanation of the "hint" parameter (and without one, it's
> hard to say what to do with the FIXME) and there is no
> sss_dp_get_domains_recv()

The hint is actually described right at the point where it is given as an 
argument to dbus call.

sss_dp_get_domains_recv() added

> Patch 0005: Modified responder_get_domain()
> It is a convention to put a TALLOC_CTX we allocate on as the first
> parameter (responder_get_domain())

I know, I mentioned this to Sumit when I was reviewing reviewing the code once 
but I forgot to fix it. Fixed now.

> I think that it might be useful to store
> CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from
> confdb on every request. The case where confdb_get_int either errors out
> or the timeout is negative should at least trigger a DEBUG message.

Done and done

> As a general question, why are the domain names case sensitive but
> subdomain names seem to be case insensitive? For example
> check_last_request() is doing a case-sensitive strcmp but
> responder_get_domain is doing a strcasecmp.

I wrote this when the same question was on my mind (you can notice the FIXME 
right above it). It will be safer to treat all domains and subdomains case 
insensitively. Is there any other place where you noticed this?

> Also please split the long DEBUG() statement in responder_get_domain()
> into two lines, it doesn't fit on my screen :-)

Done, sorry for the inconvenience ;-)

> Patch 0006: Retrieve subdomains if there is a request for fully
> qualified user
> Looks OK, will just need amendment when sss_dp_get_domains_recv() is
> implemented. Currently the patch supports users and netgroups, are there
> plans to also support services and the SSH responder? I think we can
> leave out the sudo responder for now, sudo doesn't support domains
> (yet?). Autofs in theory can send a request for a fully-qualified map,
> but it doesn't so the work can be done later, although I would prefer a
> ticket to show that we are aware of the issue.

The amendment has been done. About support of another responders - I honestly 
don't know. I wasn't aware that a request for fully qualified service/ssh 
entity can be given to SSSD. Perhaps Sumit will know more.

> Patch 0007: Ask for subdomains in responder in the first request after
> startup
> Ack, with the same comment about services and SSH as with patch #6.
> 
> Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
> Ack
> 
> Patch 0009: data provider: added subdomains
> If the bandaid patch I posted on the list the other day is accepted,
> there should be a check for the result of the sbus_get_connection() call.

Done

> The comment about fast reply in be_get_subdomains() seems misleading to
> me, there is no check for BE_REQ_FAST and the whole code seems like it
> can handle online requests only.

Yes, the comment was copied from somewhere else I think. Fixed

> As stated in the original review, there needs to be a check for
> "!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before
> calling be_file_request, otherwise you'll get crashes on domains with no
> subdomain support.

Done

> Patch 0010: IPA: Add get-domains target
> Please use the new DEBUG level macros.

Done

> You should use ipa_parse_search_base(), not sdap_parse_search_base()
> in the IPA code.

Done

> ipa_subdomains_search_base is not documented and not present in the
> configAPI.

Done

> ipa_subdomains_parse_results(): please use goto here instead of return
> +        if (new_domain_list[c] == NULL) {
> +            DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));
> +            return ENOMEM;
> +        }

Done

> The original review suggested to add a ipa_subdomain_map. If you don't
> want to go that route (I would prefer the map, too, there is a map for
> every option family in SSSD), please at least #define the attributes
> like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if
> needed. Hardcoding attribute names is bad.

Agreed. I don't want to use the map because there is really no reason for 
that. But defining constants is a good idea.

> Patch 0011: Add domain name to get_account_info request
> Ack
> 
> Patch 0012: New config options for subdomains
> See my reply in the other part of this thread. If you are goig forward
> with these options, add them to the configAPI.

Based on previous discussion I removed the part which is setting default 
shell. I intend to keep the homedir for two reasons:

I recalled why I created the config option in the first place. It's because 
override_homedir can't be set per domain. After your comment yesterday, I 
checked to be sure and IMO the man page is incorrect and the override_homedir 
only works for responder.

The second reason is that admins might want to separate homedirs for native 
users from homedirs for trusted users on some systems.

> Patch 0013: Moved expand_homedir_template() from NSS responder to
> utility code
> Can you change the DEBUG macros to the new format?

Done

> Patch 0014: Add s2n extended operation
> Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of
> the debug messages says "go [%s]", should be "got [%s]". Otherwise looks
> fine.

Done

> Patch 0015: Add ID operations in subdomains
> Ack
> 
> Patch 0016: Send PAM requests for subdomains to the right provider
> Ack
> 
> Patch 0017: Basic support for subdomains in auth provider
> Ack
> 
> Patch 0018: Carry sysdb context and domain info in be_req structure
> Ack
> 
> Patch 0019: Accept be_req instead if be_ctx in LDAP access provider
> Ack
> 
> Patch 0020: Detect subdomain request in IPA access provider
> Ack
> 
> Patch 0021: Utilize sysdb context within be_req in HBAC
> Ack

One more thing, I removed the conn_name from patch #2. I intend to keep 
new_subdomain() in patch #1 - these two are closely bound together and there 
is a circular dependency between them IIRC. Other than this I believe all 
patches are compilable.

Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subdomains.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 31121 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120405/62a32ed4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120405/62a32ed4/attachment.sig>


More information about the sssd-devel mailing list