Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/skills/add-egress-allowlist-domain/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ 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.<ecosystem>` — the normal case.
- `registry_redirect_derivations` — **not** a static section: the host is allowed only for jobs whose credentials name the matching registry. Use it when a configured registry redirects downloads to a provider-owned storage host that step 1 ruled out of the static sections. Entries are `credential_host` (exact, or leading dot for any subdomain) plus `derived` (exact hosts only; globs are rejected at startup).
- `registry_redirect_derivations` — **not** a static section: the host is allowed only for jobs whose credentials name the matching registry. Use it when a configured registry redirects downloads to a provider-owned storage host that step 1 ruled out of the static sections. Entries are `credential_host` (exact, or leading dot for any subdomain) plus `derived` (exact hosts only; globs are rejected at startup). If the redirect target is itself a shared host with the tenant in the path (e.g. path-style `s3.amazonaws.com/<bucket>/...`), use `derived_paths` (`host` plus a literal `path_prefix` such as `/bucket/`) instead of `derived`, so that only that prefix is allowed, and only for GET/HEAD.

Add the host to exactly one section. Every static section is applied to every job, so listing it twice is redundant, not safer.

Expand Down
22 changes: 21 additions & 1 deletion internal/handlers/egress_allowlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ type EgressAllowlistHandler struct {
// glob-free, so treating them as patterns (e.g. a configured "https://*.com")
// would silently disable enforcement for every matching destination.
dynamicHosts []string
// dynamicPaths are per-job host and path-prefix pairs derived from the job's
// credentials, for storage shared by many tenants on one hostname.
dynamicPaths []derivedPath
metrics MetricSender
}

Expand All @@ -71,6 +74,7 @@ func NewEgressAllowlistHandler(cfg *config.Config, env config.ProxyEnvSettings,
enforce: cfg.Experiments.Enabled(egressEnforceExperiment),
allowed: allowed,
dynamicHosts: dynamicHosts(cfg.Credentials),
dynamicPaths: registryRedirectPaths(credentialHosts(cfg.Credentials)),
metrics: metricSender,
}
}
Expand All @@ -86,7 +90,7 @@ func (h *EgressAllowlistHandler) HandleRequest(req *http.Request, proxyCtx *gopr
return req, nil
}

allowed := h.isAllowed(host)
allowed := h.isAllowed(host) || h.isPathAllowed(req, host)

// A request is actually dropped only when it is not allowlisted AND enforce
// mode is on; in observe-only mode a non-allowlisted host is logged but still
Expand Down Expand Up @@ -146,6 +150,22 @@ func (h *EgressAllowlistHandler) isAllowed(host string) bool {
return false
}

// isPathAllowed reports whether a download request matches one of the job's
// path-scoped redirect targets. Only GET and HEAD qualify: these targets serve
// pre-signed downloads, never uploads.
func (h *EgressAllowlistHandler) isPathAllowed(req *http.Request, host string) bool {
if req.Method != http.MethodGet && req.Method != http.MethodHead {
return false
}
host = strings.TrimSuffix(host, ".")
for _, p := range h.dynamicPaths {
if p.allows(host, req.URL) {
return true
}
}
return false
}

// hostMatchesAllowlistEntry reports whether host satisfies a single allowlist
// entry. An entry may be one of three forms:
// - leading-dot suffix (".github.com"): matches that domain and any subdomain;
Expand Down
28 changes: 26 additions & 2 deletions internal/handlers/egress_allowlist_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ func validateRegistryRedirectDerivations(derivations []registryRedirectDerivatio
if isGlobPattern(d.CredentialHost) {
return fmt.Errorf("credential_host %q must be an exact host or a leading-dot domain, not a glob", d.CredentialHost)
}
if len(d.Derived) == 0 {
return fmt.Errorf("credential_host %q has no derived hosts", d.CredentialHost)
if len(d.Derived) == 0 && len(d.DerivedPaths) == 0 {
return fmt.Errorf("credential_host %q has no derived hosts or paths", d.CredentialHost)
}
for _, host := range d.Derived {
if host == "" {
Expand All @@ -107,6 +107,30 @@ func validateRegistryRedirectDerivations(derivations []registryRedirectDerivatio
return fmt.Errorf("derived host %q (credential_host %q) must be an exact host: derived hosts are matched exactly", host, d.CredentialHost)
}
}
for _, p := range d.DerivedPaths {
if p.Host == "" || isGlobPattern(p.Host) || strings.HasPrefix(p.Host, ".") {
return fmt.Errorf("derived_paths host %q (credential_host %q) must be an exact host", p.Host, d.CredentialHost)
Comment on lines +111 to +112
}
if err := validatePathPrefix(p.PathPrefix); err != nil {
return fmt.Errorf("derived_paths path_prefix %q (credential_host %q): %w", p.PathPrefix, d.CredentialHost, err)
}
}
}
return nil
}

// validatePathPrefix requires a literal, normalized prefix of at least one whole
// path segment, such as "/bucket/". The trailing slash stops "/bucket" from also
// matching "/bucket-of-an-attacker".
func validatePathPrefix(prefix string) error {
if len(prefix) < 3 || !strings.HasPrefix(prefix, "/") || !strings.HasSuffix(prefix, "/") {
return fmt.Errorf("must start and end with \"/\" and name at least one segment")
}
if strings.ContainsAny(prefix, "%\\?#*[") || strings.Contains(prefix, "//") {
return fmt.Errorf("must be a literal, normalized path")
Comment on lines +129 to +130
}
if path.Clean(prefix)+"/" != prefix {
return fmt.Errorf("must not contain dot segments")
}
return nil
}
Expand Down
12 changes: 11 additions & 1 deletion internal/handlers/egress_allowlist_defaults.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ shared_registry_domains:
# forms are rejected at startup: dynamic hosts are matched
# exactly so that a crafted credential cannot widen them.
#
# derived_paths: optional {host, path_prefix} pairs for a shared host that
# must not be opened whole, e.g. path-style S3, where every
# bucket is a path on s3.amazonaws.com. The host is matched
# exactly and the request is allowed only for GET/HEAD under
# path_prefix, a literal "/segment/" ending in a slash.
#
# Every derived host is a constant. Targets whose name embeds a value taken from
# the credential (e.g. the private ECR layer bucket, which interpolates the
# region) stay in registryRedirectHosts in Go.
Expand All @@ -177,11 +183,15 @@ registry_redirect_derivations:
- d3fo0g5hm7lbuv.cloudfront.net
# Every Gemfury endpoint (pypi., npm., gem., ...) redirects downloads to
# pre-signed URLs on one Gemfury-owned bucket, reachable on both its dualstack
# and plain Transfer Acceleration endpoints.
# and plain Transfer Acceleration endpoints, and for some accounts as a
# path-style URL on the shared S3 endpoint (dependabot/proxy#308).
- credential_host: .fury.io
derived:
- gemfury.s3-accelerate.dualstack.amazonaws.com
- gemfury.s3-accelerate.amazonaws.com
derived_paths:
- host: s3.amazonaws.com
path_prefix: /gemfury/

# ecosystem_default_domains lists the public registry and CDN hosts each
# Dependabot ecosystem needs. The map is kept keyed by ecosystem for provenance
Expand Down
52 changes: 50 additions & 2 deletions internal/handlers/egress_dynamic_hosts.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"net/url"
"regexp"
"slices"
"strings"

"github.com/dependabot/proxy/internal/config"
Expand All @@ -25,8 +26,40 @@ var ecrHostPattern = regexp.MustCompile(`^[0-9]{12}\.dkr\.ecr\.([a-z0-9-]+)\.ama
type registryRedirectDerivation struct {
// CredentialHost matches exactly, or, with a leading dot, any subdomain of
// that domain but not the apex.
CredentialHost string `yaml:"credential_host"`
Derived []string `yaml:"derived"`
CredentialHost string `yaml:"credential_host"`
Derived []string `yaml:"derived"`
DerivedPaths []derivedPath `yaml:"derived_paths"`
}

// derivedPath allows one exact host, but only for GET/HEAD requests whose path
// sits under a fixed prefix. It exists for shared hosts such as path-style S3
// ("s3.amazonaws.com/<bucket>/..."), where allowing the whole host would open
// every tenant's bucket.
type derivedPath struct {
Host string `yaml:"host"`
PathPrefix string `yaml:"path_prefix"`
}

// allows reports whether u is a request for p.Host under p.PathPrefix. Both the
// escaped and decoded paths must carry the prefix, so an encoded slash
// ("/bucket%2F..%2Fother") cannot move the request into another bucket, and dot
// segments and backslashes are rejected in case either end normalizes them.
func (p derivedPath) allows(host string, u *url.URL) bool {
if !helpers.AreHostnamesEqual(host, p.Host) || u.Opaque != "" {
return false
}
if !strings.HasPrefix(u.EscapedPath(), p.PathPrefix) || !strings.HasPrefix(u.Path, p.PathPrefix) {
return false
}
if strings.Contains(u.Path, `\`) {
return false
}
for _, segment := range strings.Split(u.Path, "/") {
if segment == "." || segment == ".." {
return false
}
}
return true
}

// registryRedirectDerivations is loaded from the registry_redirect_derivations
Expand Down Expand Up @@ -62,6 +95,21 @@ func registryRedirectHosts(credHosts []string) []string {
return hosts
}

// registryRedirectPaths returns the path-scoped storage targets that the job's
// configured registries redirect downloads to.
func registryRedirectPaths(credHosts []string) []derivedPath {
var paths []derivedPath
for _, h := range credHosts {
h = strings.TrimSuffix(h, ".")
for _, derivation := range registryRedirectDerivations {
if derivation.matches(h) {
paths = append(paths, derivation.DerivedPaths...)
}
}
}
return slices.Compact(paths)
}

func (d registryRedirectDerivation) matches(host string) bool {
if strings.HasPrefix(d.CredentialHost, ".") {
return strings.HasSuffix(host, d.CredentialHost)
Expand Down
93 changes: 93 additions & 0 deletions internal/handlers/egress_dynamic_hosts_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package handlers

import (
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -288,3 +290,94 @@ func TestRegistryRedirectHosts_NotAddedWithoutGemfuryCredential(t *testing.T) {
"Gemfury storage must not be allowed for a job with no Gemfury credential: "+blocked)
}
}

func TestRegistryRedirectPaths_GemfuryPathStyleS3(t *testing.T) {
// Gemfury also redirects some downloads to its bucket in path-style form on
// the shared S3 endpoint (dependabot/proxy#308). Only that bucket's prefix
// may be reached, never the host as a whole.
h := newEgressHandlerWithCreds(config.Credentials{
{"type": "python_index", "index-url": "https://pypi.fury.io/acme/"},
})

for _, allowed := range []string{
"https://s3.amazonaws.com/gemfury/gems/abc123/pkg-1.0.0-py3-none-any.whl?x-acct=acme&X-Amz-Signature=x",
"https://s3.amazonaws.com:443/gemfury/gems/abc123/pkg-1.0.0%2Blocal-py3-none-any.whl",
"https://s3.amazonaws.com./gemfury/gems/abc123/pkg.whl",
} {
assert.Nil(t, egressResult(t, h, allowed), "Gemfury path-style download must be allowed: "+allowed)
}

for _, blocked := range []string{
// Other buckets and the bare host stay closed.
"https://s3.amazonaws.com/",
"https://s3.amazonaws.com/attacker/loot",
"https://s3.amazonaws.com/gemfury",
"https://s3.amazonaws.com/gemfuryx/loot",
"https://s3.amazonaws.com/gemfury-attacker/loot",
"https://s3.amazonaws.com/GEMFURY/loot",
// The prefix cannot be escaped with dot segments or encoded separators.
"https://s3.amazonaws.com/gemfury/../attacker/loot",
"https://s3.amazonaws.com/gemfury/%2e%2e/attacker/loot",
"https://s3.amazonaws.com/gemfury%2F..%2Fattacker/loot",
"https://s3.amazonaws.com/gemfury%2Floot",
"https://s3.amazonaws.com/gemfury/gems/..%5C..%5Cattacker/loot",
"https://s3.amazonaws.com/gemfury/./loot",
// Only the exact host gets the path rule.
"https://evil.s3.amazonaws.com/gemfury/loot",
"https://s3.us-east-1.amazonaws.com/gemfury/loot",
"https://s3.amazonaws.com.evil.com/gemfury/loot",
} {
assert.NotNil(t, egressResult(t, h, blocked), "must remain blocked: "+blocked)
}

for _, method := range []string{http.MethodPut, http.MethodPost, http.MethodDelete} {
req := httptest.NewRequestWithContext(t.Context(), method, "https://s3.amazonaws.com/gemfury/gems/x/loot", nil)
_, resp := h.HandleRequest(req, nil)
require.NotNil(t, resp, method+" must be blocked")
mustClose(resp.Body)
}
req := httptest.NewRequestWithContext(t.Context(), http.MethodHead, "https://s3.amazonaws.com/gemfury/gems/x/pkg.whl", nil)
_, resp := h.HandleRequest(req, nil)
assert.Nil(t, resp, "HEAD must be allowed")
}

func TestRegistryRedirectPaths_NotAddedWithoutGemfuryCredential(t *testing.T) {
h := newEgressHandlerWithCreds(config.Credentials{
{"type": "python_index", "index-url": "https://pypi.internal.example.com/simple"},
})
assert.NotNil(t, egressResult(t, h, "https://s3.amazonaws.com/gemfury/gems/x/loot"),
"Gemfury path-style storage must not be allowed for a job with no Gemfury credential")

assert.Empty(t, registryRedirectPaths([]string{"fury.io", "pypi.fury.io.evil.com", "evil.com"}))
assert.Equal(t,
[]derivedPath{{Host: "s3.amazonaws.com", PathPrefix: "/gemfury/"}},
registryRedirectPaths([]string{"pypi.fury.io", "npm.fury.io", "pypi.fury.io."}),
"several Gemfury credentials yield one path rule")
}

func TestValidateRegistryRedirectDerivations_PathPrefixes(t *testing.T) {
valid := registryRedirectDerivation{
CredentialHost: ".fury.io",
DerivedPaths: []derivedPath{{Host: "s3.amazonaws.com", PathPrefix: "/gemfury/"}},
}
assert.NoError(t, validateRegistryRedirectDerivations([]registryRedirectDerivation{valid}),
"derived_paths alone is a valid derivation")

for name, p := range map[string]derivedPath{
"empty host": {Host: "", PathPrefix: "/gemfury/"},
"glob host": {Host: "*.amazonaws.com", PathPrefix: "/gemfury/"},
"leading-dot host": {Host: ".amazonaws.com", PathPrefix: "/gemfury/"},
"empty prefix": {Host: "s3.amazonaws.com", PathPrefix: ""},
"root prefix": {Host: "s3.amazonaws.com", PathPrefix: "/"},
"no trailing slash": {Host: "s3.amazonaws.com", PathPrefix: "/gemfury"},
"no leading slash": {Host: "s3.amazonaws.com", PathPrefix: "gemfury/"},
"dot segment": {Host: "s3.amazonaws.com", PathPrefix: "/gemfury/../"},
"double slash": {Host: "s3.amazonaws.com", PathPrefix: "//gemfury/"},
"percent-encoded": {Host: "s3.amazonaws.com", PathPrefix: "/gem%66ury/"},
"glob in prefix": {Host: "s3.amazonaws.com", PathPrefix: "/gem*/"},
"backslash in prefix": {Host: "s3.amazonaws.com", PathPrefix: "/gem\\fury/"},
} {
d := registryRedirectDerivation{CredentialHost: ".fury.io", DerivedPaths: []derivedPath{p}}
assert.Errorf(t, validateRegistryRedirectDerivations([]registryRedirectDerivation{d}), "must reject %s", name)
}
}
Loading