diff --git a/.github/skills/add-egress-allowlist-domain/SKILL.md b/.github/skills/add-egress-allowlist-domain/SKILL.md new file mode 100644 index 0000000..daec989 --- /dev/null +++ b/.github/skills/add-egress-allowlist-domain/SKILL.md @@ -0,0 +1,181 @@ +--- +name: add-egress-allowlist-domain +description: Triage a host that was blocked by the Dependabot proxy egress allowlist, add it to internal/handlers/egress_allowlist_defaults.yaml with the correct matching form and regression tests, and open a pull request. Use when someone reports a blocked domain, asks to allowlist a host, or pastes an egress 403 from a Dependabot update job. +user-invocable: true +--- + +# Add a domain to the egress allowlist + +Use this skill when someone says something like "`repo.example.org` is blocked", "please allowlist `foo.bar.com`", or pastes a Dependabot job log showing an egress block. + +**Do not skip to editing YAML.** Most of this skill's value is step 1: a large fraction of reported hosts must *not* go into the static defaults at all. Adding them there is a security regression, not a fix. + +## Step 1 — Triage: does this host belong in the static allowlist? + +The static defaults are applied as a union to **every Dependabot job in the world**. Anything added here is reachable by every tenant. Classify the host before touching anything. + +Ask the requester for the host, the package ecosystem, and — if they have it — the job log line or what the host serves. + +| Category | Examples | Action | +|---|---|---| +| **Public, provider-controlled package infrastructure** | public registry, mirror, CDN, checksum/CRL endpoint, public VCS forge | **Add to the YAML.** Continue to step 2. | +| **Private, internal, or org-specific registry** | `artifacts.acme-corp.internal`, `acme.jfrog.io`, a self-hosted Nexus/Artifactory | **Do not add.** Tell the user to declare it under `registries:` in their `dependabot.yml`. Those hosts are allowlisted per-job automatically by `internal/handlers/egress_dynamic_hosts.go`. Stop here. | +| **Shared multi-tenant host where the tenant is in the URL path** | `dl.cloudsmith.io`, generic object-store download hosts | **Do not add.** The handler authorizes the **hostname only** — it never constrains path or method — so allowing the host grants every tenant's content to every job. Explain this and stop. | +| **User-uploadable file hosting** | `downloads.sourceforge.net`, arbitrary release-file mirrors | **Do not add** without explicit maintainer sign-off. Flag it and ask. | +| **Documentation, changelog, or homepage host** | project docs sites, blog domains | **Usually don't add.** These fail gracefully — `dependabot-core`'s metadata finder treats a non-200 as "no metadata", so the only loss is a missing changelog link in the PR body. Say so and ask whether it's worth it. | + +If the host is private or multi-tenant, the correct outcome of this skill is a clear explanation and **no code change**. That is a success, not a failure. + +**This step is the gate.** Only a host you have classified as public, provider-controlled infrastructure proceeds to step 2. Step 2 does not revisit this decision — it cannot (see the warning there) — so a misclassification here is never caught later. + +### Signals that a host is a private or tenant-specific registry + +These are not proof, but each should send you back to the table above: + +- **A wildcard certificate whose parent domain is a known multi-tenant provider** — `CN=*.jfrog.io`, `CN=*.cloudsmith.io`, `CN=*.fury.io`, `CN=*.myget.org`. This shows the *provider* owns the domain; it says nothing about the tenant being public. Note the inverse is not a signal: `*.julialang.org` and `*.huaweicloud.com` are wildcards on genuinely public infrastructure, so judge the parent domain, not the wildcard. +- **A redirect to a registry vendor's marketing site** — `package-manager.aa.com` and `dl.cloudsmith.io` both 302 to `https://cloudsmith.com/`. A private tenant fronted by a hosted registry commonly advertises its backing vendor this way. +- **An organisation name in the host** that matches the requester rather than an ecosystem (`artifactory..com`, `npm..com`, `.jfrog.io`). +- **A credentialed response** — `401`/`403` on a real artifact path means the host expects authentication, which is what `registries:` is for. + +## Step 2 — Confirm the host is safe to probe, and gather ownership evidence + +> **A passing `verify_host` does NOT authorise an addition.** It answers "is it safe for *me* to send a request here?" — not "is this host public infrastructure?". Most private registries pass it: `package-manager.aa.com`, `dl.cloudsmith.io` and `centraluhg.jfrog.io` all resolve to public IPs and serve valid certificates. A private registry that is properly internet-facing is indistinguishable from public infrastructure at this layer. Step 1 is what decides; this step only keeps the probe itself safe and collects evidence for the PR. + +Never add a host on the strength of a report alone. + +A reported hostname is untrusted input. Validate it as a bare DNS hostname *before* it reaches any command, require every resolved address to be public, and walk redirects one hop at a time — `curl -L` will happily follow a public host to loopback, RFC1918, link-local, or metadata endpoints. + +```bash +verify_host() { + local host="$1" ip ips + # Reject anything that is not a bare DNS hostname, before it reaches a command. + if ! printf '%s' "$host" | grep -qE '^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)+$'; then + echo "REJECT: not a bare DNS hostname"; return 1 + fi + # Every resolved address must be public. + ips=$(dig +short "$host" A; dig +short "$host" AAAA) + ips=$(printf '%s\n' "$ips" | grep -E '^([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+|[0-9a-fA-F:]+)$') + [ -z "$ips" ] && { echo "REJECT: does not resolve"; return 1; } + for ip in $ips; do + case "$ip" in + 10.*|127.*|0.*|169.254.*|192.168.*|::1|fc*|fd*|fe80*) echo "REJECT: non-public $ip"; return 1;; + 172.1[6-9].*|172.2[0-9].*|172.3[01].*) echo "REJECT: non-public $ip"; return 1;; + 100.6[4-9].*|100.[7-9][0-9].*|100.1[01][0-9].*|100.12[0-7].*) echo "REJECT: CGNAT $ip"; return 1;; + esac + done + echo "OK: $host -> $(printf '%s' "$ips" | tr '\n' ' ')" + # One hop at a time. Re-run verify_host on any next_hop before following it. + curl -sS -o /dev/null --max-time 15 --proto '=https' --max-redirs 0 \ + -w ' status=%{http_code} next_hop=%{redirect_url}\n' "https://$host/" || true + # Ownership evidence, not just reachability. + echo | openssl s_client -connect "$host:443" -servername "$host" 2>/dev/null \ + | openssl x509 -noout -subject -issuer 2>/dev/null | sed 's/^/ /' +} + +verify_host '' +``` + +Reachability alone is **not** sufficient evidence — it proves the host answers, not that the claimed provider controls it, and certainly not that it is public. Require authoritative corroboration: a TLS certificate whose subject/issuer belongs to the provider, an entry in a provider-published list such as `api.github.com/meta`, or the provider's own documentation. Check the certificate against the private-registry signals in step 1 before treating it as supporting evidence. A host that merely responds is not yet a candidate. + +Probe a **real artifact path**, not `/`. Root probes mislead: `data.nuget.org/` 404s and `packages.atlassian.com/` 401s, while both serve packages correctly on their real paths. + +If it redirects, re-run `verify_host` on the target before following it — **the redirect target may be the host that actually needs allowlisting**, and it is often a different one. + +## Step 3 — Choose the matching form + +Read the comment block at the top of `internal/handlers/egress_allowlist_defaults.yaml` before choosing. Three forms exist: + +- **Exact** (`storage.googleapis.com`) — matches only that host. **This is the default. Prefer it.** +- **Leading dot** (`.github.com`) — matches the domain and all subdomains. +- **Glob** (`*`, `?`, `[...]`, via `path.Match`; `*` spans dots). Values starting with `*` or `[` **must be quoted** or YAML misreads them as an alias or flow sequence. + +The rule for the non-exact forms: every label the pattern matches must be **entirely provider-controlled and never user-creatable**. If any matched label is attacker-choosable — a storage-account name, a bucket, an AWS account id, a user's pages subdomain — the form is unsafe, including infix globs. When in doubt, use the exact form. + +## Step 4 — Place the entry + +First, confirm it isn't already allowed: + +```bash +grep -n '' internal/handlers/egress_allowlist_defaults.yaml +go test ./internal/handlers/ -run TestEgressDefaults_NoRedundantEntries -count=1 +``` + +`grep` catches an exact repeat. It does **not** catch a host already covered by a leading-dot or glob entry — `foo.github.com` is redundant because `.github.com` exists — so also scan the file's leading-dot and `*` entries for one that would match. If the host is already allowed, the correct outcome is no code change; report where it is covered and stop. + +Then choose the section: + +- `github_infra_domains` — GitHub/Dependabot infrastructure only. Don't add third-party hosts here. +- `shared_registry_domains` — hosts genuinely used by more than one ecosystem. +- `ecosystem_default_domains.` — the normal case. + +Add the host to exactly one section. Every section is applied to every job, so listing it twice is redundant, not safer. + +Keep entries in the existing grouping and ordering of that section, and add a brief comment saying what the host serves when it isn't self-evident. + +**Watch the YAML anchors.** Several ecosystems are aliases and must not be edited separately: + +- `npm_and_yarn: &npm_registries` → `bun: *npm_registries` +- `pip: &python_registries` → `uv: *python_registries` +- `maven: &jvm_registries` → `gradle: *jvm_registries` +- `docker: &docker_registries` → `docker_compose`, `devcontainers` + +If the target is an alias member (e.g. `bun`), add the entry to the **anchor** definition instead. `TestEgressDefaults_AliasedEcosystemsStayInSync` enforces this. + +## Step 5 — Add regression tests + +Edit `internal/handlers/egress_allowlist_test.go`. **Both** are required. + +1. **Positive probe** — a realistic URL for the new host, added to the appropriate existing test (e.g. `TestEgressAllowlist_NewExactDomainsAllowed` or `TestEgressAllowlist_PublicRegistriesAllowed`): + + ```go + assert.Nil(t, egressResult(t, h, "https:///"), "new exact host allowed: ") + ``` + +2. **Negative child probe** — for every **exact** entry added, append `https://evil./payload` to `childProbes` in `TestEgressAllowlist_NewEntriesDoNotWidenBeyondExactHosts`. + + This matters: a *sibling* probe (`attacker.example.com` against entry `foo.example.com`) does **not** catch someone later widening the entry to a leading dot. Only a **child** probe does. Add a sibling probe as well when you also want to pin the parent namespace closed. + +Verify the negative probe actually bites by mutating your new entry to its leading-dot form and confirming the test fails, then revert. + +## Step 6 — Build, test, format + +```bash +go build ./... && go test ./internal/handlers/ -run TestEgress -count=1 && gofmt -l internal/handlers/ +``` + +`gofmt -l` must print nothing. Then run the full suite as `CONTRIBUTING.md` requires: + +```bash +script/test # Docker, -race -count=2 +``` + +If `script/test` can't run in the current environment, say so explicitly and leave the "complete test suite" checklist box in the PR **unticked**. Do not tick a box you did not verify. + +## Step 7 — Open the pull request + +Confirm with the user before pushing. Then: + +```bash +git checkout -b /allowlist- +git add internal/handlers/egress_allowlist_defaults.yaml internal/handlers/egress_allowlist_test.go +git commit +gh pr create --template .github/pull_request_template.md +``` + +`--template` opens the repository template for completion. Do not use `--fill`: it takes the title and body from commit data and skips template selection entirely, producing a PR that omits the required sections. + +If you lack write access to `dependabot/proxy`, push to a fork and use `gh pr create --repo dependabot/proxy --template .github/pull_request_template.md`. + +Complete every section of the template. The description should state what the host serves, which ecosystem needs it, the authoritative evidence that it is public provider-controlled infrastructure, and why the chosen matching form is safe. Only tick checklist boxes you actually verified. + +## Guardrails + +- Never add a host you could not reach in step 2, or for which you have no authoritative provider evidence. Reachability alone is not evidence of ownership. +- Never treat a passing `verify_host` as permission to add a host. It checks probe safety, not whether the host is public — private registries pass it routinely. Step 1 is the gate. +- Never interpolate a reported hostname into a command before validating it as a bare DNS hostname, and never follow redirects with `curl -L` during verification — validate each hop's address is public first. +- Never add a private, internal, or customer-tenant host to the static defaults — route it to `registries:` in `dependabot.yml`. +- Never widen an existing exact entry to a leading-dot or glob form as a shortcut for a subdomain report. Add the specific subdomain. +- Never add a host that is already allowed. Before editing, check for an exact repeat **and** for an existing leading-dot or glob entry that already covers it — a redundant entry changes nothing, implies the namespace is not already open, and makes deleting one copy look sufficient when it is not. `TestEgressDefaults_NoRedundantEntries` enforces this; run it before opening a PR. +- Never commit unrelated changes, and never commit scratch or triage files to the repo root. +- Treat a reported hostname as untrusted input: quote it in shell variables rather than interpolating it into the middle of a command. + diff --git a/internal/handlers/egress_allowlist_test.go b/internal/handlers/egress_allowlist_test.go index c5c3190..845387c 100644 --- a/internal/handlers/egress_allowlist_test.go +++ b/internal/handlers/egress_allowlist_test.go @@ -1,9 +1,12 @@ package handlers import ( + "maps" "net/http" "net/http/httptest" "path" + "slices" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -914,3 +917,113 @@ func TestEgressDefaults_LoadedFromYAML(t *testing.T) { assert.Contains(t, allEcosystemDomains, "registry.npmjs.org") assert.Contains(t, allEcosystemDomains, "pypi.org") } + +// TestEgressDefaults_NoRedundantEntries pins the YAML source, not the computed +// union. The union builder deduplicates, so a host listed twice in the file is +// absorbed silently and TestEgressDefaults_LoadedFromYAML still passes. That +// makes redundant entries invisible in review: they accumulate, imply a host +// needs listing in several places, and make removing one occurrence look +// sufficient when it is not. +// +// Note the aliased ecosystems (npm_and_yarn/bun, pip/uv, maven/gradle, +// docker/docker_compose/devcontainers) legitimately resolve to identical +// slices, so duplicates are counted per distinct source list, not per key. +func TestEgressDefaults_NoRedundantEntries(t *testing.T) { + withinList := func(t *testing.T, label string, hosts []string) { + t.Helper() + seen := make(map[string]struct{}, len(hosts)) + for _, host := range hosts { + key := strings.ToLower(host) + _, dup := seen[key] + assert.Falsef(t, dup, "%s lists %q more than once", label, host) + seen[key] = struct{}{} + } + } + + withinList(t, "github_infra_domains", githubInfraDomains) + withinList(t, "shared_registry_domains", sharedRegistryDomains) + + // Distinct source lists only: aliased ecosystems share one backing slice. + checked := make(map[string]bool) + for _, ecosystem := range slices.Sorted(maps.Keys(ecosystemDefaultDomains)) { + hosts := ecosystemDefaultDomains[ecosystem] + fingerprint := strings.Join(hosts, "\n") + if checked[fingerprint] { + continue + } + checked[fingerprint] = true + withinList(t, "ecosystem "+ecosystem, hosts) + } + + // A host must not be repeated across the always-applied sections. Every one + // of these is applied to every job, so a second listing is pure redundancy. + // Ecosystem keys are compared against the shared/infra lists rather than + // each other: two ecosystems legitimately naming the same public registry + // is the documented reason the union exists. + // + // knownProvenanceCopies grandfathers hosts that are deliberately listed + // twice because the ecosystem map doubles as documentation. Keep this set + // small and justify every addition — it exists so that genuinely accidental + // duplicates still fail. + knownProvenanceCopies := map[string]string{ + // ghcr.io is GitHub infrastructure, but the container ecosystems list it + // too so their entries read as a complete registry set. + "ghcr.io": "documents that container ecosystems pull from GHCR", + } + + always := map[string]string{} + for _, host := range githubInfraDomains { + always[strings.ToLower(host)] = "github_infra_domains" + } + for _, host := range sharedRegistryDomains { + key := strings.ToLower(host) + if where, ok := always[key]; ok { + assert.Failf(t, "redundant entry", + "%q is in both %s and shared_registry_domains", host, where) + } + always[key] = "shared_registry_domains" + } + for _, ecosystem := range slices.Sorted(maps.Keys(ecosystemDefaultDomains)) { + for _, host := range ecosystemDefaultDomains[ecosystem] { + key := strings.ToLower(host) + if _, allowed := knownProvenanceCopies[key]; allowed { + continue + } + if where, ok := always[key]; ok { + assert.Failf(t, "redundant entry", + "%q is in ecosystem %q and also in %s; the union applies both to every job", + host, ecosystem, where) + } + } + } + + // An exact host that an existing leading-dot or glob entry already covers is + // dead weight: it can be deleted with no behaviour change, and its presence + // falsely implies the namespace is not already open. + var patterns []string + for _, host := range allDefaultDomains() { + if strings.HasPrefix(host, ".") || isGlobPattern(host) { + patterns = append(patterns, host) + } + } + for _, host := range allDefaultDomains() { + if strings.HasPrefix(host, ".") || isGlobPattern(host) { + continue + } + for _, pattern := range patterns { + assert.Falsef(t, hostMatchesAllowlistEntry(host, pattern), + "exact entry %q is already covered by %q; remove the redundant entry "+ + "(or, if %q must stay exact, narrow %q)", host, pattern, host, pattern) + } + } +} + +// allDefaultDomains returns every host listed anywhere in the defaults file. +func allDefaultDomains() []string { + hosts := slices.Clone(githubInfraDomains) + hosts = append(hosts, sharedRegistryDomains...) + for _, ecosystem := range slices.Sorted(maps.Keys(ecosystemDefaultDomains)) { + hosts = append(hosts, ecosystemDefaultDomains[ecosystem]...) + } + return hosts +}