diff --git a/.github/agent/action-allowlist.json b/.github/agent/action-allowlist.json new file mode 100644 index 00000000..278f73e9 --- /dev/null +++ b/.github/agent/action-allowlist.json @@ -0,0 +1,14 @@ +{ + "allowed_actions": [ + "actions/attest-build-provenance", + "actions/checkout", + "actions/dependency-review-action", + "actions/download-artifact", + "actions/setup-node", + "actions/upload-artifact", + "anchore/sbom-action", + "aquasecurity/trivy-action", + "github/codeql-action", + "raven-actions/actionlint" + ] +} diff --git a/docs/security.md b/docs/security.md index 49722e66..63ff7702 100644 --- a/docs/security.md +++ b/docs/security.md @@ -787,6 +787,62 @@ script gets a narrow, documented exception — never a blanket re-enable. Playwright's browser install stays an explicit step (`npx playwright install --with-deps chromium`), so it is unaffected. +### Allowed GitHub Actions + +A pin and an allowlist answer two different questions, so both are required. The +pin (`action-pins` gate 1) says "this action cannot change under us"; it says +nothing about *who wrote it*. A pull request can add a brand-new third-party +action pinned to a malicious commit, and that commit runs on our runner with our +token — reviewed, pinned, and still an exfiltration. The allowlist says "only the +actions this project has read are allowed". + +The list is data in the tree, `.github/agent/action-allowlist.json`, as exact +`owner/repo` names — no wildcards, because `actions/*` re-opens the hole the list +exists to close. A sub-path belongs to its repository: `github/codeql-action/init` +is covered by `github/codeql-action`, since a fork of one sub-action is not a +different action. `readAllowlist` refuses to load a malformed file rather than +returning an empty list, and a name in the list that no workflow uses is a test +failure too — drift in either direction is how a list stops describing the tree. + +CI enforces the list in the `action-pins` required job. The checker is read from +the base branch, so the list it reads is the one already reviewed there; a pull +request therefore cannot grant itself an action in the same commit that uses it. +That is deliberate ordering, not an inconvenience: **the list entry lands first, +in its own reviewed change, and the workflow that uses the action comes after.** + +The GitHub setting behind it is the other half, and it is a **repository setting, +so it is the owner's action, never an agent's**: + +| Setting | Where | At the time of writing | +|---|---|---| +| `allowed_actions` | Settings → Actions → General → Actions permissions | `all` | +| `sha_pinning_required` | same page, "Require actions to be pinned to a full-length commit SHA" | `true` | + +`GET /repos/{owner}/{repo}/actions/permissions` reports both, and `PATCH` on the +same path sets them — `{"allowed_actions":"selected","sha_pinning_required":true}`, +then `PUT /repos/{owner}/{repo}/actions/permissions/selected-actions` with +`{"github_owned_allowed":false,"verified_allowed":false,"patterns_allowed":[...]}` +carrying the same `owner/repo` names as the JSON file. The settings page is the +same switch. Until they are flipped, the list is enforced by CI alone: it stops a +pull request from merging with an unreviewed action, but it does not stop the +workflow from starting if one is already on the branch. + +Procedure for adding an action: + +1. Open a T3 issue naming the action, its 40-hex pin, the tag it came from and + why no existing action does the job. An action without that reason is the + thing this control exists to refuse. +2. Add one line to `.github/agent/action-allowlist.json` in its own reviewed + change, with the justification in the pull request body. The suite's + both-directions test must still pass, so a list entry that no workflow uses + cannot ride along. +3. Land it, then open the workflow change that uses the action. The gate reads the + list from the base branch, so reversing the order reds the newer pull request + until the list is on `main`. +4. Only the owner flips `allowed_actions` to `selected` and publishes the same + patterns through `patterns_allowed`; an agent may read the settings, never write + them. + ### Secret scanning and push protection Two boundaries, deliberately redundant: GitHub's push protection refuses a diff --git a/scripts/action-pins.mjs b/scripts/action-pins.mjs index 0e8532da..8e74d051 100644 --- a/scripts/action-pins.mjs +++ b/scripts/action-pins.mjs @@ -1,7 +1,8 @@ #!/usr/bin/env node // Supply-chain gate 1, wired into CI: scan the real workflow tree and fail the -// job on any action reference that is not a 40-hex pin with its tag comment. -import { scanActorGates, scanWorkflows } from './lib/action-pins.mjs' +// job on any action reference that is not a 40-hex pin with its tag comment, or +// that names an action outside the reviewed allowlist. +import { ALLOWLIST_FILE, defaultAllowlistPath, readAllowlist, scanActorGates, scanWorkflows } from './lib/action-pins.mjs' // #640: the checker runs from the base branch, but the workflows it reads are // the pull request's own — they are the data under test, so the directory is an @@ -13,18 +14,36 @@ function argument(name, fallback) { } const directory = argument('--workflows', '.github/workflows') -const report = scanWorkflows(directory) +// #650: the list is read relative to this script, which in CI is the base +// checkout, so a pull request cannot add its own action to the list. +const allowlistPath = argument('--allowlist', defaultAllowlistPath()) +let allowlist +try { + allowlist = readAllowlist(allowlistPath) +} catch (error) { + console.error(`action-pins: ${error.message}`) + process.exit(1) +} + +const report = scanWorkflows(directory, allowlist) for (const finding of report.findings) { const hint = finding.problem === 'not-pinned' ? 'pin it to a 40-character commit sha and keep the tag as a trailing comment' - : 'add the tag it was pinned from as a trailing comment' + : finding.problem === 'not-allowed' + ? `this action is not on the reviewed list; adding it needs a separate reviewed change to ${ALLOWLIST_FILE} before a workflow may use it` + : 'add the tag it was pinned from as a trailing comment' console.error(`${finding.file}:${finding.line}: ${finding.value}: ${hint}`) } if (report.findings.length > 0) { - console.error(`action-pins: ${report.findings.length} action reference(s) are not pinned`) + const notAllowed = report.findings.filter((finding) => finding.problem === 'not-allowed').length + const notPinned = report.findings.length - notAllowed + const parts = [] + if (notPinned > 0) parts.push(`${notPinned} action reference(s) are not pinned`) + if (notAllowed > 0) parts.push(`${notAllowed} action reference(s) are not on the allowlist`) + console.error(`action-pins: ${parts.join('; ')}`) process.exit(1) } @@ -54,4 +73,4 @@ if (gates.findings.length > 0) { process.exit(1) } -console.log(`action-pins: ok (${report.checked} pinned references across ${report.files.length} workflow files; ${gates.checked} actions: write job(s) gated on the actor)`) +console.log(`action-pins: ok (${report.checked} pinned references across ${report.files.length} workflow files, all on the ${allowlist.length}-action allowlist; ${gates.checked} actions: write job(s) gated on the actor)`) diff --git a/scripts/lib/action-pins.d.mts b/scripts/lib/action-pins.d.mts index 9b853240..eb0a9f31 100644 --- a/scripts/lib/action-pins.d.mts +++ b/scripts/lib/action-pins.d.mts @@ -1,7 +1,7 @@ // Types for the action-pin checker, so the TypeScript suite can call the real // implementation and build its fixtures in a temporary directory instead of // restating the rules. -export type ActionPinProblem = 'not-pinned' | 'missing-tag-comment' +export type ActionPinProblem = 'not-pinned' | 'missing-tag-comment' | 'not-allowed' export interface ActionPinFinding { file: string @@ -16,9 +16,23 @@ export interface ActionPinReport { files: string[] } -export declare function checkWorkflow(text: string, file: string): ActionPinFinding[] +// `allowlist` is the reviewed owner/repo list; omitting it skips the allowlist +// half of the gate, which the CLI never does. +export declare function checkWorkflow(text: string, file: string, allowlist?: string[]): ActionPinFinding[] -export declare function scanWorkflows(dir: string): ActionPinReport +export declare function scanWorkflows(dir: string, allowlist?: string[]): ActionPinReport + +/** Where the reviewed list lives, relative to the repository root. */ +export declare const ALLOWLIST_FILE: string + +/** Resolves the list next to this module, so a base-branch copy reads its own. */ +export declare function defaultAllowlistPath(): string + +/** Throws on a malformed file: an unreadable list must not allow everything. */ +export declare function readAllowlist(path?: string): string[] + +/** `github/codeql-action/init@sha` -> `github/codeql-action`. */ +export declare function repositoryOf(value: string): string // One per reason the gate refuses: an ungated privileged job, an event it cannot // classify, a privileged job whose triggers it could not read, and every shape it diff --git a/scripts/lib/action-pins.mjs b/scripts/lib/action-pins.mjs index aa224b61..cdd90fe8 100644 --- a/scripts/lib/action-pins.mjs +++ b/scripts/lib/action-pins.mjs @@ -7,13 +7,71 @@ // secrets without a commit here, a review, or a diff anyone can see. The trailing // comment is required because it is what makes a re-pin a one-line, reviewable // edit (`@ # v4.4.0`) instead of an opaque sha swap nobody can check. +// +// Gate 1a, #650: a pin stops a tag from moving; it says nothing about who wrote +// the action. A pull request can add any third-party action pinned to any commit +// it likes, and that commit runs with this repository's token. So the second +// half of the rule is a list: only the actions this project has reviewed are +// allowed, and the list is data in the tree (`ALLOWLIST_FILE`) so widening it is +// a reviewed one-line diff instead of something a pull request grants itself. import { readdirSync, readFileSync } from 'node:fs' -import { join } from 'node:path' +import { dirname, join, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' const USES_LINE = /^(\s*(?:-\s*)?uses:\s*)(\S+)(.*)$/ const PINNED = /^[^\s@]+@[0-9a-f]{40}$/ const TAG_COMMENT = /(^|\s)#\s*\S+/ +// The list lives with the other machine-read policy files. `owner/repo` only: +// a wildcard would re-open exactly the hole the list closes, so `actions/*` is +// rejected as malformed rather than read as "any action under that owner". +export const ALLOWLIST_FILE = '.github/agent/action-allowlist.json' +const ACTION_REPOSITORY = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/ + +// Resolved from this file, never from the working directory: the checker runs +// from the base checkout, so the list it reads is the one already reviewed on +// the base branch. A pull request therefore cannot widen the list for itself in +// the same commit that uses a new action — the entry has to land first. +export function defaultAllowlistPath() { + return resolve(dirname(fileURLToPath(import.meta.url)), '..', '..', ALLOWLIST_FILE) +} + +// Throws on a malformed file instead of returning an empty list: a gate that +// silently allows everything when its data is unreadable is worse than no gate. +export function readAllowlist(path = defaultAllowlistPath()) { + let parsed + try { + parsed = JSON.parse(readFileSync(path, 'utf8')) + } catch (error) { + throw new Error(`${path}: the allowlist must be a JSON file (${error.message})`) + } + const list = parsed?.allowed_actions + if (!Array.isArray(list) || list.length === 0) { + throw new Error(`${path}: allowed_actions must be a non-empty array of owner/repo names`) + } + for (const entry of list) { + if (typeof entry !== 'string' || !ACTION_REPOSITORY.test(entry)) { + throw new Error(`${path}: ${JSON.stringify(entry)} is not an exact owner/repo name (wildcards are not allowed)`) + } + } + const duplicates = list.filter((entry, index) => list.indexOf(entry) !== index) + if (duplicates.length > 0) { + throw new Error(`${path}: ${duplicates.join(', ')} appears more than once`) + } + return list +} + +// `github/codeql-action/init@` belongs to `github/codeql-action`: the +// allowlist names repositories, and a sub-path is part of the repository it +// lives in rather than a separate entry someone could forget to classify. +export function repositoryOf(value) { + const at = value.indexOf('@') + const path = at === -1 ? value : value.slice(0, at) + const segments = path.split('/') + if (segments.length < 2) return path + return `${segments[0]}/${segments[1]}` +} + // A local action (`./path`) is a file in this repository and is reviewed like // any other file; a `docker://` reference is pinned by digest at the call site // and is not an action at all. Both are out of scope for this gate, and the @@ -35,7 +93,11 @@ function referenceOf(line) { // Returns one finding per `uses:` that is not a full-sha pin with a tag comment. // `file` is carried through so a caller scanning a tree can name the file. -export function checkWorkflow(text, file) { +// `allowlist` is the reviewed `owner/repo` list; omitting it skips gate 1a, which +// the CLI never does — it always passes what it read, so a caller that forgets +// the list cannot make the gate look green. +export function checkWorkflow(text, file, allowlist) { + const allow = allowlist === undefined ? null : new Set(allowlist.map((entry) => entry.toLowerCase())) const findings = [] const lines = text.split(/\r?\n/) for (let index = 0; index < lines.length; index += 1) { @@ -48,6 +110,11 @@ export function checkWorkflow(text, file) { if (!TAG_COMMENT.test(` ${reference.rest}`)) { findings.push({ file, line: index + 1, value: reference.value, problem: 'missing-tag-comment' }) } + // Checked even when the reference is otherwise malformed, so a workflow that + // adds an unreviewed action is told about the action as well as the pin. + if (allow !== null && !allow.has(repositoryOf(reference.value).toLowerCase())) { + findings.push({ file, line: index + 1, value: reference.value, problem: 'not-allowed' }) + } } return findings } @@ -55,7 +122,7 @@ export function checkWorkflow(text, file) { // Scans every *.yml / *.yaml directly under `dir`. `checked` counts the action // references in scope, so a run that found nothing to check cannot look the same // as a run where everything passed. -export function scanWorkflows(dir) { +export function scanWorkflows(dir, allowlist) { const findings = [] const files = [] let checked = 0 @@ -64,7 +131,7 @@ export function scanWorkflows(dir) { const file = join(dir, entry.name) files.push(file) const text = readFileSync(file, 'utf8') - findings.push(...checkWorkflow(text, file)) + findings.push(...checkWorkflow(text, file, allowlist)) for (const line of text.split(/\r?\n/)) { const reference = referenceOf(line) if (reference !== null && !outOfScope(reference.value)) checked += 1 diff --git a/test/actionPins.test.ts b/test/actionPins.test.ts index 2a533226..edec62a5 100644 --- a/test/actionPins.test.ts +++ b/test/actionPins.test.ts @@ -1,9 +1,9 @@ import test from 'node:test' import assert from 'node:assert/strict' -import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { checkActorGates, checkWorkflow, scanActorGates, scanWorkflows } from '../scripts/lib/action-pins.mjs' +import { ALLOWLIST_FILE, checkActorGates, checkWorkflow, readAllowlist, repositoryOf, scanActorGates, scanWorkflows } from '../scripts/lib/action-pins.mjs' // A real pin, the one this repository uses for actions/checkout. const PIN = '11d5960a326750d5838078e36cf38b85af677262' @@ -338,3 +338,78 @@ test('an event the gate cannot classify is a finding until it is classified', () assert.deepEqual(checkActorGates(workflow(` ${safe}:`, privilegedJob('repair', null)), 'fixture.yml'), [], `${safe} cannot be started by an outside account`) } }) + +// #650: a pin stops a tag from moving; it says nothing about who wrote the +// action. The policy also allows only the actions this project has reviewed, and +// the list is data in the tree so a new action is a reviewable one-line addition +// rather than something a pull request grants itself. +test('an action outside the allowlist is refused even when it is fully pinned', () => { + const text = ` - uses: stranger/exfiltrate@${PIN} # v1.0.0\n` + assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout']), [ + { file: 'fixture.yml', line: 1, value: `stranger/exfiltrate@${PIN}`, problem: 'not-allowed' }, + ]) +}) + +test('an allowlisted action passes, and a sub-path belongs to its repository', () => { + const text = [ + ` - uses: actions/checkout@${PIN} # v4.4.0`, + ` - uses: github/codeql-action/init@${PIN} # v3.28.1`, + ].join('\n') + assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout', 'github/codeql-action']), []) +}) + +test('the list is what admits an action, so widening it is a visible one-line change', () => { + const text = ` - uses: newvendor/tool@${PIN} # v2.0.0\n` + assert.equal(checkWorkflow(text, 'fixture.yml', ['actions/checkout']).length, 1) + assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout', 'newvendor/tool']), []) +}) + +test('the workflow tree in this repository passes the allowlist, not just the pin check', () => { + const allowlist = readAllowlist() + assert.ok(allowlist.length >= 10, `the reviewed list has shrunk to ${allowlist.length} entries`) + assert.deepEqual(scanWorkflows('.github/workflows', allowlist).findings, []) +}) + +// The list is the policy, so drift in either direction is a defect: an action in +// the tree that nobody added to the list (the gate would red the night after the +// merge) and an entry nobody uses (the next reader cannot tell whether it is a +// leftover or a mistake). Both are cheap to assert here and invisible otherwise. +test('the allowlist and the workflow tree name exactly the same repositories', () => { + const directory = '.github/workflows' + const used = new Set() + for (const name of readdirSync(directory)) { + if (!/\.ya?ml$/i.test(name)) continue + for (const line of readFileSync(join(directory, name), 'utf8').split(/\r?\n/)) { + const match = /^\s*(?:-\s*)?uses:\s*(\S+)/.exec(line) + if (match === null) continue + const value = match[1]?.replace(/^(['"])(.*)\1$/, '$2') ?? '' + if (value.startsWith('./') || value.startsWith('docker://')) continue + used.add(repositoryOf(value)) + } + } + const listed = new Set(readAllowlist()) + assert.deepEqual([...used].filter((repo) => !listed.has(repo)), [], 'a workflow uses an action the list does not name') + assert.deepEqual([...listed].filter((repo) => !used.has(repo)), [], 'the list names an action no workflow uses') +}) + +test('a malformed allowlist refuses to load instead of allowing everything', (t) => { + const dir = mkdtempSync(join(tmpdir(), 'action-allow-')) + t.after(() => rmSync(dir, { recursive: true, force: true })) + const write = (name: string, body: string): string => { + const path = join(dir, name) + writeFileSync(path, body) + return path + } + + assert.deepEqual(readAllowlist(write('ok.json', JSON.stringify({ allowed_actions: ['actions/checkout'] }))), ['actions/checkout']) + // A wildcard is the hole this list exists to close, so it is not "any action". + assert.throws(() => readAllowlist(write('wildcard.json', JSON.stringify({ allowed_actions: ['actions/*'] }))), /wildcards are not allowed/) + assert.throws(() => readAllowlist(write('empty.json', JSON.stringify({ allowed_actions: [] }))), /non-empty array/) + assert.throws(() => readAllowlist(write('dupe.json', JSON.stringify({ allowed_actions: ['actions/checkout', 'actions/checkout'] }))), /appears more than once/) + assert.throws(() => readAllowlist(write('missing.json', '')), /must be a JSON file/) +}) + +test('the shipped allowlist file is where the checker looks by default', () => { + assert.match(ALLOWLIST_FILE, /^\.github\/agent\/[a-z-]+\.json$/) + assert.ok(readAllowlist().includes('actions/checkout'), 'the default path must resolve to the reviewed list in this tree') +})