On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
> Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> > On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
> > > The mails grow huge. I'm going to trim the response a little and omit
> > > patches that were already acked elsewhere.
> > >
> > > Patch 0001: Ack
> > > Patch 0002: Ack
> > > Patch 0003: Ack
> > > Patch 0004: Ack
> > > Patch 0005: Ack
> > >
> > > On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
> > > > > > > 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
> > >
> > > Please also remove "subreq" from setnetgrent_send() and the
> > > declaration of setnetgrent_cb().
> > >
> > > Patch 0007: Ack
> > > Patch 0008: Ack
> > > Patch 0009: Ack
> > > Patch 0010: Ack
> > > Patch 0011: 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.
> > >
> > > Can you file a bug if the override is not working in a domain? It
> > > should and if something is not clear, then it's most probably a docs
> > > bug..
> > >
> > > > 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.
> > >
> > > If the AD users are to be treated differently than native IPA users
> > > the new option is OK.
> > >
> > > Patch 0013: 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
> > >
> > > Ack
> > >
> > > Patch 0015: Ack
> > > Patch 0016: Ack
> > > Patch 0017: Ack
> > > Patch 0018: Ack
> > > Patch 0019: Ack
> > > Patch 0020: Ack
> > > Patch 0021: Ack
> > >
> > > Becuse the only remaining review issue is a trivial one and I know
> > > that Stephen has been reviewing the patches as well, feel free to
> > > resend another patch set when/if Stephen sends his comments.
> > >
> > > Great work!
> >
> > There seems to be a startup error with non-IPA back ends:
> >
> > (Tue Apr 10 17:54:11 2012) [sssd[be[ldap]]] [load_backend_module]
> > (0x0010): Unable to load init fn sssm_ldap_subdomains_init from module
> > ldap, error: /usr/lib64/sssd/libsss_ldap.so: undefined symbol:
> > sssm_ldap_subdomains_init
>
> Here is the patch set with both things fixed. In the end I had to add the
> subdomains_provider option, it was the only solution for the linking
> issue.
>
> Thanks
> Jan
This works fine, but the new option should be added to the configAPI
(sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my
will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems
quite
high because this DEBUG message is emited for any back end which does
not support subdomains which is everything but IPA. This should be a
MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a
good idea to
default to "ipa" in the IPA provider and maybe check during the first
request if subdomains are enabled on the server -- provided that runtime
detection is possible which I honestly don't know. That way the clients
wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD
communicates with IPA. I will discuss it with him once he is back from
vacation. I'll send the patch set then.
Jan