On 11/02/2012 06:51 AM, Jakub Hrozek wrote: > 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@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. > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
First let us define a general rule about how we treat the cases: X = Is it treated as X being undefined or X having an empty value. It should be a general documented rule for the application. Current behavior is to ignore and I think it is the right one. May be it should be clearly stated in the man for sssd.conf if not yet stated. We should probably add a special value like $none$ to allow explicitly setting the filter to block all access. Realmd would have to adjust accordingly. Does it sound like a plan? -- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
