[SSSD] Unexpected behavior with 'simple_allow_users ='

Dmitri Pal dpal at redhat.com
Tue Nov 6 18:12:45 UTC 2012


On 11/06/2012 10:50 AM, Simo Sorce wrote:
> On Tue, 2012-11-06 at 10:26 -0500, Dmitri Pal wrote:
>> 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.
> I think you just described what I proposed with different words, or I am
> not understanding what you do not like.
>
> Just to be clear this is how I see it:
>
> config file  |   confdb     |   result
> ----------------------------------------------------------
> no attr      |   'ALL'      |  all user can access
> empty attr   |    NULL      |  access is denied to all
> user1, user2 | user1, user2 | only user1, user2 can access
>
>
> Simo.
>
>

I am saying I do not like to hard code anything and rely on configdb
contents at all.
That changes semantics as in the past we did not rely on its contents.
So I suggest to have two config values one is simple_allow_users and
another simple_default_allow_rule (or similar, I am not married to name).
The table is driven by the contents of the config file only (as it has
been in the past with all other attributes):

simple_allow_users | simple_default_allow_rule | result
--------------------------------------------------------
user1, user2       |  ignored                  | user1, user2
empty              |  ignored                  | access is denied to all
no attr            |  no attr (default = ALL)  | all users can access
no attr            |  "all"                    | all users can access
no attr            |  "none"                   | access is denied to all  

Hope it makes sense now.

-- 
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