Add documentation regarding security practices for github actions #2528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
itsyme
requested changes
Apr 24, 2024
| **Environment variables**, like {% raw %} `${{ secrets.GITHUB_TOKEN }}` {% endraw %}, should be limited by scope, and should be declared at the step level when possible. | ||
|
|
||
| ### Pull_request_target event | ||
| Maximum access for pull requests from public forked repositories is read access due to security reasons. Since MarkBind uses the forking workflow, this leads to limitations in the CI/CD pipeline. |
There was a problem hiding this comment.
Suggested change
| Maximum access for pull requests from public forked repositories is read access due to security reasons. Since MarkBind uses the forking workflow, this leads to limitations in the CI/CD pipeline. | |
| Read access is the most permissive access to be given for pull requests from public forked repositories to maintain security. Since MarkBind uses the forking workflow, this leads to limitations in the CI/CD pipeline. |
There was a problem hiding this comment.
Do we need the line about limitations in the CI/CD pipeline?
If yes, I'd rather put it in an info box and add more details about what the limitations are
|
|
||
| This method triggers workflows based on the latest commit of the pull request's base branch. Even if workflow files are modified or deleted on feature branches, workflows on the default branch aren't affected so you can prevent malicious code from being executed in <tooltip content="Continuous Integration">CI</tooltip> without code review. | ||
|
|
||
| Another solution that allows `pull_request_target` to work securely with `actions/checkout` used on the pull request branch, is to add an additional step of running workflow only on approval by trusted users, such that the trusted user has to check the changes in the code from the pull request to ensure there is no malicious code before running the workflow. |
There was a problem hiding this comment.
Perhaps paraphrasing this sentence into a few shorter sentences could make it more readable
| **Environment variables**, like {% raw %} `${{ secrets.GITHUB_TOKEN }}` {% endraw %}, should be limited by scope, and should be declared at the step level when possible. | ||
|
|
||
| ### Pull_request_target event | ||
| Maximum access for pull requests from public forked repositories is read access due to security reasons. Since MarkBind uses the forking workflow, this leads to limitations in the CI/CD pipeline. |
There was a problem hiding this comment.
Do we need the line about limitations in the CI/CD pipeline?
If yes, I'd rather put it in an info box and add more details about what the limitations are
KevinEyo1
force-pushed
the
security-docs
branch
from
5f1c200
to
da7d130
Compare
itsyme
reviewed
Apr 24, 2024
| Every credential used in the workflow should have the minimum required permissions to execute the job. Without explicit specification of permissions, the default permissions are used, which are usually more permissive than necessary. | ||
|
|
||
| Use the ‘permissions’ key to make sure the `GITHUB_TOKEN` is configured with the least privileges for each job. | ||
|
|
There was a problem hiding this comment.
Can add a line saying like example or somethin?
| This method triggers workflows based on the latest commit of the pull request's base branch. Workflows on the base branch aren't affected by changes on feature branches, avoiding execution of malicious code in <tooltip content="Continuous Integration">CI</tooltip>. | ||
|
|
||
| <box type="warning" seamless> | ||
| {% endraw %} |
There was a problem hiding this comment.
Why is the endraw here? Shouldn;t it be before the text?
|
|
||
| This event should not be used with `actions/checkout` as it can give write permission and secrets access to untrusted code from the forked code. Any building step, script execution, or even action call could be used to compromise the entire repository. | ||
|
|
||
| This can be fixed by adding code to ensure that the code being checked out belongs to the base branch. |
There was a problem hiding this comment.
Sorry what do you mean by 'code being checked out'?
|
|
||
| </box> | ||
|
|
||
| Another solution that allows `pull_request_target` to work securely with `actions/checkout` used on the pull request branch, is to implement running workflow only on approval by trusted users, such that the workflow is only run after checking for malicious code on the feature branch. |
There was a problem hiding this comment.
Suggested change
| Another solution that allows `pull_request_target` to work securely with `actions/checkout` used on the pull request branch, is to implement running workflow only on approval by trusted users, such that the workflow is only run after checking for malicious code on the feature branch. | |
| Another solution that allows `pull_request_target` to work securely with `actions/checkout` on the pull request branch, is to implement running workflow only on approval by trusted users. This means that the workflow is only run after a user has checked for malicious code on the feature branch manually and approves the run. |
|
|
||
| <box type="warning" seamless> | ||
|
|
||
| This method could be limiting since the code checked out is not up to date for the pull request. This means that the code checked out is not the code that is being tested. This could lead to false positives or false negatives in the testing process. |
There was a problem hiding this comment.
Can you rephrase the code checked out here also!
|
|
||
| **Environment variables**, like {% raw %} `${{ secrets.GITHUB_TOKEN }}` {% endraw %}, should be limited by scope, and should be declared at the step level when possible. | ||
|
|
||
| ### Pull_request_target event |
There was a problem hiding this comment.
Suggested change
| ### Pull_request_target event | |
| ### Security precautions when using `pull_request_target` event |
yucheng11122017
approved these changes
Apr 25, 2024
itsyme
approved these changes
Apr 25, 2024
|
@KevinEyo1 Each PR must have a SEMVER impact label, please remember to label the PR properly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
Overview of changes:
Resolves #2515
#2488
Anything you'd like to highlight/discuss:
Only included the research of practices currently being used, so things like version tagging is not documented.
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
When writing GitHub action workflows, developers might miss
out on security conventions.
Let's document current security practices in our developer guide
so that future developers can follow these conventions.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):