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.
HTH
bye,
Sumit
>
> > P.S. Based on your comments I opened
> >
https://fedorahosted.org/sssd/ticket/2997 and
> >
https://fedorahosted.org/sssd/ticket/2999 to check if we handle the
> > shell correctly in the case it is overridded.
>
> Just for the archival purposes, I assume the second link should be:
>
https://fedorahosted.org/sssd/ticket/2998
>
> Nick
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org