diff --git a/fetch/circuit_breaker.go b/fetch/circuit_breaker.go index 1ca06e4..a8cd24c 100644 --- a/fetch/circuit_breaker.go +++ b/fetch/circuit_breaker.go @@ -2,6 +2,10 @@ package fetch import ( "context" + "crypto/hmac" + "crypto/rand" + "crypto/sha256" + "encoding/hex" "errors" "fmt" "net/http" @@ -18,9 +22,26 @@ 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 keyed digest follow it. See extractRegistry. + hostlessRegistryPrefix = "hostless-url-" + 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 @@ -179,16 +200,22 @@ 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. +// +// 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 { - // 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 + 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 8bae4b9..cda4f6b 100644 --- a/fetch/circuit_breaker_test.go +++ b/fetch/circuit_breaker_test.go @@ -2,9 +2,12 @@ package fetch import ( "context" + "crypto/sha256" + "encoding/hex" "io" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -101,11 +104,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 +126,97 @@ func TestExtractRegistry(t *testing.T) { } } +// TestExtractRegistryHostlessURL covers the URLs that have no host to group by. +// 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" + + 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) + } + 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) + } + 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 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 + + 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: 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) { + 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"))