From 9b1ae30543128219dcccaa000362fff350fb959a Mon Sep 17 00:00:00 2001 From: wickedOne Date: Wed, 26 Aug 2026 18:57:01 +0200 Subject: [PATCH 1/2] Stop publishing the fetch URL as a circuit breaker identifier --- fetch/circuit_breaker.go | 31 +++++++++--- fetch/circuit_breaker_test.go | 94 +++++++++++++++++++++++++++++++++-- 2 files changed, 112 insertions(+), 13 deletions(-) diff --git a/fetch/circuit_breaker.go b/fetch/circuit_breaker.go index 1ca06e4..6cc0632 100644 --- a/fetch/circuit_breaker.go +++ b/fetch/circuit_breaker.go @@ -2,6 +2,8 @@ package fetch import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" "net/http" @@ -18,7 +20,12 @@ const ( cbInitialInterval = 30 * time.Second cbMaxInterval = 5 * time.Minute cbThreshold = 5 - maxURLTruncate = 50 + + // hostlessRegistryPrefix labels the identifier used for a URL with no host + // to group by, and hostlessRegistryDigest is how many hex characters of the + // URL's digest follow it. See extractRegistry. + hostlessRegistryPrefix = "hostless-url-" + hostlessRegistryDigest = 12 ) // CircuitBreakerFetcher wraps a Fetcher with per-registry circuit breakers. @@ -179,16 +186,24 @@ func breakerError(registry string, err error) error { return err } -// extractRegistry extracts a registry identifier from a URL for circuit breaker grouping. +// extractRegistry extracts a registry identifier from a URL for circuit breaker +// grouping. +// +// The identifier travels further than the breaker map: breakerError names it in +// the error the caller sees, and GetBreakerState hands it to callers that +// publish it (git-pkgs/proxy reports it in /health and as a Prometheus label, +// both unauthenticated). So a URL with no host to group by cannot fall back to +// the URL itself. Fetch URLs are not all configuration: composer takes them +// from a package's dist.url and helm from the chart URLs in index.yaml, so one +// that carries no host can hold a signed-URL token or an internal address, and +// returning it verbatim disclosed that. Group those under a digest of the URL +// instead — opaque, yet still one breaker per distinct URL, which is as close +// to per-host grouping as a hostless URL allows. func extractRegistry(rawURL string) string { - // Parse URL and extract host for circuit breaker grouping parsed, err := url.Parse(rawURL) if err != nil || parsed.Host == "" { - // Fallback to simple truncation - if len(rawURL) > maxURLTruncate { - return rawURL[:50] - } - return rawURL + digest := sha256.Sum256([]byte(rawURL)) + return hostlessRegistryPrefix + hex.EncodeToString(digest[:])[:hostlessRegistryDigest] } return parsed.Host } diff --git a/fetch/circuit_breaker_test.go b/fetch/circuit_breaker_test.go index 8bae4b9..4fcc4c9 100644 --- a/fetch/circuit_breaker_test.go +++ b/fetch/circuit_breaker_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -101,11 +102,6 @@ func TestExtractRegistry(t *testing.T) { url: "https://files.pythonhosted.org/packages/abc/def/file.tar.gz", expected: "files.pythonhosted.org", }, - { - name: "invalid URL", - url: "not-a-valid-url", - expected: "not-a-valid-url", - }, { name: "long URL", url: "https://very-long-hostname.example.com/path", @@ -128,6 +124,94 @@ func TestExtractRegistry(t *testing.T) { } } +// TestExtractRegistryHostlessURL covers the URLs that have no host to group by. +// The identifier for those used to be the raw URL, truncated to 50 characters, +// which disclosed whatever the URL carried: identifiers reach unauthenticated +// endpoints (git-pkgs/proxy reports them in /health and as a Prometheus label) +// and client-visible errors, while composer and helm take fetch URLs from +// upstream metadata rather than from configuration. So assert on what the +// identifier must not reveal, plus the grouping the breakers still need. +func TestExtractRegistryHostlessURL(t *testing.T) { + const secret = "top-secret-signing-token" + + tests := []struct { + name string + url string + }{ + { + name: "no scheme or host", + url: "signed-token=" + secret, + }, + { + name: "path only", + url: "/packages/pkg-1.0.0.tgz?sig=" + secret, + }, + { + name: "scheme without host", + url: "file:///srv/internal/" + secret + "/pkg-1.0.0.tgz", + }, + { + name: "unparsable", + url: "https://internal host.invalid/pkg.tgz?sig=" + secret, + }, + } + + identifiers := make(map[string]string, len(tests)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractRegistry(tt.url) + + if strings.Contains(got, secret) { + t.Errorf("extractRegistry(%q) = %q, which discloses the URL", tt.url, got) + } + want := len(hostlessRegistryPrefix) + hostlessRegistryDigest + if !strings.HasPrefix(got, hostlessRegistryPrefix) || len(got) != want { + t.Errorf("extractRegistry(%q) = %q, want %q followed by %d hex characters", + tt.url, got, hostlessRegistryPrefix, hostlessRegistryDigest) + } + if again := extractRegistry(tt.url); again != got { + t.Errorf("extractRegistry(%q) is unstable: %q then %q", tt.url, got, again) + } + if other, ok := identifiers[got]; ok { + t.Errorf("extractRegistry(%q) = %q, already used for %q: these URLs share a breaker", + tt.url, got, other) + } + identifiers[got] = tt.url + }) + } +} + +// TestGetBreakerStateHostlessURLHidesURL is the end-to-end form of +// TestExtractRegistryHostlessURL: a hostless URL that fails often enough to +// trip its breaker must not put the URL into the state map, which is what +// callers publish. +func TestGetBreakerStateHostlessURLHidesURL(t *testing.T) { + const secret = "top-secret-signing-token" + artifactURL := "signed-token=" + secret + + cbFetcher := NewCircuitBreakerFetcher(NewFetcher()) + + ctx := context.Background() + for range cbThreshold { + if _, err := cbFetcher.Fetch(ctx, artifactURL); err == nil { + t.Fatalf("Fetch(%q) succeeded, want an error", artifactURL) + } + } + + states := cbFetcher.GetBreakerState() + if len(states) != 1 { + t.Fatalf("GetBreakerState() = %v, want one entry", states) + } + for registry, state := range states { + if strings.Contains(registry, secret) { + t.Errorf("GetBreakerState() key %q discloses the fetch URL", registry) + } + if state != "open" { + t.Errorf("GetBreakerState()[%q] = %q, want open after %d failures", registry, state, cbThreshold) + } + } +} + func TestGetBreakerState(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) From d00dda9b5428a97c052ea2b7150b27ecaa49dc97 Mon Sep 17 00:00:00 2001 From: Wicliff Wolda Date: Thu, 27 Aug 2026 16:50:28 +0200 Subject: [PATCH 2/2] Key the hostless circuit breaker identifier The identifier for a URL with no host was 48 bits of unkeyed SHA-256, which anyone reading /health or the Prometheus labels could match against digests of guessed URLs, and roughly 2^24 chosen inputs sufficed to collide two distinct URLs onto one breaker. Derive it from an HMAC under a per-process random key instead, and keep 128 bits of it. Co-Authored-By: Claude Opus 5 (1M context) --- fetch/circuit_breaker.go | 40 +++++++++++++++++++++++------------ fetch/circuit_breaker_test.go | 25 +++++++++++++--------- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/fetch/circuit_breaker.go b/fetch/circuit_breaker.go index 6cc0632..a8cd24c 100644 --- a/fetch/circuit_breaker.go +++ b/fetch/circuit_breaker.go @@ -2,6 +2,8 @@ package fetch import ( "context" + "crypto/hmac" + "crypto/rand" "crypto/sha256" "encoding/hex" "errors" @@ -23,11 +25,23 @@ const ( // hostlessRegistryPrefix labels the identifier used for a URL with no host // to group by, and hostlessRegistryDigest is how many hex characters of the - // URL's digest follow it. See extractRegistry. + // URL's keyed digest follow it. See extractRegistry. hostlessRegistryPrefix = "hostless-url-" - hostlessRegistryDigest = 12 + hostlessRegistryDigest = 32 ) +// hostlessRegistryKey keys the digest that extractRegistry falls back to. It is +// drawn once per process, so identifiers stay stable for as long as the breakers +// they name, while nobody outside the process can reproduce a digest: neither to +// match an identifier against a guessed URL, nor to pick inputs that collide +// onto one breaker. +var hostlessRegistryKey = sync.OnceValue(func() []byte { + key := make([]byte, sha256.BlockSize) + // Read fills key or panics; it never returns a short read. + _, _ = rand.Read(key) + return key +}) + // CircuitBreakerFetcher wraps a Fetcher with per-registry circuit breakers. type CircuitBreakerFetcher struct { fetcher *Fetcher @@ -189,21 +203,19 @@ func breakerError(registry string, err error) error { // extractRegistry extracts a registry identifier from a URL for circuit breaker // grouping. // -// The identifier travels further than the breaker map: breakerError names it in -// the error the caller sees, and GetBreakerState hands it to callers that -// publish it (git-pkgs/proxy reports it in /health and as a Prometheus label, -// both unauthenticated). So a URL with no host to group by cannot fall back to -// the URL itself. Fetch URLs are not all configuration: composer takes them -// from a package's dist.url and helm from the chart URLs in index.yaml, so one -// that carries no host can hold a signed-URL token or an internal address, and -// returning it verbatim disclosed that. Group those under a digest of the URL -// instead — opaque, yet still one breaker per distinct URL, which is as close -// to per-host grouping as a hostless URL allows. +// Identifiers are externally observable: they name the registry in the errors +// callers see and are the keys of GetBreakerState, which callers publish. Fetch +// URLs, meanwhile, may hold secrets, since not all of them come from +// configuration. So a URL with no host to group by cannot fall back to the URL +// itself; it is grouped under a keyed digest of the URL, which discloses +// nothing yet still gives one breaker per distinct URL. func extractRegistry(rawURL string) string { parsed, err := url.Parse(rawURL) if err != nil || parsed.Host == "" { - digest := sha256.Sum256([]byte(rawURL)) - return hostlessRegistryPrefix + hex.EncodeToString(digest[:])[:hostlessRegistryDigest] + mac := hmac.New(sha256.New, hostlessRegistryKey()) + // Write never returns an error, as hash.Hash documents. + _, _ = mac.Write([]byte(rawURL)) + return hostlessRegistryPrefix + hex.EncodeToString(mac.Sum(nil))[:hostlessRegistryDigest] } return parsed.Host } diff --git a/fetch/circuit_breaker_test.go b/fetch/circuit_breaker_test.go index 4fcc4c9..cda4f6b 100644 --- a/fetch/circuit_breaker_test.go +++ b/fetch/circuit_breaker_test.go @@ -2,6 +2,8 @@ package fetch import ( "context" + "crypto/sha256" + "encoding/hex" "io" "net/http" "net/http/httptest" @@ -125,12 +127,9 @@ func TestExtractRegistry(t *testing.T) { } // TestExtractRegistryHostlessURL covers the URLs that have no host to group by. -// The identifier for those used to be the raw URL, truncated to 50 characters, -// which disclosed whatever the URL carried: identifiers reach unauthenticated -// endpoints (git-pkgs/proxy reports them in /health and as a Prometheus label) -// and client-visible errors, while composer and helm take fetch URLs from -// upstream metadata rather than from configuration. So assert on what the -// identifier must not reveal, plus the grouping the breakers still need. +// Identifiers are externally observable and a fetch URL may carry a secret, so +// assert on what the identifier must not reveal, along with the grouping the +// breakers still need. func TestExtractRegistryHostlessURL(t *testing.T) { const secret = "top-secret-signing-token" @@ -169,6 +168,11 @@ func TestExtractRegistryHostlessURL(t *testing.T) { t.Errorf("extractRegistry(%q) = %q, want %q followed by %d hex characters", tt.url, got, hostlessRegistryPrefix, hostlessRegistryDigest) } + unkeyed := sha256.Sum256([]byte(tt.url)) + if got == hostlessRegistryPrefix+hex.EncodeToString(unkeyed[:])[:hostlessRegistryDigest] { + t.Errorf("extractRegistry(%q) = %q, a digest that anyone can recompute from a guessed URL", + tt.url, got) + } if again := extractRegistry(tt.url); again != got { t.Errorf("extractRegistry(%q) is unstable: %q then %q", tt.url, got, again) } @@ -182,9 +186,9 @@ func TestExtractRegistryHostlessURL(t *testing.T) { } // TestGetBreakerStateHostlessURLHidesURL is the end-to-end form of -// TestExtractRegistryHostlessURL: a hostless URL that fails often enough to -// trip its breaker must not put the URL into the state map, which is what -// callers publish. +// TestExtractRegistryHostlessURL: a hostless URL that fails often enough to trip +// its breaker must not put the URL into the state map that callers publish, and +// every one of those failures must land on the same breaker. func TestGetBreakerStateHostlessURLHidesURL(t *testing.T) { const secret = "top-secret-signing-token" artifactURL := "signed-token=" + secret @@ -200,7 +204,8 @@ func TestGetBreakerStateHostlessURLHidesURL(t *testing.T) { states := cbFetcher.GetBreakerState() if len(states) != 1 { - t.Fatalf("GetBreakerState() = %v, want one entry", states) + t.Fatalf("GetBreakerState() = %v, want one entry: the identifier has to be stable "+ + "within a fetcher for the breaker to count failures", states) } for registry, state := range states { if strings.Contains(registry, secret) {