[SSSD] Support for subdomains - stage 1

Jan Zelený jzeleny at redhat.com
Fri Apr 6 11:52:36 UTC 2012


> On Thu, Apr 05, 2012 at 10:46:55AM +0200, Jan Zelený wrote:
> > > 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
> 
> Ack
> 
> > > Patch 0002: Add some utility functions for subdomains
> > > Ack
> 
> Ack
> 
> > > Patch 0003: Add conn_name to allow different names for domains and
> > > connections
> > > Ack
> 
> 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
> 
> Added but not used anywhere (and not exported in any header). The point
> is that the requests that call sss_dp_get_domains_send also need to call
> sss_dp_get_domains_recv in their callback to be notified of potential
> failures.

I could have sworn I added that as well. Nevertheless, now it's really there.

> > > 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?
> 
> Only in unit tests where it doesn't really matter.
> 
> > > 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 ;-)
> 
> OK, ack now
> 
> > > 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.
> 
> No, sss_dp_get_domains_recv() is not used anywhere.

Fixed

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

Fixed

> > > 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
> 
> Ack
> 
> > > 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.
> 
> Ack
> 
> > > 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.
> 
> What was your testcase and what exactly wasn't working for you?
> 
> The option works fine for me in a subdomain and the code seems to be quite
> clear (get_homedir_override())

I know it is straightforward, but it didn't work for me. However when looking 
a bit deeper I discovered a bug in completely different part of the patch set. 
This is now fixed and the override works as expected.

> > The second reason is that admins might want to separate homedirs for
> > native users from homedirs for trusted users on some systems.
> 
> Then let's extend the homedir template by a subdomain name or "full
> domain name" option.

I'm not sure about this. For one thing this functionality is already there. 
But I was considering scenario when the path to subdomain homedirs is 
completely different. Something like this:

/home/ipa/%u - the main IPA domain
/home/ad/%u  - AD subdomains

I there is a consensus that it is not necessary, I can remove it. But frankly 
I can see us having exactly this RFE several months later.

> > > Patch 0013: Moved expand_homedir_template() from NSS responder to
> > > utility code
> > > Can you change the DEBUG macros to the new format?
> > 
> > Done
> 
> Ack
> 
> > > 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
> 
> GCC emits these warnings when compiling with -O2:
> src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done':
> src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared
> here
> src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared
> here
> src/providers/ipa/ipa_s2n_exop.c: In function
> 'ipa_s2n_get_acct_info_send':
> src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared
> here

All fixed

> > > 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.
> 
> They are compilable on their own which is great.


Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subdomains.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 30900 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120406/fd6e1cc0/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/20120406/fd6e1cc0/attachment.sig>


More information about the sssd-devel mailing list