Skip to content

Commit 66a2e99

Browse files
authored
HADOOP-19868: ci: add comments from security review of new actions (#8450)
Since GitHub workflows are tricky to secure properly for public repositories, we're adding `# Security:` comments to workflows with explanations (informal proofs) that they are safe. This helps future authors avoid vulnerability foot-guns when modifying these workflows.
1 parent e344f64 commit 66a2e99

4 files changed

Lines changed: 30 additions & 0 deletions

File tree

.github/workflows/build_and_test.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919

2020
name: "Build"
2121

22+
# Security: write privileges are safe since this is triggered
23+
# only by `push` (implying user has write access).
2224
on:
2325
push:
2426
branches:

.github/workflows/notify_test_workflow.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@
2525

2626
name: "PR update"
2727

28+
# Security: `pull_request_target` events can be risky, but we:
29+
# 1. Only grant write access for checks (a.k.a. check runs).
30+
# 2. We are careful with untrusted inputs (i.e. don't use user-supplied
31+
# variables in ways vulnerable to shell injection, etc.)
32+
# 3. We don't check out the fork's head ref.
2833
on:
2934
pull_request_target:
3035
types: [opened, reopened, synchronize]

.github/workflows/tmpl_build_and_test.yml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ on:
4242
Candidates: "build-only".
4343
default: '[ "build-only" ]'
4444

45+
# Default to minimal permissions for workflow.
4546
permissions:
4647
packages: read
4748

@@ -78,6 +79,9 @@ jobs:
7879
steps:
7980
- name: Set up Outputs
8081
id: variables
82+
# Security: passing inputs.{os, branch} through workflow (above) inputs removes
83+
# ability to do shell injection below.
84+
# See: https://securitylab.github.com/resources/github-actions-untrusted-input/
8185
run: |
8286
# Convert to lowercase to meet Docker repo name requirement
8387
REPO_OWNER=$(echo "${{ github.repository_owner }}" | tr '[:upper:]' '[:lower:]')
@@ -86,6 +90,23 @@ jobs:
8690
name: Build Image ${{ inputs.os }}-${{ inputs.branch }}
8791
runs-on: ubuntu-24.04
8892
needs: [ precondition ]
93+
# Security: this does not leak write access for our image repository to
94+
# forked repos.
95+
#
96+
# We have `packages: write` permissions for our GITHUB_TOKEN below. However:
97+
#
98+
# - For `pull_request`, GitHub downgrades GITHUB_TOKEN permissions to
99+
# read-only.
100+
# - For `push` triggers on a fork, the GITHUB_TOKEN retains write
101+
# permissions, but the `push` is happening in the context of the fork, not
102+
# the upstream repo.
103+
# - `pull_request_target` events (not used here) use the base repo's
104+
# default branch. This prevents an attacker from adding code in a pull
105+
# request which prints / leaks secrets, etc.. Also the GITHUB_TOKEN we
106+
# use to grant access to writing to the image repo. is scoped to the
107+
# repository it runs on. Even if a fork were to gain access to a
108+
# GITHUB_TOKEN with write privileges, it doesn't grant access for other
109+
# repositories to write.
89110
permissions:
90111
packages: write
91112
outputs:

.github/workflows/update_build_status.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ on:
2323
schedule:
2424
- cron: "*/15 * * * *"
2525

26+
# Security: privileged (can write) workflow is only triggered via schedule,
27+
# so issues associated with forks do not apply.
2628
jobs:
2729
update:
2830
name: "Update Build Status"

0 commit comments

Comments
 (0)