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 <preichl(a)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