Merge pull request #3666 from mulkieran/bandit

Add bandit and pyright checks to test code

.githooks/pre-commit

@@ -12,7 +12,7 @@ make fmt-ci &&
 	make clippy &&
 	make yamllint &&
 	make tmtlint &&
-	make pylint &&
+	make lint &&
 	make check-typos || exit 1
 
 export PYTHONPATH=$PWD/tests/client-dbus/src

.github/workflows/main.yml

@@ -192,11 +192,17 @@ jobs:
       - name: Install dependencies
         run: >
           dnf install -y
+          bandit
           make
+          pip
           pylint
           python3-dbus
-      - name: Run pylint
-        run: make -f Makefile pylint
+      - name: Install pyright
+        run: pip install --user pyright
+      - name: Run lint
+        run: >
+          PATH=${PATH}:/github/home/.local/bin
+          make -f Makefile lint
 
   python-based-tests:
     runs-on: ubuntu-22.04

.github/workflows/support.yml

@@ -39,6 +39,7 @@ jobs:
       matrix:
         include:
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen
@@ -47,7 +48,9 @@ jobs:
               python3-pyudev
               python3-semantic_version
               python3-tenacity
-            task: PYTHONPATH=./src make -f Makefile lint
+            task: >
+              PATH=${PATH}:/github/home/.local/bin
+              PYTHONPATH=./src make -f Makefile lint
             working-directory: ./tests/client-dbus
           - dependencies: black python3-isort
             task: make -f Makefile fmt-ci
@@ -64,7 +67,10 @@ jobs:
           dnf install -y
           make
           ncurses
+          pip
           ${{ matrix.dependencies }}
+      - name: Install pyright
+        run: pip install --user pyright
       - name: Run test
         run: ${{ matrix.task }}
         working-directory: ${{ matrix.working-directory }}

.github/workflows/weekly.yml

@@ -17,6 +17,7 @@ jobs:
         include:
           # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen
@@ -25,7 +26,9 @@ jobs:
               python3-pyudev
               python3-semantic_version
               python3-tenacity
-            task: PYTHONPATH=./src make -f Makefile lint
+            task: >
+              PATH=${PATH}:/github/home/.local/bin
+              PYTHONPATH=./src make -f Makefile lint
             working-directory: ./tests/client-dbus
           - dependencies: black python3-isort
             task: make -f Makefile fmt-ci
@@ -45,10 +48,13 @@ jobs:
           dnf install -y
           make
           ncurses
+          pip
           python-unversioned-command
           ${{ matrix.dependencies }}
       - name: Display Python version
         run: python --version
+      - name: Install pyright
+        run: pip install --user pyright
       - name: ${{ matrix.task }}
         run: ${{ matrix.task }}
         working-directory: ${{ matrix.working-directory }}

Makefile

@@ -430,8 +430,10 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl
 	cargo clippy ${CLIPPY_OPTS}
 
 ## Lint Python parts of the source code
-pylint:
+lint:
 	pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm
+	bandit ./src/bin/utils/stratis-decode-dm --skip B101
+	pyright ./src/bin/utils/stratis-decode-dm
 
 .PHONY:
 	audit
@@ -477,7 +479,7 @@ pylint:
 	install-udev-binaries
 	install-udev-cfg
 	license
-	pylint
+	lint
 	test
 	test-valgrind
 	test-loop

tests/client-dbus/Makefile

@@ -1,9 +1,17 @@
 UNITTEST_OPTS = --verbose
 
+# Ignore bandit B404 errors. Any import of the subprocess module causes this
+# error. We know what we are doing when we import that module and do not
+# need to be warned.
+BANDIT_SKIP = --skip B404,B603,B311
+
 .PHONY: lint
 lint:
 	pylint src/stratisd_client_dbus --ignore=_introspect.py
 	pylint tests --disable=R0801
+	bandit --recursive ./src ${BANDIT_SKIP}
+	bandit --recursive ./tests ${BANDIT_SKIP},B101
+	pyright
 
 .PHONY: startup-tests
 startup-tests:

tests/client-dbus/src/stratisd_client_dbus/_implementation.py

@@ -16,7 +16,7 @@
 """
 
 # isort: STDLIB
-import xml.etree.ElementTree as ET
+import xml.etree.ElementTree as ET  # nosec B405
 
 # isort: FIRSTPARTY
 from dbus_client_gen import managed_object_class, mo_query_builder
@@ -31,9 +31,9 @@
 )
 from ._introspect import SPECS
 
-_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])
-_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])
-_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])
+_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])  # nosec B314
+_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])  # nosec B314
+_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])  # nosec B314
 
 pools = mo_query_builder(_POOL_SPEC)
 filesystems = mo_query_builder(_FILESYSTEM_SPEC)
@@ -46,11 +46,15 @@
 
 ObjectManager = make_class(
     "ObjectManager",
-    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),
+    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),  # nosec B314
     TIME_OUT,
 )
-Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT)
-Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT)
+Report = make_class(
+    "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT  # nosec B314
+)
+Manager = make_class(
+    "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT  # nosec B314
+)
 Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT)
 Pool = make_class("Pool", _POOL_SPEC, TIME_OUT)
 Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT)

tests/client-dbus/tests/udev/_loopback.py

@@ -105,6 +105,8 @@ def create_devices(self, number):
         :return: list of keys for the devices
         :rtype: list of uuid.UUID
         """
+        assert self.dir is not None
+
         tokens = []
 
         first_creation = self.count == 0
@@ -220,6 +222,8 @@ def destroy_all(self):
         Detach all the devices and delete the file(s) and directory!
         :return: None
         """
+        assert self.dir is not None
+
         self.destroy_devices()
         os.rmdir(self.dir)
         self.dir = None

tests/client-dbus/tests/udev/_utils.py

@@ -243,7 +243,7 @@ def settle():
     :return: None
     """
     time.sleep(2)
-    subprocess.check_call(["udevadm", "settle"])
+    subprocess.check_call(["/usr/bin/udevadm", "settle"])
 
 
 def wait_for_udev_count(expected_num):

tests/client-dbus/tests/udev/test_predict.py

@@ -94,7 +94,7 @@ def _call_blockdev_size(dev):
     :rtype: str
     """
     with subprocess.Popen(
-        ["blockdev", "--getsize64", dev],
+        ["/usr/sbin/blockdev", "--getsize64", dev],
         stdout=subprocess.PIPE,
     ) as command:
         outs, _ = command.communicate()

tests/client-dbus/tests/udev/test_revert.py

@@ -42,6 +42,9 @@
     settle,
 )
 
+_MOUNT = "/usr/bin/mount"
+_UMOUNT = "/usr/bin/umount"
+
 
 def write_file(mountdir, filename):
     """
@@ -99,7 +102,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -119,7 +122,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             write_file(mountdir, file2)
 
             Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True)
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
@@ -131,12 +134,12 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
             self.read_file(mountdir, file2)
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             # Now stop the pool, which should tear down the devices
             (_, return_code, message) = Manager.Methods.StopPool(
@@ -167,14 +170,14 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
     def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
         """
@@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             Filesystem.Properties.MergeScheduled.Set(
                 get_object(snap_object_path_1), True
             )
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
 
@@ -285,12 +288,12 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])