[SSSD] Unexpected behavior with 'simple_allow_users ='

Dmitri Pal dpal at redhat.com
Fri Nov 2 12:57:16 UTC 2012


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 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.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at 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/






More information about the sssd-devel mailing list