Stop publishing the fetch URL as a circuit breaker identifier

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
 }

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"))