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