dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.69k stars 1.02k forks source link

Dependabot triggered Actions cant access secrets or use a writable token #3253

Closed WtfJoke closed 3 years ago

WtfJoke commented 3 years ago

Moderator note: If you're here because your Dependabot triggered actions are broken, read our updated docs or jump to https://github.com/dependabot/dependabot-core/issues/3253#issuecomment-852541544 for a FAQ

Package manager/ecosystem npm

Manifest contents prior to update

Updated dependency

What you expected to see, versus what you actually saw Since ~08.03.21 our dependabot pull requests fail, because they cant access the npm private registry anymore. We figured out, the reason is because dependabot cant read secrets anymore (see https://github.com/github/docs/pull/4397/files).

When we rerun the pull requests they succeed as the used GITHUB_TOKEN has permission to read the secret.

Is there a solution or workaround in place?

Images of the diff or a link to the PR, issue or logs

https://github.com/github/docs/pull/4397/files

mdreizin commented 3 years ago

I can confirm that. Now Dependabot can't read secrets anymore at all.

mdreizin commented 3 years ago

Any secrets are not available for Dependabot.

mdreizin commented 3 years ago

Can't share links to private repos, but here is a couple of public ones:

https://github.com/mdreizin/chrome-bookmarks-alfred-workflow/pull/96 https://github.com/mdreizin/gatsby-plugin-robots-txt/pull/418

mdreizin commented 3 years ago

I contacted yesterday support team and they denied that issue and refused to fix.

Could you please fix it as soon as possible, because many users are affected including paid ones?

mercuriete commented 3 years ago

the team support answer me with this:

We recently made changes to dependabot which means they will receive a read-only GITHUB_TOKEN and will not have access to any secrets available in the repository.

https://github.blog/changelog/2021-02-19-github-actions-workflows-triggered-by-dependabot-prs-will-run-with-read-only-permissions/

So it is not a bug its a feature.

My use case is I am auto merging branches after a CI passes using this action: https://github.com/ahmadnassri/action-dependabot-auto-merge

and now my workflow is totally broken.

Please add automerge to dependabot.

Thanks.

mercuriete commented 3 years ago

I am trying to fix the problem using a workaround: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target

it is mention in that post but I think with pull_request_target the tokens are insecure to forks. So I am only using that workaround to private repositories.

derTobsch commented 3 years ago

I have the same issue see https://github.com/synyx/urlaubsverwaltung/pull/1882/checks?check_run_id=2075418072

mercuriete commented 3 years ago

yeah, I can confirm that moving from

on: pull_request to on: pull_request_target

fix the problem

but you need to read the docs first -> https://securitylab.github.com/research/github-actions-preventing-pwn-requests

the main problem as a CI developer is that if you make a change on the pipeline you can't see the change until is in the base branch which is difficult for CI developing.

I hope it helps someone.

EDIT: It only work with GITHUB_TOKEN if you need personal access token it won't work :(

mdreizin commented 3 years ago

Recommended solution by using pull_request_target only for GITHUB_TOKEN and the rest secrets will be empty:

https://github.com/mdreizin/gatsby-plugin-robots-txt/blob/master/.github/workflows/dev.yml#L6 https://github.com/mdreizin/gatsby-plugin-robots-txt/pull/400/checks?check_run_id=2077439179

derTobsch commented 3 years ago

see https://github.community/t/dependabot-prs-and-workflow-secrets/163269 and https://github.community/t/dependabot-doesnt-see-github-actions-secrets/167104

jasperroel commented 3 years ago

I'm running into the same issue. Steps taken so far (since https://github.blog/changelog/2021-02-19-github-actions-workflows-triggered-by-dependabot-prs-will-run-with-read-only-permissions/ mentions that switching to pull_request_target should grant access to repository secrets again).

Unfortunately, even these steps seem to only switch the GITHUB_TOKEN to a write, but I still do not see any of the other expected secrets.

1 Update pull_request to pull_request_target

Change

on:
  pull_request:
    branches:
      - develop

to

on:
  pull_request_target:
    branches:
      - develop

2 Limit to dependabot

Added if: github.actor == 'dependabot[bot]' so that the jobs with secrets only run for Dependabot

3 Modify the checkout to still grab the head checkout

Change:

      - name: Checkout
        uses: actions/checkout@v2.3.4

to

      - name: Checkout
        uses: actions/checkout@v2.3.4
        with:
          ref: ${{ github.event.pull_request.head.sha }}

As other mentioned that seems to switch the ${{ secrets.GITHUB_TOKEN }} from a read-only to a write token, but other secrets remain unaccessible.

For example:

      - name: Automerge Dependabot
        uses: actions/github-script@v3
        with:
          github-token: ${{ secrets.PAT_PUSH_TOKEN }}
          script: |
            const output = `@dependabot squash and merge`;

            github.issues.createComment({
              issue_number: context.issue.number,
              owner: context.repo.owner,
              repo: context.repo.repo,
              body: output
            })

fails with a

Error: Input required and not supplied: github-token
    at getInput (/home/runner/work/_actions/actions/github-script/v3/dist/index.js:1452:15)
Error: Unhandled error: Error: Input required and not supplied: github-token
mercuriete commented 3 years ago

people from this issue might want to subscribe to this issue: https://github.com/dependabot/dependabot-core/issues/2268

we all are facing the same issue.

BTW, I tested the alternative on: workflow_run: suggested here: https://securitylab.github.com/research/github-actions-preventing-pwn-requests

and again it work for github_token but not for secrets (at least for me) with that alternative the secrets are all empty.

mercuriete commented 3 years ago

I will try to move from dependabot to renovate to asset the feasibility of the migration. right now dependabot is broken for enterprise users that make use of GitHub packages to upload/download to/from npm private registries and a lot of enterprise use cases. (AWS deployments are broken as well when the job is triggered by dependabot)

renovate: https://www.whitesourcesoftware.com/free-developer-tools/renovate

mdreizin commented 3 years ago

@mercuriete The biggest issue with on: workflow_run is missing ability to fail the whole pipeline, because they run in different context.

For instance, you have ci workflow and cd one which depends on ci via on: workflow_run (which needs access to your secrets) you won't see if cd fails or not without visiting ie https://github.com/dependabot/dependabot-core/actions to see actual status.

mdreizin commented 3 years ago

@mercuriete I am also considering migrating to renovate bot.

The funny thing: all PRs created by dependabot on that repo also fail https://github.com/dependabot/dependabot-core/pulls?q=is%3Aopen+is%3Apr+label%3Adependencies :)

The support team still has not provided any ways how to get rid of that issue.

mdreizin commented 3 years ago

Maybe anybody from Dependabot team could provide a solution to fix missing secrets on PR created by dependabot?

mercuriete commented 3 years ago

hahaha it seems dependabot is eating its own dog food https://es.wikipedia.org/wiki/Dogfooding

https://github.com/dependabot/dependabot-core/pull/3258/checks?check_run_id=2083846045

I'm glad they are facing the same issue because they have to fix it by themselves.

mdreizin commented 3 years ago

@mercuriete I am sorry, but they failed now by other reasons.

I would happy to see how they would fix that:

https://github.com/dependabot/dependabot-core/blob/main/.github/workflows/ci.yml#L46

before:

https://github.com/dependabot/dependabot-core/pull/3160/checks?check_run_id=1924805335

after:

https://github.com/dependabot/dependabot-core/pull/3249/checks?check_run_id=2063635316

result:

secrets are empty

asciimike commented 3 years ago

Hey folks, Dependabot PM here.

First off, apologies for the quick change and continued brokenness 😞 .

What changed?

The specific change is twofold, in that during pull_request triggered workflows:

Additionally, a bug was introduced where pull_request_target also had these properties, which made the migration process worse. This was fixed at ~2300 UTC on March 11th, so pull_request_target should be working for those of you trying to use it.

Why was this change made?

This change was made in response to reported vulnerabilities in these types of workflows, and we felt that it critical to ensure our developers were protected before they were publicly disclosed.

Specifically, a malicious dependency could execute code to exfiltrate secrets or perform something other malicious action in the repo (deleting files, etc.).

By making secrets unavailable and tokens read only, this prevents a compromised dependency from being able to exfiltrate those secrets or perform any malicious write actions.

How do I fix a broken workflow?

As per the advice in https://securitylab.github.com/research/github-actions-preventing-pwn-requests, you can either use pull_request_target or create two workflows, one untrusted that generates whatever artifacts, and one trusted that does the push of the changes.

Taking a simple example:

### .github/workflows/dependabot_automerge.yml
### This workflow now has no secrets and a read-only token
name: Dependabot Workflow
on:
  pull_request

jobs:
  do-stuff:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - uses: actions/checkout

Either use pull_request_target:

### .github/workflows/dependabot_automerge.yml
### As of 2300 UTC on 11 March, this workflow has secrets and a read-write token
name: Dependabot Workflow
on:
  pull_request_target

jobs:
  do-stuff:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - uses: actions/checkout
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          github-token: ${{ secrets.GITHUB_TOKEN }}

Or make it two workflows:

### .github/workflows/dependabot_pr.yml
### This workflow doesn't have access to secrets and has a read-only token
name: Dependabot PR Check
on:
  pull_request

jobs:
  check-dependabot:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - run: echo "PR created by Dependabot"

Which triggers:

### .github/workflows/dependabot_automerge.yml
### This workflow has access to secrets and a read-write token
name: Dependabot Automerge
on:
  workflow_run:
    workflows: ["Dependabot PR Check"]
    types: 
      - completed

jobs:
  do-stuff:
    runs-on: ubuntu-latest
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    steps:
      - uses: actions/checkout
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          github-token: ${{ secrets.GITHUB_TOKEN }}

Again, I'm really sorry for the churn and brokenness here, but hopefully this should get folks moving in the right direction 🙇🏻

asciimike commented 3 years ago

Re: @mercuriete's

I will try to move from dependabot to renovate to asset the feasibility of the migration. right now dependabot is broken for enterprise users that make use of GitHub packages to upload/download to/from npm private registries and a lot of enterprise use cases. (AWS deployments are broken as well when the job is triggered by dependabot)

renovate: https://www.whitesourcesoftware.com/free-developer-tools/renovate

Private registry support is coming out shortly, so stay tuned and follow this issue and the changelog.

derTobsch commented 3 years ago

Hey folks, Dependabot PM here.

First off, apologies for the quick change and continued brokenness disappointed .

What changed?

The specific change is twofold, in that during pull_request triggered workflows:

* your `GITHUB_TOKEN` is read-only

* `secrets` can't be accessed

Additionally, a bug was introduced where pull_request_target also had these properties, which made the migration process worse. This was fixed at ~2300 UTC on March 11th, so pull_request_target should be working for those of you trying to use it.

Why was this change made?

This change was made in response to reported vulnerabilities in these types of workflows, and we felt that it critical to ensure our developers were protected before they were publicly disclosed.

Specifically, a malicious dependency could execute code to exfiltrate secrets or perform something other malicious action in the repo (deleting files, etc.).

By making secrets unavailable and tokens read only, this prevents a compromised dependency from being able to exfiltrate those secrets or perform any malicious write actions.

How do I fix a broken workflow?

As per the advice in https://securitylab.github.com/research/github-actions-preventing-pwn-requests, you can either use pull_request_target or create two workflows, one untrusted that generates whatever artifacts, and one trusted that does the push of the changes.

Taking a simple example:

### .github/workflows/dependabot_automerge.yml
### This workflow now has no secrets and a read-only token
name: Dependabot Automerge
on:
  pull_request

jobs:
  automerge:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - uses: ...

Either use pull_request_target:

### .github/workflows/dependabot_automerge.yml
### As of 2300 UTC on 11 March, this workflow has secrets and a read-write token
name: Dependabot Automerge
on:
  pull_request_target

jobs:
  automerge:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - uses: ...
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}

Or make it two workflows:

### .github/workflows/dependabot_pr.yml
### This workflow doesn't have access to secrets and has a read-only token
name: Dependabot PR Check
on:
  pull_request

jobs:
  check_dependabot:
    runs-on: ubuntu-latest
    if: ${{ github.actor == 'dependabot[bot]' }}
    steps:
      - run: echo "PR created by Dependabot"

Which triggers:

### .github/workflows/dependabot_automerge.yml
### This workflow has access to secrets and a read-write token
name: Dependabot Automerge
on:
  workflow_run:
    workflows: ["Dependabot PR Check"]
    types: 
      - completed

jobs:
  automerge:
    runs-on: ubuntu-latest
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    steps:
      - uses: ...

Again, I'm really sorry for the churn and brokenness here, but hopefully this should get folks moving in the right direction 🙇🏻

Hey @asciimike,

I already tried that but without success. The workflows that that get triggered via workflow_run has no access to the secrets see https://github.com/synyx/urlaubsverwaltung/runs/2093235834?check_suite_focus=true - are my workflows and trigger correct?

What I did with a little more details:

name: Urlaubsverwaltung CI

on:
  schedule:
    - cron: "2 4 * * *"
  push:
    branches:
      - master
      - v3.x
  pull_request:
  workflow_dispatch:

jobs:
  build:
    name: build and analyse
    runs-on: ubuntu-20.04
    steps:
      - name: Checkout repository
        uses: actions/checkout@v2
        ...
      - name: Build
        run: ./mvnw --batch-mode -Pcoverage clean verify
...

and this triggeres the workflow https://github.com/synyx/urlaubsverwaltung/blob/master/.github/workflows/update-assets-manifest.yml e.g. with

name: Update assets-manifest

on:
  workflow_run:
    workflows: [ "Urlaubsverwaltung CI" ]
    branches: [ "dependabot/npm_and_yarn/**" ]
    types:
      - completed
  workflow_dispatch:

jobs:
  build:
    runs-on: ubuntu-20.04
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    steps:
      - name: Checkout repository
        uses: actions/checkout@v2
        with:
          persist-credentials: false
     ...
      - name: Assets-Manifest push
        uses: ad-m/github-push-action@v0.6.0
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          branch: ${{ github.ref }}

but now the github token does not have the permission to push to this branch.

mercuriete commented 3 years ago

same here, I tried yesterday both cases and even though github_token was present, secrets are not available making all normal use cases unusable.

use cases broken: 1- yarn install where you are pulling packages from private registry (npm) 2- docker login/docker pull from images from private registry 3- send reports to sonarqube.io

everything that involves a personal access token.

Please give us a working example of private registries making use a PAT.

thanks

asciimike commented 3 years ago

@derTobsch: I just set up a workflow run triggered by a Dependabot PR and I'm seeing the same behavior 😞

It seems like the answer is "workflow_run inherits the secrets and token from the previous PR" (the docs seem to indicate this, though the actions team indicated that it should have the secrets and a write token). The answer is to use pull_request_target (which I have confirmed has a read-write token), though at that point, there's no reason to use the second workflow (and indeed, my testing shows that the workflow_run runs as read-only without secrets, so you can't use it!).

@mercuriete: 1 and 2 will be solved more elegantly via https://github.com/github/roadmap/issues/67, which is imminent. 3 will have to use pull_request_target or wait for the change as above.

asciimike commented 3 years ago

I have confirmed that workflow_run now works as expected, with the workflow now able to access secrets and a read-write GITHUB_TOKEN. I believe this is now back to a place where folks should be able to fix themselves in a fairly straightforward way.

essenmitsosse commented 3 years ago

Secrets in Dependabot PRs are considered a security risk, since an updated dependency could introduce malicious code, that could steal secrets — as far as I understand the basis for this issue.

Switching to pull_request_target or workflow_run allows the PR to read those secrets, but doesn't this just open up the security issue again?

As far as I understand, all this would mean, that there is no secure way to use Dependabot on a repository that for example needs an NPM token to install a private dependency. But technical that would also mean that on such a repo, I would have to manually check every dependency update for malicious code — which seems completely unfeasable.

Am I missing something here or is this the situation we're in?

ruudk commented 3 years ago

For anyone else that is having issues with this mess: I forgot to add ref: ${{ github.event.pull_request.head.sha }} on actions/checkout when I replaced on: pull_request with on: pull_request_target. If you don't add this you are basically checking out the main branch on your PR builds 🤯

derTobsch commented 3 years ago

If I use workflow_run now for my sonarcloud or other workflows that are considered insecure now. How can I achieve that these workflows are able to add comments to the original PR? Or how can they be published in the 'All checks have passed' section in a PR?

sonarsource community issue: https://community.sonarsource.com/t/github-action-how-does-the-sonar-maven-plugin-know-on-which/40101

ejntaylor commented 3 years ago

@derTobsch Try: http://github.com/haya14busa/action-workflow_run-status

I was unable to get this to work with matrix but otherwise think it will help you.

derTobsch commented 3 years ago

@derTobsch Try: http://github.com/haya14busa/action-workflow_run-status

I was unable to get this to work with matrix but otherwise think it will help you.

k, this seems the way to go. I already added this, but hoped there is a default github action way to do this.

boredland commented 3 years ago

This is a total nightmare. I hate it.

derTobsch commented 3 years ago

Is there a way to prevent a step to run if it was triggered by a pull_request event? Then for now I would just deactivate the steps with secrets.

asciimike commented 3 years ago

@mercuriete (and anyone else using secrets to access registries) we just launched support for private registries in beta, so you should be unblocked for doing that in more well supported way.

@essenmitsosse re:

Secrets in Dependabot PRs are considered a security risk, since an updated dependency could introduce malicious code, that could steal secrets — as far as I understand the basis for this issue.

Switching to pull_request_target or workflow_run allows the PR to read those secrets, but doesn't this just open up the security issue again?

As far as I understand, all this would mean, that there is no secure way to use Dependabot on a repository that for example needs an NPM token to install a private dependency.

For this use case in particular, see the above: we just added support for private registries, so the majority of folks shouldn't need to do this in Actions anymore.

Short answer to other cases: because Dependabot effectively runs untrusted code during the update process, yes.

I think the first thing we can do to make this better is fetch secrets from the newly released Dependabot secret store (currently used for private registries, above), which would limit the blast radius to Dependabot's secrets (likely just the npm token, as opposed to your AWS credentials or whatever deployment secrets used).

There are probably some other things we could do to Actions (e.g. disallow external networking, or at least when the action runs with a new network connection, you'd have to whitelist it) that would prevent a certain class of exfiltration/code execution risks, but again, it's not guaranteed.

But technical that would also mean that on such a repo, I would have to manually check every dependency update for malicious code — which seems completely unfeasable.

Most larger/more security conscious organizations mirror packages from public registries into internal registries after they undergo a security review, if they're allowed at all, specifically to solve for this.

Dependabot can't guarantee that the new versions are free of bugs or malware. There are related tools (e.g. GitHub's code scanning, JFrog Xray, etc.) that provide insight into what's actually running, and over time I'm sure that we can get better about providing signals of correctness or trustworthyness, but we're not there yet :/

@derTobsch, re:

Is there a way to prevent a step to run if it was triggered by a pull_request event? Then for now I would just deactivate the steps with secrets.

EDIT: It's not documented but: if: ${{ github.event.workflow_run.event == 'pull_request' }} should work.

WtfJoke commented 3 years ago

I think the first thing we can do to make this better is fetch secrets from the newly released Dependabot secret store (currently used for private registries, above), which would limit the blast radius to Dependabot's secrets (likely just the npm token, as opposed to your AWS credentials or whatever deployment secrets used).

That would be very helpful for us. 👍

We use it for read access to a private npm registry (artifactory).

asciimike commented 3 years ago

@WtfJoke I'd strongly recommend using https://docs.github.com/en/github/administering-a-repository/configuration-options-for-dependency-updates#npm-registry in your flow, if possible. Should be a streamlined experience for npm from a private registry like artifactory.

WtfJoke commented 3 years ago

@asciimike That looks promising. Thanks for the hint!

~Just skimmed through, it looks like its possible to provide multiple urls, is that correct?~ EDIT: Looks like :) Does it also support scopes, because we use multiple scoped registries

essenmitsosse commented 3 years ago

@asciimike Thanks for the reply and the clearification. I see that the whole security issue in general is a huge topic., that can't be easily fixed. Just adding the secret to the dependabot secrets doesn't result in the old workflow working again — so there seems to be more overhead to configure. So right now, I'm just switchting to pull_request_target to save myself the headache.

But this brings up the question of what making dependabot harder to use really achives here. Basically I have a readonly NPM Token stored in my secrets and all I want to be able to say is "That is save for dependabot to use and I'm aware of the implications".

Also in most cases, a dependabot PR will merged if it passes, thus ending up one way or the other in the build pipeline that has all the secrets. For some repos that build pipeline will kick in immediately after merging. Plus: you are already controlling which secrets to pass to which build step, so if your deploy credentials are passed to your install step, that's a configuration issue.

So this introduces a lot of complexity to a build pipeline, while not really adressing the underlying issue. It merely makes people aware of the situation, that they shouldn't leak their deploy credentials to an untrusted process.

So the question for me is: does this really solve any problem or does it just make it a bit harder to foot gun yourself — without any really protection — while complicating things for the majority of users?

Or is there something I'm missing here?

mercuriete commented 3 years ago

IMHO the security issue should be addressed by the owner of the repository. yarn install is insecure by design even on your local workstation. to mitigate that, it is recommended to not to do post-install scripts like this:

      - name: Install dependencies skip post-install
        # Skip post-install scripts here, as a malicious
        # script could steal NODE_AUTH_TOKEN.
        run: yarn install --ignore-scripts --frozen-lockfile
        env:
          NODE_AUTH_TOKEN: ${{ secrets.GPR_READ_TOKEN }}
      - name: Install dependencies with post-install
        # `yarn install --force` will run all those post-install scripts for us.
        run: yarn install --force --frozen-lockfile
        env:
          NODE_AUTH_TOKEN: "FAKE_TOKEN"

That pipeline is broken right now because secrets are not accessible when dependabot triggers a pipeline.

Can somebody confirm that pulll_request_target fix the problem? because I tried a few days ago and I wasn't working.

For me this change is disruptive and the security concern should be addressed by the security team inside our side.

Thanks for your support.

PS: I am talking on private repositories where nobody can make PR outside our control. PS2: IMHO the previous behaviour should be restored on private repos with a configuration checkbox. PS3: Funny comic about this issue: https://xkcd.com/1172/

kevinlul commented 3 years ago

I'm still confused, are we meant to have main workflows trigger off of workflow_run instead? This week our team has had a lot of Dependabot stuff break. The first was the change from pull_request to pull_request_target for our own labelling workflow, since Dependabot PRs are treated as external. The second I'm trying to figure out what to do here, but the behaviour I'm observing is that workflows on the main branch triggered due to a commit via @dependabot squash and merge do not have access to secrets.

asciimike commented 3 years ago

@WtfJoke re:

Does it also support scopes, because we use multiple scoped registries

Yes, if you've got scopes set up on the registry, it should support scopes. Depending on how you've got registries set up you may have to have multiple updaters segmented using allow or ignore, but it should work.

@kevinlul re:

The second I'm trying to figure out what to do here, but the behaviour I'm observing is that workflows on the main branch triggered due to a commit via @dependabot squash and merge do not have access to secrets.

Is this a push event? If so, I believe that has the same restrictions as pull_request (and pull_request_review, and pull_request_review_comment). I think the answer is that you'll have to do the trampolining through workflow_run like I did above :/

@mercuriete re:

Can somebody confirm that pulll_request_target fix the problem? because I tried a few days ago and I wasn't working.

Yes. It was broken initially, but has since been fixed. I've confirmed that it gets secrets and a read-write token as the blog post specifies.

dentarg commented 3 years ago

So the question for me is: does this really solve any problem or does it just make it a bit harder to foot gun yourself — without any really protection — while complicating things for the majority of users?

@essenmitsosse I think it solves the problem that GitHub had: their Dependabot pull requests ran with access to your secrets. You enabled @dependabot and got owned by an unmerged PR, not a great experience for you and not a great story for GitHub.

If you merge a PR with a compromised dependancy, that's on you, not GitHub.

asciimike commented 3 years ago

@mercuriete re:

IMHO the security issue should be addressed by the owner of the repository. yarn install is insecure by design even on your local workstation. to mitigate that, it is recommended to not to do post-install scripts like this:

Dependabot runs with the equivalent of --ignore-scripts on by default (and indeed there's no way to turn it off to allow scripts to run).

Unfortunately this behavior is common for many package managers (bundler,pip, mix...). Most of the time it's because the package manager DSL is just another file in the programming language, so people can do weird and wonderful things (the XKCD applies equally to this). For these three languages, we provide an insecure-external-code-execution to allow folks to break out and do the potentially unsafe thing if required.

Similar to the --ignore-scripts flag, pip has provided a nice workaround for this in the form of the --only-binary :all: flag, which disallows source packages. We're obviously in favor of more package managers doing this.

@essenmitsosse re:

So this introduces a lot of complexity to a build pipeline, while not really adressing the underlying issue. It merely makes people aware of the situation, that they shouldn't leak their deploy credentials to an untrusted process.

So the question for me is: does this really solve any problem or does it just make it a bit harder to foot gun yourself — without any really protection — while complicating things for the majority of users?

In the views of the security researcher(s) who reported the vulnerability, the team at GitHub who reviewed the report and agreed with its assessment, and the Actions team who made the change, the belief is that this change makes developers as a whole safer.

When building developer tools I believe in two principles: 1) the defaults should be safe, 2) there should be escape hatches that let developers who assume the risks perform unsafe actions. In general, unsafe tools fail as developers get burnt by them and stop using them (or build safer default on top of them), while tools that are too safe never gain widespread adoption because they are too limited (see the referenced XKCD--developers like to do weird things).

Finding that balance is difficult and ever changing. This change lands on the side of "making the defaults safer" at the expense of "requiring more developers to consciously assume risk and use the provided escape hatch." Previously, everyone was unconsciously assuming that same risk!

We strive to update our products with safer defaults and without loss of functionality or decreased usability, even in the face of a changing vulnerability landscape. Switching where secrets are fetched from or allowing developers to scope their token permissions on a per-workflow basis will give developers more freedom to do things that were previously potentially unsafe, and make them safer, potentially eventually allowing us to relax restrictions.

I'm working on getting the docs (and that changelog post) updated to contain all of this information so we have a single source. In the future, we clearly have work to do to communicate these changes better, and I've already brought up better ways of notifying developers of impending breakage, especially on the short timelines often required by a disclosure schedule.

asciimike commented 3 years ago

@mercuriete re:

IMHO the previous behaviour should be restored on private repos with a configuration checkbox.

You specifically mean "if dependabot is pulling from a private repo/registry, we should allow pull_request etc. runs to get secrets and have a read-write token"? The thought here being that "if I own the registry, I trust that no malicious code has gotten in"?

mercuriete commented 3 years ago

@mercuriete re:

IMHO the previous behaviour should be restored on private repos with a configuration checkbox.

You specifically mean "if dependabot is pulling from a private repo/registry, we should allow pull_request etc. runs to get secrets and have a read-write token"? The thought here being that "if I own the registry, I trust that no malicious code has gotten in"?

Yes, I meant that. Maybe I am missing something. What I meant was... I am the administrator of the organization so nobody can create a malicious PR. So for private repositories where all users are members and have user access control managed by the administrator I can control who is able to create a PR/fork.

Thanks for your clarifications! I will try with pull_request_target today. thanks!

derTobsch commented 3 years ago

@asciimike maybe you can help me out. Are there different event_types or information in the github context for 'forked pull requests 'and 'normal pull requests'? I cannot find anything about that just the 'pull_request' event_name.

cahaseler commented 3 years ago

I'm still having trouble figuring out what the solution here is. Our use case is the following:

On Pull Request in private controlled repo that only trusted people can push to, trigger a GA workflow to run a series of cypress tests to confirm that the updated package doesn't break core functionality. This requires installing the app and running a node server using a bunch of secrets stored in Github Secrets. Without access to all those secrets, the app doesn't start.

Now that dependabot can't read those secrets, it's not clear what we're supposed to do. Using Pull_Request_Target doesn't help because that runs on the base commit that's already been tested, not the new code.

From what I'm reading here, you've essentially removed any useful functionality dependabot might have for my team. Is there another product that does something similar and still works? As a paying Github enterprise customer, is there a way to get back the feature I paid for and worked fine last week?

covertbert commented 3 years ago

Just to clarify @asciimike, is the plan to also make the new "Dependabot secrets" available to Dependabot-created, pull_request-triggered workflow runs? Or is the long term solution to implement one of the workarounds mentioned above?

covertbert commented 3 years ago

@cahaseler I think you want to read this comment for the problem you're having.

khitrenovich commented 3 years ago

@cahaseler I think you want to read this comment for the problem you're having.

I don't think the combination of pull_request_target and ref: ${{ github.event.pull_request.head.sha }} fully solves the issue - not for us, at least. We do have several checks, some are pretty complicated. I want those checks to run on all the PRs (whether they come from dependabot or not), some of them will run on master too and some may be triggered manually. Having to replicate those checks specifically for dependabot PRs does not seem manageable...

feelepxyz commented 3 years ago

👋 We've set up a sample workflow that runs on workflow_run and has write access to dependabot branches here: https://github.com/dependabot/dependabot-actions-workflow/tree/main/.github/workflows

asciimike commented 3 years ago

@mercuriete and @cahaseler:

I am the administrator of the organization so nobody can create a malicious PR. So for private repositories where all users are members and have user access control managed by the administrator I can control who is able to create a PR/fork.

On Pull Request in private controlled repo that only trusted people can push to, trigger a GA workflow to run a series of cypress tests to confirm that the updated package doesn't break core functionality. This requires installing the app and running a node server using a bunch of secrets stored in Github Secrets. Without access to all those secrets, the app doesn't start.

The issue isn't that "a malicious PR is created by someone in your org", the issue is that Dependabot creates a PR that pulls in a new version of a dependency, and that dependency may contain malicious code. If everyone on this thread is operating in an environment where they either use no external dependencies, or they have a world class security organization that vets every single update prior to it being available in a private registry, it would be "safe" to operate without this check, but practically, the majority of software organizations don't operate this way.

To make it even more concrete, something like the eslint credential theft could run in this environment and steal your credentials. We want to ensure that developers are safe from this by default, and made aware of it for those who choose to do it, hence the big scary-looking red box in the docs and the link to the blog post on how to protect yourself.

@derTobsch:

maybe you can help me out. Are there different event_types or information in the github context for 'forked pull requests 'and 'normal pull requests'? I cannot find anything about that just the 'pull_request' event_name.

You're looking for these docs: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories

@covertbert:

Just to clarify @asciimike, is the plan to also make the new "Dependabot secrets" available to Dependabot-created, pull_request-triggered workflow runs? Or is the long term solution to implement one of the workarounds mentioned above?

We just launched them publicly on Monday, so we don't have immediate plans to fork over (especially since we've got some learning to do on how this change was made), but it's something we've discussed internally.

@khitrenovich:

I don't think the combination of pull_request_target and ref: ${{ github.event.pull_request.head.sha }} fully solves the issue - not for us, at least.

What are you observing as the difference in behavior between:

on:
  pull_request

jobs:
  do-some-work:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2

which is presumably what existed before, and

on:
  pull_request_target

jobs:
  do-some-work:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
        with:
          ref: ${{ github.event.pull_request.head.sha }}