CI: Add bandit to pre-commit (fixes #1110) (#1523)

* fixes #1110

Co-authored-by: Bread Genie <[email protected]>

Author: terriko

.github/actions/spelling/allow.txt

@@ -234,6 +234,7 @@ nitishsaini
 nlk
 noopener
 noreferrer
+nosec
 nowdailynever
 nplurals
 ntp
@@ -341,6 +342,7 @@ unicode
 uniq
 unittest
 url
+urlopen
 usecase
 username
 usr

.github/workflows/pythonapp.yml

@@ -17,7 +17,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        tool: ['isort', 'black', 'pyupgrade', 'flake8', 'format_checkers']
+        tool: ['isort', 'black', 'pyupgrade', 'flake8', 'format_checkers', 'bandit']
     steps:
       - uses: actions/checkout@v2
       - uses: actions/setup-python@v2

.pre-commit-config.yaml

@@ -14,12 +14,18 @@ repos:
     hooks:
     - id: pyupgrade
       args: ["--py37-plus"]
-      
+
 -   repo: https://github.com/pycqa/flake8
     rev: 4.0.1
     hooks:
     - id: flake8
 
+-   repo: https://github.com/PyCQA/bandit
+    rev: 1.7.1
+    hooks:
+    - id: bandit
+      args: ["-c", "bandit.conf"]
+
 -   repo: local
     hooks:
     - id: format_checkers

CONTRIBUTING.md

@@ -17,6 +17,7 @@ If you've already contributed to other open source projects, contributing to the
     - [Using pre-commit to run linters automatically](#using-pre-commit-to-run-linters-automatically)
     - [Running isort by itself](#running-isort-by-itself)
     - [Running black by itself](#running-black-by-itself)
+    - [Running bandit by itself](#running-bandit-by-itself)
     - [Other linting tools](#other-linting-tools)
   - [Making a new branch & pull request](#making-a-new-branch--pull-request)
     - [Commit message tips](#commit-message-tips)
@@ -160,6 +161,7 @@ CVE Binary Tool uses a few tools to improve code quality and readability:
 - `black` provides automatic style formatting.  This will give you basic [PEP8](https://www.python.org/dev/peps/pep-0008/) compliance. (PEP8 is where the default python style guide is defined.)
 - `flake8` provides additional code "linting" for more complex errors like unused imports.
 - `pyupgrade` helps us be forward compatible with new versions of python.
+- `bandit` is more of a static analysis tool than a linter and helps us find potential security flaws in the code.
 
 We provide a `dev-requirements.txt` file which includes all the precise versions of tools as they'll be used in GitHub Actions.  You an install them all using pip:
 
@@ -206,6 +208,26 @@ files you've changed because you won't have to scroll through a pile of
 auto-formatting changes to find your own modifications.  However, you can also
 specify a whole folder using ```./```
 
+### Running bandit by itself
+
+We have a configuration file for bandit called `bandit.conf` that you should use.  This disables a few of the checkers and disables scanning of the test directory.
+
+To run it on all the code we scan, use the following:
+
+```bash
+bandit -c bandit.conf -r cve_bin_tool/
+```
+
+You can also run it on individual files:
+
+```bash
+bandit -c bandit.conf filename.py
+```
+
+If you run it without the config file, it will run a few extra checkers and will run on test code, so you'll get additional warnings.
+
+Bandit helps you target manual code review, but bandit issues aren't always things that need to be fixed, just reviewed.  If you have a bandit finding that doesn't actually need a fix, you can mark it as reviewed using a `# nosec` comment.  If possible, include details as to why the bandit results are ok for future reviewers.  For example, we have comments like `#nosec uses static https url above` in cases where bandit prompted us to review the variable being passed to urlopen().
+
 ### Other linting tools
 
 As well as `black` for automatically making sure code adheres to the style guide, we use `flake8` to help us find things like unused imports.  The [flake8 documentation](https://flake8.pycqa.org/en/latest/user/index.html) covers what you need to know about running it.

bandit.conf

@@ -85,10 +85,19 @@
 tests:
 
 # (optional) list skipped test IDs here, eg '[B101, B406]':
-skips: ['B603', 'B607', 'B404']
+skips: ['B603', 'B607', 'B404', "B608"]
+# B603, B607 and B404 are all subprocess-related.
+# B608 should be re-enabled when multi-line issues can be marked with nosec
+
 # Explantion: cve-bin-tool is at heart a shell script that calls other processes.
 # Switching to pure python has significant performance impacts.
 
+exclude_dirs:
+  - "test/"
+  - "/test/"
+  - "./test/"
+  - "./build/lib/test/"
+
 ### (optional) plugin settings - some test plugins require configuration data
 ### that may be given here, per-plugin. All bandit test plugins have a built in
 ### set of sensible defaults and these will be used if no configuration is

cve_bin_tool/available_fix/redhat_cve_tracker.py

@@ -74,8 +74,8 @@ def cve_info(
 
     def get_data(self, cve_number: str, product: str):
         try:
-            full_query = f"{RH_CVE_API}/{cve_number}.json"
-            response = request.urlopen(full_query).read().decode("utf-8")
+            full_query = f"{RH_CVE_API}/{cve_number}.json"  # static https url above
+            response = request.urlopen(full_query).read().decode("utf-8")  # nosec
             return loads(response)
         except error.HTTPError as e:
             LOGGER.debug(e)

dev-requirements.txt

@@ -2,3 +2,4 @@ black==21.12b0
 isort==5.10.1
 pre-commit==2.16.0
 flake8==4.0.1
+bandit==1.7.1