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(a)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(a)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?