From 95cef1bb86a9e8d48a3809aac9437277c9454a82 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 0a7641e1..59e8b761 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 @@ def check(self): 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 @@ def check(self): 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 47281712..c86be0d0 100644 --- a/src/ipahealthcheck/ipa/files.py +++ b/src/ipahealthcheck/ipa/files.py @@ -123,7 +123,8 @@ def check(self): 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 10824ab5..6e3ec383 100644 --- a/tests/test_core_files.py +++ b/tests/test_core_files.py @@ -17,7 +17,8 @@ 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'