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