Change in vdsm[master]: logging: Add log handler that enforces perms

Dima Kuznetsov has uploaded a new change for review.

Change subject: logging: Add log handler that enforces perms
......................................................................

logging: Add log handler that enforces perms

This handler, extending WatchedFileHandler, given the desired uid + gid of the
log file, checks that the running process satisfies the uid + gid
requirement.

Change-Id: I0a4d7212cb311b22e4fb60ffdc45163a496a74d6
Signed-off-by: Dima Kuznetsov 
---
M vdsm/logUtils.py
1 file changed, 16 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/28/26728/1

diff --git a/vdsm/logUtils.py b/vdsm/logUtils.py
index 6949d44..dd16249 100644
--- a/vdsm/logUtils.py
+++ b/vdsm/logUtils.py
@@ -19,6 +19,8 @@
 #

import logging
+import logging.handlers
+import os
 import sys
 from functools import wraps
 from inspect import ismethod
@@ -163,3 +165,17 @@
             raise
         except:
             self.handleError(record)
+
+
+class EnforcingWatchedFileHandler(logging.handlers.WatchedFileHandler):
+    def __init__(self, uid, gid, *args, **kwargs):
+        self._uid = uid
+        self._gid = gid
+        logging.handlers.WatchedFileHandler.__init__(self, *args, **kwargs)
+
+    def _open(self):
+        if (os.geteuid() != self._uid) or (os.getegid() != self._gid):
+            raise RuntimeError(
+                "Attempt to open log with incorrect credentials"
+            )
+        return logging.handlers.WatchedFileHandler._open(self)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: logging: Add log handler that enforces perms
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8015/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8128/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7225/ : SUCCESS

Yaniv Bronhaim has posted comments on this change.

Change subject: logging: Add log handler that enforces perms
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

i'd like to here your thoughts about enforce this handler to avoid more permissions issues with log files

http://gerrit.ovirt.org/#/c/26728/1//COMMIT_MSG
Commit Message:

Line 7: logging: Add log handler that enforces perms
Line 8: 
Line 9: This handler, extending WatchedFileHandler, given the desired uid + gid of the
Line 10: log file, checks that the running process satisfies the uid + gid
Line 11: requirement.
otherwise raise RuntimeException
Line 12: 
Line 13: Change-Id: I0a4d7212cb311b22e4fb60ffdc45163a496a74d6

Itamar Heim has posted comments on this change.

Change subject: logging: Add log handler that enforces perms
......................................................................

Patch Set 1:

ping

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: logging: Add log handler that enforces perms
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/26728/1/vdsm/logUtils.py
File vdsm/logUtils.py:

Line 166:         except:
Line 167:             self.handleError(record)
Line 168: 
Line 169: 
Line 170: class EnforcingWatchedFileHandler(logging.handlers.WatchedFileHandler):
I do not mind using this extra safety measure, but the name of class must be changed (it's not clear what is enforced here), and please include a user for this class (in this or in a follow up patch).
Line 171:     def __init__(self, uid, gid, *args, **kwargs):
Line 172:         self._uid = uid
Line 173:         self._gid = gid
Line 174:         logging.handlers.WatchedFileHandler.__init__(self, *args, **kwargs)

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9407/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10191/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10347/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5273/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3431/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Dima Kuznetsov has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/26728/1/vdsm/logUtils.py
File vdsm/logUtils.py:

Line 166:         except:
Line 167:             self.handleError(record)
Line 168: 
Line 169: 
Line 170: class EnforcingWatchedFileHandler(logging.handlers.WatchedFileHandler):
> I do not mind using this extra safety measure, but the name of class must b

Done, hope the new name is better.

Also, could not find a way to use uid/gid with fileConfig (as they are not known at code generation time), so this class now accepts user name and group name.
Line 171:     def __init__(self, uid, gid, *args, **kwargs):
Line 172:         self._uid = uid
Line 173:         self._gid = gid
Line 174:         logging.handlers.WatchedFileHandler.__init__(self, *args, **kwargs)

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9413/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10197/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10353/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5279/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3437/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Yaniv Bronhaim has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/26728/3/vdsm/logger.conf.in
File vdsm/logger.conf.in:

Line 51: 
Line 52: [handler_metadata]
Line 53: class=logUtils.UserGroupEnforcingHandler
Line 54: args=('@VDSMUSER@', '@VDSMGROUP@', '@VDSMLOGDIR@/vdsm.log',)
Line 55: level=WARNING
you changed the name of the log

copy paste is evil from time to time ...
Line 56: formatter=long
Line 57: 
Line 58: [handler_console]
Line 59: class: StreamHandler

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Dima Kuznetsov has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/26728/3/vdsm/logger.conf.in
File vdsm/logger.conf.in:

Line 51: 
Line 52: [handler_metadata]
Line 53: class=logUtils.UserGroupEnforcingHandler
Line 54: args=('@VDSMUSER@', '@VDSMGROUP@', '@VDSMLOGDIR@/vdsm.log',)
Line 55: level=WARNING
> you changed the name of the log

you're right :(
Line 56: formatter=long
Line 57: 
Line 58: [handler_console]
Line 59: class: StreamHandler

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Yaniv Bronhaim has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 4:

I wonder:

1. if we need to touch the metadata.log at all which i don't find where we use - dima, do you see the usage? does really only vdsm user touch this log ?

2. if supervdsm and update logs also should have such check that only root user will be able to open them

but for now I prefer not to add more noises . this is good enough to avoid the permission changes we had in the recent past

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9425/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10209/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10365/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5291/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3449/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9428/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10212/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10368/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5294/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3452/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Yaniv Bronhaim has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 5: Code-Review+1

have you tested it ?

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Dima Kuznetsov has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 5:

this patch needs http://gerrit.ovirt.org/#/c/27193/ to pass

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10498/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11283/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11440/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Saggi Mizrahi has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 6: Code-Review+2

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

Dima Kuznetsov has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 7: Verified+1

Rebased, verified by:

Building, installing, and starting vdsm without errors.
Made sure vdsm.log and supervdsm.log are active and being logged to.

Tell me if any other verification should be done.

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 7:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10925/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11867/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11710/ : SUCCESS

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

log: Change vdsm log file to enforce user/group

This commit adds a new log handler that extends WatchedFileHandler,
This handler checks current user and group and checks with the expected
values, to make sure logs are created with correct permissions. In case of bad
access RuntimeError is thrown.

This patch also updates logger.conf to use this handler.

Change-Id: I0a4d7212cb311b22e4fb60ffdc45163a496a74d6
Signed-off-by: Dima Kuznetsov 
Reviewed-on: http://gerrit.ovirt.org/26728
Reviewed-by: Saggi Mizrahi 
---
M vdsm/logUtils.py
M vdsm/logger.conf.in
M vdsm/svdsm.logger.conf.in
M vdsm/vdsm
4 files changed, 36 insertions(+), 5 deletions(-)

Approvals:
  Saggi Mizrahi: Looks good to me, approved
  Dima Kuznetsov: Verified

Change in vdsm[master]: log: Change vdsm log file to enforce user/group

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: log: Change vdsm log file to enforce user/group
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5703/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/68/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3861/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1726/ : SUCCESS