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 <[email protected]>
freeipa · Apr 6, 2022 · 95cef1b · 95cef1b
1 parent 715bd0d
commit 95cef1b
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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'