[SSSD] [PATCHES] SUDO: IPA provider

Jakub Hrozek jhrozek at redhat.com
Fri May 3 17:47:44 UTC 2013


On Fri, May 03, 2013 at 06:52:57PM +0200, 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 <lslebodn at redhat.com>
> >> >> +
> >> >> + 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

Ack!



More information about the sssd-devel mailing list