On Mon, May 23, 2016 at 10:26:37PM +0300, Nikolai Kondrashov wrote:
> Hi Sumit,
>
> Thank you for the reply!
>
> First of all, I assume you meant "shell" whenever you used "homedir"
oops, sorry, yes get_homedir_override() is just before
get_shell_override() in nsssrv_cmd.c and I slipped into the wrong
function.
> previously. Please see my further replies below.
>
> On 05/20/2016 02:50 PM, Sumit Bose wrote:
> > On Fri, May 20, 2016 at 01:54:33PM +0300, Nikolai Kondrashov wrote:
> > > On 05/20/2016 01:39 PM, Sumit Bose wrote:
> > > > On Fri, May 20, 2016 at 12:53:51PM +0300, Nikolai Kondrashov wrote:
> > > > > On 05/19/2016 02:41 PM, Sumit Bose wrote:
> > > > > > On Thu, May 12, 2016 at 04:29:07PM +0300, Nikolai Kondrashov wrote:
> > > > > > > On 04/26/2016 11:32 AM, Sumit Bose wrote:
> > > > > > > > On Mon, Apr 25, 2016 at 09:16:22PM +0300, Nikolai Kondrashov
> > > > > > > > wrote:
> > > > > > > > > On 04/11/2016 07:44 PM, Sumit Bose wrote:
> > > > > > > > > > On Fri, Apr 08, 2016 at 07:31:59PM +0300, Nikolai
> > > > > > > > > > Kondrashov wrote:
> > > > > > > > > > > On 04/06/2016 02:06 PM, Sumit Bose wrote:
> > > > > > > > > > > > I wonder if it would makes sense to add the cached user
> > > > > > > > > > > > object to preq
> > > > > > > > > > > > in pam_check_user_search() to avoid the lookup in
> > > > > > > > > > > > pam_reply_export_shell(). The data is already allocated
> > > > > > > > > > > > on preq and as
> > > > > > > > > > > > far as I can see never freed explicitly, so it wouldn't
> > > > > > > > > > > > even cost more
> > > > > > > > > > > > memory.
> > > > > > > > > > >
> > > > > > > > > > > Sure, that would be nice. However it's really hard for me
> > > > > > > > > > > to tell where that
> > > > > > > > > > > would come from, where it's actually retrieved and what's
> > > > > > > > > > > the lifetime would
> > > > > > > > > > > be. I really miss documentation there.
> > > > > > > > > > >
> > > > > > > > > > > Could you suggest the change, perhaps?
> > > > > > > > > >
> > > > > > > > > > sure, please have a look at attached (untested) patch. With
> > > > > > > > > > this you start in
> > > > > > > > > > pam_reply_export_shell() with
> > > > > > > > > >
> > > > > > > > > > + shell = ldb_msg_find_attr_as_string(preq->user_obj,
> > > > > > > > > > SYSDB_SHELL, NULL);
> > > > > > > > > > + if (shell == NULL) {
> > > > > > > > > > + DEBUG(SSSDBG_CRIT_FAILURE, "user has no shell\n");
> > > > > > > > > > + ret = ENOENT;
> > > > > > > > > > + goto done;
> > > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > Thanks a lot Sumit, this is very helpful! However, the
> > > > > > > > > problem is the non-UPN
> > > > > > > > > case is requesting the user with sysdb_getpwnam_with_views and
> > > > > > > > > pam_reply_export_shell needs the non-overridden shell to pass
> > > > > > > > > it to tlog-rec,
> > > > > > > > > as local override is the mechanism used to enable tlog-rec at
> > > > > > > > > the moment.
> > > > > > > > >
> > > > > > > > > So, it seems we need the second lookup in
> > > > > > > > > pam_reply_export_shell after all.
> > > > > > > > > Or am I missing something?
> > > > > > > >
> > > > > > > > The *_with_views() calls add the override data with the
> > > > > > > > OVERRIDE_PREFIX,
> > > > > > > > so SYSDB_SHELL is still the original one while OVERRIDE_PREFIX
> > > > > > > > SYSDB_SHELL
> > > > > > > > is the overridden one if there is any.
> > > > > > > >
> > > > > > > > There is something special with AD users and the default view.
> > > > > > > > If the
> > > > > > > > shell for an AD user is overridden in the default view it is
> > > > > > > > already
> > > > > > > > applied and SYSDB_SHELL will show it. The original shell from
> > > > > > > > AD can be
> > > > > > > > found in ORIGINALAD_PREFIX SYSDB_SHELL if it is needed here.
> > > > > > >
> > > > > > > Thank you, Sumit.
> > > > > > >
> > > > > > > I looked at this for a while and believe I can do it. However, do
> > > > > > > we really
> > > > > > > want it done this way? It would create a tighter coupling between
> > > > > > > the code
> > > > > > > adding these fields (SYSDB_SHELL with various prefixes) and the
> > > > > > > shell export
> > > > > > > code, which would complicate maintenance.
> > > > > > >
> > > > > > > I mean, as far as I see it, pam_reply_export_shell would have to
> > > > > > > check if it's
> > > > > > > the AD case, if it is, it will have to use ORIGINALAD_PREFIX
> > > > > > > SYSDB_SHELL, if
> > > > > > > it is not, the it will have to use SYSDB_SHELL. Now, if any of
> > > > > > > the condition
> > > > > > > and names change, for example as the result of investigating
> > > > > > > those tickets you
> > > > > > > created, it will break. Do we want to track another copy of the
> > > > > > > logic?
> > > > > > >
> > > > > > > Is the optimization we're trying to achieve here worth it?
> > > > > > > Wouldn't it be
> > > > > > > better to limit ourselves to a narrower and better-defined
> > > > > > > interface of
> > > > > > > sysdb_getpwnam at least for now? Sure, there might be a
> > > > > > > performance penalty,
> > > > > > > but we're just trying things out and we don't actually know how
> > > > > > > often this
> > > > > > > feature will be used and in which conditions. I.e. we don't
> > > > > > > measure it yet.
> > > > > > >
> > > > > > > What if we take the middle road and add an option enabling
> > > > > > > "session
> > > > > > > recording", which would turn the original shell exporting on,
> > > > > > > which would
> > > > > > > simply use sysdb_getpwnam?
> > > > > > >
> > > > > > > I presume we will need such a switch anyway and for the start it
> > > > > > > can do just
> > > > > > > this one thing (i.e. exporting the shell). When we implement
> > > > > > > central control,
> > > > > > > it would also enable acting on the corresponding attributes from
> > > > > > > LDAP.
> > > > > > >
> > > > > > > Then later, if performance would be a problem, we can optimize
> > > > > > > this.
> > > > > > >
> > > > > > > Now, I'm not entirely sure adding such an option would be the
> > > > > > > best idea, as
> > > > > > > perhaps we can avoid it. I.e. simply have the recording activated
> > > > > > > for a user
> > > > > > > if there's corresponding data in LDAP, in the future when we
> > > > > > > implement it.
> > > > > > >
> > > > > > > What do you think?
> > > > > >
> > > > > > (sorry for the delay)
> > > > > >
> > > > > > I think it is not so much about tight or not so tight coupling. The
> > > > > > main
> > > > > > point is to determine the shell as the nss responder does. The nss
> > > > > > responder use sysdb_getpwnam_with_views() and
> > > > > > get_homedir_override() to
> > > > > > get the right shell. The pam responder already uses
> > > > > > sysdb_getpwnam_with_views() so all needed data is already
> > > > > > available, you
> > > > > > cannot use plain sysdb_getpwnam() because it won't contain the
> > > > > > override
> > > > > > data if any.
> > > > > >
> > > > > > So what is missing is to make get_homedir_override() public so that
> > > > > > you
> > > > > > can use it in pam_reply_export_shell(). Unfortunately
> > > > > > get_homedir_override() depends on some nss responder specific
> > > > > > options,
> > > > > > override_homedir, homedir_substring and fallback_homedir. Those
> > > > > > options
> > > > > > must be read by the pam responder as well to make sure that the pam
> > > > > > responder puts the same shell in the environment as the nss
> > > > > > responder
> > > > > > would return.
> > > > >
> > > > > Thanks, Sumit, but now I'm confused :)
> > > > >
> > > > > I think we understand this completely differently.
> > > > > The "pam_reply_export_shell" needs the *original*, *not* overridden
> > > > > shell,
> > > > > because the override is how we configure tlog-rec use (we set the
> > > > > shell
> > > > > override to point to tlog-rec). Exporting the original shell lets
> > > > > tlog-rec
> > > > > know what is the actual shell user needs, so it can start that.
> > > >
> > > > ah, was thought tlog will be switched on differently, so you either can
> > > > have tlog or an overriden shell. And yes, in this case you do not have
> > > > to
> > > > check the overriden shell.
> > > >
> > > > But I think you still need an evaluation similar to
> > > > get_homedir_override() because e.g. if there are no POSIX attributes in
> > > > AD users from AD will not have a shell set at all and the shell might be
> > > > chosen based on SSSD's shell related config options.
> > > >
> > > > Instead of sss_view_ldb_msg_find_attr_as_string() you should use
> > > > ldb_msg_find_attr_as_string() and if this is already tlog (this is the
> > > > case where the tlog shell is added to the default override) you should
> > > > check if ORIGINALAD_PREFIX SYSDB_SHELL exists.
>
> I'm sorry, I don't quite understand this sentence. How can it already be
> "tlog" and what is the "default override"?
>
> Could you please elaborate on this piece of logic?
If I understood you comment correctly tlog will be enabled for a user by
using overrides, so you either use local overrides
sss_override user-add user_name --shell=/bin/tloc-rec
or IPA overrides
idoverrideuser-add view_name user_name --shell=/bin/tloc-rec
In IPA there is a special view called "Default Trust View". If overrides
are configured for this view the values are not stored in the SSSD cache
in separate override objects but in the related user object directly
replacinfg the original value, if any. The original value is then
available in the attribute prefixed by ORIGINALAD_PREFIX. So if you call
e.g
idoverrideuser-add "Default Trust View" [email protected]
--shell=/bin/tloc-rec
SYSDB_SHELL will contain /bin/tloc-rec and ORIGINALAD_PREFIX SYSDB_SHELL
the shell as set in AD. The reason for this is the trusted users in the
"Default Trust View" should be behave in the same way as IPA users
without any special handling.
>
> > > > If you still have an empty shell at this point you should apply the SSSD
> > > > config options.
> > > >
> > > > Finally if there is still no shell /bin/sh should be used. If
> > > > getpwnam() does not return a shell for the user login and sshd (and I
> > > > hope all other applications as well) use /bin/sh. The nss responder will
> > > > return an empty shell entry in this case as well letting the caller
> > > > decide. But in the case here we should add /bin/sh to the environment
> > > > variable if nothing else was found.
> > >
> > > Thanks, Sumit! However, before I try to fully understand the above, isn't
> > > there a function of some kind, that decides which shell the user should
> > > have
> > > *without* applying the override? I mean, that logic should be implemented
> > > somewhere already, and reimplementing it in pam_reply_export_shell would
> > > be a
> > > bad form.
> >
> > Unfortunately get_homedir_override() is the only one we have because
> > currently currently the only use-case was
> > get-the-effective-shell-for-the-user which of course includes overrides
> > and SSSD config option evaluation. But as said the main difference is in
> > the initial part where sss_view_ldb_msg_find_attr_as_string() is
> > currently used. I think you can just add a flag to
> > get_homedir_override() which indicates the tlog use case and the
> > ldb_msg_find_attr_as_string() for SYSDB_SHELL and eventually for
> > ORIGINALAD_PREFIX SYSDB_SHELL and continue with the option processing.
> > Since the struct nss_ctx argument is specific for the nss responder you
> > might want to replace this with the actual config options
> > homedir_substr, override_homedir and fallback_homedir as well.
> > This way you do not need to reimplement it but only extend what is
> > already available to your use-case.
> >
> > > Doesn't sysdb_getpwnam do that?
> >
> > sysdb_getpwnam only reads data from the cache but the cache basically
> > has what is read from LDAP without additional processing, so the
> > SYSDB_SHELL might even already have the tlog shell in the default
> > override case. Since you do not need the additional override attributes
> > from other view it would be possible to use sysdb_getpwnam() the
> > additional processing is still needed.
>
> Ah, after reading the source code for quite a while, I think I finally
> understand what's going on and what you mean :)
>
> Now, I think adding a special parameter to get_shell_override() to switch
> between ldb_msg_find_attr_as_string() and
> sss_view_ldb_msg_find_attr_as_string() will make the function more complex and
> harder to understand. What if I simply make it accept a shell string and
> retreive that outside the function? The only user of the function
> (fill_pwent()) does a lot of that for the other attributes anyway.
yes, fill_pwent() does this but I think it would be better to have the
whole processing of a single attribute in a separate function like
get_shell_override() and get_homedir_override(). This would make
fill_pwent() which currently has more than 200 lines-of-code more
readable in the long run.
>
> For that matter, what if I do the similar thing for the other get_*_override
> functions just to keep it uniform?
>
> Regardless of that, what would be the proper way to expose these functions to
> the pamsrv_cmd.c module (and I think they all should be exposed at once, as
> they're doing a similar thing), since they're in nsssrv_cmd.c? Do I move them
> to another module and rename (to which and rename how?), do I put them into a
> header file (which and under which names), do I create a new header file,
> something else? I can't quite catch the conventions here.
I think src/responder/common/responder.h would be the right header and
src/responder/common/responder_common.c or a new file in
src/responder/common/ the right place to add the code.
bye,
Sumit
>
> > Btw, I just check which attributes are read by sysdb_getpwnam() and
> > sysdb_getpwnam_with_views() and found that ORIGINALAD_PREFIX SYSDB_SHELL
> > is missing. I think the easiest here is to add it to SYSDB_PW_ATTRS.
>
> Alright, I'll do that.
>
> Thanks for all your help, Sumit!
>
> Nick
> _______________________________________________
> sssd-devel mailing list
> [email protected]
> https://lists.fedorahosted.org/admin/lists/[email protected]
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
