On (08/12/14 10:11), Pavel Reichl wrote:
>
>On 12/05/2014 10:47 AM, Lukas Slebodnik wrote:
>>On (13/11/14 12:24), Pavel Reichl wrote:
>>>On 06/04/2014 07:49 PM, Pavel Reichl wrote:
>>>>On Wed, 2014-06-04 at 16:11 +0200, Jakub Hrozek wrote:
>>>>>On Wed, Jun 04, 2014 at 09:58:20AM +0200, Sumit Bose wrote:
>>>>>>On Wed, Jun 04, 2014 at 08:42:21AM +0200, Jakub Hrozek wrote:
>>>>>>>On Tue, Jun 03, 2014 at 04:22:51PM -0400, Simo Sorce wrote:
>>>>>>>>On Tue, 2014-06-03 at 15:52 -0400, Stephen Gallagher wrote:
>>>>>>>>>On 06/03/2014 08:26 AM, Pavel Reichl wrote:
>>>>>>>>>>Hello,
>>>>>>>>>>
>>>>>>>>>>I noticed that if using simple access provider and having
>>>>>>>>>>non-existing group or user in access/deny list then access will be
>>>>>>>>>>denied and "su: System error" will be printed.
>>>>>>>>>>
>>>>>>>>>>I think it's OK to simply skip non-existing objects on allow_list.
>>>>>>>>>>
>>>>>>>>>>I'm not so sure what to do in case of deny lists. Should we also
>>>>>>>>>>just skip them or should we deny the user and print more
>>>>>>>>>>appropriate message ("su: Permission denied")?
>>>>>>>>>I agree that skipping (and logging) on allow lists is fine.
>>>>>>>>>
>>>>>>>>>For deny lists, it implies that either 1) the admin typoed the
>>>>>>>>>user/group name in the list or 2) that the user/group was removed from
>>>>>>>>>LDAP.
>>>>>>>>>
>>>>>>>>>In the first case, we're potentially dealing with privilege leakage
>>>>>>>>>(someone who shouldn't have access has it due to an admin
>>>>>>>>>misconfiguration). In the second case, this is perhaps just normal
>>>>>>>>>operating changes and shouldn't require client modification.
>>>>>>>>>
>>>>>>>>>As I type this, I become more certain that the correct approach here
>>>>>>>>>should be to log this with a better message (in both
>>>>>>>>>SSSDBG_CRIT_FAILURE and sss_log) and just proceed as if it didn't
>>>>>>>>>exist.
>>>>>>>>>
>>>>>>>>>A better message would perhaps be:
>>>>>>>>>"The [user|group] %s does not exist. Possible typo in
>>>>>>>>>simple_[allow|deny]_[users|groups]"
>>>>>>>>The secure thing to do is to fail, because you do not know with
>>>>>>>>certainty who should be allowed.
>>>>>>>So if an admin typoes a group, noone can log in? That might effectivelly
>>>>>>>lock out legitimate access that would subsequently use sudo vi to fix the
>>>>>>>typo..
>>>>>>I think we can skip with a log message in the allow case because then
>>>>>>access is still only allowed if another entry matches.
>>>>>>
>>>>>>In the deny case I would prefer a reject as well. With this we would
>>>>>>make the allow list more comfortable to use and people might rethink
>>>>>>their deny list usage in environments where groups are often deleted or
>>>>>>renamed.
>>>>>>
>>>>>>bye,
>>>>>>Sumit
>>>>>OK, that sounds like a far compromise. I also hope noone would use the
>>>>>deny list then. Thanks!
>>>>Hello,
>>>>new patches are attached.
>>>>
>>>>1st patch:
>>>>I amended the debug messages as Stephen suggested.
>>>>Non-existing objects are ignored on allow lists, but imply access denied
>>>>on deny lists.
>>>>
>>>>2nd patch:
>>>>amended mam page - documenting missing objects from deny lists.
>>>>
>>>>3rd patch:
>>>>Minor optimization - don't continue matching username with names from
>>>>simple_allow_list after successful match.
>>>>
>>>I have rebased 1st and 3rd patch as there are users who might benefit from
>>>the changes immediately.
>>>From d4e70ce1c81476e4546709c898c75afb881d7b9c Mon Sep 17 00:00:00 2001
>>>From: Pavel Reichl <prei...@redhat.com>
>>>Date: Wed, 4 Jun 2014 17:41:31 +0100
>>>Subject: [PATCH 1/3] simple access provider: non-existing object
>>>
>>>Not existing user/group in simple_allow_users/simple_allow_groups should not
>>>imply access denied.
>>>---
>>>src/providers/simple/simple_access_check.c | 36
>>>+++++++++++++++++++++---------
>>>1 file changed, 26 insertions(+), 10 deletions(-)
>>>
>>>diff --git a/src/providers/simple/simple_access_check.c
>>>b/src/providers/simple/simple_access_check.c
>>>index
>>>13c66d58f71225a6c458c19e7fb9d26fd15c08ea..01f1d392a81fac2d1f7e237fee56df649b192cb4
>>> 100644
>>>--- a/src/providers/simple/simple_access_check.c
>>>+++ b/src/providers/simple/simple_access_check.c
>>>@@ -24,6 +24,11 @@
>>>#include "util/sss_utf8.h"
>>>#include "db/sysdb.h"
>>>
>>>+#define NON_EXIST_USR_ALLOW "The user %s does not exist. Possible typo in
>>>simple_allow_users.\n"
>>>+#define NON_EXIST_USR_DENY "The user %s does not exist. Possible typo in
>>>simple_deny_users.\n"
>>>+#define NON_EXIST_GRP_ALLOW "The group %s does not exist. Possible typo in
>>>simple_allow_groups.\n"
>>>+#define NON_EXIST_GRP_DENY "The group %s does not exist. Possible typo in
>>>simple_deny_groups.\n"
>>>+
>>>static bool
>>>is_posix(const struct ldb_message *group)
>>>{
>>>@@ -53,9 +58,11 @@ simple_check_users(struct simple_ctx *ctx, const char
>>>*username,
>>> domain = find_domain_by_object_name(ctx->domain,
>>> ctx->allow_users[i]);
>>> if (domain == NULL) {
>>>- DEBUG(SSSDBG_CRIT_FAILURE, "Invalid user %s!\n",
>>>- ctx->allow_users[i]);
>>>- return EINVAL;
>>>+ DEBUG(SSSDBG_CRIT_FAILURE, NON_EXIST_USR_ALLOW,
>>>+ ctx->allow_users[i]);
>>>+ sss_log(SSS_LOG_CRIT, NON_EXIST_USR_ALLOW,
>>>+ ctx->allow_users[i]);
>>>+ continue;
>>> }
>>>
>>> if (sss_string_equal(domain->case_sensitive, username,
>>>@@ -86,9 +93,12 @@ simple_check_users(struct simple_ctx *ctx, const char
>>>*username,
>>> domain = find_domain_by_object_name(ctx->domain,
>>> ctx->deny_users[i]);
>>> if (domain == NULL) {
>>>- DEBUG(SSSDBG_CRIT_FAILURE, "Invalid user %s!\n",
>>>- ctx->deny_users[i]);
>>>+ DEBUG(SSSDBG_CRIT_FAILURE, NON_EXIST_USR_DENY,
>>>+ ctx->deny_users[i]);
>>>+ sss_log(SSS_LOG_CRIT, NON_EXIST_USR_DENY,
>>>+ ctx->deny_users[i]);
>>> return EINVAL;
>>>+
>>This extra line needn't be added.
>>
>>Otherwise code looks good to me but I need to run some tests.
>>
>>LS
>OK, updated patches attached.
>
Thank you.
>From e536c51728d2c99ff4fe7146b0a27b40c51164b3 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <prei...@redhat.com>
>Date: Wed, 4 Jun 2014 17:41:31 +0100
>Subject: [PATCH 1/2] simple access provider: non-existing object
>
>Not existing user/group in simple_allow_users/simple_allow_groups should not
>imply access denied.
>---
ACK
>From e6b832abb42512f14ad183817bdf75b7b263bf37 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <prei...@redhat.com>
>Date: Wed, 4 Jun 2014 18:24:08 +0100
>Subject: [PATCH 2/2] simple-access-provider: break matching allowed users
>
>Stop matching username with names in simple_allow_users after positive
>match.
>---
ACK
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
