On 11/06/2012 09:24 AM, Simo Sorce wrote:
On Tue, 2012-11-06 at 15:10 +0100, Ondrej Kos wrote:
> On 11/06/2012 02:52 PM, Simo Sorce wrote:
>> On Tue, 2012-11-06 at 14:46 +0100, Ondrej Kos wrote:
>>> On 11/02/2012 05:32 PM, Simo Sorce wrote:
>>>> On Fri, 2012-11-02 at 10:10 -0400, Dmitri Pal wrote:
>>>>> On 11/02/2012 09:50 AM, Stef Walter wrote:
>>>>>> On 11/02/2012 01:57 PM, Dmitri Pal wrote:
>>>>>>> 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.
>>>>>> Understood.
>>>>>>
>>>>>> But to me it seems that this policy does not make sense in this
>>>>>> situation. It's completely counter intuitive that the
following, instead
>>>>>> of allowing no users, allows any user.
>>>>>>
>>>>>> simple_allow_users =
>>>>>>
>>>>>> Sure, you can say that anyone deploying sssd, should read over
every
>>>>>> line of the documentation and be well informed.
>>>>> One does not preclude the other. The point about man pages is just
to
>>>>> make a statement explicit not implied. I do not expect everyone to
read
>>>>> it but it is better to be explicit.
>>>>>> But there's also
>>>>>> something to be said for readability and
principle-of-least-surprise.
>>>>> Sure. Whatever we decide should be consistent and logical.
>>>>> I seems that so far we had the rule that empty values are ignored.
>>>>> Is it consistent and logical to change it for one attribute?
>>>>> I doubt but your argument is valid too.
>>>>> May be the way out is to have a different attribute
>>>>> simple_default_allow_rule = [all] | [none]
>>>>> By default it will be "all" meaning that if the
simple_allow_users is
>>>>> not specified or empty then all users are allowed - current behavior
>>>>> If it is set to none then if the filter is not specified no one is
allowed.
>>>>>
>>>>> Would that be more logical?
>>>> I was thinking along these lines too.
>>>>
>>>> If we have a default of ALL, then 'none' is not necessary at all
as long
>>>> as a 'simple_allow_users = ' line in the conf causes the
attribute to be
>>>> completely removed from confdb
>>>> at that point the code, not finding the special default of 'ALL'
would
>>>> cause everyone to be denied as Stef expects.
>>>> The bonus is that an admin can explicitly allow all by setting ALL,
>>>> which is also clearer than having to delete the configure attribute or
>>>> disable the access check plugin.
>>>>
>>>> I do not like the $ suggestion as in Windows machine names have the $
>>>> sign in it so $none$ and none$ would be valid yet easily confused.
>>>>
>>>> 'ALL' is used in many tools so it is unlikely someone will have
an upper
>>>> case user or group name named that way, and if so .. well I guess they
>>>> will have to cope.
>>>>
>>>> There is one other character that nobosy uses in usernames and that is
>>>> ':' because historically that's the spearator in /etc/passwd
so it
>>>> simply can't be used in the user name.
>>>>
>>>> So maybe ':ALL' is also a possibility, or maybe we consider
':ALL' the
>>>> 'escaping' version to be able to reference and actual entity
called
>>>> 'ALL' (I would prefer the escaping rule).
>>>>
>>>> Simo.
>>>>
>>>>
>>> So should i prepare patch introducing this behaviour?
>>>
>>> - add configuration option
>>> simple_default_allow_rule = [true|false] / [all|-?-]
>>>
>>> - if <false>
>>> simple_allow_* list would be expected to contain some users/groups,
>>> empty list results in no-one being allowed.
>>>
>>> - if <true|all>
>>> all users are allowed by default, limitations are derived from
>>> simple_deny_* lists.
>>>
>>>
>>> is this correct?
>> Nope
>>
>>> or should we use something like:
>> This is what we were proposing.
>>
>>> simple_allow_users = <ALL_keyword> | <user list>
>>>
>>> and not add the new option at all? in that case, what would be the
>>> keyword and how should we handle the situation, when the configuration
>>> file would contain the following:
>>>
>>> simple_allow_users = <ALL_keyword>, user1, user2, ...
>> users here would simply be redunant as the ALL keyword alreay matches
>> everyone.
>>
> the question here is, should we treat this as an configuration error, or
> allow it?
allow it, it could be a temporary change where the admin wants to keep
the original list around.
>>> also this method would require *simple_allow_users* rule to be present,
>>> or would be handled as empty.
>> when empty it will mean that nobody is allowed. (the default internally
>> if not present at all is 'ALL'
> that is the problem. we can't distinguish if list is empty or not
> present at all without modifying confdb
That's why I say we have a default of ALL that we set in the confdb
before reading the config file, ie if the confdb returns NULL it means
that the file had 'simple_allow_users = ' and 'ALL' was explicitly
removed.
Simo.
I do not like it. I think the option when you control the default via a
different config item like described in the first variant is better than
hard coding it.
The default should be ALL but there should be a way to change it IMO.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/