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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
