diff --git a/.github/skills/add-egress-allowlist-domain/SKILL.md b/.github/skills/add-egress-allowlist-domain/SKILL.md index 1a9cddd..c6f355f 100644 --- a/.github/skills/add-egress-allowlist-domain/SKILL.md +++ b/.github/skills/add-egress-allowlist-domain/SKILL.md @@ -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.` — 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//...`), 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. diff --git a/internal/handlers/egress_allowlist.go b/internal/handlers/egress_allowlist.go index 74c535e..17cb401 100644 --- a/internal/handlers/egress_allowlist.go +++ b/internal/handlers/egress_allowlist.go @@ -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 } @@ -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, } } @@ -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 @@ -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; diff --git a/internal/handlers/egress_allowlist_defaults.go b/internal/handlers/egress_allowlist_defaults.go index 2de1bed..9fd1c9a 100644 --- a/internal/handlers/egress_allowlist_defaults.go +++ b/internal/handlers/egress_allowlist_defaults.go @@ -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 == "" { @@ -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) + } + 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") + } + if path.Clean(prefix)+"/" != prefix { + return fmt.Errorf("must not contain dot segments") } return nil } diff --git a/internal/handlers/egress_allowlist_defaults.yaml b/internal/handlers/egress_allowlist_defaults.yaml index 65d0bb2..c610abc 100644 --- a/internal/handlers/egress_allowlist_defaults.yaml +++ b/internal/handlers/egress_allowlist_defaults.yaml @@ -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. @@ -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 diff --git a/internal/handlers/egress_dynamic_hosts.go b/internal/handlers/egress_dynamic_hosts.go index 4816e8d..83ca3a9 100644 --- a/internal/handlers/egress_dynamic_hosts.go +++ b/internal/handlers/egress_dynamic_hosts.go @@ -4,6 +4,7 @@ import ( "fmt" "net/url" "regexp" + "slices" "strings" "github.com/dependabot/proxy/internal/config" @@ -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//..."), 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 @@ -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) diff --git a/internal/handlers/egress_dynamic_hosts_test.go b/internal/handlers/egress_dynamic_hosts_test.go index 5c12a2b..b7cf5f7 100644 --- a/internal/handlers/egress_dynamic_hosts_test.go +++ b/internal/handlers/egress_dynamic_hosts_test.go @@ -1,6 +1,8 @@ package handlers import ( + "net/http" + "net/http/httptest" "testing" "github.com/stretchr/testify/assert" @@ -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) + } +}