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

Jakub Hrozek jhrozek at redhat.com
Wed Sep 12 12:28:38 UTC 2012


On Wed, Sep 12, 2012 at 02:23:41PM +0200, Sumit Bose wrote:
> On Wed, Sep 12, 2012 at 10:19:57AM +0200, Pavel Březina wrote:
> > On 09/11/2012 06:54 PM, Sumit Bose wrote:
> > >On Mon, Sep 10, 2012 at 01:52:53PM +0200, Pavel Březina wrote:
> > >>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.
> > >
> > >Thank you for the new version. I only have a few comments left:
> > >  - provider_enabled is not used
> > >  - in the (!responder_enabled && provider != NULL && strcmp(provider, NO_PROVIDER)
> > >    case, please just print the debug message and call load_backend_module()
> > >    afterwards. So that the provider is loaded as requested in the config
> > >    file. This would also avoid the strange debug message
> > >(Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init_sudo] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work!
> > >(Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init] (0x2000): SUDO backend target successfully loaded from provider [(null)].
> > 
> > Thank you. I had the changes prepared, but I forgot to amend the commit.
> > The patch is attached.
> 
> ACK
> 
> bye,
> Sumit

Pushed to master.



More information about the sssd-devel mailing list