From c7e5c7a7c507a7aec6b6cd73d36b20023fff438e Mon Sep 17 00:00:00 2001 From: Jeroen van Erp Date: Mon, 20 Jul 2026 16:39:44 +0200 Subject: [PATCH] fix: plain-http enforces use of http on bearer token fetch (#677) (#678) Co-authored-by: Adam Martin --- cmd/hauler/cli/store/copy.go | 2 +- pkg/content/registry.go | 47 +++++++++++++++- pkg/content/registry_test.go | 106 +++++++++++++++++++++++++++++++++-- 3 files changed, 146 insertions(+), 9 deletions(-) diff --git a/cmd/hauler/cli/store/copy.go b/cmd/hauler/cli/store/copy.go index a3b50e9..6f2f2d1 100644 --- a/cmd/hauler/cli/store/copy.go +++ b/cmd/hauler/cli/store/copy.go @@ -181,7 +181,7 @@ func CopyCmd(ctx context.Context, o *flags.CopyOpts, s *store.Layout, targetRef Insecure: o.Insecure, } // Shared across every per-artifact RegistryTarget below to keep connections pooled. - registryClient := content.NewRegistryHTTPClient(registryOpts) + registryClient := content.NewRegistryHTTPClient(components[1], registryOpts) // Pre-build a map from base ref → image manifest digest so that sig/att/sbom // descriptors (which store the base image ref, not the cosign tag) can be routed diff --git a/pkg/content/registry.go b/pkg/content/registry.go index 63d996b..e4b148d 100644 --- a/pkg/content/registry.go +++ b/pkg/content/registry.go @@ -23,14 +23,50 @@ type RegistryTarget struct { resolver remotes.Resolver } +// plainHTTPRoundTripper rewrites outgoing https:// requests to http:// for a +// single registry authority (host:port). This is required when PlainHTTP is +// set: the Docker authorizer follows the Bearer realm URL from the +// WWW-Authenticate header literally, and registries like Harbor always +// advertise an https:// realm regardless of the incoming transport, so the +// token fetch fails with "server gave HTTP response to HTTPS client" unless +// the scheme is rewritten before the request leaves the client. +// +// The rewrite is scoped to the registry's exact authority on purpose: a +// plain-http registry may legitimately 301-redirect blob fetches to a real +// HTTPS object store or CDN on a different host (or even a different port on +// the same host), and those must NOT be downgraded. host must already be a +// bare authority (no path) -- NewRegistryHTTPClient strips any path before +// building this struct, since req.URL.Host is never anything but the authority. +type plainHTTPRoundTripper struct { + inner http.RoundTripper + host string // registry authority (host:port), the only host we downgrade +} + +func (r plainHTTPRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + if req.URL.Scheme == "https" && req.URL.Host == r.host { + reqCopy := req.Clone(req.Context()) + reqCopy.URL.Scheme = "http" + req = reqCopy + } + return r.inner.RoundTrip(req) +} + // NewRegistryHTTPClient builds an *http.Client configured for opts, cloning // http.DefaultTransport rather than mutating it in place, which would leak // InsecureSkipVerify into every other HTTP client in the process. // +// host is the registry this client talks to (e.g. "localhost:5000"). Callers +// such as cmd/hauler/cli/store/copy.go derive it from a target reference's +// remainder after "://", so it may arrive as "host:port/repo/path"; any path +// is stripped to the bare authority before use. When opts.PlainHTTP is set, +// that authority scopes a targeted https->http rewrite (see +// plainHTTPRoundTripper) so a legitimate cross-host https redirect (e.g. to a +// CDN or object store) is not downgraded. +// // Build this once and share it across all RegistryTargets for a copy: a // transport per target defeats connection pooling and can exhaust file // descriptors on large copies. -func NewRegistryHTTPClient(opts RegistryOptions) *http.Client { +func NewRegistryHTTPClient(host string, opts RegistryOptions) *http.Client { var transport *http.Transport if dt, ok := http.DefaultTransport.(*http.Transport); ok { transport = dt.Clone() @@ -41,7 +77,14 @@ func NewRegistryHTTPClient(opts RegistryOptions) *http.Client { if opts.Insecure { transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} } - return &http.Client{Transport: transport} + var rt http.RoundTripper = transport + if opts.PlainHTTP { + // host may arrive as "registry:port/repo/path" (copy.go passes + // components[1]); req.URL.Host is only ever the authority, so match on that. + authority, _, _ := strings.Cut(host, "/") + rt = plainHTTPRoundTripper{inner: transport, host: authority} + } + return &http.Client{Transport: rt} } // NewRegistryTarget returns a RegistryTarget that pushes to host (e.g. "localhost:5000"). diff --git a/pkg/content/registry_test.go b/pkg/content/registry_test.go index 327325f..83cef38 100644 --- a/pkg/content/registry_test.go +++ b/pkg/content/registry_test.go @@ -13,6 +13,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" ) @@ -30,7 +31,7 @@ func TestNewRegistryTarget_InsecureSkipsTLSVerification(t *testing.T) { host := strings.TrimPrefix(srv.URL, "https://") opts := RegistryOptions{Insecure: true} - target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(opts)) + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(host, opts)) _, err := target.Resolve(context.Background(), host+"/library/test:latest") if err == nil { @@ -53,8 +54,17 @@ func TestNewRegistryTarget_InsecureSkipsTLSVerification(t *testing.T) { // HTTPS endpoint signed by a private CA. With --insecure --plain-http, // hauler should dial http, follow the redirect to https, and skip cert // verification on the redirected request. +// +// The two httptest servers both listen on 127.0.0.1, on different ports, so +// this also proves that the PlainHTTP https->http rewrite is scoped +// precisely enough to leave the redirect target's scheme alone: a blanket +// rewrite of every outgoing https request (as opposed to one scoped to the +// registry's own host:port) would downgrade this redirect to http and the +// TLS-only redirect target would never be reached. func TestNewRegistryTarget_InsecurePlainHTTPFollowsHTTPSRedirect(t *testing.T) { + var tlsReached atomic.Bool tlsSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsReached.Store(true) w.WriteHeader(http.StatusNotFound) })) defer tlsSrv.Close() @@ -67,7 +77,7 @@ func TestNewRegistryTarget_InsecurePlainHTTPFollowsHTTPSRedirect(t *testing.T) { host := strings.TrimPrefix(httpSrv.URL, "http://") opts := RegistryOptions{Insecure: true, PlainHTTP: true} - target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(opts)) + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(host, opts)) _, err := target.Resolve(context.Background(), host+"/library/test:latest") if err == nil { @@ -77,6 +87,90 @@ func TestNewRegistryTarget_InsecurePlainHTTPFollowsHTTPSRedirect(t *testing.T) { if strings.Contains(lower, "certificate signed by unknown authority") || strings.Contains(lower, "certificate") { t.Fatalf("expected no certificate verification error after following http->https redirect with Insecure: true, got: %v", err) } + if !tlsReached.Load() { + t.Fatal("plain-http downgraded a cross-host https redirect: TLS endpoint was never reached") + } +} + +// TestNewRegistryTarget_PlainHTTPRewritesBearerTokenFetchScheme reproduces +// issue #677: a plain-http Bearer-auth registry (e.g. Harbor) that always +// advertises an https:// realm in its WWW-Authenticate challenge, even +// though the registry itself is only reachable over plain http. The token +// realm is on the SAME host:port as the registry (Harbor's own token +// service is co-located behind the same reverse proxy), which is what makes +// the host-scoped rewrite in plainHTTPRoundTripper apply here. Before the +// fix, the containerd Docker authorizer dialed the https realm literally +// and failed with "server gave HTTP response to HTTPS client". +func TestNewRegistryTarget_PlainHTTPRewritesBearerTokenFetchScheme(t *testing.T) { + var registrySrv *httptest.Server + registrySrv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/service/token" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"token":"fake-token"}`)) + return + } + realm := "https://" + strings.TrimPrefix(registrySrv.URL, "http://") + "/service/token" + w.Header().Set("WWW-Authenticate", `Bearer realm="`+realm+`",service="registry"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer registrySrv.Close() + + host := strings.TrimPrefix(registrySrv.URL, "http://") + + opts := RegistryOptions{PlainHTTP: true} + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(host, opts)) + + _, err := target.Resolve(context.Background(), host+"/library/test:latest") + if err == nil { + t.Fatalf("expected an error resolving against a 401-only fake registry, got nil") + } + lower := strings.ToLower(err.Error()) + if strings.Contains(lower, "server gave http response to https client") { + t.Fatalf("plain-http Bearer token fetch dialed the https realm literally instead of being rewritten to http, got: %v", err) + } +} + +// TestNewRegistryTarget_PlainHTTPRewritesBearerTokenFetchScheme_PathBearingHost +// reproduces the real call path used by `hauler store copy registry://`: +// cmd/hauler/cli/store/copy.go derives its host argument from +// strings.SplitN(targetRef, "://", 2)[1], which for a target reference like +// "oci://harbor:80/library" is "harbor:80/library" -- host:port WITH the +// repo path still attached, not a clean authority. NewRegistryHTTPClient +// must normalize that down to just the authority before comparing against +// req.URL.Host (which is never anything but the authority), or the +// plainHTTPRoundTripper rewrite silently never fires and #677 recurs in +// production even though the "clean host" tests above pass. +func TestNewRegistryTarget_PlainHTTPRewritesBearerTokenFetchScheme_PathBearingHost(t *testing.T) { + var registrySrv *httptest.Server + registrySrv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/service/token" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"token":"fake-token"}`)) + return + } + realm := "https://" + strings.TrimPrefix(registrySrv.URL, "http://") + "/service/token" + w.Header().Set("WWW-Authenticate", `Bearer realm="`+realm+`",service="registry"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer registrySrv.Close() + + hostPort := strings.TrimPrefix(registrySrv.URL, "http://") + // Mirrors copy.go's components[1]: host:port with a repo path attached. + componentsOne := hostPort + "/library" + + opts := RegistryOptions{PlainHTTP: true} + target := NewRegistryTarget(componentsOne, opts, NewRegistryHTTPClient(componentsOne, opts)) + + _, err := target.Resolve(context.Background(), hostPort+"/library/test:latest") + if err == nil { + t.Fatalf("expected an error resolving against a 401-only fake registry, got nil") + } + lower := strings.ToLower(err.Error()) + if strings.Contains(lower, "server gave http response to https client") { + t.Fatalf("plain-http Bearer token fetch dialed the https realm literally instead of being rewritten to http (path-bearing host arg like copy.go passes), got: %v", err) + } } // TestNewRegistryTarget_InsecureAppliesToBearerTokenFetch reproduces the case @@ -106,7 +200,7 @@ func TestNewRegistryTarget_InsecureAppliesToBearerTokenFetch(t *testing.T) { host := strings.TrimPrefix(registrySrv.URL, "https://") opts := RegistryOptions{Insecure: true} - target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(opts)) + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(host, opts)) _, err := target.Resolve(context.Background(), host+"/library/test:latest") if err == nil { @@ -164,7 +258,7 @@ func TestNewRegistryTarget_SchemeSelection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - target := NewRegistryTarget(host, tt.opts, NewRegistryHTTPClient(tt.opts)) + target := NewRegistryTarget(host, tt.opts, NewRegistryHTTPClient(host, tt.opts)) _, err := target.Resolve(context.Background(), host+"/library/test:latest") @@ -192,7 +286,7 @@ func TestNewRegistryHTTPClient_DoesNotLeakGlobalTLSConfig(t *testing.T) { } before := dt.TLSClientConfig - _ = NewRegistryHTTPClient(RegistryOptions{Insecure: true}) + _ = NewRegistryHTTPClient("registry.example.com", RegistryOptions{Insecure: true}) if dt.TLSClientConfig != before { t.Fatalf("NewRegistryHTTPClient mutated the global http.DefaultTransport.TLSClientConfig: before=%+v after=%+v", before, dt.TLSClientConfig) @@ -226,7 +320,7 @@ func TestNewRegistryHTTPClient_FallsBackWhenDefaultTransportIsNotHTTPTransport(t t.Fatalf("NewRegistryHTTPClient panicked with a non-*http.Transport DefaultTransport: %v", r) } }() - client = NewRegistryHTTPClient(RegistryOptions{Insecure: true}) + client = NewRegistryHTTPClient("registry.example.com", RegistryOptions{Insecure: true}) }() if client == nil {