Re: [SSSD] [PATCHES] SUDO: IPA provider

[Lukas Slebodnik]

On (03/05/13 18:52), Lukas Slebodnik wrote:
> On (03/05/13 14:02), Jakub Hrozek wrote:
> > On Mon, Apr 29, 2013 at 11:49:09AM +0200, Lukas Slebodnik wrote:
> > > On (28/04/13 21:59), Jakub Hrozek wrote:
> > > > On Fri, Apr 26, 2013 at 06:08:19PM +0200, Lukas Slebodnik wrote:
> > > > > On (25/04/13 20:27), Jakub Hrozek wrote:
> > > > > > On Thu, Apr 25, 2013 at 01:29:49PM +0200, Lukas Slebodnik wrote:
> > > > > > > ehlo,
> > > > > > > 
> > > > > > > I am attaching two patches.
> > > > > > > First patch make retrieving host information more reusable. It is a preparation
> > > > > > > for easily reusing existing ldap code in SUDO IPA provider in second patch.
> > > > > > > 
> > > > > > > I am attaching two patches.
> > > > > > > First patch make retrieving host information much more reusable. It is
> > > > > > > a preparation for easily reusing existing ldap code in SUDO IPA provider
> > > > > > > in the second patch.
> > > > > > > 
> > > > > > > LS
> > > > > >
> > > > > > Patch 0001: Ack
> > > > > >
> > > > > > Patch 0002: Could we reduce the code duplication between ipa_sudo_init()
> > > > > > and ldap_sudo_init()? Maybe create a function like ldap_sudo_common_init
> > > > > > or sdap_sudo_init that would do the common initialization work.
> > > > > >
> > > > > > The sssd-sudo manpage must be amended to include an IPA example
> > > > > > according to this new provider.
> > > > > >
> > > > > > The ldap_sudo options must be added to the configAPI to the file
> > > > > > src/config/etc/sssd.api.d/sssd-ipa.conf - there should be a section
> > > > > > called [provider/ipa/sudo] that lists all the ldap_sudo_* options.
> > > > > >
> > > > > > I haven't tested the patches at all yet. Does the sudo_provider inherit
> > > > > > its value from the id_provider? I think it should so that the
> > > > > > configuration is as minimal as possible
> > > > >
> > > > > Rewritten patch is attached.
> > > > > sdap_sudo_init is called from ipa_sudo_init
> > > > > In this case, first refactoring patch isn't needed.
> > > > > man pages updated (I am expecting comments here)
> > > > > options added to src/config/etc/
> > > > >
> > > > > LS
> > > >
> > > > Builds fine, the unit tests pass, basic functionality works OK as well
> > > > (I only tested the basics, though, logged in as admin and performed sudo),
> > > > the options are present in the configAPI. I still have a couple of comments,
> > > > but in general, the patch is moving in the right direction.
> > > > > --- a/src/config/SSSDConfigTest.py
> > > > > +++ b/src/config/SSSDConfigTest.py
> > > > > @@ -715,8 +715,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
> > > > >          domain = SSSDConfig.SSSDDomain('sssd', self.schema)
> > > > >      control_provider_dict = {
> > > > >        'ipa': ['id', 'auth', 'access', 'chpass', 'autofs', 'session',
> > > > >                'hostid', 'subdomains'],
> > > > >        'ipa': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs',
> > > > >                'session','hostid', 'subdomains'],
> > > >                            ^^ 
> > > >                        missing whitespace
> > >
> > > Fixed
> > > > >          'ad': ['id', 'auth', 'access', 'chpass'],
> > > > >          'local': ['id', 'auth', 'chpass'],
> > > > >          'ldap': ['id', 'auth', 'access', 'chpass', 'sudo', 'autofs'],
> > > > >        SSSD has native support of IPA provider for sudo compat tree.
> > > >
> > > > What about a new paragraph like this:
> > > > When the SSSD is configured to use the IPA provider, the sudo provider
> > > > is automatically enabled. The sudo search base is configured to use the
> > > > compat tree (ou=sudoers,$DC).
> > >
> > > Man pages updated.
> > > > > +/*
> > > > > SSSD
> > > > > IPA Provider Initialization functions
> > > > > Authors:
> > > > > Lukas Slebodnik
> > > > > Copyright (C) 2013 Red Hat
> > > > > This program is free software; you can redistribute it and/or modify
> > > > > it under the terms of the GNU General Public License as published by
> > > > > the Free Software Foundation; either version 3 of the License, or
> > > > > (at your option) any later version.
> > > > > This program is distributed in the hope that it will be useful,
> > > > > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > GNU General Public License for more details.
> > > > > You should have received a copy of the GNU General Public License
> > > > > along with this program.  If not, see http://www.gnu.org/licenses/.
> > > > > */
> > > >
> > > > We tend to prefix multi-line comments with a leading "*":
> > > > http://freeipa.org/page/Coding_Style#Comments
> > >
> > > I copied src/providers/ipa/ipa_autofs.c to ipa_sudo.c
> > > Every file starts with the same license comment in directory src/provides/ipa
> > >
> > > Do you still think, that it should be fixed?
> > > > > +#include "providers/ipa/ipa_common.h"
> > > > > +#include "providers/ipa/ipa_id.h"
> > > > > +#include "providers/ipa/ipa_auth.h"
> > > > >
> > > > > +#include "providers/ldap/sdap_sudo.h"
> > > > > +#include "db/sysdb_sudo.h"
> > > > >
> > > > > +struct bet_ops ipa_sudo_ops = {
> > > > > .handler = sdap_sudo_handler,
> > > > > .finalize = NULL,
> > > > > .check_online = sdap_check_online
> > > > >
> > > > > +};
> > > >
> > > > The handlers seem wrong to me -- by default, when id_provider=ipa the ops
> > > > structure would be initialized from sssm_ipa_id_init and would point to
> > > > ipa_id_ops, but if it wasn't, then ipa_sudo init would initialize it to
> > > > ipa_sudo_ops with completely different handlers.
> > > >
> > > > I haven't really been able to trigger the crash due to the talloc_move
> > > > (see below), but I guess that if you set id_provider=ldap &&
> > > > sudo_provider=ipa, then sssd_be would crash with SIGABRT as the
> > > > sdap_sudo_handler would have received ipa_id_ctx but would expect
> > > > sdap_id_ctx. I *think* that you'll have to create a wrapper similar to
> > > > ipa_check_online that just passes the sdap_id_ctx to the correct
> > > > function.
> > >
> > > I tried id_provider=ldap and sudo_provider=ipa, but I was unable to
> > > authenticate or receive informations with command getent.
> > > > [snip]
> > > > > *ops = &ipa_sudo_ops;
> > > > > ipa_options = id_ctx->ipa_options;
> > > > > ldap_options = id_ctx->sdap_id_ctx->opts;
> > > > > ipa_options->id->sudorule_map = talloc_move(ipa_options->id,
> > > > >                                            &ldap_options->sudorule_map);
> > > >
> > > > I don't quite understand the talloc_move? It seems to break more
> > > > esoteric configurations (id_provider=ldap, sudo_provider=ipa) and in
> > > > general do you expect the ipa_id_ctx to outlive the ldap_options? In
> > > > that case, let's do a deep-copy and not worry about performance, the
> > > > startup is done just once and there are much more expensive operations
> > > > than duplicating a memory array (like reading the keytab from disk and
> > > > searching for a principal there).
> > >
> > > I think that shallow copy is good enough.
> > >
> > > Patch attached
> > >
> > > LS
> >
> > We should remove ipa_sudo_ops and simply use sdap_ops now.
> >
> > sdap_check_online wouldn't work anyway because it expects sdap_id_ctx
> > and would be passed in sdap_sudo_ctx.
> >
> > When reviewing the patch I realized that the .finalize handler is never
> > called from back ends. We should investigate that:
> > https://fedorahosted.org/sssd/ticket/1904
> >
> > There are also two whitespace issues in ipa_sudo_init.
> >
> > Functionally the patch works good.
>
> Thank you for review.
>
> Patch is attached.
>
> LS

I used patch from previous mail and it clearly applies to sssd 1.9 branch.
I tested sssd 1.9 with patch on RHEL6 and everything works like expected.
But I am author of this patch, so someone else should also test it.

I thing it could be pushed to sssd 1.9 branch.

LS