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(a)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:
There are also two whitespace issues in ipa_sudo_init.
Functionally the patch works good.