Run the backport bot in CI - #3416
Conversation
91df775 to
db872ad
Compare
f1eca9f to
caf1347
Compare
db872ad to
5230da1
Compare
caf1347 to
e9becee
Compare
5230da1 to
3dc664f
Compare
e9becee to
7c1ee95
Compare
3dc664f to
b0af9b0
Compare
7c1ee95 to
c9dc173
Compare
b0af9b0 to
46a366f
Compare
c9dc173 to
af8be4e
Compare
46a366f to
58aa3b4
Compare
af8be4e to
db6b557
Compare
a98ee5d to
8f25b64
Compare
db6b557 to
dc7c482
Compare
8f25b64 to
d678198
Compare
dc7c482 to
4396667
Compare
| # The backport bot gets its own OIDC role, pinned to its own workflow file the | ||
| # same way autofix's role is above. Without a role of its own, any workflow in | ||
| # the repository could assume it and reach the shared Bedrock role below | ||
| backport_oidc_role_name = "AwsLcGitHubActionsBackportOidcRole" | ||
| self.backport_oidc_role = iam.Role(self, id=backport_oidc_role_name, role_name=backport_oidc_role_name, |
There was a problem hiding this comment.
Is it possible to create a shared role with autofix to use bedrock for both?
d678198 to
b0c0197
Compare
A pull request labelled needs-backport now opens its own backport pull requests when it merges, so the seven branch case needs nobody at a keyboard Two jobs, and the split is the point. analyze is the only job that reaches the model and it has contents: read. publish is the only job that can write and it never reaches the model. Repository content, which the model reads, is therefore never handled by a job holding a token that could change the repository. The verdict moves between them as an artifact The workflow assumes its own AwsLcGitHubActionsBackportOidcRole, pinned by job_workflow_ref to this one file, and that role is what chains into the shared AwsLcGitHubActionsBedrockRole that aws#3376 renamed for exactly this. The bot cannot borrow autofix's role and general CI cannot borrow either The pinning cuts both ways, which is the half of autofix's pattern that is easy to miss. The general AwsLcGitHubActionsOidcRole now excludes this workflow file too, so the bot cannot skip its own role and assume the general one to reach the rest of CI In CI the checkout already is aws/aws-lc, so the branches have nowhere else to go. --push-to-aws-lc says so out loud, is refused anywhere else, and push_branch will only ever push a branch named backport-, so the escape has two independent limits rather than one The checkout is the merge commit, never the pull request head, so no untrusted code runs
b0c0197 to
7ea55a0
Compare
Co-authored-by: Doug Chapman <dougch@amazon.com>
The publish job was already moved to v7. v7 is what the rest of the repo uses, 157 workflows to 5, so the two jobs matching each other is the point
| needs: analyze | ||
| permissions: | ||
| contents: write # to push the backport branches | ||
| pull-requests: write # to open and comment on the backport pull requests |
There was a problem hiding this comment.
For this to work, we need to turn on Allow GitHub Actions to create and approve pull requests under Settings → Actions → General -> Workflow Permissions -> Allow GitHub Actions to create and approve pull requests. is this on?
This poses a security risk due to a GHA having perms to open a PR. However, since this is for backports, which are already established fixes, it might not be a big risk, but for autofix, had a discussion with @justsmth and he recommended against not turning this on as it could open a can of worms. If this was discussed beforehand then ignore.
| oidcRole: AwsLcGitHubActionsBackportOidcRole | ||
| roleName: AwsLcGitHubActionsBedrockRole | ||
| - name: Install the AI client | ||
| run: pip3 install --user anthropic boto3 |
There was a problem hiding this comment.
Pin to a major version release so any vulnerabilities can be tracked
Both run blocks interpolated ${{ }} straight into the shell, which is the
pattern GitHub's script injection guidance warns about: the expression is
expanded before the shell sees it, so the value becomes part of the command
rather than an argument to it. The two values here are a SHA and a number, so
there is nothing to inject today, but the shape is the finding
FIX_SHA and PR_NUMBER now arrive as environment variables and the scripts
quote them, matching what the rest of the repository does
Raised by AppSec against the backport-stack/ci branch, V2265233159
Issues:
Addresses
P425131803Description of changes:
analyze,applyandpublishstill need someone to run them after a fix merges.This pull request adds
.github/workflows/backport-bot.yml, which runs all three automatically when a pull request labelledneeds-backportmerges. The decision to backport stays with the reviewers who apply the label, not with the tool.It also adds the bot's own OIDC role to the CDK stack, chaining into the shared
AwsLcGitHubActionsBedrockRolethat #3376 renamed for exactly this.Stacked on #3415. The base here is
backport-stack/publish, a scaffolding branch holding the commits below it, so this diff is only the 7 files this change touches. I will retarget it tomainas the stack lands. Please don't merge it into the scaffolding branch.Call-outs:
Two jobs, and the split is the point.
analyzeis the only job that reaches the model and it hascontents: read.publishis the only job that can write and it never reaches the model. So repository content, which the model reads, is never handled by a job holding a token that could change the repository. The verdict moves between them as an artifact.The checkout is the merge commit, never the pull request head, so no untrusted code runs.
The role is pinned by
job_workflow_refto this one workflow file. The bot can't borrow autofix's role and general CI can't borrow either. Renaming the file breaks the trust policy until the stack is redeployed.The pinning cuts both ways, following what autofix already does: the general
AwsLcGitHubActionsOidcRolenow excludes this workflow file too, so the bot can't skip its own role and assume the general one to reach the rest of CI. Synthesized the stack to confirm both exclusions land in the trust policy and that the Bedrock role ends up with one statement per OIDC role.In CI the checkout already is
aws/aws-lc, so the branches have nowhere else to go.--push-to-aws-lcsays so out loud, is refused anywhere else, andpush_branchonly ever pushes a branch namedbackport-, so the escape has two independent limits.Two things this needs that merging it doesn't provide, and I can't do either: the CDK stack has to be deployed before the workflow can assume the new role, and a pull request opened with
GITHUB_TOKENstarts no workflow, so the backport pull requests would arrive with no CI of their own. Getting them tested needs a token that isn'tGITHUB_TOKEN.Uses Share Bedrock model settings between autofix and util/backport #3395 (merged) for
.github/workflows/ai-config.json, which the workflow reads for the region.Testing:
Unit tests - 6 new, 173 total:
They cover the
--push-to-aws-lcescape, that the refusal still stands without it, and that only abackport-branch can be pushed.CDK - synthesized the OIDC stack (
aws-cdk-lib+constructs, no deploy) and read the rendered trust policies:AwsLcGitHubActionsBackportOidcRolepinned tobackport-bot.yml@*,AwsLcGitHubActionsOidcRoleexcluding bothautofix_integration_failures.yml@*andbackport-bot.yml@*, andAwsLcGitHubActionsBedrockRoletrusting the autofix and backport OIDC roles withsts:AssumeRole+sts:TagSession. Checked the staging account path rendersaws-lc-stagingin all of them.Workflow - parsed it to confirm the two jobs hold the permissions above,
analyzehas no write access,publishhas noid-token, the settings step runs beforeconfigure-aws-credentials, and the CDK pin matches this filename. I can't run the bot end to end until the role is deployed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.