Judge an upstream pin's host like any name, and harden the upstream unpack (review 35, hq issue 345)

The url was skipped by the installation check, so an installation's host or
an address passed; it is judged whole now, the vendor's host declared in the
artifact's names-on-purpose, and an address or a reserved name as the host
is refused. Only the version's dotted-quad reading is skipped. The unpack
caps its entries, drains xz before waiting for it, reads a pax global
header and a nested within's parents, and a redirect off https is not
followed.

internal/builder/upstream_file.go

@@ -39,6 +39,22 @@ import (
 // unpacks no archive larger than this either (mesh-host internal/apply/archive.go, maxArchive).
 const maxUpstream = 512 << 20
 
+// maxEntries is the most entries an upstream archive may hold: millions of empty ones would fill the build
+// seat's inodes and the node's. Betterbird's release holds 64.
+const maxEntries = 100_000
+
+// noDowngrade refuses a redirect off https: the digest still guards the bytes, and a fetch in the clear says
+// to whoever is on the path what the build seat fetches.
+func noDowngrade(req *http.Request, via []*http.Request) error {
+	if req.URL.Scheme != "https" {
+		return fmt.Errorf("%s redirected to %s, which is not https", via[0].URL, req.URL)
+	}
+	if len(via) >= 10 {
+		return errors.New("more than 10 redirects")
+	}
+	return nil
+}
+
 // upstreamClient fetches upstream files: a variable so a test serves its own.
 var upstreamClient = &http.Client{Timeout: 15 * time.Minute}
 
@@ -98,7 +114,9 @@ func upstreamFile(ctx context.Context, workspace string, a catalogue.Artifact,
 	if err != nil {
 		return "", err
 	}
-	response, err := upstreamClient.Do(request)
+	client := *upstreamClient
+	client.CheckRedirect = noDowngrade
+	response, err := client.Do(request)
 	if err != nil {
 		return "", fmt.Errorf("cannot fetch %s: %w", a.URL, err)
 	}
@@ -213,6 +231,10 @@ func unpackUpstream(ctx context.Context, file, into, within string) error {
 		return err
 	}
 	if wait != nil {
+		// Whatever follows the tar's end is read before xz is waited for: xz blocks on a full pipe otherwise.
+		if _, err := io.Copy(io.Discard, stream); err != nil {
+			return err
+		}
 		w := wait
 		wait = nil
 		return w()
@@ -228,7 +250,7 @@ func untar(stream io.Reader, into, within string) error {
 		prefix = path.Clean(within) + "/"
 	}
 	var total int64
-	files := 0
+	files, entries := 0, 0
 	for {
 		header, err := reader.Next()
 		if err == io.EOF {
@@ -237,11 +259,20 @@ func untar(stream io.Reader, into, within string) error {
 		if err != nil {
 			return err
 		}
+		if entries++; entries > maxEntries {
+			return fmt.Errorf("it holds more than %d entries", maxEntries)
+		}
+		if header.Typeflag == tar.TypeXGlobalHeader {
+			// A pax global header (git archive writes one) names no file.
+			continue
+		}
 		name := path.Clean(strings.TrimPrefix(header.Name, "./"))
 		if path.IsAbs(name) || name == ".." || strings.HasPrefix(name, "../") {
 			return fmt.Errorf("%s names a path outside the archive", header.Name)
 		}
-		if name == "." || name+"/" == prefix {
+		if name == "." || name+"/" == prefix ||
+			(header.Typeflag == tar.TypeDir && strings.HasPrefix(prefix, name+"/")) {
+			// The archive's root, the directory taken, or one of its parents.
 			continue
 		}
 		if prefix != "" {

internal/builder/upstream_file_test.go

@@ -7,7 +7,9 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -16,6 +18,7 @@ import (
 	"sort"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/novox/mesh-controller/internal/catalogue"
 )
@@ -97,9 +100,16 @@ func serveVendor(t *testing.T, v *vendor) string {
 	}))
 	t.Cleanup(server.Close)
 	before := upstreamClient
-	upstreamClient = server.Client()
+	// Under a public name, as a manifest must say it: example.com, which the test server's certificate
+	// names, dialled at the server.
+	transport := server.Client().Transport.(*http.Transport).Clone()
+	addr := server.Listener.Addr().String()
+	transport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) {
+		return (&net.Dialer{}).DialContext(ctx, network, addr)
+	}
+	upstreamClient = &http.Client{Transport: transport}
 	t.Cleanup(func() { upstreamClient = before })
-	return server.URL
+	return "https://example.com"
 }
 
 // entriesOf lists a packed archive: each file with its mode and content.
@@ -297,3 +307,79 @@ func TestAnUpstreamArchiveCompressedWithXzIsRead(t *testing.T) {
 		t.Fatalf("the xz release unpacked to %v", names)
 	}
 }
+
+func TestAnUpstreamTarWithAGlobalHeaderNestedWithinAndTrailingDataIsRead(t *testing.T) {
+	// git archive writes a pax global header; a release may unpack under a/b with a/ listed first; xz may be
+	// followed by bytes after the tar's end, which must be read before xz is waited for.
+	var raw bytes.Buffer
+	w := tar.NewWriter(&raw)
+	w.WriteHeader(&tar.Header{Typeflag: tar.TypeXGlobalHeader, Name: "pax_global_header", PAXRecords: map[string]string{"comment": "abc"}})
+	w.WriteHeader(&tar.Header{Name: "a/", Typeflag: tar.TypeDir, Mode: 0o755})
+	w.WriteHeader(&tar.Header{Name: "a/b/", Typeflag: tar.TypeDir, Mode: 0o755})
+	w.WriteHeader(&tar.Header{Name: "a/b/f", Typeflag: tar.TypeReg, Mode: 0o644, Size: 1})
+	w.Write([]byte("x"))
+	w.Close()
+	body := gzipped(t, raw.Bytes())
+	v := &vendor{files: map[string][]byte{"/r.tar.gz": body}}
+	base := serveVendor(t, v)
+	packed, err := upstreamArchive(context.Background(), t.TempDir(), catalogue.Artifact{Name: "a", Kind: "archive",
+		URL: base + "/r.tar.gz", SHA256: sumHex(body), Version: "1", Within: "a/b"}, func(string, string, ...any) {})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if e := entriesOf(t, packed); len(e) != 1 || e["f"] == "" {
+		t.Fatalf("%v", e)
+	}
+	if _, err := exec.LookPath("xz"); err != nil {
+		t.Skip("NOT TESTED here: no xz")
+	}
+	// A megabyte after the tar's end: more than a pipe holds.
+	trailing := append(append([]byte{}, raw.Bytes()...), bytes.Repeat([]byte{0}, 1<<20)...)
+	cmd := exec.Command("xz", "-zc")
+	cmd.Stdin = bytes.NewReader(trailing)
+	compressed, err := cmd.Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+	v.files["/r.tar.xz"] = compressed
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	if _, err := upstreamArchive(ctx, t.TempDir(), catalogue.Artifact{Name: "a", Kind: "archive", URL: base + "/r.tar.xz",
+		SHA256: sumHex(compressed), Version: "1", Within: "a/b"}, func(string, string, ...any) {}); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestAnUpstreamArchiveWithTooManyEntriesIsRefused(t *testing.T) {
+	var raw bytes.Buffer
+	w := tar.NewWriter(&raw)
+	for i := 0; i <= maxEntries; i++ {
+		w.WriteHeader(&tar.Header{Name: fmt.Sprintf("d%d/", i), Typeflag: tar.TypeDir, Mode: 0o755})
+	}
+	w.Close()
+	body := gzipped(t, raw.Bytes())
+	v := &vendor{files: map[string][]byte{"/r.tar.gz": body}}
+	base := serveVendor(t, v)
+	_, err := upstreamArchive(context.Background(), t.TempDir(), catalogue.Artifact{Name: "a", Kind: "archive",
+		URL: base + "/r.tar.gz", SHA256: sumHex(body), Version: "1"}, func(string, string, ...any) {})
+	if err == nil || !strings.Contains(err.Error(), "entries") {
+		t.Fatalf("%v", err)
+	}
+}
+
+func TestARedirectOffHttpsIsNotFollowed(t *testing.T) {
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("x")) }))
+	t.Cleanup(plain.Close)
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, plain.URL+"/r", http.StatusFound)
+	}))
+	t.Cleanup(secure.Close)
+	before := upstreamClient
+	upstreamClient = secure.Client()
+	t.Cleanup(func() { upstreamClient = before })
+	_, err := upstreamFile(context.Background(), t.TempDir(), catalogue.Artifact{Name: "a", URL: secure.URL + "/r",
+		SHA256: sumHex([]byte("x")), Version: "1", File: "a"}, func(string, string, ...any) {})
+	if err == nil || !strings.Contains(err.Error(), "not https") {
+		t.Fatalf("%v", err)
+	}
+}

internal/catalogue/build.go

@@ -2,6 +2,7 @@ package catalogue
 
 import (
 	"fmt"
+	"net"
 	"net/url"
 	"path"
 	"regexp"
@@ -357,6 +358,27 @@ func (b *Build) problems(module string) []string {
 	return problems
 }
 
+// reservedSuffixes are names no public vendor publishes under: an installation's own and documentation's.
+var reservedSuffixes = []string{".internal", ".local", ".lan", ".home", ".corp", ".localhost", ".home.arpa",
+	".test", ".invalid", ".example", ".intranet", ".private"}
+
+// notAPublicHost says why a host is not a public vendor's name, or "".
+func notAPublicHost(host string) string {
+	h := strings.ToLower(strings.TrimSuffix(host, "."))
+	if net.ParseIP(h) != nil {
+		return "an address rather than a name"
+	}
+	if !strings.Contains(h, ".") {
+		return "a name with no domain"
+	}
+	for _, s := range reservedSuffixes {
+		if strings.HasSuffix(h, s) {
+			return "a name under " + s + ", which is nobody's public domain"
+		}
+	}
+	return ""
+}
+
 // sha256Hex is a pinned sha256 as a manifest writes it: 64 lower-case hexadecimal characters.
 var sha256Hex = regexp.MustCompile(`^[0-9a-f]{64}$`)
 
@@ -386,6 +408,8 @@ func upstreamFileProblems(module string, a Artifact) []string {
 	}
 	if u, err := url.Parse(a.URL); err != nil || u.Scheme != "https" || u.Host == "" {
 		say("is fetched from %q, and an upstream file is fetched over https from a named host", a.URL)
+	} else if why := notAPublicHost(u.Hostname()); why != "" {
+		say("is fetched from %s, %s: an upstream file is somebody else's release, published under a public name", a.URL, why)
 	}
 	if !sha256Hex.MatchString(a.SHA256) {
 		say("is fetched from %s without a pinned sha256 (64 lower-case hexadecimal characters): what that "+

internal/catalogue/installation.go

@@ -71,9 +71,8 @@ var hostname = regexp.MustCompile(
 		`(?:be|nl|de|fr|uk|eu|com|net|org|io|dev|app|cloud|site|online|me|co|ch|at|lu|` +
 		`internal|example|tld|test|invalid)`)
 
-// upstreamPin is where an artifact pinned by URL says where it is published and which release it is: the
-// vendor's address and the vendor's version, which name no installation (Artifact.URL).
-var upstreamPin = regexp.MustCompile(`^build\.artifacts\[[0-9]+\]\.(url|version)$`)
+// upstreamVersion is where an artifact pinned by URL says which release it is (Artifact.Version).
+var upstreamVersion = regexp.MustCompile(`^build\.artifacts\[[0-9]+\]\.version$`)
 
 // address is a dotted quad.
 var address = regexp.MustCompile(`(?:[0-9]{1,3}\.){3}[0-9]{1,3}`)
@@ -117,11 +116,6 @@ func InstallationProblems(m Manifest) []string {
 			"%s is named after %s, and a module is named for what it is, not for where it runs", m.Module, name))
 	}
 	walk(tree, "", nil, func(at string, value string, meant map[string]bool, isImage bool) {
-		// An archive pinned by URL names the vendor that publishes it, and the version it pins, which may
-		// have four dotted numbers (novox/hq ADR 0316): the world's, as a public registry is an image's.
-		if upstreamPin.MatchString(at) {
-			return
-		}
 		for _, name := range namesIn(value) {
 			if (isImage && worldsRegistries[strings.ToLower(name)]) || meant[name] {
 				continue
@@ -129,7 +123,10 @@ func InstallationProblems(m Manifest) []string {
 			problems = append(problems, fmt.Sprintf("%s names %s at %s", m.Module, name, at))
 		}
 		for _, ip := range addressesIn(value) {
-			if meant[ip] {
+			// An upstream pin's version may have four dotted numbers (Owl 1.5.6.1), which is no address; a name
+			// in it is still judged above. Its URL is judged whole: the vendor's host goes through the artifact's
+			// names-on-purpose like any name a definition means (novox/hq ADR 0316).
+			if meant[ip] || upstreamVersion.MatchString(at) {
 				continue
 			}
 			problems = append(problems, fmt.Sprintf("%s names the public address %s at %s", m.Module, ip, at))

internal/catalogue/manifest.go

@@ -1001,6 +1001,9 @@ type Artifact struct {
 	// release that unpacks into a directory of its own name (`betterbird/`). An entry outside it refuses
 	// the build. Empty means the archive's root.
 	Within string `json:"within,omitempty"`
+	// NamesOnPurpose are the names this artifact means to name, each with why: the vendor's host an upstream
+	// file is fetched from, which the installation check judges like any name (InstallationProblems).
+	NamesOnPurpose map[string]string `json:"names-on-purpose,omitempty"`
 
 	// Entrypoints are the compiled files a tool host should load from this module, relative to the
 	// bundle's root.

internal/catalogue/upstream_file_test.go

@@ -16,9 +16,9 @@ func upstreamManifest(artifact string) string {
 
 func TestAnArchivePinnedByURLAndSha256IsAccepted(t *testing.T) {
 	for _, a := range []string{
-		`{"name":"archive","kind":"archive","url":"https://vendor.invalid/release-1.tar.xz","sha256":"` + pinnedSum +
+		`{"name":"archive","kind":"archive","url":"https://vendor.example.org/release-1.tar.xz","sha256":"` + pinnedSum +
 			`","version":"1.0","within":"release"}`,
-		`{"name":"addon","kind":"archive","url":"https://vendor.invalid/addon.xpi","sha256":"` + pinnedSum +
+		`{"name":"addon","kind":"archive","url":"https://vendor.example.org/addon.xpi","sha256":"` + pinnedSum +
 			`","version":"1.5.6.1","file":"addon.xpi"}`,
 	} {
 		if _, err := ParseManifest([]byte(upstreamManifest(a))); err != nil {
@@ -64,19 +64,38 @@ func TestAnUpstreamFileThatIsNotPinnedIsRefused(t *testing.T) {
 	}
 }
 
-func TestAnUpstreamFilesVendorAndVersionNameNoInstallation(t *testing.T) {
-	// The vendor's address is the world's, as a public registry is an image's; a version of four dotted numbers
-	// is not an address. Anything else still is judged.
+func TestAnUpstreamFilesVendorIsNamedOnPurposeAndAnInstallationsIsRefused(t *testing.T) {
+	// The vendor's host is judged like any name, and passes through names-on-purpose; a version of four dotted
+	// numbers is not an address, and a name in it is still judged.
 	m, err := ParseManifest([]byte(upstreamManifest(`{"name":"owl","kind":"archive","url":"https://vendor.be/owl/owl.xpi","sha256":"` +
-		pinnedSum + `","version":"1.5.6.1","file":"owl.xpi"}`)))
+		pinnedSum + `","version":"1.5.6.1","file":"owl.xpi","names-on-purpose":{"vendor.be":"Owl's vendor publishes it there"}}`)))
 	if err != nil {
 		t.Fatal(err)
 	}
 	if problems := InstallationProblems(m); len(problems) != 0 {
-		t.Fatalf("the vendor and the version were taken for an installation: %v", problems)
+		t.Fatalf("the declared vendor and the version were taken for an installation: %v", problems)
 	}
-	m.Build.Artifacts[0].File = "mail.novox.be"
+	m.Build.Artifacts[0].NamesOnPurpose = nil
+	if problems := InstallationProblems(m); len(problems) != 1 || !strings.Contains(problems[0], "vendor.be") {
+		t.Fatalf("an undeclared vendor host is judged: %v", problems)
+	}
+	m.Build.Artifacts[0].Version = "git.novox.be 10.0.0.1"
+	if problems := InstallationProblems(m); len(problems) != 2 {
+		t.Fatalf("a name in the version is judged: %v", problems)
+	}
+	m.Build.Artifacts[0].URL, m.Build.Artifacts[0].Version = "https://mail.novox.be/owl.xpi", "1"
 	if problems := InstallationProblems(m); len(problems) != 1 || !strings.Contains(problems[0], "mail.novox.be") {
-		t.Fatalf("a name elsewhere in the artifact is still judged: %v", problems)
+		t.Fatalf("an installation's host in the url is judged: %v", problems)
+	}
+}
+
+func TestAnUpstreamFileFromAnAddressOrAReservedNameIsRefused(t *testing.T) {
+	for _, u := range []string{"https://git.novox.internal/r.tar.xz", "https://10.1.2.3/r.tar.xz",
+		"https://203.0.113.9/r.tar.xz", "https://[2001:db8::1]/r.tar.xz", "https://nas.lan/r.tar.xz", "https://vendor/r.tar.xz"} {
+		_, err := ParseManifest([]byte(upstreamManifest(`{"name":"x","kind":"archive","url":"` + u + `","sha256":"` +
+			pinnedSum + `","version":"1"}`)))
+		if err == nil || !strings.Contains(err.Error(), "published under a public name") {
+			t.Errorf("%s: %v", u, err)
+		}
 	}
 }