Skip to content

Add missing public infra domains to egress allowlist - #276

Merged
v-abhishekbhaskar merged 1 commit into
mainfrom
abhishekbhaskar/add-missing-public-infra-domains
Sep 29, 2026
Merged

v-abhishekbhaskar merged 1 commit into
mainfrom
abhishekbhaskar/add-missing-public-infra-domains

Conversation

@v-abhishekbhaskar

Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

Expands the egress allowlist using traffic recorded during the proxy-egress-enforce rollout, and adds one dynamic-derivation rule so a large class of blocks is handled without static entries.

Static hosts added (all anonymous, provider-controlled package infrastructure):

  • NuGet — data.nuget.org
  • Maven/Gradle — maven-central.storage.googleapis.com, maven-central.storage-download.googleapis.com, repo.osgeo.org, androidx.dev, packages.atlassian.com, maven.artifacts.atlassian.com
  • Julia — julialang-s3.julialang.org
  • npm/Bun — pkg.pr.new, unofficial-builds.nodejs.org
  • Docker — docker.elastic.co, docker-auth.elastic.co, docker-registry-production.d24a988e385e0074d717b6bdaea58f0d.r2.cloudflarestorage.com, docker.getcollate.io
  • Shared — mirrors.huaweicloud.com

Also moves nodejs.org from go_modules to npm_and_yarn (organizational only — the handler applies the union), and derives ECR prod-<region>-starport-layer-bucket hosts per job from the credential set instead of allowlisting ~30 regional buckets.

Fixes #264. Fixes #275. Fixes dependabot/dependabot-core#16413.

Anything you want to highlight for special attention from reviewers?

Redirect chains, not just reported hosts. Both #264 and the Atlassian entry needed a host the issue never mentioned: Elastic's blobs 307 to Cloudflare R2, and packages.atlassian.com 301s to maven.artifacts.atlassian.com. Allowlisting only the reported host fixes metadata and still fails the download. Egress telemetry only records the first hop, so it systematically under-reports.

ECR is derived, not globbed. prod-*-starport-layer-bucket.s3.*.amazonaws.com is directly exploitable, not just theoretically risky: prod-evil-starport-layer-bucket returns NoSuchBucket today, so an attacker could claim it. Regions are interpolated from the job's own ECR credentials and matched exactly.

One entry was dropped on review. f.feedz.io was in the original triage but is structurally identical to dl.cloudsmith.io, which this file deliberately excludes: shared host, tenant in the URL path, and the allowlist authorizes hostname only. Added a test pinning Cloudsmith closed so the precedent is enforced rather than only commented.

pkg.pr.new is a judgment call. Same shape (namespace in path), but no per-tenant request logs — the specific mechanism the Cloudsmith note relies on. Documented inline; happy to drop it if reviewers disagree.

Header amended. The file said virtual-hosted <bucket>.storage.googleapis.com subdomains "stay blocked", which maven-central.* contradicts. Reworded to "blocked as a class, added one exact registered bucket at a time".

data.nuget.org 404s on every path. It's decommissioned Microsoft infrastructure (cert CN=api.nuget.org) still hit by older clients. Allowlisting converts a hard proxy block into a 404 the client already handles — the same failure mode as the pnpm issue.

How will you know you've accomplished your goal?

Every host was verified on the wire before being added — anonymous fetch of a real artifact, following redirects, with ownership confirmed via TLS cert or the api.github.com/meta domains.packages list.

New tests: TestEgressAllowlist_PublicEcosystemMirrorsAllowed, TestEgressAllowlist_PublicVendorOCIRegistriesAllowed, TestEgressAllowlist_NodeRuntimeDownloadsAllowed, and three TestRegistryRedirectHosts_* cases.

Each new entry has a child probe (evil.<host>) and, where a parent namespace is involved, a sibling probe, so the exact-match semantics fail loudly if an entry is ever widened. Mutation-verified: widening maven-central to a leading dot fails two tests; dropping the Atlassian redirect target fails one; re-adding dl.cloudsmith.io fails two.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@v-abhishekbhaskar v-abhishekbhaskar self-assigned this Sep 28, 2026
Copilot AI balanced review requested due to automatic review settings September 28, 2026 23:56
@v-abhishekbhaskar
v-abhishekbhaskar requested a review from a team as a code owner September 28, 2026 23:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Security-sensitive allowlist expansion, particularly the documented multi-tenant exception, warrants final human validation.

Review effort: Balanced
Findings: None

What changed in this PR

Expands proxy egress support for public package infrastructure while safely deriving private ECR storage redirects.

Changes:

  • Adds exact public registry, mirror, CDN, and runtime hosts.
  • Derives regional ECR Starport bucket hosts from credentials.
  • Adds allow/block boundary tests for all new behavior.
File Description
internal/​handlers/​egress_dynamic_hosts.go Derives ECR redirect hosts.
internal/​handlers/​egress_dynamic_hosts_test.go Tests safe ECR derivation.
internal/​handlers/​egress_allowlist_test.go Tests new hosts and isolation boundaries.
internal/​handlers/​egress_allowlist_defaults.yaml Adds public infrastructure domains.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@v-abhishekbhaskar
v-abhishekbhaskar merged commit 90ee7d2 into main Sep 29, 2026
111 of 112 checks passed
@v-abhishekbhaskar
v-abhishekbhaskar deleted the abhishekbhaskar/add-missing-public-infra-domains branch September 29, 2026 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants