From 7a23f8f6a0d791cb10200f98004f551be90fb724 Mon Sep 17 00:00:00 2001 From: Alex Rodriguez <131964409+ezekiel-alexrod@users.noreply.github.com> Date: Wed, 7 Oct 2026 11:30:07 +0200 Subject: [PATCH] feat(agent): add --plain-http for a registry without TLS A registry that serves plain HTTP under a host name could not be reached: go-containerregistry only falls back to HTTP for a loopback or private address. The manager and imagecachectl import now take --plain-http, which marks every registry insecure. The client still tries HTTPS first, and tries HTTP too when HTTPS fails or is slow. It is off by default and no manifest sets it. The agent logs a warning at startup and the command prints one before it pulls. It excludes --ca-file and --insecure-skip-tls-verify: it is for a registry without TLS, so a certificate setting next to it is a mistake. Relates-to: MK8S-445 --- README.md | 2 + agent/DESIGN.md | 9 +++ agent/README.md | 4 ++ agent/cmd/manager/main.go | 10 ++- agent/internal/cli/cli.go | 26 ++++++-- agent/internal/cli/cli_test.go | 88 +++++++++++++++++++++++++ agent/internal/puller/puller.go | 29 +++++++-- agent/internal/puller/puller_test.go | 96 ++++++++++++++++++++++++++++ 8 files changed, 250 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 7d8aaa9..528bc4b 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,8 @@ the same rule: absolute, and without `..`. by a private CA, on top of the system ones. It is checked on every run, and never read for an archive. `--insecure-skip-tls-verify` accepts any certificate instead, for a test cluster only, and prints a warning. +`--plain-http` reaches a registry that serves plain HTTP, and prints a warning +too. It excludes the two certificate flags. The import refuses to replace a directory without a sentinel. The command runs as root, so a wrong name or cache path that lands on a directory the store did diff --git a/agent/DESIGN.md b/agent/DESIGN.md index 4f02a1f..48fd9de 100644 --- a/agent/DESIGN.md +++ b/agent/DESIGN.md @@ -247,6 +247,15 @@ Pulling and extraction use unusable one stops the agent at startup rather than failing every pull. `--insecure-skip-tls-verify` turns verification off for test clusters. It excludes `--ca-file`, so neither setting wins in silence. +- On its own, the library uses plain HTTP only for a loopback or private IP + address, or a `localhost` host name. + `--plain-http` marks every registry insecure (`name.Insecure`). The client + then tries HTTPS first, and also HTTP when HTTPS fails or takes more than + 300ms: the first answer wins, on every pull. So the flag also weakens a + registry that serves HTTPS: a certificate error no longer fails the pull, + and anyone on the path can answer the HTTP attempt (port 80 when the + reference names no port). It is for a registry without TLS, and excludes + both certificate flags rather than mixing the two modes. The cache images are regular container images (they must remain importable and mountable with `ctr` by provisioning tooling), so an artifact-oriented diff --git a/agent/README.md b/agent/README.md index 8c91efb..6f31e71 100644 --- a/agent/README.md +++ b/agent/README.md @@ -135,6 +135,10 @@ On a test cluster set up by hand, `--insecure-skip-tls-verify` accepts any registry certificate instead. The agent logs a warning at startup. It excludes `--ca-file`, and no manifest here sets it: never use it on a real node. +A registry that serves plain HTTP needs `--plain-http`. The agent logs a +warning at startup. It excludes `--ca-file` and `--insecure-skip-tls-verify`, +and no manifest here sets it. + Leader election is deliberately absent. Every agent converges the node it runs on, so there is nothing to elect a leader for. diff --git a/agent/cmd/manager/main.go b/agent/cmd/manager/main.go index 86a7bd2..15910d1 100644 --- a/agent/cmd/manager/main.go +++ b/agent/cmd/manager/main.go @@ -70,7 +70,7 @@ func main() { var resyncPeriod time.Duration var cachePath string var caFile string - var skipVerify bool + var skipVerify, plainHTTP bool var tlsOpts []func(*tls.Config) flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") @@ -100,6 +100,9 @@ func main() { "Read once at startup: restart the agent after rotating it.") flag.BoolVar(&skipVerify, "insecure-skip-tls-verify", false, "Accept any registry certificate. For a test cluster set up by hand only.") + flag.BoolVar(&plainHTTP, "plain-http", false, + "Allow plain HTTP to every registry. For a registry without TLS. "+ + "Excludes --ca-file and --insecure-skip-tls-verify.") opts := zap.Options{ Development: true, } @@ -207,7 +210,7 @@ func main() { // Checked before anything starts: an unusable CA would otherwise surface // as a failed pull on every node and every resource. - remote, err := puller.NewRemote(puller.TLS{CAFile: caFile, SkipVerify: skipVerify}) + remote, err := puller.NewRemote(puller.TLS{CAFile: caFile, SkipVerify: skipVerify, PlainHTTP: plainHTTP}) if err != nil { setupLog.Error(err, "unable to set up registry TLS") os.Exit(1) @@ -215,6 +218,9 @@ func main() { if skipVerify { setupLog.Info("WARNING: registry certificates are not verified (--insecure-skip-tls-verify)") } + if plainHTTP { + setupLog.Info("WARNING: registries may be reached over plain HTTP (--plain-http)") + } fw, err := controller.NewFSWatcher(nodeName) if err != nil { diff --git a/agent/internal/cli/cli.go b/agent/internal/cli/cli.go index 47da771..ee415b5 100644 --- a/agent/internal/cli/cli.go +++ b/agent/internal/cli/cli.go @@ -52,9 +52,9 @@ const importExample = ` imagecachectl import --name worker-1-0-0 registry.examp // success, ExitInterrupted when the context was cancelled, 1 otherwise. func Run(ctx context.Context, args []string, out, errOut io.Writer) int { var name, cachePath, caFile string - var skipVerify bool + var skipVerify, plainHTTP bool importCmd := &cobra.Command{ - Use: "import --name [--cache-path ] [--ca-file | --insecure-skip-tls-verify] ", + Use: "import --name [--cache-path ] [--ca-file | --insecure-skip-tls-verify | --plain-http] ", Short: "Fill the image cache from a registry or a docker archive", Long: importLong, Example: importExample, @@ -73,8 +73,14 @@ func Run(ctx context.Context, args []string, out, errOut io.Writer) int { if caFile != "" && skipVerify { return errors.New("--ca-file and --insecure-skip-tls-verify exclude each other") } + if plainHTTP && caFile != "" { + return errors.New("--ca-file and --plain-http exclude each other") + } + if plainHTTP && skipVerify { + return errors.New("--insecure-skip-tls-verify and --plain-http exclude each other") + } err := do(cmd.Context(), cachePath, name, args[0], - puller.TLS{CAFile: caFile, SkipVerify: skipVerify}, out, errOut) + puller.TLS{CAFile: caFile, SkipVerify: skipVerify, PlainHTTP: plainHTTP}, out, errOut) // An interrupted run is not a failure to diagnose: the extraction // publishes by rename, so nothing half written is left behind. if errors.Is(err, context.Canceled) { @@ -90,6 +96,8 @@ func Run(ctx context.Context, args []string, out, errOut io.Writer) int { "PEM `file` of CA certificates to trust for the registry, on top of the system ones") importCmd.Flags().BoolVar(&skipVerify, "insecure-skip-tls-verify", false, "accept any registry certificate, for a test cluster set up by hand only") + importCmd.Flags().BoolVar(&plainHTTP, "plain-http", false, + "allow plain HTTP to the registry, for a registry without TLS") _ = importCmd.MarkFlagRequired("name") root := &cobra.Command{ @@ -147,8 +155,13 @@ func do(ctx context.Context, cachePath, name, source string, tls puller.TLS, out if err != nil { return err } - if _, remote := src.(puller.Remote); remote && tls.SkipVerify { - printf(errOut, "imagecachectl: warning: the registry certificate is not verified\n") + if _, remote := src.(puller.Remote); remote { + if tls.SkipVerify { + printf(errOut, "imagecachectl: warning: the registry certificate is not verified\n") + } + if tls.PlainHTTP { + printf(errOut, "imagecachectl: warning: the registry may be reached over plain HTTP\n") + } } // Checked before anything reads through it, so that an unusable path is @@ -202,7 +215,8 @@ func do(ctx context.Context, cachePath, name, source string, tls puller.TLS, out // not own. Shape alone also means a path that is not there fails naming the // file rather than coming back with a complaint about a reference. // -// The TLS settings only matter to a registry, so an archive never reads them. +// The TLS and plain HTTP settings only matter to a registry, so an archive +// never reads them. func pullerFor(source string, tls puller.TLS) (puller.Puller, error) { switch { case strings.HasPrefix(source, string(os.PathSeparator)), diff --git a/agent/internal/cli/cli_test.go b/agent/internal/cli/cli_test.go index 9f5bfd0..c57dd6c 100644 --- a/agent/internal/cli/cli_test.go +++ b/agent/internal/cli/cli_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "io" + "net" "net/http" "net/http/httptest" "os" @@ -19,6 +20,7 @@ import ( "github.com/google/go-containerregistry/pkg/registry" "github.com/google/go-containerregistry/pkg/v1/empty" "github.com/google/go-containerregistry/pkg/v1/mutate" + "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/google/go-containerregistry/pkg/v1/tarball" "github.com/scality/image-cache/agent/internal/cache" ) @@ -715,3 +717,89 @@ func TestImportRecordsTheSameImageWhateverTheSource(t *testing.T) { t.Errorf("the archive recorded %v and the registry %v for the same image", layers[0], layers[1]) } } + +// servedByName is served, but the reference names example.com and the +// library default transport sends every address to the server for the rest +// of the test. go-containerregistry already uses plain HTTP for a loopback +// registry, so only a host name shows what --plain-http changes. +func servedByName(t *testing.T, files map[string][]byte) string { + t.Helper() + ref := served(t, files) + host, path, _ := strings.Cut(ref, "/") + _, port, _ := net.SplitHostPort(host) + + before := remote.DefaultTransport + t.Cleanup(func() { remote.DefaultTransport = before }) + tr := before.(*http.Transport).Clone() + tr.Proxy = nil + dialer := &net.Dialer{} + tr.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + return dialer.DialContext(ctx, network, host) + } + remote.DefaultTransport = tr + return "example.com:" + port + "/" + path +} + +func TestPlainHTTPReachesARegistryWithoutTLS(t *testing.T) { + ref := servedByName(t, map[string][]byte{pauseTarPath: []byte("pause")}) + + code, _, errOut := run(t, importCmd, nameFlag, resourceName, "--cache-path", t.TempDir(), ref) + if code != 1 { + t.Fatalf("exit = %d without --plain-http, want 1 (%s)", code, errOut) + } + // Go's own message for an HTTPS request answered in plain HTTP. + if !strings.Contains(errOut, "HTTP response to HTTPS client") { + t.Errorf("stderr = %s, want an HTTPS request refused", errOut) + } + + cacheDir := t.TempDir() + code, _, errOut = run(t, importCmd, nameFlag, resourceName, "--cache-path", cacheDir, + "--plain-http", ref) + if code != 0 { + t.Fatalf("exit = %d, want 0 (%s)", code, errOut) + } + if !strings.Contains(errOut, "plain HTTP") { + t.Errorf("stderr does not warn about plain HTTP: %s", errOut) + } + got, err := os.ReadFile(filepath.Join(cacheDir, resourceName, "pause.tar")) + if err != nil { + t.Fatal(err) + } + if string(got) != "pause" { + t.Errorf("pause.tar = %q, want %q", got, "pause") + } +} + +func TestPlainHTTPExcludesTheCertificateFlags(t *testing.T) { + ref := served(t, map[string][]byte{pauseTarPath: []byte("pause")}) + for _, flags := range [][]string{ + {"--ca-file", filepath.Join(t.TempDir(), "ca.crt")}, + {"--insecure-skip-tls-verify"}, + } { + t.Run(flags[0], func(t *testing.T) { + args := append([]string{importCmd, nameFlag, resourceName, "--cache-path", t.TempDir(), + "--plain-http"}, flags...) + code, _, errOut := run(t, append(args, ref)...) + if code != 1 { + t.Fatalf("exit = %d, want 1 (%s)", code, errOut) + } + if !strings.Contains(errOut, flags[0]) || !strings.Contains(errOut, "--plain-http") { + t.Errorf("stderr does not name both flags: %s", errOut) + } + }) + } +} + +// An archive reaches no registry, so the flag says nothing about it. +func TestPlainHTTPIsNotAnnouncedForAnArchive(t *testing.T) { + path := archive(t, map[string][]byte{pauseTarPath: []byte("pause")}) + + code, _, errOut := run(t, importCmd, nameFlag, resourceName, "--cache-path", t.TempDir(), + "--plain-http", path) + if code != 0 { + t.Fatalf("exit = %d, want 0 (%s)", code, errOut) + } + if strings.Contains(errOut, "plain HTTP") { + t.Errorf("stderr warns about plain HTTP for an archive: %s", errOut) + } +} diff --git a/agent/internal/puller/puller.go b/agent/internal/puller/puller.go index 3753fcd..af9d0ff 100644 --- a/agent/internal/puller/puller.go +++ b/agent/internal/puller/puller.go @@ -62,22 +62,28 @@ type Puller interface { type Remote struct { // transport is nil for go-containerregistry's default. transport http.RoundTripper + // plainHTTP marks every registry as insecure, see TLS.PlainHTTP. + plainHTTP bool } -// TLS says how a Remote checks the registry certificate. +// TLS says how a Remote reaches the registry and checks its certificate. type TLS struct { // CAFile is a PEM file of CAs trusted on top of the system ones. CAFile string // SkipVerify accepts any certificate. It is meant for a test cluster set // up by hand, never for a node. SkipVerify bool + // PlainHTTP lets the pull use plain HTTP. It is for a registry without + // TLS, so it excludes the two certificate settings. + PlainHTTP bool } -// NewRemote returns a Remote that checks certificates as cfg says. The CA +// NewRemote returns a Remote that reaches the registry as cfg says. The CA // file is read once: a rotated CA needs a new Remote. func NewRemote(cfg TLS) (Remote, error) { - if cfg == (TLS{}) { - return Remote{}, nil + // Without a certificate setting, the library default transport does. + if cfg.CAFile == "" && !cfg.SkipVerify { + return Remote{plainHTTP: cfg.PlainHTTP}, nil } // The library declares its default as an *http.Transport. return newRemote(cfg, remote.DefaultTransport.(*http.Transport)) @@ -86,11 +92,16 @@ func NewRemote(cfg TLS) (Remote, error) { // newRemote builds on base; tests hand it a transport that dials their own // server. func newRemote(cfg TLS, base *http.Transport) (Remote, error) { - t := base.Clone() switch { case cfg.CAFile != "" && cfg.SkipVerify: return Remote{}, errors.Wrap(ErrTLS, errors.WithDetail("a CA file is pointless when certificates are not verified")) + case cfg.PlainHTTP && (cfg.CAFile != "" || cfg.SkipVerify): + return Remote{}, errors.Wrap(ErrTLS, + errors.WithDetail("plain HTTP is for a registry without TLS, it takes no certificate setting")) + } + t := base.Clone() + switch { case cfg.SkipVerify: t.TLSClientConfig = &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12} default: @@ -145,7 +156,13 @@ func (r Remote) Resolve(ctx context.Context, ref string) (Image, error) { // open resolves ref to its linux/amd64 image. It reads the manifest and the // configuration, never a layer: those are fetched as the stream is read. func (r Remote) open(ctx context.Context, ref string) (v1.Image, Image, error) { - parsed, err := name.ParseReference(ref) + var nameOpts []name.Option + if r.plainHTTP { + // The library then races HTTPS and HTTP, see "Pulling and + // extraction" in agent/DESIGN.md. + nameOpts = append(nameOpts, name.Insecure) + } + parsed, err := name.ParseReference(ref, nameOpts...) if err != nil { return nil, Image{}, errors.Wrap(ErrReference, errors.CausedBy(err), errors.WithProperty("source", ref)) diff --git a/agent/internal/puller/puller_test.go b/agent/internal/puller/puller_test.go index ea5b069..6604481 100644 --- a/agent/internal/puller/puller_test.go +++ b/agent/internal/puller/puller_test.go @@ -6,6 +6,7 @@ import ( "compress/gzip" "context" "crypto/rand" + "crypto/tls" "crypto/x509" "encoding/json" "encoding/pem" @@ -1256,3 +1257,98 @@ func TestNewRemoteRefusesACAWithSkipVerify(t *testing.T) { t.Fatalf("err = %v, want ErrTLS", err) } } + +// plainRegistry serves an in-memory registry over plain HTTP and returns a +// reference to an image pushed there, and a transport that reaches the +// server. The reference names example.com for the reason tlsRegistry gives: +// go-containerregistry already uses HTTP for a loopback registry. +func plainRegistry(t *testing.T) (ref string, base *http.Transport) { + t.Helper() + srv := httptest.NewServer(registry.New()) + t.Cleanup(srv.Close) + + img, err := crane.Image(map[string][]byte{etcdTarPath: []byte(etcdBody)}) + if err != nil { + t.Fatal(err) + } + host := strings.TrimPrefix(srv.URL, "http://") + if err := crane.Push(img, host+workerRefPath); err != nil { + t.Fatal(err) + } + + base = remote.DefaultTransport.(*http.Transport).Clone() + base.Proxy = nil + dialer := &net.Dialer{} + base.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + return dialer.DialContext(ctx, network, host) + } + _, port, _ := net.SplitHostPort(host) + return "example.com:" + port + workerRefPath, base +} + +func TestRemotePullNeedsPlainHTTPForARegistryWithoutTLS(t *testing.T) { + ref, base := plainRegistry(t) + + _, _, err := Remote{transport: base}.Pull(context.Background(), ref) + if !errors.Is(err, ErrPull) { + t.Fatalf("err = %v, want ErrPull without plain HTTP", err) + } + // Go's own message for an HTTPS request answered in plain HTTP. + if !strings.Contains(err.Error(), "HTTP response to HTTPS client") { + t.Errorf("err = %v, want an HTTPS request refused", err) + } + + rc, _, err := Remote{transport: base, plainHTTP: true}.Pull(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + hdrs, err := readAll(t, rc) + if err != nil { + t.Fatal(err) + } + if got := names(hdrs); !slices.Contains(got, etcdTarPath) { + t.Errorf("entries = %v, want %s", got, etcdTarPath) + } +} + +// A registry that serves HTTPS stays reachable with plain HTTP allowed. The +// test server only speaks TLS, so a pull that succeeds went over HTTPS. +func TestRemotePullWithPlainHTTPStillReachesAnHTTPSRegistry(t *testing.T) { + ref, caFile, base := tlsRegistry(t) + pool, err := caPool(caFile) + if err != nil { + t.Fatal(err) + } + base.TLSClientConfig = &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12} + + rc, _, err := Remote{transport: base, plainHTTP: true}.Pull(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + if _, err := readAll(t, rc); err != nil { + t.Fatal(err) + } +} + +func TestNewRemoteWithPlainHTTPKeepsTheDefaultTransport(t *testing.T) { + r, err := NewRemote(TLS{PlainHTTP: true}) + if err != nil { + t.Fatal(err) + } + if r.transport != nil || !r.plainHTTP { + t.Errorf("remote = %+v, want the library default transport and plain HTTP", r) + } +} + +func TestNewRemoteRefusesPlainHTTPWithACertificateSetting(t *testing.T) { + for label, cfg := range map[string]TLS{ + "CA file": {PlainHTTP: true, CAFile: "ca.crt"}, + "skip verify": {PlainHTTP: true, SkipVerify: true}, + } { + t.Run(label, func(t *testing.T) { + if _, err := NewRemote(cfg); !errors.Is(err, ErrTLS) { + t.Fatalf("err = %v, want ErrTLS", err) + } + }) + } +}