[SSSD] Support for subdomains - stage 1

Jakub Hrozek jhrozek at redhat.com
Thu Apr 5 19:17:06 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.

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

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

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

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

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


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







More information about the sssd-devel mailing list