From 26ea7cd4fd4b2f8661f9cbc421ca0a02c5f4082e Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Thu, 28 Apr 2022 17:22:47 -0400 Subject: [PATCH] Allow multiple file modes in the FileChecker Resolves: #2072708 --- ...ltiple-file-modes-in-the-FileChecker.patch | 198 ++++++++++++++++++ freeipa-healthcheck.spec | 6 +- 2 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 0006-Allow-multiple-file-modes-in-the-FileChecker.patch diff --git a/0006-Allow-multiple-file-modes-in-the-FileChecker.patch b/0006-Allow-multiple-file-modes-in-the-FileChecker.patch new file mode 100644 index 0000000..2595f81 --- /dev/null +++ b/0006-Allow-multiple-file-modes-in-the-FileChecker.patch @@ -0,0 +1,198 @@ +From 621a32bb892fa36cb2fd78a3f30f6080300b0bef Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Tue, 5 Apr 2022 09:25:45 -0400 +Subject: [PATCH] Allow multiple file modes in the FileChecker + +There are some cases where a strict file mode is not +necessary. The kadmind.log is one. + +It is owned root:root so there is no real difference +between 0600 and 0640. So allow both. + +https://bugzilla.redhat.com/show_bug.cgi?id=2058239 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/files.py | 31 ++++++++++++++++------ + src/ipahealthcheck/ipa/files.py | 3 ++- + tests/test_core_files.py | 44 +++++++++++++++++++++++++++++--- + 3 files changed, 65 insertions(+), 13 deletions(-) + +diff --git a/src/ipahealthcheck/core/files.py b/src/ipahealthcheck/core/files.py +index 0a7641e..59e8b76 100644 +--- a/src/ipahealthcheck/core/files.py ++++ b/src/ipahealthcheck/core/files.py +@@ -16,7 +16,7 @@ class FileCheck: + files is a tuple of tuples. Each tuple consists of: + (path, expected_perm, expected_owner, expected_group) + +- perm is in the form of a POSIX ACL: e.g. 0440, 0770. ++ perm is a POSIX ACL as either a string or tuple: e.g. 0440, (0770,). + owner and group are names or a tuple of names, not uid/gid. + + If owner and/or group are tuples then all names are checked. +@@ -33,6 +33,8 @@ class FileCheck: + owner = tuple((owner,)) + if not isinstance(group, tuple): + group = tuple((group,)) ++ if not isinstance(mode, tuple): ++ mode = tuple((mode,)) + if not os.path.exists(path): + for type in ('mode', 'owner', 'group'): + key = '%s_%s' % (path.replace('/', '_'), type) +@@ -43,19 +45,32 @@ class FileCheck: + stat = os.stat(path) + fmode = str(oct(stat.st_mode)[-4:]) + key = '%s_mode' % path.replace('/', '_') +- if mode != fmode: +- if mode < fmode: ++ if fmode not in mode: ++ if len(mode) == 1: ++ modes = mode[0] ++ else: ++ modes = 'one of {}'.format(','.join(mode)) ++ if all(m < fmode for m in mode): + yield Result(self, constants.WARNING, key=key, +- path=path, type='mode', expected=mode, ++ path=path, type='mode', expected=modes, + got=fmode, + msg='Permissions of %s are too permissive: ' +- '%s and should be %s' % (path, fmode, mode)) +- if mode > fmode: ++ '%s and should be %s' % ++ (path, fmode, modes)) ++ elif all(m > fmode for m in mode): + yield Result(self, constants.ERROR, key=key, +- path=path, type='mode', expected=mode, ++ path=path, type='mode', expected=modes, + got=fmode, + msg='Permissions of %s are too restrictive: ' +- '%s and should be %s' % (path, fmode, mode)) ++ '%s and should be %s' % ++ (path, fmode, modes)) ++ else: ++ yield Result(self, constants.ERROR, key=key, ++ path=path, type='mode', expected=modes, ++ got=fmode, ++ msg='Permissions of %s are unexpected: ' ++ '%s and should be %s' % ++ (path, fmode, modes)) + else: + yield Result(self, constants.SUCCESS, key=key, + type='mode', path=path) +diff --git a/src/ipahealthcheck/ipa/files.py b/src/ipahealthcheck/ipa/files.py +index 4728171..c86be0d 100644 +--- a/src/ipahealthcheck/ipa/files.py ++++ b/src/ipahealthcheck/ipa/files.py +@@ -123,7 +123,8 @@ class IPAFileCheck(IPAPlugin, FileCheck): + self.files.append((paths.IPA_CUSTODIA_AUDIT_LOG, + 'root', 'root', '0644')) + +- self.files.append((paths.KADMIND_LOG, 'root', 'root', '0600')) ++ self.files.append((paths.KADMIND_LOG, 'root', 'root', ++ ('0600', '0640'))) + self.files.append((paths.KRB5KDC_LOG, 'root', 'root', '0640')) + + inst = api.env.realm.replace('.', '-') +diff --git a/tests/test_core_files.py b/tests/test_core_files.py +index 10824ab..6e3ec38 100644 +--- a/tests/test_core_files.py ++++ b/tests/test_core_files.py +@@ -17,7 +17,8 @@ nobody = pwd.getpwnam('nobody') + files = (('foo', 'root', 'root', '0660'), + ('bar', 'nobody', 'nobody', '0664'), + ('baz', ('root', 'nobody'), ('root', 'nobody'), '0664'), +- ('fiz', ('root', 'bin'), ('root', 'bin'), '0664'),) ++ ('fiz', ('root', 'bin'), ('root', 'bin'), '0664'), ++ ('zap', ('root', 'bin'), ('root', 'bin'), ('0664', '0640'),)) + + + def make_stat(mode=33200, uid=0, gid=0): +@@ -27,6 +28,13 @@ def make_stat(mode=33200, uid=0, gid=0): + mode = 0660 + owner = root + group = root ++ ++ Cheat sheet equivalents: ++ 0600 = 33152 ++ 0640 = 33184 ++ 0644 = 33188 ++ 0660 = 33200 ++ 0666 = 33206 + """ + # (mode, ino, dev, nlink, uid, gid, size, atime, mtime, ctime) + return posix.stat_result((mode, 1, 42, 1, uid, gid, 0, 1, 1, 1,)) +@@ -81,6 +89,11 @@ def test_files_owner(mock_stat): + assert my_results.results[3].kw.get('msg') == \ + 'Ownership of fiz is nobody and should be one of root,bin' + ++ assert my_results.results[4].result == constants.WARNING ++ assert my_results.results[4].kw.get('got') == 'nobody' ++ assert my_results.results[4].kw.get('expected') == 'root,bin' ++ assert my_results.results[4].kw.get('type') == 'owner' ++ + + @patch('os.stat') + def test_files_group(mock_stat): +@@ -119,6 +132,11 @@ def test_files_group(mock_stat): + assert my_results.results[3].kw.get('msg') == \ + 'Group of fiz is nobody and should be one of root,bin' + ++ assert my_results.results[4].result == constants.WARNING ++ assert my_results.results[4].kw.get('got') == 'nobody' ++ assert my_results.results[4].kw.get('expected') == 'root,bin' ++ assert my_results.results[4].kw.get('type') == 'group' ++ + + @patch('os.stat') + def test_files_mode(mock_stat): +@@ -133,17 +151,35 @@ def test_files_mode(mock_stat): + assert my_results.results[0].result == constants.SUCCESS + assert my_results.results[1].result == constants.ERROR + +- mock_stat.return_value = make_stat(mode=33152) ++ # Too restrictive ++ mock_stat.return_value = make_stat(mode=33152) # 0600 + results = capture_results(f) + my_results = get_results(results, 'mode') + assert my_results.results[0].result == constants.ERROR + assert my_results.results[1].result == constants.ERROR ++ assert my_results.results[2].result == constants.ERROR ++ assert my_results.results[3].result == constants.ERROR ++ assert my_results.results[4].result == constants.ERROR + +- mock_stat.return_value = make_stat(mode=33206) ++ # Too permissive ++ mock_stat.return_value = make_stat(mode=33206) # 0666 + results = capture_results(f) + my_results = get_results(results, 'mode') + assert my_results.results[0].result == constants.WARNING + assert my_results.results[1].result == constants.WARNING ++ assert my_results.results[2].result == constants.WARNING ++ assert my_results.results[3].result == constants.WARNING ++ assert my_results.results[4].result == constants.WARNING ++ ++ # Too restrictive with allowed multi-mode value ++ mock_stat.return_value = make_stat(mode=33184) # 0640 ++ results = capture_results(f) ++ my_results = get_results(results, 'mode') ++ assert my_results.results[0].result == constants.ERROR ++ assert my_results.results[1].result == constants.ERROR ++ assert my_results.results[2].result == constants.ERROR ++ assert my_results.results[3].result == constants.ERROR ++ assert my_results.results[4].result == constants.SUCCESS + + + @patch('os.path.exists') +@@ -157,7 +193,7 @@ def test_files_not_found(mock_exists): + + for type in ('mode', 'group', 'owner'): + my_results = get_results(results, type) +- assert len(my_results.results) == 4 ++ assert len(my_results.results) == len(f.files) + for result in my_results.results: + assert result.result == constants.SUCCESS + assert result.kw.get('msg') == 'File does not exist' +-- +2.31.1 + diff --git a/freeipa-healthcheck.spec b/freeipa-healthcheck.spec index fb5b42e..81be27c 100644 --- a/freeipa-healthcheck.spec +++ b/freeipa-healthcheck.spec @@ -17,7 +17,7 @@ Name: %{prefix}-healthcheck Version: 0.9 -Release: 5%{?dist} +Release: 6%{?dist} Summary: Health check tool for %{productname} BuildArch: noarch License: GPLv3 @@ -30,6 +30,7 @@ Patch0002: 0002-Handle-files-that-don-t-exist-in-FileCheck.patch Patch0003: 0003-Allow-for-HIDDEN_SERVICE-when-checking-ADTRUST-servi.patch Patch0004: 0004-Use-the-subject-base-from-the-IPA-configuration-not-.patch Patch0005: 0005-Unify-command-line-options-and-configuration.patch +Patch0006: 0006-Allow-multiple-file-modes-in-the-FileChecker.patch Requires: %{name}-core = %{version}-%{release} Requires: %{prefix}-server @@ -159,6 +160,9 @@ PYTHONPATH=src PATH=$PATH:$RPM_BUILD_ROOT/usr/bin pytest-3 tests/test_* %changelog +* Thu Apr 28 2022 Rob Crittenden - 0.9-6 +- Allow multiple file modes in the FileChecker (#2072708) + * Wed Apr 06 2022 Rob Crittenden - 0.9-5 - Add CLI options to healthcheck configuration file (#2070981)