Skip to content

feat: extend GHA integration to support pull_request_target events#341

Merged
maxrake merged 10 commits into
mainfrom
gh_pr_target
Dec 5, 2023
Merged

feat: extend GHA integration to support pull_request_target events#341
maxrake merged 10 commits into
mainfrom
gh_pr_target

Conversation

@maxrake
Copy link
Copy Markdown
Contributor

@maxrake maxrake commented Oct 24, 2023

feat!: extend GHA integration to support pull_request_target events

This change makes it possible to use the GitHub Action (GHA) integration
for PRs that originate from forks, by enabling workflows to run on the
pull_request_target event.

This change comes with some heavy security considerations. In order for
the GHA to run, a workflow using it will need to checkout the PRs code.
This requires very careful attention so as not to allow so-called
"pwn requests":

To that end, the documentation updates that go with this change will be
explicit in their warnings about how to properly use the Phylum GHA in
the most secure way possible. This effectively means the workflow should
be limited to executing the bare minimum required for Phylum analysis.

Further, the Phylum analysis that does run will need to do so in a way
that does not allow for arbitrary code execution. Therefore, this change
also makes use of the Phylum CLI feature to disable lockfile generation
for untrusted contexts like the pull_request_target event trigger in
the GitHub Actions platform that this PR is looking to support.

Unfortunately, this approach does not allow for repositories with only
manifests (e.g., libraries). That is, manifests without a corresponding
lockfile. Those will fail to parse, with a new return code unique to
that situation. The only recourse, which is given in a log message and
will be spelled out more explicitly in the corresponding documentation
updates, is to reccomend adding a lockfile instead of or along with the
manifest.

Additional changes made include:

  • Add predicate for if Phylum sandbox will work in current environment
  • Create a CLIExitCode enumeration to track Phylum CLI exit codes
  • Update ReturnCode enumeration for tracking phylum-ci return codes
    • Add descriptions
    • Make a distinction between policy failures and general failures
    • Add new code for when no dependency files were provided or detected
    • Add new code for when a manifest is attempted to be parsed but
      lockfile generation has been disabled
  • Ensure lockfiles provided as arguments are only filtered once
  • Use phylum project status instead of phylum project list for
    setting the repository URL
  • Update unit tests
  • Format and refactor throughout
    • Show the final return code, after any modifications

Closes #331

BREAKING CHANGE: Phylum CLI installs before v5.9.0-rc2 are no longer
supported. A version with support for disabling lockfile generation and
skipping sandbox usage is required.

BREAKING CHANGE: The phylum-ci return code for a policy violation that
results from a Phylum analysis has been changed from 1 to 2 in order to
make it distinct from the default failure code that is generated for all
raised SystemExit exceptions with a message instead of a code.

Checklist

  • Does this PR have an associated issue (i.e., closes #<issueNum> in description above)?
  • Have you ensured that you have met the expected acceptance criteria?
  • Have you created sufficient tests?
    • Manual testing was performed with a fork of a personal repo
  • Have you updated all affected documentation?

Testing

Changes from this PR have been built in Docker images and hosted on Docker Hub for my account, as the gh_pr_target tag.

Screenshots from the manual testing may be added here soon.

This change makes it possible to use the GitHub Action (GHA) integration
for PRs that originate from forks, by enabling workflows to run on the
`pull_request_target` event. This change is simple enough, but comes
with some heavy security considerations. In order for the GHA to run, a
workflow using it will need to checkout the PRs code. This requires very
careful attention so as not to allow so-called "pwn requests":

* securitylab.github.com/research/github-actions-preventing-pwn-requests
* blog.gitguardian.com/github-actions-security-cheat-sheet

To that end, the documentation updates that go with this change will be
explicit in their warnings about how to properly use the Phylum GHA in
the most secure way possible. This effectively means the workflow should
be limited to executing the bare minimum required for Phylum analysis.

Further, the Phylum analysis that does run will need to do so in a way
that does not allow for arbitrary code execution. Therefore, this PR is
intended to be held back until the Phylum CLI adds some additional
protections around lockfile generation.

Closes #331
@maxrake maxrake self-assigned this Oct 24, 2023
@maxrake maxrake requested a review from a team as a code owner October 24, 2023 17:40
@maxrake maxrake requested a review from cd-work October 24, 2023 17:40
Comment thread src/phylum/ci/ci_github.py
@cd-work cd-work removed their request for review November 28, 2023 20:36
This change makes it possible to use the GitHub Action (GHA) integration
for PRs that originate from forks, by enabling workflows to run on the
`pull_request_target` event.

This change comes with some heavy security considerations. In order for
the GHA to run, a workflow using it will need to checkout the PRs code.
This requires very careful attention so as not to allow so-called
"pwn requests":

* https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
* https://blog.gitguardian.com/github-actions-security-cheat-sheet/

To that end, the documentation updates that go with this change will be
explicit in their warnings about how to properly use the Phylum GHA in
the most secure way possible. This effectively means the workflow should
be limited to executing the bare minimum required for Phylum analysis.

Further, the Phylum analysis that does run will need to do so in a way
that does not allow for arbitrary code execution. Therefore, this change
also makes use of the Phylum CLI feature to disable lockfile generation
for untrusted contexts like the `pull_request_target` event trigger in
the GitHub Actions platform that this PR is looking to support.

Unfortunately, this approach does not allow for repositories with only
manifests (e.g., libraries). That is, manifests without a corresponding
lockfile. Those will fail to parse, with a new return code unique to
that situation. The only recourse, which is given in a log message and
will be spelled out more explicitly in the corresponding documentation
updates, is to reccomend adding a lockfile instead of or along with the
manifest.

Additional changes made include:

* Add predicate for if Phylum sandbox will work in current environment
* Create a `CLIExitCode` enumeration to track Phylum CLI exit codes
* Update `ReturnCode` enumeration for tracking `phylum-ci` return codes
  * Add descriptions
  * Make a distinction between _policy_ failures and general failures
  * Add new code for when no dependency files were provided or detected
  * Add new code for when a manifest is attempted to be parsed but
    lockfile generation has been disabled
* Ensure lockfiles provided as arguments are only filtered once
* Use `phylum project status` instead of `phylum project list` for
  setting the repository URL
* Update unit tests
* Format and refactor throughout
  * Show the final return code, _after_ any modifications

Closes #331

BREAKING CHANGE: Phylum CLI installs before v5.9.0-rc2 are no longer
supported. A version with support for disabling lockfile generation and
skipping sandbox usage is required.

BREAKING CHANGE: The `phylum-ci` return code for a policy violation that
results from a Phylum analysis has been changed from 1 to 2 in order to
make it distinct from the default failure code that is generated for all
raised `SystemExit` exceptions with a message instead of a code.
@maxrake maxrake requested a review from cd-work December 1, 2023 00:25
Comment thread src/phylum/ci/ci_base.py Outdated
@maxrake maxrake requested a review from cd-work December 1, 2023 22:16
@maxrake maxrake merged commit 6ed6c14 into main Dec 5, 2023
@maxrake maxrake deleted the gh_pr_target branch December 5, 2023 18:07
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.

Extend GHA integration to support pull_request_target events

2 participants