[SSSD] Unexpected behavior with 'simple_allow_users ='

Jakub Hrozek jhrozek at redhat.com
Fri Nov 2 10:51:54 UTC 2012


On Fri, Nov 02, 2012 at 10:09:53AM +0100, Ondrej Kos wrote:
> On 11/01/2012 09:51 PM, Simo Sorce wrote:
> >On Thu, 2012-11-01 at 16:09 -0400, Dmitri Pal wrote:
> >>On 11/01/2012 03:04 PM, Simo Sorce wrote:
> >>>On Thu, 2012-11-01 at 10:53 -0400, Dmitri Pal wrote:
> >>>>On 11/01/2012 09:11 AM, Simo Sorce wrote:
> >>>>>On Thu, 2012-11-01 at 12:03 +0100, Michal Židek wrote:
> >>>>>>On 10/30/2012 04:53 PM, Ondrej Kos wrote:
> >>>>>>>On 10/17/2012 03:28 PM, Michal Židek wrote:
> >>>>>>>>On 10/16/2012 03:45 PM, Stef Walter wrote:
> >>>>>>>>>On 10/16/2012 02:04 PM, Jakub Hrozek wrote:
> >>>>>>>>>>I was wondering for a while whether to change the behaviour directly in
> >>>>>>>>>>confdb_get_string_as_list() but I think the attached patch takes a
> >>>>>>>>>>better
> >>>>>>>>>>approach because the other consumers of confdb_get_string_as_list() do
> >>>>>>>>>>not see any difference between empty and missing parameter.
> >>>>>>>>>Yeah figures.
> >>>>>>>>>
> >>>>>>>>>>The patch works as advertized, there is just one compilation warning:
> >>>>>>>>>>
> >>>>>>>>>>src/providers/simple/simple_access.c: In function
> >>>>>>>>>>'get_conf_list_or_empty':
> >>>>>>>>>>src/providers/simple/simple_access.c:284:9: warning: unused variable
> >>>>>>>>>>'r'
> >>>>>>>>>>[-Wunused-variable]
> >>>>>>>>>New patch attached.
> >>>>>>>>>
> >>>>>>>>>Thanks for the review.
> >>>>>>>>>
> >>>>>>>>>Stef
> >>>>>>>>>
> >>>>>>>>Hi Stef,
> >>>>>>>>
> >>>>>>>>your patch solves the problem with *empty* 'simple_allow_users = ', but
> >>>>>>>>it introduces new problem with *nonexistent* simple_allow_users. With
> >>>>>>>>your patch, if no simple_allow_users list was specified, it behaves as
> >>>>>>>>if empty simple_allow_users was provided and denies access to all users.
> >>>>>>>>
> >>>>>>>>I think this new behaviour could brake existing configurations and it
> >>>>>>>>also differs from the behaviour described in man pages.
> >>>>>>>>
> >>>>>>>>Michal
> >>>>>>>>
> >>>>>>>>_______________________________________________
> >>>>>>>>sssd-devel mailing list
> >>>>>>>>sssd-devel at lists.fedorahosted.org
> >>>>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>>>>I fixed the new problem and prepared patch that applies on top of this one.
> >>>>>>>attached
> >>>>>>>
> >>>>>>>O.
> >>>>>>>
> >>>>>>It is still not working correctly. With your patch *empty*
> >>>>>>simple_allow_users list allows access to everyone (like it should be
> >>>>>>with *nonexistent* list).
> >>>>>>
> >>>>>>The behaviour should be like this:
> >>>>>>simple_allow_users = user1, user2 // allow access to user1 and user2
> >>>>>>simple_allow_users =              // do not allow access to anyone
> >>>>>>/* nothing */                     // allow access to everyone
> >>>>>>
> >>>>>>
> >>>>>>I think that the problem is that our .conf file parser ignores empty
> >>>>>>values. Do we have ticket to track this issue?
> >>>>>Nope, and it is not that easy to change IIRC.
> >>>>>
> >>>>>I think for now it would be easier to use a user name of 'none' to
> >>>>>indicate none has access. The only issue would be if a user 'none'
> >>>>>exists.
> >>>>>
> >>>>>Simo.
> >>>>>
> >>>>Is this hard because config DB pre-populates all the default values in
> >>>>the DB and then overwrites it from INI file?
> >>>>Is this the case?
> >>>It's because the DB cannot hold an empty value.
> >>>With the current interface empty value = No value.
> >>>
> >>>>It seems that it should be easy to differentiate whether something is
> >>>>defined and empty value.
> >>>We might be able to do it, maybe we should have a ticket.
> >>>
> >>>>I do not think it is an INI problem. Seems like something is not done
> >>>>correctly in the config DB layer.
> >>>>IMO we should have a ticket to improve that if this is really a problem.
> >>>It is a confdb limitation, but it has never been a problem before, we
> >>>never had to make a difference, and in general I do not like much to use
> >>>these semantics, I feel confused about this syntax if I put on my admin
> >>>hat.
> >>>
> >>>Simo.
> >>>
> >>
> >>May be it should be "$none$" to express none to not ever collide with
> >>allowed names.
> >>Then an empty value would be treated as not defined i.e. everyone allowed.
> >>However in this specific case does "none" really makes sense? Can you
> >>imagine the real world use case that would lead to setting this option
> >>to "none"? Nobody would be able to log into the system is this a use
> >>case we need to worry about?
> >
> >Probably not, we should just ignore it I guess.
> >
> >Simo.
> >
> >
> I also thought it'd be quite useless to have the list set, but
> nobody in it, resulting in no one being able to log in, but as we
> discussed it off-list, it appeared it's by design.
> 
> So how should I now proceed with the fix? What is the behaviour we
> consider right?
> 
> O.

I would prefer to fix this in the SSSD as well. It's not very typical
situation, but if Stef ran into the issue, chances are that other
automated programs that change the contents of sssd.conf would be in
the same situation, too.

I don't think we can to be overly defensive when it comes to access
control :-)

So I would prefer to fixup Stef's patch. We might *also* want the $none$
special case as Dmitri and Simo suggested, but I would prefer to be
defensive about the case that Stef presented.



More information about the sssd-devel mailing list