[SSSD] [PATCHES] SUDO: IPA provider

Lukas Slebodnik lslebodn at redhat.com
Mon May 6 22:02:43 UTC 2013


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 <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

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



More information about the sssd-devel mailing list