[SSSD] Unexpected behavior with 'simple_allow_users ='

Simo Sorce simo at redhat.com
Tue Nov 6 15:50:03 UTC 2012


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.


-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list