From 3b8abb02ca07006ecd7f2dd768055c4850e2be5d Mon Sep 17 00:00:00 2001 From: Richard Hope Date: Sat, 4 Jul 2026 11:20:00 +1000 Subject: [PATCH] Give ci-workflow-review first-class GitLab coverage The skill named GitLab CI in scope but its concrete patterns were all GitHub-specific, so a GitLab review leaned on the agent to translate and could miss GitLab-only hazards. Each checklist category now names both platforms' mechanics, grounded in GitLab's pipeline security, CI/CD job token, and runner security docs: - injection: $CI_MERGE_REQUEST_TITLE / $CI_COMMIT_MESSAGE into script: alongside the github.event equivalents - PPE: fork MR pipelines running the fork's edited .gitlab-ci.yml - token scope: CI_JOB_TOKEN allowlist and masked/protected variables alongside GITHUB_TOKEN permissions - pinning: remote include:/CI-CD Catalog components and image digests - runners: privileged Docker/DinD and shell executors on shared runners (CI_JOB_TOKEN and cross-project source theft) Adds a GitLab eval fixture (privileged DinD runner + MR-title injection in a fork-reachable job) next to the existing GitHub one; the eval runner and structure test now allow multiple fixtures per skill via a - directory suffix. Bumped to 0.2.0; plugin tree regenerated. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 21 ++-- .../skills/ci-workflow-review/SKILL.md | 105 +++++++++++------- evals/README.md | 4 + .../base/.gitlab-ci.yml | 9 ++ .../change/.gitlab-ci.yml | 26 +++++ .../ci-workflow-review-gitlab/expected.yaml | 7 ++ .../skills/ci-workflow-review/meta.yaml | 2 +- .../skills/ci-workflow-review/skill.md | 105 +++++++++++------- tests/test_eval_fixtures.py | 4 +- 9 files changed, 194 insertions(+), 89 deletions(-) create mode 100644 evals/fixtures/ci-workflow-review-gitlab/base/.gitlab-ci.yml create mode 100644 evals/fixtures/ci-workflow-review-gitlab/change/.gitlab-ci.yml create mode 100644 evals/fixtures/ci-workflow-review-gitlab/expected.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7429d..45a0d3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,15 +13,18 @@ All notable changes to this project are documented here. The format is based on a comment), and a Dependabot config keeps the pins and dev dependencies current (#34). -### Added - -- `ci-workflow-review` skill (0.1.0) — reviews CI/CD pipeline changes for - injection and poisoned pipeline execution (untrusted `github.event` - interpolation, `pull_request_target` + head checkout), credential and token - scope, unpinned third-party steps, artifact/cache integrity, and runner - exposure. Classified against the OWASP Top 10 CI/CD Security Risks - (CICD-SEC-1–10) with patterns from GitHub's Actions hardening guide. Ships - with an eval fixture (fork-triggerable title injection). +- `ci-workflow-review` skill (0.2.0) — reviews CI/CD pipeline changes for + injection and poisoned pipeline execution (untrusted `github.event` / + GitLab predefined-variable interpolation, `pull_request_target` + head + checkout, fork MR pipelines), credential and token scope + (`GITHUB_TOKEN` permissions, `CI_JOB_TOKEN` allowlist, masked/protected + variables), unpinned third-party steps and `include:`s, artifact/cache + integrity, and runner exposure (self-hosted runners, GitLab privileged + Docker/DinD and shell executors). Classified against the OWASP Top 10 CI/CD + Security Risks (CICD-SEC-1–10) with patterns from GitHub's Actions hardening + guide and GitLab's pipeline/job-token/runner security guidance. Ships with + GitHub and GitLab eval fixtures (fork-triggerable title injection; a + privileged DinD runner plus MR-title injection in a fork-reachable job). - `iac-review` skill (0.1.0) — reviews infrastructure-as-code changes (Terraform, CloudFormation, Kubernetes/Helm, Dockerfiles) for network exposure, wildcard IAM, secrets in code/state, missing encryption, container diff --git a/claude-plugin/skills/ci-workflow-review/SKILL.md b/claude-plugin/skills/ci-workflow-review/SKILL.md index fa17705..32e639d 100644 --- a/claude-plugin/skills/ci-workflow-review/SKILL.md +++ b/claude-plugin/skills/ci-workflow-review/SKILL.md @@ -11,18 +11,25 @@ configuration — GitHub Actions workflows, GitLab CI, Jenkins, and similar — the ways a pipeline can be hijacked or made to leak credentials. Findings are classified against the [OWASP Top 10 CI/CD Security Risks](https://owasp.org/www-project-top-10-ci-cd-security-risks/) -(CICD-SEC-1–10), with concrete patterns drawn from GitHub's +(CICD-SEC-1–10). The concrete patterns come from GitHub's [security hardening for GitHub Actions](https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions) -guidance; the patterns generalize to other CI systems. Pair with -`dependency-review` for the packages a build installs and `security-review` -for application code. +and GitLab's [pipeline security](https://docs.gitlab.com/ee/ci/pipelines/pipeline_security.html), +[CI/CD job token](https://docs.gitlab.com/ee/ci/jobs/ci_job_token.html), and +[runner security](https://docs.gitlab.com/runner/security/) guidance; the +CICD-SEC categories apply to any CI system. Pair with `dependency-review` for +the packages a build installs and `security-review` for application code. Pipeline config is code that runs with credentials. Treat every value an -outside contributor can influence — PR titles and bodies, branch names, commit -messages, author names, issue text — as attacker-controlled input that must -never reach a shell or a privileged context unquoted. Exploitability hinges on -**who can trigger the workflow** and **what the job can reach**: establish -both before judging severity. +outside contributor can influence — MR/PR titles and bodies, branch names, +commit messages, author names, issue text — as attacker-controlled input that +must never reach a shell or a privileged context unquoted. Exploitability +hinges on **who can trigger the pipeline** and **what the job can reach**: +establish both before judging severity. + +The checklist below names both GitHub and GitLab mechanics for each pattern; +map them to whatever CI system the diff actually uses (the injection, +token-scope, pinning, and runner-isolation concerns are universal even where +the syntax differs). ## Scope @@ -33,61 +40,81 @@ both before judging severity. (`action.yml`), reusable workflows, `.gitlab-ci.yml`, `Jenkinsfile`, `azure-pipelines.yml`, `.circleci/`, Buildkite/Tekton configs. 3. Read the whole workflow around each hunk, not just the diff — triggers, - `permissions`, and secrets interact across the file, and a guard may sit - outside the changed lines. -4. For each changed job, establish the trigger surface: can a fork PR, an + token permissions, and secrets interact across the file, and a guard may + sit outside the changed lines. +4. For each changed job, establish the trigger surface: can a fork MR/PR, an issue event, or an unauthenticated actor cause it to run, and with which - token and secrets? + token and secrets? On GitLab, note whether the job runs on a protected + branch/tag (so protected variables and runners are in reach) or in a fork + MR pipeline (which executes the fork's `.gitlab-ci.yml`). ## What to look for (by CICD-SEC category) ### Poisoned pipeline execution & injection (CICD-SEC-4, CICD-SEC-1) -- **Untrusted interpolation into scripts** — expressions such as - `${{ github.event.pull_request.title }}`, `.body`, branch names, commit - messages, or author names expanded inside `run:` — attacker-controlled text - becomes shell. Route the value through `env:` and reference it quoted - (`"$TITLE"`), or pass it as an action argument. -- **Privileged trigger + untrusted code** — `pull_request_target` or +- **Untrusted interpolation into scripts** — attacker-controlled text expanded + into a shell step becomes shell. GitHub: `${{ github.event.pull_request.title }}`, + `.body`, branch/commit/author fields inside `run:`. GitLab: predefined + variables like `$CI_MERGE_REQUEST_TITLE`, `$CI_MERGE_REQUEST_SOURCE_BRANCH_NAME`, + or `$CI_COMMIT_MESSAGE` inside `script:`. Route the value through an `env:` + var and reference it quoted (`"$TITLE"`), or pass it as an argument — never + inline the raw expression. +- **Privileged trigger + untrusted code** — GitHub: `pull_request_target` or `workflow_run` combined with a checkout of the PR head (`ref: github.event.pull_request.head.sha`) runs attacker code with secrets - and a write token. + and a write token. GitLab: a fork MR pipeline runs the fork's edited + `.gitlab-ci.yml`, so any protected variable or runner reachable from fork + MRs is exposed — check `rules:`/`only:` that let MR pipelines touch + protected resources. - Executing files an outside contributor can modify (build scripts, Makefiles, `package.json` lifecycle hooks) inside a privileged job. - Deploy or release jobs newly reachable without a required review, - environment protection rule, or branch gate (insufficient flow control). + environment protection rule, or protected-branch/tag gate (insufficient + flow control). ### Credential hygiene & token scope (CICD-SEC-6, CICD-SEC-5, CICD-SEC-2) -- Missing or broadened `permissions:` — the workflow inherits a broad default - `GITHUB_TOKEN`; set `permissions: contents: read` at the top and raise - per-job only as needed. +- Over-broad automatic token. GitHub: missing or broadened `permissions:` — + set `permissions: contents: read` at the top and raise per-job only as + needed. GitLab: `CI_JOB_TOKEN` cross-project access left wide — keep the + target project's job-token allowlist enabled and minimal; a disabled + allowlist lets a pipeline in any project use a leaked token against this one. - Secrets in plaintext in the pipeline file; secrets passed as command-line arguments (visible in logs and process lists); derived/transformed secrets that will not be masked; a whole JSON credential blob where one field is - needed. -- Secrets or privileged runners newly exposed to jobs that fork PRs can + needed. GitLab: CI/CD variables that should be **masked** and **protected** + (restricted to protected branches/tags) but are not, especially any variable + reachable from a fork MR pipeline. +- Secrets or privileged runners newly exposed to jobs that fork MRs/PRs can trigger. - Long-lived cloud keys stored as secrets where short-lived OIDC federation - is available. + (GitHub OIDC, GitLab ID tokens) is available. ### Third-party steps (CICD-SEC-3, CICD-SEC-8) -- Actions, orbs, or plugins referenced by **mutable tag or branch** - (`uses: some/action@v3`, `@main`) instead of a full commit SHA — the only - immutable reference; a compromised action sees every secret its job gets. -- New third-party steps or reusable workflows from outside the org with no - provenance check. +- Third-party build blocks referenced by **mutable tag or branch** instead of + a pinned commit SHA — the only immutable reference; a compromised one sees + every secret its job gets. GitHub: `uses: some/action@v3` / `@main`. GitLab: + remote `include:` (or CI/CD Catalog components) pinned to a branch/tag rather + than a commit; also container `image:` pinned by tag rather than digest. +- New third-party steps, reusable workflows, or `include:`d config from outside + the org with no provenance check. ### Artifacts, caches & runners (CICD-SEC-9, CICD-SEC-7) -- Artifacts produced by an untrusted workflow run consumed by a privileged - job without validation; caches writable from fork PRs feeding privileged - builds (cache poisoning). -- Self-hosted runners exposed to public-repo or fork-PR workloads; secrets - and credentials resident on the runner image. -- `continue-on-error:` added to a security check; debug flags that echo - secrets or environment into logs (also CICD-SEC-10). +- Artifacts produced by an untrusted run consumed by a privileged job without + validation; caches writable from fork MRs/PRs feeding privileged builds + (cache poisoning). +- Runners exposed to untrusted jobs. GitHub: self-hosted runners on + public-repo or fork-PR workloads. GitLab: a **privileged Docker executor** + or docker-in-docker (root on the runner host), or a **shell executor**, on a + shared/non-ephemeral runner that also runs untrusted or fork-MR jobs — one + job can steal another's `CI_JOB_TOKEN` and source. Prefer ephemeral, + non-privileged runners; restrict privileged/DinD runners to protected + branches. +- Secrets or credentials resident on the runner image. +- `continue-on-error:`/`allow_failure: true` added to a security check; debug + flags that echo secrets or the environment into logs (also CICD-SEC-10). ## Output diff --git a/evals/README.md b/evals/README.md index afb07ca..89fc410 100644 --- a/evals/README.md +++ b/evals/README.md @@ -44,6 +44,10 @@ CI validates fixture structure (`tests/test_eval_fixtures.py`): the fixture loads, targets a bundled skill, the plant is part of the diff, and the repo builds — no API calls. +A skill may have more than one fixture: name the directory for the skill, or +add a `-` suffix (e.g. `ci-workflow-review-gitlab`) and set the +`skill:` field in `expected.yaml` to the skill it exercises. + ## Adding a fixture Keep it minimal: the smallest `base/` that gives the change context, one diff --git a/evals/fixtures/ci-workflow-review-gitlab/base/.gitlab-ci.yml b/evals/fixtures/ci-workflow-review-gitlab/base/.gitlab-ci.yml new file mode 100644 index 0000000..ae4f53a --- /dev/null +++ b/evals/fixtures/ci-workflow-review-gitlab/base/.gitlab-ci.yml @@ -0,0 +1,9 @@ +stages: + - test + +unit-tests: + stage: test + image: python:3.12-slim + script: + - pip install -r requirements.txt + - pytest diff --git a/evals/fixtures/ci-workflow-review-gitlab/change/.gitlab-ci.yml b/evals/fixtures/ci-workflow-review-gitlab/change/.gitlab-ci.yml new file mode 100644 index 0000000..72eea5b --- /dev/null +++ b/evals/fixtures/ci-workflow-review-gitlab/change/.gitlab-ci.yml @@ -0,0 +1,26 @@ +stages: + - test + - build + +unit-tests: + stage: test + image: python:3.12-slim + script: + - pip install -r requirements.txt + - pytest + +build-image: + stage: build + image: docker:24 + services: + - docker:24-dind + variables: + DOCKER_TLS_CERTDIR: "" + tags: + - privileged + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + script: + - echo "Building for MR: $CI_MERGE_REQUEST_TITLE" + - docker build -t "$CI_REGISTRY_IMAGE:$CI_COMMIT_SHA" . + - docker push "$CI_REGISTRY_IMAGE:$CI_COMMIT_SHA" diff --git a/evals/fixtures/ci-workflow-review-gitlab/expected.yaml b/evals/fixtures/ci-workflow-review-gitlab/expected.yaml new file mode 100644 index 0000000..3fe35a2 --- /dev/null +++ b/evals/fixtures/ci-workflow-review-gitlab/expected.yaml @@ -0,0 +1,7 @@ +skill: ci-workflow-review +plants: + - file: .gitlab-ci.yml + keywords: [injection, CI_MERGE_REQUEST_TITLE, untrusted] + - file: .gitlab-ci.yml + keywords: [privileged, dind, docker-in-docker, runner] +max-findings: 6 diff --git a/src/skilldeck/skills/ci-workflow-review/meta.yaml b/src/skilldeck/skills/ci-workflow-review/meta.yaml index 22bf8ef..42fe9e2 100644 --- a/src/skilldeck/skills/ci-workflow-review/meta.yaml +++ b/src/skilldeck/skills/ci-workflow-review/meta.yaml @@ -1,7 +1,7 @@ name: ci-workflow-review description: Review CI/CD pipeline changes for injection, credential exposure, and supply-chain risk, aligned to the OWASP Top 10 CI/CD Security Risks. category: security -version: 0.1.0 +version: 0.2.0 supported-agents: - claude - codex diff --git a/src/skilldeck/skills/ci-workflow-review/skill.md b/src/skilldeck/skills/ci-workflow-review/skill.md index ed774d9..59e5052 100644 --- a/src/skilldeck/skills/ci-workflow-review/skill.md +++ b/src/skilldeck/skills/ci-workflow-review/skill.md @@ -5,18 +5,25 @@ configuration — GitHub Actions workflows, GitLab CI, Jenkins, and similar — the ways a pipeline can be hijacked or made to leak credentials. Findings are classified against the [OWASP Top 10 CI/CD Security Risks](https://owasp.org/www-project-top-10-ci-cd-security-risks/) -(CICD-SEC-1–10), with concrete patterns drawn from GitHub's +(CICD-SEC-1–10). The concrete patterns come from GitHub's [security hardening for GitHub Actions](https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions) -guidance; the patterns generalize to other CI systems. Pair with -`dependency-review` for the packages a build installs and `security-review` -for application code. +and GitLab's [pipeline security](https://docs.gitlab.com/ee/ci/pipelines/pipeline_security.html), +[CI/CD job token](https://docs.gitlab.com/ee/ci/jobs/ci_job_token.html), and +[runner security](https://docs.gitlab.com/runner/security/) guidance; the +CICD-SEC categories apply to any CI system. Pair with `dependency-review` for +the packages a build installs and `security-review` for application code. Pipeline config is code that runs with credentials. Treat every value an -outside contributor can influence — PR titles and bodies, branch names, commit -messages, author names, issue text — as attacker-controlled input that must -never reach a shell or a privileged context unquoted. Exploitability hinges on -**who can trigger the workflow** and **what the job can reach**: establish -both before judging severity. +outside contributor can influence — MR/PR titles and bodies, branch names, +commit messages, author names, issue text — as attacker-controlled input that +must never reach a shell or a privileged context unquoted. Exploitability +hinges on **who can trigger the pipeline** and **what the job can reach**: +establish both before judging severity. + +The checklist below names both GitHub and GitLab mechanics for each pattern; +map them to whatever CI system the diff actually uses (the injection, +token-scope, pinning, and runner-isolation concerns are universal even where +the syntax differs). ## Scope @@ -27,61 +34,81 @@ both before judging severity. (`action.yml`), reusable workflows, `.gitlab-ci.yml`, `Jenkinsfile`, `azure-pipelines.yml`, `.circleci/`, Buildkite/Tekton configs. 3. Read the whole workflow around each hunk, not just the diff — triggers, - `permissions`, and secrets interact across the file, and a guard may sit - outside the changed lines. -4. For each changed job, establish the trigger surface: can a fork PR, an + token permissions, and secrets interact across the file, and a guard may + sit outside the changed lines. +4. For each changed job, establish the trigger surface: can a fork MR/PR, an issue event, or an unauthenticated actor cause it to run, and with which - token and secrets? + token and secrets? On GitLab, note whether the job runs on a protected + branch/tag (so protected variables and runners are in reach) or in a fork + MR pipeline (which executes the fork's `.gitlab-ci.yml`). ## What to look for (by CICD-SEC category) ### Poisoned pipeline execution & injection (CICD-SEC-4, CICD-SEC-1) -- **Untrusted interpolation into scripts** — expressions such as - `${{ github.event.pull_request.title }}`, `.body`, branch names, commit - messages, or author names expanded inside `run:` — attacker-controlled text - becomes shell. Route the value through `env:` and reference it quoted - (`"$TITLE"`), or pass it as an action argument. -- **Privileged trigger + untrusted code** — `pull_request_target` or +- **Untrusted interpolation into scripts** — attacker-controlled text expanded + into a shell step becomes shell. GitHub: `${{ github.event.pull_request.title }}`, + `.body`, branch/commit/author fields inside `run:`. GitLab: predefined + variables like `$CI_MERGE_REQUEST_TITLE`, `$CI_MERGE_REQUEST_SOURCE_BRANCH_NAME`, + or `$CI_COMMIT_MESSAGE` inside `script:`. Route the value through an `env:` + var and reference it quoted (`"$TITLE"`), or pass it as an argument — never + inline the raw expression. +- **Privileged trigger + untrusted code** — GitHub: `pull_request_target` or `workflow_run` combined with a checkout of the PR head (`ref: github.event.pull_request.head.sha`) runs attacker code with secrets - and a write token. + and a write token. GitLab: a fork MR pipeline runs the fork's edited + `.gitlab-ci.yml`, so any protected variable or runner reachable from fork + MRs is exposed — check `rules:`/`only:` that let MR pipelines touch + protected resources. - Executing files an outside contributor can modify (build scripts, Makefiles, `package.json` lifecycle hooks) inside a privileged job. - Deploy or release jobs newly reachable without a required review, - environment protection rule, or branch gate (insufficient flow control). + environment protection rule, or protected-branch/tag gate (insufficient + flow control). ### Credential hygiene & token scope (CICD-SEC-6, CICD-SEC-5, CICD-SEC-2) -- Missing or broadened `permissions:` — the workflow inherits a broad default - `GITHUB_TOKEN`; set `permissions: contents: read` at the top and raise - per-job only as needed. +- Over-broad automatic token. GitHub: missing or broadened `permissions:` — + set `permissions: contents: read` at the top and raise per-job only as + needed. GitLab: `CI_JOB_TOKEN` cross-project access left wide — keep the + target project's job-token allowlist enabled and minimal; a disabled + allowlist lets a pipeline in any project use a leaked token against this one. - Secrets in plaintext in the pipeline file; secrets passed as command-line arguments (visible in logs and process lists); derived/transformed secrets that will not be masked; a whole JSON credential blob where one field is - needed. -- Secrets or privileged runners newly exposed to jobs that fork PRs can + needed. GitLab: CI/CD variables that should be **masked** and **protected** + (restricted to protected branches/tags) but are not, especially any variable + reachable from a fork MR pipeline. +- Secrets or privileged runners newly exposed to jobs that fork MRs/PRs can trigger. - Long-lived cloud keys stored as secrets where short-lived OIDC federation - is available. + (GitHub OIDC, GitLab ID tokens) is available. ### Third-party steps (CICD-SEC-3, CICD-SEC-8) -- Actions, orbs, or plugins referenced by **mutable tag or branch** - (`uses: some/action@v3`, `@main`) instead of a full commit SHA — the only - immutable reference; a compromised action sees every secret its job gets. -- New third-party steps or reusable workflows from outside the org with no - provenance check. +- Third-party build blocks referenced by **mutable tag or branch** instead of + a pinned commit SHA — the only immutable reference; a compromised one sees + every secret its job gets. GitHub: `uses: some/action@v3` / `@main`. GitLab: + remote `include:` (or CI/CD Catalog components) pinned to a branch/tag rather + than a commit; also container `image:` pinned by tag rather than digest. +- New third-party steps, reusable workflows, or `include:`d config from outside + the org with no provenance check. ### Artifacts, caches & runners (CICD-SEC-9, CICD-SEC-7) -- Artifacts produced by an untrusted workflow run consumed by a privileged - job without validation; caches writable from fork PRs feeding privileged - builds (cache poisoning). -- Self-hosted runners exposed to public-repo or fork-PR workloads; secrets - and credentials resident on the runner image. -- `continue-on-error:` added to a security check; debug flags that echo - secrets or environment into logs (also CICD-SEC-10). +- Artifacts produced by an untrusted run consumed by a privileged job without + validation; caches writable from fork MRs/PRs feeding privileged builds + (cache poisoning). +- Runners exposed to untrusted jobs. GitHub: self-hosted runners on + public-repo or fork-PR workloads. GitLab: a **privileged Docker executor** + or docker-in-docker (root on the runner host), or a **shell executor**, on a + shared/non-ephemeral runner that also runs untrusted or fork-MR jobs — one + job can steal another's `CI_JOB_TOKEN` and source. Prefer ephemeral, + non-privileged runners; restrict privileged/DinD runners to protected + branches. +- Secrets or credentials resident on the runner image. +- `continue-on-error:`/`allow_failure: true` added to a security check; debug + flags that echo secrets or the environment into logs (also CICD-SEC-10). ## Output diff --git a/tests/test_eval_fixtures.py b/tests/test_eval_fixtures.py index 6be2087..73a1f97 100644 --- a/tests/test_eval_fixtures.py +++ b/tests/test_eval_fixtures.py @@ -33,10 +33,12 @@ def _bundled_skill_names(): def test_every_fixture_dir_is_named_for_its_skill(): + # A skill may have several fixtures: the directory is either the skill name + # or the skill name plus a "-" suffix (e.g. a GitLab variant). assert FIXTURE_DIRS, "no eval fixtures found" for path in FIXTURE_DIRS: fixture = run_evals.load_fixture(path) - assert fixture.skill == path.name + assert path.name == fixture.skill or path.name.startswith(fixture.skill + "-") @pytest.mark.parametrize("path", FIXTURE_DIRS, ids=lambda p: p.name)