From 67ba360666ce44fb3fd96e20a3528e405eca68f6 Mon Sep 17 00:00:00 2001 From: Charles Coggins Date: Wed, 25 Oct 2023 10:33:15 -0500 Subject: [PATCH 1/4] docs: add support for `pull_request_target` events This is the documentation update that goes with phylum-dev/phylum-ci#331 Changes made include: * Update the action tagline and marketplace description * Provide a better value proposition * Add information about supporting pull requests from repository forks * Pre-requisite event for workflow triggers * How to check out the PR repository * Add warnings about the security implications of supporting PR forks * Provide links explaining the risks * Provide guidance on how to use the Phylum GHA in a secure manner * Change more instances of `lock.file` to `dependency.file` --- README.md | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++--- action.yml | 2 +- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 21747ed..2d057ee 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ ![GitHub last commit](https://img.shields.io/github/last-commit/phylum-dev/phylum-analyze-pr-action) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)][CoC] -A GitHub Action using Phylum to automatically analyze Pull Requests for changes to package manager dependency files. +A GitHub Action to analyze dependencies with Phylum to protect your code against increasingly sophisticated attacks and get peace of mind to focus on your work. [license]: https://github.com/phylum-dev/phylum-analyze-pr-action/blob/main/LICENSE [issues]: https://github.com/phylum-dev/phylum-analyze-pr-action/issues @@ -45,6 +45,7 @@ The pre-requisites for using this image are: * [Contact Phylum][phylum_contact] or [register][app_register] to gain access * See also [`phylum auth register`][phylum_register] command documentation * Consider using a bot or group account for this token + * Forked repos require the `pull_request_target` event, to allow secret access * Access to the Phylum API endpoints * That usually means a connection to the internet, optionally via a proxy * Support for on-premises installs are not available at this time @@ -150,6 +151,35 @@ on: - develop ``` +Allowing pull requests from forked repositories requires using the `pull_request_target` event since the Phylum API +key is stored as a secret and the `pull_request` event does not provide access to secrets when the PR comes from a +fork. + +```yaml +on: + pull_request: + # Allow PRs from forked repos to access secrets, like the Phylum API key + pull_request_target: +``` + +> ⚠️ **WARNING** ⚠️ +> +> Using the `pull_request_target` event for forked repositories requires additional configuration when +> [checking out the repo](#checking-out-the-repository). Be aware that such a configuration has security implications +> if done improperly. Please take the time to understand and mitigate the risks: +> +> * GitHub Security Lab: ["Preventing pwn requests"][gh_pwn] +> * GitGuardian: ["GitHub Actions Security Best Practices"][gha_security] +> +> Minimal suggestions include: +> +> * Use a separate workflow for the Phylum Analyze PR action +> * Do not provide access to any secrets beyond the Phylum API key +> * Limit the steps in the job to two: checking out the PR's code and using the Phylum action + +[gh_pwn]: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ +[gha_security]: https://blog.gitguardian.com/github-actions-security-cheat-sheet/ + ### Permissions When using the default `GITHUB_TOKEN` provided automatically at the start of each workflow run, it is good practice to @@ -203,6 +233,34 @@ the local working copy is always pristine and history is available to pull the r fetch-depth: 0 ``` +Allowing pull requests from forked repositories [requires using the `pull_request_target` event](#workflow-trigger) +and checking out the head of the forked repository: + +```yaml + steps: + - name: Checkout the repo + uses: actions/checkout@v4 + with: + fetch-depth: 0 + # Specifying the head of the forked repository's PR branch + # is required to get any proposed dependency file changes. + ref: ${{ github.event.pull_request.head.sha }} +``` + +> ⚠️ **WARNING** ⚠️ +> +> Using the `pull_request_target` event for forked repositories and checking out the pull request's code has security +> implications if done improperly. Please take the time to understand and mitigate the risks: +> +> * GitHub Security Lab: ["Preventing pwn requests"][gh_pwn] +> * GitGuardian: ["GitHub Actions Security Best Practices"][gha_security] +> +> Minimal suggestions include: +> +> * Use a separate workflow for the Phylum Analyze PR action +> * Do not provide access to any secrets beyond the Phylum API key +> * Limit the steps in the job to two: checking out the PR's code and using the Phylum action + ### Action Inputs The action inputs are used to ensure the `phylum-ci` tool is able to perform it's job. @@ -280,7 +338,7 @@ view the [script options output][script_options] for the latest release. # and committing the generated `.phylum_project` file. cmd: phylum-ci --lockfile requirements-prod.txt # Specify multiple explicit dependency file paths - cmd: phylum-ci --lockfile requirements-prod.txt path/to/lock.file + cmd: phylum-ci --lockfile requirements-prod.txt path/to/dependency.file # Install a specific version of the Phylum CLI. cmd: phylum-ci --phylum-release 4.8.0 --force-install # Mix and match for your specific use case. @@ -288,7 +346,7 @@ view the [script options output][script_options] for the latest release. phylum-ci \ -vv \ --lockfile requirements-dev.txt \ - --lockfile requirements-prod.txt path/to/lock.file \ + --lockfile requirements-prod.txt path/to/dependency.file \ --lockfile Cargo.toml \ --force-analysis \ --all-deps diff --git a/action.yml b/action.yml index 229a301..ca7a13b 100644 --- a/action.yml +++ b/action.yml @@ -9,7 +9,7 @@ --- name: Phylum Analyze PR author: Phylum, Inc. -description: Analyze dependencies in a pull request with Phylum +description: Analyze dependencies with Phylum to protect your code against increasingly sophisticated attacks and get peace of mind to focus on your work branding: icon: check-circle color: blue From 17db89a9ec39239abb0a6a07a7fba672ce363066 Mon Sep 17 00:00:00 2001 From: Charles Coggins Date: Mon, 4 Dec 2023 14:50:24 -0600 Subject: [PATCH 2/4] docs: PR suggestions and update marketplace description --- README.md | 12 +++++++----- action.yml | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 2d057ee..e374683 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ information on [lockfile generation][lockfile_generation] can be found for curre ## Getting Started -Phylum analysis of dependencies can be added to existing CI workflows or on it's own with this minimal configuration: +Phylum analysis of dependencies can be added to existing CI workflows or on its own with this minimal configuration: ```yaml name: Phylum_analyze @@ -166,7 +166,8 @@ on: > > Using the `pull_request_target` event for forked repositories requires additional configuration when > [checking out the repo](#checking-out-the-repository). Be aware that such a configuration has security implications -> if done improperly. Please take the time to understand and mitigate the risks: +> if done improperly. Attackers may be able to obtain repository write permissions or steal repository secrets. +> Please take the time to understand and mitigate the risks: > > * GitHub Security Lab: ["Preventing pwn requests"][gh_pwn] > * GitGuardian: ["GitHub Actions Security Best Practices"][gha_security] @@ -250,7 +251,8 @@ and checking out the head of the forked repository: > ⚠️ **WARNING** ⚠️ > > Using the `pull_request_target` event for forked repositories and checking out the pull request's code has security -> implications if done improperly. Please take the time to understand and mitigate the risks: +> implications if done improperly. Attackers may be able to obtain repository write permissions or steal repository +> secrets. Please take the time to understand and mitigate the risks: > > * GitHub Security Lab: ["Preventing pwn requests"][gh_pwn] > * GitGuardian: ["GitHub Actions Security Best Practices"][gha_security] @@ -263,7 +265,7 @@ and checking out the head of the forked repository: ### Action Inputs -The action inputs are used to ensure the `phylum-ci` tool is able to perform it's job. +The action inputs are used to ensure the `phylum-ci` tool is able to perform its job. A [Phylum token][phylum_tokens] with API access is required to perform analysis on project dependencies. [Contact Phylum][phylum_contact] or [register][app_register] to gain access. @@ -387,7 +389,7 @@ installed version of the Phylum CLI and all required tools needed for [lockfile An advantage of using the default Docker image is that the complete environment is packaged and made available with components that are known to work together. -One disadvantage to the default image is it's size. It can take a while to download and may provide more tools than +One disadvantage to the default image is its size. It can take a while to download and may provide more tools than required for your specific use case. Special `slim` tags of the `phylum-ci` image are provided as an alternative. These tags differ from the default image in that they do not contain the required tools needed for [lockfile generation][lockfile_generation] (with the exception of the `pip` tool). The `slim` tags are significantly diff --git a/action.yml b/action.yml index ca7a13b..20c1830 100644 --- a/action.yml +++ b/action.yml @@ -9,7 +9,7 @@ --- name: Phylum Analyze PR author: Phylum, Inc. -description: Analyze dependencies with Phylum to protect your code against increasingly sophisticated attacks and get peace of mind to focus on your work +description: Scan dependencies with Phylum to block software supply chain attacks branding: icon: check-circle color: blue From d1ada28483caa79d601fd670a30d5cd8446fdb4a Mon Sep 17 00:00:00 2001 From: Charles Coggins Date: Tue, 5 Dec 2023 13:27:48 -0600 Subject: [PATCH 3/4] docs: add FAQs section to README The new FAQs section seeks to address some common questions that may arise from making use of the new `pull_request_target` support. Namely, why analysis fails for PRs from forked repositories and how to remove that failure. --- README.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/README.md b/README.md index e374683..4608028 100644 --- a/README.md +++ b/README.md @@ -480,6 +480,55 @@ options are the same as [already documented](#getting-started). [container_step]: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsuses +## FAQs + +### Why does Phylum report a failing status check if it shows a successful analysis comment? + +It is possible to get a successful Phylum analysis comment on the PR **and also** have the Phylum action report a +failing status check. This happens when one or more dependency files fails the filtering process while at least one +dependency file passes the filtering process **and** the Phylum analysis. + +The failing status check is meant to serve as an indication to the repository owner that an issue exists with at least +one of the dependency files submitted, whether they intended it or not. The reasoning is that it is better to be +explicit about possible failures, allowing for review of the logs and correction, than to silently ignore the failure +and possibly allow untrusted code into the repository. + +There are several reasons a dependency file may fail the filtering process and each failure will be included in the logs +as a warning. The file may not exist or it may exist, but only as an empty file. The file may fail to be parsed by +Phylum. The dependency files can be manifests or lockfiles and they can either be provided explicitly or automatically +detected when not provided. Sometimes the automatic detection will misattribute a file as a manifest or assign the wrong +lockfile type. As detailed in the ["Supported Dependency Files"](#supported-dependency-files) section, the +recommendation for this situation is to specify the path and lockfile type explicitly in a `.phylum_project` file at +the root of the project repository. + +### Why does analysis fail for PRs from forked repositories? + +Another reason why Phylum reports +[failing status checks](#why-does-phylum-report-a-failing-status-check-if-it-shows-a-successful-analysis-comment) is for +`pull_request_target` events where manifests are provided. Using `pull_request_target` events for forked repositories +has security implications if done improperly. Attackers may be able to obtain repository write permissions or steal +repository secrets. A more comprehensive enumeration of the risks can be found here: + +* GitHub Security Lab: ["Preventing pwn requests"][gh_pwn] +* GitGuardian: ["GitHub Actions Security Best Practices"][gha_security] + +This GitHub action disables lockfile generation to prevent arbitrary code execution in an untrusted context, like PRs +from forks. This means that provided manifests are unable to be parsed by Phylum since parsing first requires generating +a lockfile from the manifest. A unique error code and warning message is provided so as to better signal the +implication: the resolved dependencies from the manifest have NOT been analyzed by Phylum. Care should be taken to +inspect changes manually before allowing a manifest to be used in a trusted context. + +One way to avoid this situation and enable automatic Phylum analysis is to add a lockfile instead of or along with the +manifest. This is recommended even for libraries. A great case for why +[lockfiles should be committed on **all** projects][lockfiles_always] was made by the folks who built the Yarn package +manager for the npm ecosystem and the same advice applies to other ecosystems. Adding the lockfile is good, but it will +need to be paired with additional workflow logic that requires it to be updated and current with the corresponding +manifest as part of the PR submission. Otherwise, it will be possible to supply a valid and benign lockfile while +updating the manifest with malicious entries (perhaps transitively) to be included the next time the lockfile is +refreshed. + +[lockfiles_always]: https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/ + ## License Copyright (C) 2022 Phylum, Inc. From c856197cb35790617c07a8a7313e9c32f598a194 Mon Sep 17 00:00:00 2001 From: Charles Coggins Date: Tue, 5 Dec 2023 14:20:04 -0600 Subject: [PATCH 4/4] docs: apply PR suggestion --- README.md | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/README.md b/README.md index 4608028..14bd994 100644 --- a/README.md +++ b/README.md @@ -518,17 +518,6 @@ a lockfile from the manifest. A unique error code and warning message is provide implication: the resolved dependencies from the manifest have NOT been analyzed by Phylum. Care should be taken to inspect changes manually before allowing a manifest to be used in a trusted context. -One way to avoid this situation and enable automatic Phylum analysis is to add a lockfile instead of or along with the -manifest. This is recommended even for libraries. A great case for why -[lockfiles should be committed on **all** projects][lockfiles_always] was made by the folks who built the Yarn package -manager for the npm ecosystem and the same advice applies to other ecosystems. Adding the lockfile is good, but it will -need to be paired with additional workflow logic that requires it to be updated and current with the corresponding -manifest as part of the PR submission. Otherwise, it will be possible to supply a valid and benign lockfile while -updating the manifest with malicious entries (perhaps transitively) to be included the next time the lockfile is -refreshed. - -[lockfiles_always]: https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/ - ## License Copyright (C) 2022 Phylum, Inc.