[SSSD] [PATCHES] backend: initialize sudo only when it is enabled in services

Pavel Březina pbrezina at redhat.com
Mon Sep 10 11:52:53 UTC 2012


On 09/10/2012 10:48 AM, Sumit Bose wrote:
> On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
>> On 09/07/2012 11:31 AM, Sumit Bose wrote:
>>> On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
>>>> 0001 fixes https://fedorahosted.org/sssd/ticket/1458
>>>> 0002 fixes a memory leak when be_process_init() fails. I think it
>>>> should be fixed, even though the backend is in this case immediately
>>>> terminated
>>>
>>> Hi,
>>>
>>> the patches compile without errors and work as expected. Nevertheless I
>>> have a few comments:
>>>
>>> 0001:
>>>   - I think it make sense to skip the initialization of the sudo target
>>>     if there is no responder configured. But to avoid surprises on the
>>>     user side I would recommend to do the initialization if it is
>>>     configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
>>>     give a warning in the logs that the corresponding responder will not
>>>     be running.
>>
>> I discussed this with Jakub before writing the patch. sudo_provider
>> currently defaults to id_provider. If I get it right, you are
>> suggesting to make the default value "none".
>
> No, sorry for not being clear here. I think the id_provider default is
> right. I just wanted to say that if for whatever reasons the
> sudo_provider parameter is used explicitly in sssd.conf the
> sudo_provider should be initialization even if sudo is not listed in
> services.
>
> bye,
> Sumit
>
>>
>> We agreed with Jakub that this makes more pressure on the user to
>> understand the configuration so we chose not to do it. Although the
>> configurations I've been seeing so far are mostly generated by
>> authconfig or ipa-client-install (where are all providers set
>> explicitly) so I don't think it is a problem after all. What do you say
>> Jakub?
>>
>> If we decide to do it this way, it will also require changes in man page.
>>
>>>> +    ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
>>>> +                            CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
>>>> +    if (ret != EOK) {
>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
>>>> +              ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
>>>> +        return ret;
>>>> +    }
>>>> +
>>>
>>>   - I would prefer to use confdb_get_string_as_list() and use strcmp()
>>>     later on, to be on the safe side.
>>>
>>> 0002: ACK
>>>
>>> bye,
>>> Sumit

OK, that makes sense. Patches are attached.

I moved the code into separate function.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-be_process_init-free-ctx-on-error.patch
Type: text/x-patch
Size: 5528 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120910/7d5329e2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-backend-initialize-sudo-only-when-it-is-enabled-in-s.patch
Type: text/x-patch
Size: 3651 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120910/7d5329e2/attachment-0001.bin>


More information about the sssd-devel mailing list