diff --git a/cmd/hauler/cli/store/copy.go b/cmd/hauler/cli/store/copy.go index 360357b..9f3723f 100644 --- a/cmd/hauler/cli/store/copy.go +++ b/cmd/hauler/cli/store/copy.go @@ -180,6 +180,8 @@ func CopyCmd(ctx context.Context, o *flags.CopyOpts, s *store.Layout, targetRef PlainHTTP: o.PlainHTTP, Insecure: o.Insecure, } + // Shared across every per-artifact RegistryTarget below to keep connections pooled. + registryClient := content.NewRegistryHTTPClient(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 @@ -256,7 +258,7 @@ func CopyCmd(ctx context.Context, o *flags.CopyOpts, s *store.Layout, targetRef // so a shared tracker would mark shared blobs as "already exists" after // the first image, skipping the per-repository blob link creation that // Docker Distribution requires for manifest validation. - target := content.NewRegistryTarget(components[1], registryOpts) + target := content.NewRegistryTarget(components[1], registryOpts, registryClient) var pushed ocispec.Descriptor if err := retry.Operation(ctx, o.StoreRootOpts, ro, func() error { var copyErr error diff --git a/pkg/content/registry.go b/pkg/content/registry.go index edf8138..92a2f39 100644 --- a/pkg/content/registry.go +++ b/pkg/content/registry.go @@ -2,6 +2,7 @@ package content import ( "context" + "crypto/tls" "fmt" "net/http" "strings" @@ -22,9 +23,33 @@ type RegistryTarget struct { resolver remotes.Resolver } +// 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. +// +// 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 { + var transport *http.Transport + if dt, ok := http.DefaultTransport.(*http.Transport); ok { + transport = dt.Clone() + } else { + // Replaced by instrumentation or a test harness. + transport = &http.Transport{} + } + if opts.Insecure { + transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + } + return &http.Client{Transport: transport} +} + // NewRegistryTarget returns a RegistryTarget that pushes to host (e.g. "localhost:5000"). -func NewRegistryTarget(host string, opts RegistryOptions) *RegistryTarget { +// client must also back the authorizer: otherwise Bearer token fetches fall back to +// http.DefaultClient and ignore opts.Insecure. +func NewRegistryTarget(host string, opts RegistryOptions, client *http.Client) *RegistryTarget { authorizer := cdocker.NewDockerAuthorizer( + cdocker.WithAuthClient(client), cdocker.WithAuthCreds(func(h string) (string, string, error) { if opts.Username != "" { return opts.Username, opts.Password, nil @@ -52,11 +77,11 @@ func NewRegistryTarget(host string, opts RegistryOptions) *RegistryTarget { return nil, err } scheme := "https" - if opts.PlainHTTP || opts.Insecure { + if opts.PlainHTTP { scheme = "http" } return []cdocker.RegistryHost{{ - Client: http.DefaultClient, + Client: client, Authorizer: authorizer, Scheme: scheme, Host: host, diff --git a/pkg/content/registry_test.go b/pkg/content/registry_test.go new file mode 100644 index 0000000..327325f --- /dev/null +++ b/pkg/content/registry_test.go @@ -0,0 +1,242 @@ +package content + +// registry_test.go covers the TLS/scheme wiring in NewRegistryHTTPClient() +// and NewRegistryTarget(). It reproduces a v2 regression where +// RegistryOptions.Insecure was a dead field for TLS purposes (the resolver +// always used http.DefaultClient, which has no TLS configuration) and was +// also incorrectly conflated with PlainHTTP when selecting the http/https +// scheme, plus a follow-up regression where Insecure was wired into the +// registry client but not into the Docker Bearer-auth token-fetch client. + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestNewRegistryTarget_InsecureSkipsTLSVerification reproduces the case +// where a registry serves TLS with a self-signed certificate and the caller +// passes --insecure. Before the fix, the resolver's Client was +// http.DefaultClient (no TLS configuration), so the request would fail with +// a certificate verification error even though Insecure was set. +func TestNewRegistryTarget_InsecureSkipsTLSVerification(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + host := strings.TrimPrefix(srv.URL, "https://") + + opts := RegistryOptions{Insecure: true} + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(opts)) + + _, err := target.Resolve(context.Background(), host+"/library/test:latest") + if err == nil { + t.Fatalf("expected an error resolving against a fake registry that returns 404, got nil") + } + lower := strings.ToLower(err.Error()) + if strings.Contains(lower, "certificate") { + t.Fatalf("expected no certificate verification error with Insecure: true, got: %v", err) + } + // "http://" (not "https://") in the error means Insecure wrongly + // forced a plain-http dial against this TLS-only server -- the + // historical conflation of Insecure with PlainHTTP. + if strings.Contains(err.Error(), "http://") { + t.Fatalf("expected https scheme to be used (Insecure must not force plain http), got: %v", err) + } +} + +// TestNewRegistryTarget_InsecurePlainHTTPFollowsHTTPSRedirect reproduces the +// user's exact scenario: a registry that serves HTTP but 301-redirects to an +// 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. +func TestNewRegistryTarget_InsecurePlainHTTPFollowsHTTPSRedirect(t *testing.T) { + tlsSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer tlsSrv.Close() + + httpSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, tlsSrv.URL+r.URL.Path, http.StatusMovedPermanently) + })) + defer httpSrv.Close() + + host := strings.TrimPrefix(httpSrv.URL, "http://") + + opts := RegistryOptions{Insecure: true, PlainHTTP: true} + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(opts)) + + _, err := target.Resolve(context.Background(), host+"/library/test:latest") + if err == nil { + t.Fatalf("expected an error resolving against a fake registry that returns 404, got nil") + } + lower := strings.ToLower(err.Error()) + 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) + } +} + +// TestNewRegistryTarget_InsecureAppliesToBearerTokenFetch reproduces the case +// of a Bearer-auth registry (Harbor, Zot, Distribution + a token server) +// backed by a private CA. The registry responds 401 with a WWW-Authenticate +// Bearer challenge pointing at a token endpoint that is itself self-signed +// TLS. Before the fix, the resolver's TLS-aware client was only wired into +// RegistryHost.Client, never into the docker authorizer -- so the token +// fetch fell back to http.DefaultClient and failed cert verification even +// with Insecure: true. Basic-auth registries never exercise this 401 -> +// token-fetch path, which is why the other tests in this file didn't catch +// it. +func TestNewRegistryTarget_InsecureAppliesToBearerTokenFetch(t *testing.T) { + tokenSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"token":"fake-token"}`)) + })) + defer tokenSrv.Close() + + registrySrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer realm="`+tokenSrv.URL+`/token",service="registry"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer registrySrv.Close() + + host := strings.TrimPrefix(registrySrv.URL, "https://") + + opts := RegistryOptions{Insecure: true} + target := NewRegistryTarget(host, opts, NewRegistryHTTPClient(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, "certificate") { + t.Fatalf("expected the Bearer token fetch to honor Insecure and skip cert verification, got: %v", err) + } +} + +// TestNewRegistryTarget_SchemeSelection asserts that only PlainHTTP selects +// the http scheme, and that Insecure alone does not force http. This guards +// against the historical conflation of the two flags regressing. +// +// It uses a plain (non-TLS) httptest server as the target. When a resolver +// mistakenly dials https against a plain HTTP server, Go's net/http client +// surfaces the well-known error "http: server gave HTTP response to HTTPS +// client" -- that string's presence/absence tells us which scheme the +// resolver actually used, without needing to expose any internals. +func TestNewRegistryTarget_SchemeSelection(t *testing.T) { + plainSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer plainSrv.Close() + + host := strings.TrimPrefix(plainSrv.URL, "http://") + + tests := []struct { + name string + opts RegistryOptions + wantSchemeMismatch bool // true when the resolver dialed https against this plain http server + }{ + { + name: "neither flag set defaults to https", + opts: RegistryOptions{}, + wantSchemeMismatch: true, + }, + { + name: "plainHTTP alone selects http", + opts: RegistryOptions{PlainHTTP: true}, + wantSchemeMismatch: false, + }, + { + name: "insecure alone does not select http", + opts: RegistryOptions{Insecure: true}, + wantSchemeMismatch: true, + }, + { + name: "insecure and plainHTTP together select http", + opts: RegistryOptions{Insecure: true, PlainHTTP: true}, + wantSchemeMismatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target := NewRegistryTarget(host, tt.opts, NewRegistryHTTPClient(tt.opts)) + + _, err := target.Resolve(context.Background(), host+"/library/test:latest") + + gotMismatch := err != nil && strings.Contains(strings.ToLower(err.Error()), "server gave http response to https client") + if gotMismatch != tt.wantSchemeMismatch { + t.Fatalf("scheme mismatch detected = %v (err=%v), want %v", gotMismatch, err, tt.wantSchemeMismatch) + } + }) + } +} + +// TestNewRegistryHTTPClient_DoesNotLeakGlobalTLSConfig asserts that building +// an insecure client never mutates http.DefaultTransport in place, which +// would leak InsecureSkipVerify into every other HTTP client in the process. +// It compares http.DefaultTransport.TLSClientConfig before and after +// construction (rather than asserting it is nil) because the Go standard +// library itself lazily populates that field the first time DefaultTransport +// performs a TLS dial elsewhere in the test binary -- the property under +// test is that *construction* leaves it untouched, not that it is globally +// pristine. +func TestNewRegistryHTTPClient_DoesNotLeakGlobalTLSConfig(t *testing.T) { + dt, ok := http.DefaultTransport.(*http.Transport) + if !ok { + t.Fatalf("http.DefaultTransport is not *http.Transport: %T", http.DefaultTransport) + } + before := dt.TLSClientConfig + + _ = NewRegistryHTTPClient(RegistryOptions{Insecure: true}) + + if dt.TLSClientConfig != before { + t.Fatalf("NewRegistryHTTPClient mutated the global http.DefaultTransport.TLSClientConfig: before=%+v after=%+v", before, dt.TLSClientConfig) + } +} + +// stubRoundTripper is a minimal http.RoundTripper used to stand in for +// http.DefaultTransport in TestNewRegistryHTTPClient_FallsBackWhenDefaultTransportIsNotHTTPTransport. +// It is deliberately not a *http.Transport, so the comma-ok type assertion in +// NewRegistryHTTPClient takes its "not ok" branch. +type stubRoundTripper struct{} + +func (stubRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { + return nil, nil +} + +// TestNewRegistryHTTPClient_FallsBackWhenDefaultTransportIsNotHTTPTransport +// guards against a panic if something in the process (instrumentation, a +// test harness) has replaced http.DefaultTransport with a RoundTripper that +// isn't *http.Transport. NewRegistryHTTPClient must fall back to a plain +// *http.Transport instead of panicking on the type assertion. +func TestNewRegistryHTTPClient_FallsBackWhenDefaultTransportIsNotHTTPTransport(t *testing.T) { + original := http.DefaultTransport + http.DefaultTransport = stubRoundTripper{} + defer func() { http.DefaultTransport = original }() + + var client *http.Client + func() { + defer func() { + if r := recover(); r != nil { + t.Fatalf("NewRegistryHTTPClient panicked with a non-*http.Transport DefaultTransport: %v", r) + } + }() + client = NewRegistryHTTPClient(RegistryOptions{Insecure: true}) + }() + + if client == nil { + t.Fatalf("expected a non-nil client") + } + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected client.Transport to be *http.Transport, got %T", client.Transport) + } + if transport.TLSClientConfig == nil || !transport.TLSClientConfig.InsecureSkipVerify { + t.Fatalf("expected InsecureSkipVerify to be honored on the fallback transport") + } +}