Unexpected behavior with 'simple_allow_users ='

[Stef Walter]

When I have the following in a domain in sssd.conf:

access_provider = simple
simple_allow_users =

... any user is allowed to log in, despite the list being empty. The 
documentation states:

   ·   If either or both "allow" lists are provided, all users are denied
       unless they appear in the list.

The list is provided, albeit empty. The simple access provider however 
treats it as if it is not provided.

Since sssd.conf is often machine driven, this sort of unexpected 
behavior leads to security problems like: removing a user from the 
simple_allow_users acl leads to any user being allowed.

I've worked around this behavior in realmd, by using a comma:

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=56027
Patch: https://bugs.freedesktop.org/attachment.cgi?id=68615

Attached is a rough patch to sssd which fixes the problem. If you think 
it's worth fixing, I'll do more testing on it.

Cheers,

Stef

[Jakub Hrozek]

On Tue, Oct 16, 2012 at 11:47:12AM +0200, Stef Walter wrote:
>  When I have the following in a domain in sssd.conf:
>  
>  access_provider = simple
>  simple_allow_users =
>  
>  ... any user is allowed to log in, despite the list being empty. The
>  documentation states:
>  
>    ·   If either or both "allow" lists are provided, all users are denied
>        unless they appear in the list.
>  
>  The list is provided, albeit empty. The simple access provider
>  however treats it as if it is not provided.
>  
>  Since sssd.conf is often machine driven, this sort of unexpected
>  behavior leads to security problems like: removing a user from the
>  simple_allow_users acl leads to any user being allowed.
>  
>  I've worked around this behavior in realmd, by using a comma:
>  
>  Bug: https://bugs.freedesktop.org/show_bug.cgi?id=56027
>  Patch: https://bugs.freedesktop.org/attachment.cgi?id=68615
>  
>  Attached is a rough patch to sssd which fixes the problem. If you
>  think it's worth fixing, I'll do more testing on it.
>  
>  Cheers,
>  
>  Stef 
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.

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]

[Stef Walter]

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

[Michal Židek]

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

[Ondrej Kos]

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.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1/5C Brno 1 office
irc:   okos @ #brno

[Michal Židek]

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?

Michal

[Simo Sorce]

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.

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

[Dmitri Pal]

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 seems that it should be easy to differentiate whether something is
defined and empty value.

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.


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Simo Sorce]

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.

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

[Dmitri Pal]

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? 

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Simo Sorce]

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.


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

[Ondrej Kos]

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.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Jakub Hrozek]

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.

[Dmitri Pal]

On 11/02/2012 06:51 AM, Jakub Hrozek wrote:
>  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?
www.redhat.com/carveoutcosts/

[Stef Walter]

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. But there's also
something to be said for readability and principle-of-least-surprise.

>  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. 
Sure. If this really is the way that sssd wants to handle this, then
realmd can work around the issue with a special value like $none$. As
long as such a string does not run afoul of sssd user name validation or
some such (either now or in the future).

Cheers,

Stef

[Jakub Hrozek]

On Fri, Nov 02, 2012 at 02:50:45PM +0100, 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. But there's also
>  something to be said for readability and principle-of-least-surprise. 
That is exactly what I meant to say with my earlier post.

Again, this is access control. I would prefer to be more paranoid and
defensive than less.

[Dmitri Pal]

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?

> > 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.
>  Sure. If this really is the way that sssd wants to handle this, then
>  realmd can work around the issue with a special value like $none$.  
This is still a discussion.

>  As
>  long as such a string does not run afoul of sssd user name validation or
>  some such (either now or in the future). 
$none$ is proposed because AFAIR POSIX does not allow names to start with $.

>  Cheers,
>
>  Stef
>  

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Simo Sorce]

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.


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

[Ondrej Kos]

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? or should we use something like:

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

also this method would require *simple_allow_users* rule to be present, 
or would be handled as empty.



O.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Simo Sorce]

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.

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

Simo.

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

[Ondrej Kos]

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?

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

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Simo Sorce]

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.

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

[Dmitri Pal]

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/

[Simo Sorce]

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

[Dmitri Pal]

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/

[Simo Sorce]

On Tue, 2012-11-06 at 13:12 -0500, Dmitri Pal wrote:
>  On 11/06/2012 10:50 AM, Simo Sorce wrote: 
>  > 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. 
Yes, now I understand what you mean, but it is just too complex.
It is just not worth doing, it took me several minutes to understand the
table above, that tells me normal admins will just be confused and
writing documentation around how it works will be challenging, and we
call this 'simple' access method.

If this is the complexity we come to I think we should start
reconsidering the issue from scratch, so I went to the man page to see
what we say there:

        The simple access provider grants or denies access based on an
        access or deny list of user or group names. The following rules
        apply:
        
                • If all lists are empty, access is granted
                • If any list is provided, the order of evaluation is
                allow,deny. This means that any matching deny rule will
                supersede any matched allow rule.
                • If either or both "allow" lists are provided, all
                users are denied unless they appear in the list.
                • If only "deny" lists are provided, all users are
                granted access unless they appear in the list.


This tells me the expected behavior is that if all lists are empty the
access is granted (valid for the first 3 points).

For the latter point we just need to make sure that a value of 'ALL'
does not allow a user in if a deny list is provided.

So adding a default of 'ALL' to simple_allow_users (what I proposed) is
perfectly consistent with the current behavior (which is why i proposed
it).

The proposal is that if someone types: "simple_allow_users = " it means
that no users are in the list and access should simply be denied, this
currently does not work, access is still allowed.

We need to decide if we want to change this behavior or not.

If we want to change it then my proposal is sufficient, provided we
properly change the code, if not then let's do nothing.

I think adding another attribute is simply out of the picture, it makes
the whole scheme just way to complex just to handle an insignificant
corner case.

If that's the only viable agreement I propose we simply do nothing.

Simo.

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

[Dmitri Pal]

On 11/06/2012 01:45 PM, Simo Sorce wrote:
>  On Tue, 2012-11-06 at 13:12 -0500, Dmitri Pal wrote:
> > On 11/06/2012 10:50 AM, Simo Sorce wrote:
> >> 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.
>  Yes, now I understand what you mean, but it is just too complex.
>  It is just not worth doing, it took me several minutes to understand the
>  table above, that tells me normal admins will just be confused and
>  writing documentation around how it works will be challenging, and we
>  call this 'simple' access method.
>
>  If this is the complexity we come to I think we should start
>  reconsidering the issue from scratch, so I went to the man page to see
>  what we say there:
>
>          The simple access provider grants or denies access based on an
>          access or deny list of user or group names. The following rules
>          apply:
>          
>                  • If all lists are empty, access is granted
>                  • If any list is provided, the order of evaluation is
>                  allow,deny. This means that any matching deny rule will
>                  supersede any matched allow rule.
>                  • If either or both "allow" lists are provided, all
>                  users are denied unless they appear in the list.
>                  • If only "deny" lists are provided, all users are
>                  granted access unless they appear in the list.
>
>
>  This tells me the expected behavior is that if all lists are empty the
>  access is granted (valid for the first 3 points).
>
>  For the latter point we just need to make sure that a value of 'ALL'
>  does not allow a user in if a deny list is provided.
>
>  So adding a default of 'ALL' to simple_allow_users (what I proposed) is
>  perfectly consistent with the current behavior (which is why i proposed
>  it).
>
>  The proposal is that if someone types: "simple_allow_users = " it means
>  that no users are in the list and access should simply be denied, this
>  currently does not work, access is still allowed. 
Following the first bullet in man
page "if all lists are empty the
access is granted".
It works as advertised right?
So I do not see why anything needs to be changed then.

>  We need to decide if we want to change this behavior or not.
>
>  If we want to change it then my proposal is sufficient, provided we
>  properly change the code, if not then let's do nothing.
>
>  I think adding another attribute is simply out of the picture, it makes
>  the whole scheme just way to complex just to handle an insignificant
>  corner case.
>
>  If that's the only viable agreement I propose we simply do nothing.
>
>  Simo.
>  

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Stephen Gallagher]

On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
>  On 11/06/2012 01:45 PM, Simo Sorce wrote:
> >                  • If all lists are empty, access is granted
> >                  • If any list is provided, the order of evaluation is
> >                  allow,deny. This means that any matching deny rule will
> >                  supersede any matched allow rule.
> >                  • If either or both "allow" lists are provided, all
> >                  users are denied unless they appear in the list.
> >                  • If only "deny" lists are provided, all users are
> >                  granted access unless they appear in the list. 
<snip>
>  Following the first bullet in man page "if all lists are empty
> the
>  access is granted".
>  It works as advertised right?
>  So I do not see why anything needs to be changed then.
>  
Yeah, that phrasing certainly seems to make it pretty clear that 
'simple_allow_users = ' is an empty list. I would prefer that we not 
change the meaning of this because it *would* be a 
backwards-incompatible change. This strikes me as something we could 
stick in a FAQ somewhere: "Be wary if you are using automated tools to 
generate this option. Specifying no values here is equivalent to 
omitting the option entirely. If you really want to specify no users 
are allowed, it's preferable to use 'access_provider = deny'."

[Simo Sorce]

On Tue, 2012-11-06 at 14:00 -0500, Stephen Gallagher wrote:
>  On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
>  > On 11/06/2012 01:45 PM, Simo Sorce wrote:
>  >>                  • If all lists are empty, access is granted
>  >>                  • If any list is provided, the order of evaluation is
>  >>                  allow,deny. This means that any matching deny rule will
>  >>                  supersede any matched allow rule.
>  >>                  • If either or both "allow" lists are provided, all
>  >>                  users are denied unless they appear in the list.
>  >>                  • If only "deny" lists are provided, all users are
>  >>                  granted access unless they appear in the list.
>  <snip>
>  > Following the first bullet in man page "if all lists are empty the
>  > access is granted".
>  > It works as advertised right?
>  > So I do not see why anything needs to be changed then.
>  >
>  
>  Yeah, that phrasing certainly seems to make it pretty clear that 
>  'simple_allow_users = ' is an empty list. I would prefer that we not 
>  change the meaning of this because it *would* be a 
>  backwards-incompatible change. This strikes me as something we could 
>  stick in a FAQ somewhere: "Be wary if you are using automated tools to 
>  generate this option. Specifying no values here is equivalent to 
>  omitting the option entirely. If you really want to specify no users 
>  are allowed, it's preferable to use 'access_provider = deny'." 
Agreed, let's kill off this thread and the proposal.
Sorry Ondrej and Stef, seem like changing this is just not desirable.

Simo.

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

[Dmitri Pal]

On 11/06/2012 02:09 PM, Simo Sorce wrote:
>  On Tue, 2012-11-06 at 14:00 -0500, Stephen Gallagher wrote:
> > On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
> >> On 11/06/2012 01:45 PM, Simo Sorce wrote:
> >>>                  • If all lists are empty, access is granted
> >>>                  • If any list is provided, the order of evaluation is
> >>>                  allow,deny. This means that any matching deny rule will
> >>>                  supersede any matched allow rule.
> >>>                  • If either or both "allow" lists are provided,
> all
> >>>                  users are denied unless they appear in the list.
> >>>                  • If only "deny" lists are provided, all users
> are
> >>>                  granted access unless they appear in the list.
> > <snip>
> >> Following the first bullet in man page "if all lists are empty the
> >> access is granted".
> >> It works as advertised right?
> >> So I do not see why anything needs to be changed then.
> >>
> > Yeah, that phrasing certainly seems to make it pretty clear that 
> > 'simple_allow_users = ' is an empty list. I would prefer that we not 
> > change the meaning of this because it *would* be a 
> > backwards-incompatible change. This strikes me as something we could 
> > stick in a FAQ somewhere: "Be wary if you are using automated tools to 
> > generate this option. Specifying no values here is equivalent to 
> > omitting the option entirely. If you really want to specify no users 
> > are allowed, it's preferable to use 'access_provider = deny'."
>  Agreed, let's kill off this thread and the proposal.
>  Sorry Ondrej and Stef, seem like changing this is just not desirable.
>
>  Simo.
>  
ack. IMO it should be just clarified in the man page.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Ondrej Kos]

On 11/06/2012 11:07 PM, Dmitri Pal wrote:
>  On 11/06/2012 02:09 PM, Simo Sorce wrote:
> > On Tue, 2012-11-06 at 14:00 -0500, Stephen Gallagher wrote:
> >> On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
> >>> On 11/06/2012 01:45 PM, Simo Sorce wrote:
> >>>>                   • If all lists are empty, access is granted
> >>>>                   • If any list is provided, the order of evaluation is
> >>>>                   allow,deny. This means that any matching deny rule
> will
> >>>>                   supersede any matched allow rule.
> >>>>                   • If either or both "allow" lists are
> provided, all
> >>>>                   users are denied unless they appear in the list.
> >>>>                   • If only "deny" lists are provided, all
> users are
> >>>>                   granted access unless they appear in the list.
> >> <snip>
> >>> Following the first bullet in man page "if all lists are empty the
> >>> access is granted".
> >>> It works as advertised right?
> >>> So I do not see why anything needs to be changed then.
> >>>
> >> Yeah, that phrasing certainly seems to make it pretty clear that
> >> 'simple_allow_users = ' is an empty list. I would prefer that we not
> >> change the meaning of this because it *would* be a
> >> backwards-incompatible change. This strikes me as something we could
> >> stick in a FAQ somewhere: "Be wary if you are using automated tools to
> >> generate this option. Specifying no values here is equivalent to
> >> omitting the option entirely. If you really want to specify no users
> >> are allowed, it's preferable to use 'access_provider = deny'."
> > Agreed, let's kill off this thread and the proposal.
> > Sorry Ondrej and Stef, seem like changing this is just not desirable.
> >
> > Simo.
> >
>  ack. IMO it should be just clarified in the man page.
>  
patch for manpage attached

O.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno

[Stephen Gallagher]

On Wed 07 Nov 2012 05:07:14 AM EST, Ondrej Kos wrote:
>  On 11/06/2012 11:07 PM, Dmitri Pal wrote:
> > On 11/06/2012 02:09 PM, Simo Sorce wrote:
> >> On Tue, 2012-11-06 at 14:00 -0500, Stephen Gallagher wrote:
> >>> On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
> >>>> On 11/06/2012 01:45 PM, Simo Sorce wrote:
> >>>>>                   • If all lists are empty, access is granted
> >>>>>                   • If any list is provided, the order of
> >>>>> evaluation is
> >>>>>                   allow,deny. This means that any matching deny
> >>>>> rule will
> >>>>>                   supersede any matched allow rule.
> >>>>>                   • If either or both "allow" lists are
> provided,
> >>>>> all
> >>>>>                   users are denied unless they appear in the list.
> >>>>>                   • If only "deny" lists are provided, all
> users are
> >>>>>                   granted access unless they appear in the list.
> >>> <snip>
> >>>> Following the first bullet in man page "if all lists are empty the
> >>>> access is granted".
> >>>> It works as advertised right?
> >>>> So I do not see why anything needs to be changed then.
> >>>>
> >>> Yeah, that phrasing certainly seems to make it pretty clear that
> >>> 'simple_allow_users = ' is an empty list. I would prefer that we not
> >>> change the meaning of this because it *would* be a
> >>> backwards-incompatible change. This strikes me as something we could
> >>> stick in a FAQ somewhere: "Be wary if you are using automated tools to
> >>> generate this option. Specifying no values here is equivalent to
> >>> omitting the option entirely. If you really want to specify no users
> >>> are allowed, it's preferable to use 'access_provider =
> deny'."
> >> Agreed, let's kill off this thread and the proposal.
> >> Sorry Ondrej and Stef, seem like changing this is just not desirable.
> >>
> >> Simo.
> >>
> > ack. IMO it should be just clarified in the man page.
> >
>  patch for manpage attached
>
>  O.
>  
Ack

[Jakub Hrozek]

On Wed, Nov 07, 2012 at 09:03:43AM -0500, Stephen Gallagher wrote:
>  On Wed 07 Nov 2012 05:07:14 AM EST, Ondrej Kos wrote:
>  >On 11/06/2012 11:07 PM, Dmitri Pal wrote:
>  >>On 11/06/2012 02:09 PM, Simo Sorce wrote:
>  >>>On Tue, 2012-11-06 at 14:00 -0500, Stephen Gallagher wrote:
>  >>>>On Tue 06 Nov 2012 01:54:46 PM EST, Dmitri Pal wrote:
>  >>>>>On 11/06/2012 01:45 PM, Simo Sorce wrote:
>  >>>>>>                  • If all lists are empty, access is granted
>  >>>>>>                  • If any list is provided, the order of
>  >>>>>>evaluation is
>  >>>>>>                  allow,deny. This means that any matching deny
>  >>>>>>rule will
>  >>>>>>                  supersede any matched allow rule.
>  >>>>>>                  • If either or both "allow" lists are
> provided,
>  >>>>>>all
>  >>>>>>                  users are denied unless they appear in the
> list.
>  >>>>>>                  • If only "deny" lists are provided,
> all users are
>  >>>>>>                  granted access unless they appear in the list.
>  >>>><snip>
>  >>>>>Following the first bullet in man page "if all lists are empty
> the
>  >>>>>access is granted".
>  >>>>>It works as advertised right?
>  >>>>>So I do not see why anything needs to be changed then.
>  >>>>>
>  >>>>Yeah, that phrasing certainly seems to make it pretty clear that
>  >>>>'simple_allow_users = ' is an empty list. I would prefer that we
> not
>  >>>>change the meaning of this because it *would* be a
>  >>>>backwards-incompatible change. This strikes me as something we could
>  >>>>stick in a FAQ somewhere: "Be wary if you are using automated tools
> to
>  >>>>generate this option. Specifying no values here is equivalent to
>  >>>>omitting the option entirely. If you really want to specify no users
>  >>>>are allowed, it's preferable to use 'access_provider =
> deny'."
>  >>>Agreed, let's kill off this thread and the proposal.
>  >>>Sorry Ondrej and Stef, seem like changing this is just not desirable.
>  >>>
>  >>>Simo.
>  >>>
>  >>ack. IMO it should be just clarified in the man page.
>  >>
>  >patch for manpage attached
>  >
>  >O.
>  >
>  
>  Ack 
Pushed to master and sssd-1-9

[Jakub Hrozek]

On Thu, Nov 01, 2012 at 03:04:11PM -0400, Simo Sorce wrote:
>  It's because the DB cannot hold an empty value.
>  With the current interface empty value = No value.
>   
You can easily check for an option being present with
confdb_get_param(). Then look at its value to determine if there is any
(and if it's a valid list).

[Ondrej Kos]

On 11/02/2012 11:50 AM, Jakub Hrozek wrote:
>  On Thu, Nov 01, 2012 at 03:04:11PM -0400, Simo Sorce wrote:
> > It's because the DB cannot hold an empty value.
> > With the current interface empty value = No value.
> >
>
>  You can easily check for an option being present with
>  confdb_get_param(). Then look at its value to determine if there is any
>  (and if it's a valid list).
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>  
confdb_get_param returns zero (EOK) for all lists of simple rules, 
whether they're present in the config file or not. If the list is empty, 
then the values pointer points to null, same as if the list is not filed 
at all.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: [phone]
cell:  [phone]
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno