On Tue, 2014-04-22 at 19:00 +0200, Pavel Reichl wrote:
Hello,
I tested the patches and they seem to me to be working flawlessly (so
ACK to both).
I just noticed little nitpick:
It was proposed:
> What we want to check here is that group/other do not have access.
> The check should be:
> if ((stated_mode & 0077) != 0) { /* wrong perms */ }
but instead it is required for user to have permission to read and
permissions to write and execute are optional. But I guess it doesn't
really matter.
Thank you for testing, and yeah I decided to check at least for write
permission, I think it is better to respect a lack of read permission
which normally the root account would override.
I have attached 2 patches fixing little issues of logging messages
which I encountered during testing.
See comments inline.
differences
between files
attachment
(0001-Monitor-fix-message-wrong-perm.-mode-on-config-file.patch)
From b2e9350391a99085440e1b8ff816f0c72e72d14c Mon Sep 17 00:00:00 2001
From: Pavel Reichl <reichl.pavel(a)gmail.com>
Date: Tue, 22 Apr 2014 14:40:05 +0100
Subject: [PATCH 1/2] Monitor: fix message wrong perm. mode on config
file
Add missing new line in message.
Add supporded access mode.
---
src/monitor/monitor.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index
85655de75138ca35a18c09c961814191fd528e55..a7e6a8485debd27ed0d24b787f3f583a4487a333
100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -86,8 +86,8 @@
/* Warning messages */
#define CONF_FILE_PERM_ERROR_MSG "Cannot read config file %s. Please
check "\
- "if permissions are 0600 and the
file is "\
- "owned by root.root."
+ "if permissions are 0600 or 0400 and
the "\
+ "file is owned by root.root.\n"
Maybe we should change this to be something like:
"Please check that the file is accessible only by the owner and owned by
the root user."
Because technically all these will match: 0700, 0600, 0500, 0400.
Listing them all seem excessive.
differences
between files
attachment
(0002-util-Fix-file-mode-in-debug-message.patch)
From 186424590d617dd28ff28c8ca7d5dbb509d914a3 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <reichl.pavel(a)gmail.com>
Date: Tue, 22 Apr 2014 17:43:17 +0100
Subject: [PATCH 2/2] util: Fix file mode in debug message
---
src/util/check_and_open.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/check_and_open.c b/src/util/check_and_open.c
index
80ec00e1023a05c3cf736b98ef27e61695766069..a6d0e09470027382991fe67888d366e82d282cf6
100644
--- a/src/util/check_and_open.c
+++ b/src/util/check_and_open.c
@@ -106,7 +106,7 @@ static errno_t perform_checks(struct stat
*stat_buf,
if ((st_mode & ALLPERMS) != (mode & ALLPERMS)) {
DEBUG(SSSDBG_CRIT_FAILURE,
"File has the wrong mode [%.7o], expected [%.7o].\n",
- (st_mode & ALLPERMS), (mode & ALLPERMS));
+ (stat_buf->st_mode & ALLPERMS), (mode & ALLPERMS));
return EINVAL;
}
I am pretty sure st_mode & ALLPERMS is intentional, althugh I see it is
debatable. If you want to show the original untouched mode I think you
will also need then to print the mask, to make it clear what bits we
really care for in mode.
Otherwise a file with st_mode of 0777 which we test against a mode of
0600 with a mask of 0622 (ie we care for it not being writable and we do
not care if read or execute bits are set) would become confusing.
With my version we print:
File has the wrong mode [0622], expected [0600]
Yours would print:
File has the wrong mode [0777], expected [0600]
So mine is not precise, but it tells you exactly what bits are out of
place.
Yours tells you what the file permissions actually are, but also tells
you that you should chmod 600, when in reality chmod go-w would be
sufficient.
I think mine would give a better clue, but I see possible
confusion both
ways ...
Simo.
--
Simo Sorce * Red Hat, Inc * New York