[SSSD] [PATCHES] SUDO: IPA provider

Jakub Hrozek jhrozek at redhat.com
Sun Apr 28 19:59:12 UTC 2013


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

>              '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).

> +/*
> + 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

> +
> +#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.

[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).



More information about the sssd-devel mailing list