-
Notifications
You must be signed in to change notification settings - Fork 7
ImplicitPersistCredentials and GlobalPermissionsBlock #12
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
Changes from all commits
9034c5b
1de0f66
266636a
3ddc503
c95bc6b
6697923
c963421
473e0da
a95da7a
fbd1cfd
d5c0d7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -395,6 +395,121 @@ This rule only looks for user supplied branches being checked out for `pull_requ | |||||
| |-------------|------------------------|---------------------------------------------------------------| | ||||||
| | `risky_events` | ["pull_request_target", "workflow_dispatch"] | An array of Github events you consider risky. | | ||||||
|
|
||||||
| ### ImplicitPersistCredentials | ||||||
|
|
||||||
| By default, all versions of the official [`actions/checkout`](https://github.com/actions/checkout) have a default value of `true` for the `persist-credentials` setting. This means the temporary credentials generated to clone a repository's source code will be written to disk. This is necessary if you intend on using the `git` command line tool to further interact with this repository (e.g. check out a specific branch or push new commits to it). However, if you don't intend on doing this, this opens up an unnecessary risk. Untrusted or otherwise malicious code can find these temporary credentials and use them to access source code and other resources that would otherwise not have been exposed by your workflow. For example, if during a supply chain attack someone steals your job's environment variables, they may be able to use these credentials from their own system to access private repositories. | ||||||
|
|
||||||
| For example, take the following workflow: | ||||||
|
|
||||||
| ```yaml | ||||||
| name: Ruby CI (PR) | ||||||
|
|
||||||
| on: [pull_request] | ||||||
|
|
||||||
| jobs: | ||||||
| bundle-install: | ||||||
| name: Bundle install | ||||||
| runs-on: ubuntu-latest | ||||||
|
|
||||||
| steps: | ||||||
| - name: Checkout repository | ||||||
| uses: actions/checkout@v6 | ||||||
|
|
||||||
| - name: Set up Ruby | ||||||
| uses: ruby/setup-ruby@v1 | ||||||
|
|
||||||
| - name: Install dependencies | ||||||
| run: bundle install | ||||||
| ``` | ||||||
|
|
||||||
| Because `persist-credentials` is not specified, its value is `true` by default. That means the temporary Github credentials used to clone the repository will be either written to `.git/config` or to a temporary directory. Both of these locations will be accessible to subsequent steps, so if for example we were installing a malicious dependency with the `bundle install` command, it could find those credentials on disk and send them to someone via the network, letting them clone this repository and potentially access even other repositories. In this workflow, we aren't interacting with git beyond that checkout, so we don't need to persist the credential. We can fix this by setting `persist-credentials` to `false`: | ||||||
|
6f6d6172 marked this conversation as resolved.
|
||||||
|
|
||||||
| ```yaml | ||||||
| # ... | ||||||
| steps: | ||||||
| - name: Checkout repository | ||||||
| uses: actions/checkout@v6 | ||||||
| with: | ||||||
| persist-credentials: false | ||||||
| ``` | ||||||
|
|
||||||
| However, if you do need to interact with the repository via git, you should explicitly set `persist-credentials` to `true`. This way, you signal to your reviewers that your workflow interacts with the local checkout in a way that needs this credential. | ||||||
|
|
||||||
| Note, there is [a Github Issue tracking this](https://github.com/actions/checkout/issues/485) that has been open for a few years by now. | ||||||
|
6f6d6172 marked this conversation as resolved.
6f6d6172 marked this conversation as resolved.
|
||||||
|
|
||||||
| ### GlobalPermissionsBlock | ||||||
|
|
||||||
| [The permissions block](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack. | ||||||
|
||||||
| [The permissions block](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack. | |
| [The permissions block](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the suggested url literally redirects to my url. what r u doin
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| module Claws | ||
| module Rule | ||
| class GlobalPermissionsBlock < BaseRule | ||
| description <<~DESC | ||
| Permissions should be set at the job level, not globally at the workflow level. | ||
| Because jobs will often need varying permissions, it's better to specify a set | ||
| of permissions for each individual job, minimizing potential misuse from | ||
| untrusted code in a job with permissions it never needed in the first place. | ||
|
|
||
| This rule will flag workflows that have multiple jobs and a root level | ||
| permissions block. If there is a root level permissions block but just one job, | ||
| it will not be flagged. | ||
|
|
||
| For more information: | ||
| https://github.com/betterment/claws/blob/main/README.md#globalpermissionsblock | ||
| DESC | ||
|
|
||
| on_workflow :test_root_level_permissions | ||
|
|
||
| def test_root_level_permissions(workflow:, job:, step:) # rubocop:disable Lint/UnusedMethodArgument | ||
|
6f6d6172 marked this conversation as resolved.
|
||
| root_permission_block_line = workflow.keys.filter { |x| x == "permissions" }.first&.line | ||
| return if root_permission_block_line.nil? | ||
|
|
||
| job_count = workflow["jobs"]&.count || 0 | ||
| return if job_count < 2 | ||
|
|
||
| Violation.new( | ||
| line: root_permission_block_line, | ||
| description: | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| module Claws | ||
| module Rule | ||
| class ImplicitPersistCredentials < BaseRule | ||
| description <<~DESC | ||
| By default, actions/checkout will store generated credentials to disk so that | ||
| subsequent git operations will not require reauthentication. These credentials | ||
| will be available to subsequent steps and jobs. This may be undesirable and | ||
| potentially unsafe in scenarios where these credentials may be accessible to | ||
| untrusted code. In these cases, if these credentials are stolen they can be used | ||
| externally by an attacker to clone repositories that would otherwise have been | ||
| inaccessible. | ||
|
|
||
| If you know you will not need to access this repository for the rest of your | ||
| workflow, consider setting `persist-credentials` to false. Conversely, | ||
| explicitly set it to true if you know you will need these credentials. | ||
|
|
||
| For more information: | ||
| https://github.com/betterment/claws/blob/main/README.md#implicitpersistcredentials | ||
| DESC | ||
|
|
||
| on_step %( | ||
| $step.meta.action.name == "actions/checkout" && | ||
| !contains([true, false], dig($step, "with.persist-credentials")) | ||
| ), highlight: "uses" | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| RSpec.describe Claws::Rule::GlobalPermissionsBlock do | ||
| before do | ||
| load_detection | ||
| end | ||
|
|
||
| context "with default configuration" do | ||
| it "flags a workflow with a top level permissions block if there is more than one job" do | ||
| violations = analyze(<<~YAML) | ||
| name: publish docs | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pages: write | ||
| id-token: write | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
|
|
||
| deploy: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| environment: | ||
| name: github-pages | ||
| steps: | ||
| - name: Deploy to GitHub Pages | ||
| id: deployment | ||
| uses: actions/deploy-pages@v4 | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(1) | ||
| expect(violations[0].line).to eq(8) | ||
|
6f6d6172 marked this conversation as resolved.
|
||
| expect(violations[0].name).to eq("GlobalPermissionsBlock") | ||
| end | ||
|
|
||
| it "flags a workflow for global permissions even if some jobs specify their own" do | ||
| violations = analyze(<<~YAML) | ||
| name: publish docs | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| # default for all jobs | ||
| permissions: | ||
| contents: read | ||
| pages: write | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
|
|
||
| deploy: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| # override defaults | ||
| permissions: | ||
| contents: read | ||
| pages: write | ||
| id-token: write | ||
| environment: | ||
| name: github-pages | ||
| steps: | ||
| - name: Deploy to GitHub Pages | ||
| id: deployment | ||
| uses: actions/deploy-pages@v4 | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(1) | ||
| expect(violations[0].line).to eq(9) | ||
|
6f6d6172 marked this conversation as resolved.
|
||
| expect(violations[0].name).to eq("GlobalPermissionsBlock") | ||
| end | ||
|
|
||
| it "does not flag a workflow with a top level permissions block if there is just one job" do | ||
| violations = analyze(<<~YAML) | ||
| name: pretend to publish docs | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pages: write | ||
| id-token: write | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(0) | ||
| end | ||
|
|
||
| it "does not flag a workflow if there is no top level permissions block" do | ||
| violations = analyze(<<~YAML) | ||
| name: publish docs | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| jobs: | ||
| build: | ||
| permissions: | ||
| contents: read | ||
| pages: write | ||
| id-token: write | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
|
|
||
| deploy: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| environment: | ||
| name: github-pages | ||
| steps: | ||
| - name: Deploy to GitHub Pages | ||
| id: deployment | ||
| uses: actions/deploy-pages@v4 | ||
| YAML | ||
|
|
||
| expect(violations.count).to eq(0) | ||
| end | ||
| end | ||
| end | ||
|
6f6d6172 marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.