Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add git-audit support when building the package artifacts #307

Merged
merged 10 commits into from
Sep 8, 2022

Conversation

jenstroeger
Copy link
Owner

@jenstroeger jenstroeger commented Sep 5, 2022

Related to #5 (comment): runs git-audit when the requirements.txt file is generated.

@behnazh this PR has two commits where I tried to use git-audit in two different ways:

  • commit 19cbd9e: as part of the local Makefile when generating the requirements.txt file — but that adds a new dependency to the pip-audit package for a pruned tree which makes the run a little ugly and adds a hard-wired dependency into our Makefile;
  • commit aae32ed: as a Github Action — which required a little massaging of the Makefile when it builds the requirements.txt because we can’t git-audit the package itself and had to exclude it from the requirements.txt for the audit run.*

I wonder if we should document that git-audit runs and checks all dependent packages?

—————
* And now the Action installs the pip-audit package…

@jenstroeger jenstroeger requested a review from behnazh September 5, 2022 01:35
@jenstroeger jenstroeger changed the title feat: add git-audit to the "requirements" Makefile recipe feat: add git-audit (take one and two) Sep 5, 2022
@jenstroeger
Copy link
Owner Author

As a third alternative (as discussed) here is the change that audits all packages instead of only shipping packages:

index b7c0b3c..302cfad 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -60,19 +60,25 @@ jobs:
       # those targets first and only builds the package if they succeed.
       # Build the sdist and wheel distribution of the package and docs as a zip file.
       run: make dist
+    # Generate the requirements.txt that contains the hash digests of the dependencies and
+    # generate the SBOM using CyclonDX SBOM generator.
+    - name: Generate requirements and SBOM for all packages
+      run: make requirements sbom
+    # Audit all required packages for security vulnerabilities.
+    - name: Audit required packages
+      uses: pypa/[email protected]
+      with:
+        inputs: requirements.txt
+    # Remove the old requirements.txt file (which includes _all_ packages) and generate a
+    # new one for the package and its actual and required dependencies only.
+    - name: Pruning requirements for shipping package
+      run: make prune requirements
     - name: Compute package hash
       if: matrix.os == env.ARTIFACT_OS && matrix.python == env.ARTIFACT_PYTHON
       id: compute-hash
       shell: bash
       run: |
         set -euo pipefail
-        # Generate the requirements.txt that contains the hash digests of the dependencies and
-        # generate the SBOM using CyclonDX SBOM generator.
-        make requirements sbom
-        # Remove the old requirements.txt file (which includes _all_ packages) and generate a
-        # new one for the package and its actual and required dependencies only.
-        rm requirements.txt
-        make prune requirements
         # Find the paths to the files that will be included in the release.
         TARBALL_PATH=$(find dist -name "*.tar.gz")
         WHEEL_PATH=$(find dist -name "*.whl")
@@ -85,12 +91,6 @@ jobs:
         echo "Digest of artifacts is $DIGEST."
         # Set the computed sha digest as the output of this job.
         echo "::set-output name=artifacts-sha256::$DIGEST"
-    # Audit the required packages for security vulnerabilities.
-    - name: Audit required packages
-      if: matrix.os == env.ARTIFACT_OS && matrix.python == env.ARTIFACT_PYTHON
-      uses: pypa/[email protected]
-      with:
-        inputs: requirements.txt
     # For now only generate artifacts for the specified OS and Python version in env variables.
     # Currently reusable workflows do not support setting strategy property from the caller workflow.
     - name: Upload the package artifact for debugging and release

However, I think this is a disagreeable approach because the pip-audit Action installs a whole bunch of additional packages (see this run) which introduces new packages and dependencies outside of the SBOM we just created.

@jenstroeger
Copy link
Owner Author

jenstroeger commented Sep 5, 2022

Commit 2ad6a60 adds pip-audit as part of the dev dependencies, and the Build workflow then audits all packages for all platform runners (see for example this run). I also added a new audit goal to the Makefile such that users can audit their packages locally.

Should we mention the use of bandit and pip-audit in the Security Analysis section?

@behnazh the SBOM is generated from the ./requirements.txt file which does not include the package itself. So we probably want to build the SBOM from dist/package-$(VERSION)-requirements.txt instead?

@behnazh alternatively, we could remove the --requirement command line option from the git-audit run, which would audit the installed packages instead of the requirements.txt file. In that case, we can go back to the original Makefile and the SBOM would include the package itself again. Auditing the installed packages should be ok considering that the requirements.txt is created from those same installed packages…

@jenstroeger
Copy link
Owner Author

See issue pypa/pip-audit#365 for the failing runs on Windows.

@jenstroeger jenstroeger marked this pull request as draft September 5, 2022 13:41
@jenstroeger jenstroeger changed the title feat: add git-audit (take one and two) feat: add git-audit (takes one, two, and three) Sep 5, 2022
@jenstroeger jenstroeger marked this pull request as ready for review September 5, 2022 14:14
@jenstroeger jenstroeger changed the title feat: add git-audit (takes one, two, and three) feat: add git-audit support when building the package artifacts Sep 7, 2022
.github/workflows/build.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jenstroeger jenstroeger merged commit 8e59418 into staging Sep 8, 2022
@jenstroeger jenstroeger deleted the add-pip-audit branch September 9, 2022 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants