Skip to content
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

Add App Token to release workflow so CI jobs run #7324

Merged
merged 1 commit into from
May 17, 2023

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented May 16, 2023

Adding an App Token to the release workflow so CI jobs run.

This App Token is currently stored as an actions secret. Actions prevents forks from reading secrets, so we'd have to explicitly merge a PR in order to leak the secret. Actions also redacts secrets, so we should be mostly safe from typos.

If we later decide we want to protect it further, we have two options:

  1. Migrate it to a deployment secret for increased security. This way any workflow run accessing the secret requires explicit approval from the maintainers. Unfortunately, the workflow will just hang in a "waiting" state, but not easily notify the user that it needs approval. I prototyped the workflow, and it was pretty clunky--we had to trigger the workflow, remember to manually approve it, and only then the PR shows up... vs we'd like to use the PR showing up every week automatically as a reminder to cut a release. Vs here the PR just shows up and waits for us to merge it, CI will already have run.
  2. Migrate the entire workflow to a private repo. This way we reduce the odds of leaking the secret, but the workflows are a little complicated because a workflow in another repo can't trigger "on release". In other words, it'd be easy to migrate this "generate PR" workflow, but the secret we care more about is the RubyGems API key and that'd still be in the clear unless we migrated it. So we'd end up having to have a small trigger workflow in this repo that watches for releases, then triggers the workflow in the private repo.

Both of the above options are technically more secure, but it's primarily defense-in-depth as actions will already keep the current state secure unless we really screw up a PR review or fat-finger a major major typo. So after discussion we decided the inconvenience of the above two options aren't worth the increased defense-in-depth they provide.

We went with an application token rather than a PAT so that the token isn't tied to a particular developer's account. The app is scoped to only this specific repo, so if it's compromised it won't allow privilege escalation to other repos.

Related:

@jeffwidman jeffwidman requested a review from a team as a code owner May 16, 2023 00:05
@jeffwidman jeffwidman force-pushed the add-pat-to-release-workflow branch from ee1290a to 6f3f06f Compare May 16, 2023 00:10
@jeffwidman jeffwidman changed the title Add PAT to release workflow so CI jobs run Add App Token to release workflow so CI jobs run May 16, 2023
@jeffwidman jeffwidman force-pushed the add-pat-to-release-workflow branch 3 times, most recently from f57c270 to 3cc5d41 Compare May 16, 2023 22:02
@jeffwidman jeffwidman mentioned this pull request May 16, 2023
jeffwidman added a commit that referenced this pull request May 17, 2023
I kept having problems while testing #7324
until I realized that because I was doing my testing on a branch,
`actions/checkout` was starting from the branch and then opening a PR
against `main`, so it included not only the commit generated within this
workflow, but also whatever other commits I'd made during my testing.

For this particular workflow, we always want to start from `main` as
we're building a PR that should only contain the version bump commit.
@jeffwidman jeffwidman changed the base branch from main to always-checkout-from-main May 17, 2023 06:48
@jeffwidman jeffwidman force-pushed the add-pat-to-release-workflow branch from 241a09f to 40ee416 Compare May 17, 2023 06:51
@jeffwidman
Copy link
Member Author

I was able to trigger a successful run here: https://github.com/dependabot/dependabot-core/actions/runs/5000268607

Note that this required the change from #7335

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I supposed could change the "configure git user" step to run with this new actions user instead of

git config user.name "github-actions[bot]"
# Specifying the full email allows the avatar to show up: https://github.com/orgs/community/discussions/26560
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"

See for example this PR where the PR is created by one bot, but the commit is created by a different bot:

I don't think we could easily make it so the PR gets created by the GitHub Actions bot instead of our custom bot...

Base automatically changed from always-checkout-from-main to main May 17, 2023 09:37
jeffwidman added a commit that referenced this pull request May 17, 2023
I kept having problems while testing #7324
until I realized that because I was doing my testing on a branch,
`actions/checkout` was starting from the branch and then opening a PR
against `main`, so it included not only the commit generated within this
workflow, but also whatever other commits I'd made during my testing.

For this particular workflow, we always want to start from `main` as
we're building a PR that should only contain the version bump commit.
Adding an App Token to the release workflow so CI jobs run.

This App Token is currently stored as an actions secret. Actions prevents forks from reading secrets, so we'd have to explicitly merge a PR in order to leak the secret. Actions also redacts secrets, so we should be mostly safe from typos.

If we later decide we want to protect it further, we have two options:

1. Migrate it to a deployment secret for increased security. This way _any_ workflow run accessing the secret requires explicit approval from the maintainers. Unfortunately, the workflow will just hang in a "waiting" state, but not easily notify the user that it needs approval. I prototyped the workflow, and it was pretty clunky--we had to trigger the workflow, remember to manually approve it, and only then the PR shows up... vs we'd like to use the PR showing up every week automatically as a reminder to cut a release. Vs here the PR just shows up and waits for us to merge it, CI will already have run.
2. Migrate the entire workflow to a private repo. This way we reduce the odds of leaking the secret, but the workflows are a little complicated because a workflow in another repo can't trigger "on release". In other words, it'd be easy to migrate this "generate PR" workflow, but the secret we care more about is the RubyGems API key and that'd still be in the clear unless we migrated it. So we'd end up having to have a small trigger workflow in this repo that watches for releases, then triggers the workflow in the private repo.

Both of the above options are technically more secure, but it's primarily defense-in-depth as actions will already keep the current state secure unless we really screw up a PR review or fat-finger a major major typo. So after discussion we decided the inconvenience of the above two options aren't worth the increased defense-in-depth they provide.

We went with an application token rather than a PAT so that the token isn't tied to a particular developer's account. The app is scoped to only this specific repo, so if it's compromised it won't allow privilege escalation to other repos.
@jeffwidman jeffwidman force-pushed the add-pat-to-release-workflow branch from 40ee416 to 43bf29e Compare May 17, 2023 09:38
@jeffwidman jeffwidman enabled auto-merge (squash) May 17, 2023 09:39
@jeffwidman jeffwidman merged commit 41b319c into main May 17, 2023
@jeffwidman jeffwidman deleted the add-pat-to-release-workflow branch May 17, 2023 09:42
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…dabot#7335)

I kept having problems while testing dependabot#7324
until I realized that because I was doing my testing on a branch,
`actions/checkout` was starting from the branch and then opening a PR
against `main`, so it included not only the commit generated within this
workflow, but also whatever other commits I'd made during my testing.

For this particular workflow, we always want to start from `main` as
we're building a PR that should only contain the version bump commit.
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
Adding an App Token to the release workflow so CI jobs run.

This App Token is currently stored as an actions secret. Actions prevents forks from reading secrets, so we'd have to explicitly merge a PR in order to leak the secret. Actions also redacts secrets, so we should be mostly safe from typos.

If we later decide we want to protect it further, we have two options:

1. Migrate it to a deployment secret for increased security. This way _any_ workflow run accessing the secret requires explicit approval from the maintainers. Unfortunately, the workflow will just hang in a "waiting" state, but not easily notify the user that it needs approval. I prototyped the workflow, and it was pretty clunky--we had to trigger the workflow, remember to manually approve it, and only then the PR shows up... vs we'd like to use the PR showing up every week automatically as a reminder to cut a release. Vs here the PR just shows up and waits for us to merge it, CI will already have run.
2. Migrate the entire workflow to a private repo. This way we reduce the odds of leaking the secret, but the workflows are a little complicated because a workflow in another repo can't trigger "on release". In other words, it'd be easy to migrate this "generate PR" workflow, but the secret we care more about is the RubyGems API key and that'd still be in the clear unless we migrated it. So we'd end up having to have a small trigger workflow in this repo that watches for releases, then triggers the workflow in the private repo.

Both of the above options are technically more secure, but it's primarily defense-in-depth as actions will already keep the current state secure unless we really screw up a PR review or fat-finger a major major typo. So after discussion we decided the inconvenience of the above two options aren't worth the increased defense-in-depth they provide.

We went with an application token rather than a PAT so that the token isn't tied to a particular developer's account. The app is scoped to only this specific repo, so if it's compromised it won't allow privilege escalation to other repos.
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.

3 participants