[SECURITY] Remove vulnerabilities from GH workflows#468
Conversation
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
There was a problem hiding this comment.
Pull request overview
Hardens this repository’s GitHub Actions configuration to mitigate workflow-trigger and credential-leak risks identified in issue #467, while also adding basic ownership/automation scaffolding for safer ongoing CI/CD maintenance.
Changes:
- Switch Python PR workflows from
pull_request_targettopull_requestand remove same-repo guards. - Mask CodeArtifact-derived credentials/URLs and pass them via
envinstead of inline interpolation. - Add Dependabot config for GitHub Actions updates and introduce an initial
CODEOWNERS.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CODEOWNERS |
Adds initial ownership for .github changes. |
.github/workflows/test-schema.yaml |
Updates core GitHub actions used in the Go schema validation workflow. |
.github/workflows/reusable-check-python-package-versions.yaml |
Updates action versions, switches Python version sourcing, and masks CodeArtifact index URL before output/usage. |
.github/workflows/publish-python-packages.yaml |
Adds workflow_dispatch and masks CodeArtifact token before output/usage; passes sensitive values via env. |
.github/workflows/check-python-package-versions.yaml |
Switches trigger to pull_request and removes same-repo guard while calling the reusable workflow. |
.github/workflows/check-python-code.yaml |
Switches trigger to pull_request, removes same-repo guard, and updates checkout action usage. |
.github/dependabot.yml |
Adds weekly Dependabot updates for GitHub Actions. |
Comments suppressed due to low confidence (1)
.github/workflows/publish-python-packages.yaml:16
workflow_dispatchdefines inputs (e.g.,aws_iam_role_name,domain,repository) but the workflow continues to use hard-coded values later (account/region/domain/repo/role). Either wire these inputs into the AWS configure step and CodeArtifact script invocations, or remove the unused inputs to avoid misleading operators.
workflow_dispatch:
inputs:
aws_iam_role_name:
description: The name of the IAM role to assume for accessing CodeArtifact
type: string
required: false
default: GithubActions_Schema_CodeArtifact_Publish
domain:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: John McCall <john@overturemaps.org>
Move the "Set up Python" step in reusable-check-python-package-versions.yaml to run after actions/checkout so the .python-version file is available to actions/setup-python and the subsequent "uv sync" can see repository packages. Also add a small whitespace/formatting tweak in test-schema.yaml (blank line between checkout and Go setup) for readability. Signed-off-by: John McCall <john@overturemaps.org>
|
Thanks for this John. I'll get chance to look at it eventually, but help me with the first bullet:
Let's make sure this is true. The reason
The IAM roles that can be assumed are very defensively scoped; one is read-only, and the only thing the other can do is publish to the Python sub-repo of CodeArtifact. |
Yeah, that can be achieved without using an elevated context that schema/.github/workflows/reusable-check-python-package-versions.yaml Lines 69 to 72 in 39a24f2 schema/.github/workflows/reusable-check-python-package-versions.yaml Lines 85 to 88 in 39a24f2 There are a few use cases for |
Signed-off-by: John McCall <john@overturemaps.org>
Right, but the content of the before commit is important. It should reflect the state of the current state of the branch in the official repo, not any state that comes with the PR. Can you confirm that's true? |
Yes, this is the core functionality of GitHub's checkout action and in this case is not affected at all by the change in event trigger:
|
Signed-off-by: John McCall <john@overturemaps.org>
This reverts commit e71c186.
Adam Lastowka (Rachmanin0xFF)
left a comment
There was a problem hiding this comment.
LGTM; I'm no workflow wizard but it seems like Vic's concerns were addressed + the workflow updates make sense
|
Victor Schappert (@vcschapp) I've added some specifics about your concerns - can you take another look? Happy to set up some time to review in person if that is helpful. |
Context
See #467 for findings that we need to remediate.
Changes
🔒 Replace
pull_request_targetwithpull_requestincheck-python-code.yamlandcheck-python-package-versions.yaml- neither workflow requires the elevated permissions thatpull_request_targetgrants, and its use created a path for contributors to execute arbitrary code with AWS IAM credentialsAn artificial touch to one of the project files triggered these modified jobs as a sanity check
🔒 Remove the now-redundant same-repo
ifguards from both workflows🔒 Mask CodeArtifact auth tokens before writing to
$GITHUB_OUTPUTinreusable-check-python-package-versions.yamlandpublish-python-packages.yaml🔒 Pass sensitive values via
env:into consuming steps rather than direct${{ steps.*.outputs.* }}interpolation inrun:scripts♻️Updated blessed GHA providers (
github,astral) to current major versions🧑🦲 Stubbed out a CODEOWNERS
.githubdirectory to ensure smooth, safe changes to CI/CD for this public repository