On Fri, May 27, 2016 at 02:00:12PM +0300, Nikolai Kondrashov wrote:
On 05/26/2016 12:36 PM, Sumit Bose wrote:
> On Mon, May 23, 2016 at 10:26:37PM +0300, Nikolai Kondrashov wrote:
> > 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.
No problem, happens to everyone, was easy to figure out :)
> > 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:
> > > > > 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" ad_user(a)ad.domain
--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.
Right. Thank you for the explanation, Sumit. I heard about ID views, but I
didn't think they also implement overrides. IIRC, the original idea suggested
by SSSD developers was to use local overrides only.
Basically overrides from IPA and local overrides are the same only the
data source is different. Please note that both are mutually exclusive.
On IPA clients you should define the overrides on the IPA server. In
other environments, e.g. on a AD client, local overrides can be used.
Using ID views for this actually sounds great to me. However, as I know little
of them, I'd like someone else to check if that will be fine regarding overall
design and intended use.
Martin, Jakub, perhaps you can comment?
Otherwise, Sumit, would this kind of logic do for retrieving the actual
(original) user shell:
if ORIGINALAD_PREFIX SYSDB_SHELL is set, use it,
else use SYSDB_SHELL
There are 2 cases to consider
1. DOM_HAS_VIEWS(domain)
This is the case where a view other than the default view is defined on
IPA or the local overrides are used. Here the override shell from the
view is OVERRIDE_PREFIX SYSDB_SHELL. So if tlog is activated this
attribute should contain /bin/tloc-rec. The original/default shell can
be found in SYSDB_SHELL in this case (it can be empty). If, in the IPA
case, tlog is activated in the default view of the user as well,
SYSDB_SHELL will contain /bin/tloc-rec as well. In this case you have to
look at ORIGINALAD_PREFIX SYSDB_SHELL which contains the shell read from
AD or is empty. As mentioned before /bin/sh should be used in the shell
is still empty after processing the shell related SSSD options.
2. !DOM_HAS_VIEWS(domain)
This is the default case, i.e. no IPA view or local override is applied.
But there still might be an override in the IPA default view. So you
should still check if SYSDB_SHELL contains /bin/tloc-rec in this case
and tlog is activated. If it is, you can find the shell from AD in
ORIGINALAD_PREFIX SYSDB_SHELL. As before if this is empty, process SSSD
options and finally replace a still empty shell with /bin/sh.
I think it would be good to set the environment variable only if tlog is
activated for the user. Otherwise people will start asking sooner or
later why we set the variable unconditionally. If you call the
unmodified version of get_shell_override() it should return the
/bin/tloc-rec if tlog is activated. If you now call get_shell_override()
where instead of
user_shell = sss_view_ldb_msg_find_attr_as_string(dom, msg, SYSDB_SHELL, NULL);
if (!user_shell) {
....
you call
if (tlog_mode) {
user_shell = ldb_msg_find_attr_as_string(dom, msg, SYSDB_SHELL, NULL);
if (user_shell != NULL && strcmp(user_shell, "/bin/tloc_rec") ==
0) {
user_shell = ldb_msg_find_attr_as_string(dom, msg, ORIGINALAD_PREFIX SYSDB_SHELL,
NULL);
}
} else {
user_shell = sss_view_ldb_msg_find_attr_as_string(dom, msg, SYSDB_SHELL, NULL);
}
if (!user_shell) {
/* Check whether there is a default shell specified */
if (dom->default_shell) {
return talloc_strdup(mem_ctx, dom->default_shell);
} else if (nctx->default_shell) {
return talloc_strdup(mem_ctx, nctx->default_shell);
}
if (tlog_mode) {
return talloc_strdup(mem_ctx, "/bin/sh");
} else {
return NULL;
}
}
it should basically do what I tried to describe above.
HTH
bye,
Sumit
>
> > > > 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.
>
> Alright.
>
> > > 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.
>
> Alright, I'll do that.
>
> Thank you, Sumit!
>
> Nick