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