feat(agent): add --plain-http for a registry without TLS (MK8S-445)

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

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

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.
 

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,14 +210,17 @@ 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)
 	}
 	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 {

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 <resource> [--cache-path <dir>] [--ca-file <file> | --insecure-skip-tls-verify] <source>",
+		Use:     "import --name <resource> [--cache-path <dir>] [--ca-file <file> | --insecure-skip-tls-verify | --plain-http] <source>",
 		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)),

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)
+	}
+}

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