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
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]
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
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
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@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.
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@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
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@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.
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@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.
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@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.
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@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?
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@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.
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@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.
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@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.
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@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@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?
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
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.
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
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.
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.
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.
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
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.
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.
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.
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.
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.
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.
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'."
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.
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.
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.
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 ofevaluation 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
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
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).
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@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.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Michal Židek -
Ondrej Kos -
Simo Sorce -
Stef Walter -
Stephen Gallagher