ociruntime: reapply tar unpack (#8023)

This change include a few small changes inside: - Reapply "ociruntime: replace tar extraction subprocess with in-process (#7589)" (#8019) - Add a basic image pull test and guard it with a separate, manual Bazel test target to help verifying common images. - Fix issues with the previous implementation. Ensure that parent directory is created for file tar entries. Ignore unknown tar entry types (i.e. devices).
buildbuddy-io · Dec 13, 2024 · f5ce15e · f5ce15e
1 parent f791cb1
commit f5ce15e
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 38 deletions.
diff --git a/enterprise/server/remote_execution/containers/ociruntime/BUILD b/enterprise/server/remote_execution/containers/ociruntime/BUILD
@@ -52,6 +52,7 @@ go_test(
         "test.workload-isolation-type": "firecracker",
         "test.container-image": "docker://gcr.io/flame-public/net-tools@sha256:ac701954d2c522d0d2b5296323127cacaaf77627e69db848a8d6ecb53149d344",
         "test.EstimatedComputeUnits": "2",
+        "test.EstimatedFreeDiskBytes": "10GB",
     },
     # NOTE: if testing locally, use --test_sharding_strategy=disabled to work
     # around the networking package not supporting cross-process locks.

diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
@@ -1,14 +1,15 @@
 package ociruntime
 
 import (
+	"archive/tar"
 	"bytes"
 	"context"
 	"crypto/rand"
 	"encoding/json"
 	"errors"
 	"flag"
 	"fmt"
-	"io/fs"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -1281,6 +1282,10 @@ func (s *ImageStore) pull(ctx context.Context, imageName string, creds oci.Crede
 
 // downloadLayer downloads and extracts the given layer to the given destination
 // dir. The extracted layer is suitable for use as an overlayfs lowerdir.
+//
+// For reference implementations, see:
+//   - Podman: https://github.com/containers/storage/blob/664fe5d9b95004e1be3eee004d56a1715c8ca790/pkg/archive/archive.go#L707-L729
+//   - Moby (Docker): https://github.com/moby/moby/blob/9633556bef3eb20dfe888903660c3df89a73605b/pkg/archive/archive.go#L726-L735
 func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
 	rc, err := layer.Uncompressed()
 	if err != nil {
@@ -1294,58 +1299,97 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
 	}
 	defer os.RemoveAll(tempUnpackDir)
 
-	// TODO: avoid tar command.
-	cmd := exec.CommandContext(ctx, "tar", "--extract", "--directory", tempUnpackDir)
-	var stderr bytes.Buffer
-	cmd.Stdin = rc
-	cmd.Stderr = &stderr
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-	if err := cmd.Run(); err != nil {
-		return status.UnavailableErrorf("download and extract layer tarball: %s: %q", err, stderr.String())
-	}
-
-	// Convert whiteout files to overlayfs format.
-	err = filepath.WalkDir(tempUnpackDir, func(path string, entry fs.DirEntry, err error) error {
+	tr := tar.NewReader(rc)
+	for {
+		header, err := tr.Next()
+		if err == io.EOF {
+			break
+		}
 		if err != nil {
-			return err
+			return status.UnavailableErrorf("download and extract layer tarball: %s", err)
 		}
 
-		if !entry.Type().IsRegular() {
-			return nil
+		if slices.Contains(strings.Split(header.Name, string(os.PathSeparator)), "..") {
+			return status.UnavailableErrorf("tar entry is not clean: %q", header.Name)
 		}
 
-		base := filepath.Base(path)
-		dir := filepath.Dir(path)
-		const whiteoutPrefix = ".wh."
-
-		// Directory whiteouts
-		if base == whiteoutPrefix+whiteoutPrefix+".opq" {
-			if err := unix.Setxattr(dir, "trusted.overlay.opaque", []byte{'y'}, 0); err != nil {
-				return fmt.Errorf("setxattr on deleted dir: %w", err)
+		// filepath.Join applies filepath.Clean to all arguments
+		file := filepath.Join(tempUnpackDir, header.Name)
+		base := filepath.Base(file)
+		dir := filepath.Dir(file)
+
+		if header.Typeflag == tar.TypeDir ||
+			header.Typeflag == tar.TypeReg ||
+			header.Typeflag == tar.TypeSymlink ||
+			header.Typeflag == tar.TypeLink {
+			// Ensure that parent dir exists
+			if err := os.MkdirAll(dir, os.ModePerm); err != nil {
+				return status.UnavailableErrorf("create directory: %s", err)
 			}
-			if err := os.Remove(path); err != nil {
-				return fmt.Errorf("remove directory whiteout marker: %w", err)
-			}
-			return nil
+		} else {
+			log.CtxInfof(ctx, "Ignoring unsupported tar header %q type %q in oci layer", header.Name, header.Typeflag)
+			continue
 		}
 
-		// File whiteouts
+		const whiteoutPrefix = ".wh."
+		// Handle whiteout
 		if strings.HasPrefix(base, whiteoutPrefix) {
+			// Directory whiteout
+			if base == whiteoutPrefix+whiteoutPrefix+".opq" {
+				if err := unix.Setxattr(dir, "trusted.overlay.opaque", []byte{'y'}, 0); err != nil {
+					return status.UnavailableErrorf("setxattr on deleted dir: %s", err)
+				}
+				continue
+			}
+
+			// File whiteout: Mark the file for deletion in overlayfs.
 			originalBase := base[len(whiteoutPrefix):]
 			originalPath := filepath.Join(dir, originalBase)
 			if err := unix.Mknod(originalPath, unix.S_IFCHR, 0); err != nil {
-				return fmt.Errorf("mknod for whiteout marker: %w", err)
+				return status.UnavailableErrorf("mknod for whiteout marker: %s", err)
 			}
-			if err := os.Remove(path); err != nil {
-				return fmt.Errorf("remove directory whiteout marker: %w", err)
-			}
-			return nil
+			continue
 		}
 
-		return nil
-	})
-	if err != nil {
-		return status.UnavailableErrorf("walk layer dir: %s", err)
+		switch header.Typeflag {
+		case tar.TypeDir:
+			if err := os.MkdirAll(file, os.FileMode(header.Mode)); err != nil {
+				return status.UnavailableErrorf("create directory: %s", err)
+			}
+		case tar.TypeReg:
+			f, err := os.OpenFile(file, os.O_CREATE|os.O_WRONLY, os.FileMode(header.Mode))
+			if err != nil {
+				return status.UnavailableErrorf("create file: %s", err)
+			}
+			if _, err := io.Copy(f, tr); err != nil {
+				f.Close()
+				return status.UnavailableErrorf("copy file content: %s", err)
+			}
+			if err := f.Chown(header.Uid, header.Gid); err != nil {
+				f.Close()
+				return status.UnavailableErrorf("chown file: %s", err)
+			}
+			f.Close()
+		case tar.TypeSymlink:
+			// Symlink's target is only evaluated at runtime, inside the container context.
+			// So it's safe to have the symlink targeting paths outside unpackdir.
+			if err := os.Symlink(header.Linkname, file); err != nil {
+				return status.UnavailableErrorf("create symlink: %s", err)
+			}
+			if err := os.Lchown(file, header.Uid, header.Gid); err != nil {
+				return status.UnavailableErrorf("chown link: %s", err)
+			}
+		case tar.TypeLink:
+			target := filepath.Join(tempUnpackDir, header.Linkname)
+			if !strings.HasPrefix(target, tempUnpackDir) {
+				return status.UnavailableErrorf("breakout attempt detected with link: %q -> %q", header.Name, header.Linkname)
+			}
+			// Note that this will call linkat(2) without AT_SYMLINK_FOLLOW,
+			// so if target is a symlink, the hardlink will point to the symlink itself and not the symlink target.
+			if err := os.Link(target, file); err != nil {
+				return status.UnavailableErrorf("create hard link: %s", err)
+			}
+		}
 	}
 
 	if err := os.Rename(tempUnpackDir, destDir); err != nil {

diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go
@@ -1404,3 +1404,63 @@ func hasMountPermissions(t *testing.T) bool {
 	require.NoError(t, err, "unmount")
 	return true
 }
+
+// TestPullImage is a simple integration test that pulls images from a public repository.
+// Can be used as a smoke test to verify that the image pulling functionality works.
+// This is setup as a separate, manual test target in BUILD file to help aid development.
+//
+// Example:
+//
+//	bazel test \
+//	     --config=remote \
+//	     --test_output=all \
+//	     --test_sharding_strategy=disabled \
+//	     --test_tag_filters=+docker \
+//	     --test_filter=TestPullImage \
+//	     --test_env=TEST_PULLIMAGE=1 \
+//	     enterprise/server/remote_execution/containers/ociruntime:ociruntime_test
+func TestPullImage(t *testing.T) {
+	if os.Getenv("TEST_PULLIMAGE") == "" {
+		t.Skip("Skipping integration test..")
+	}
+
+	for _, tc := range []struct {
+		name  string
+		image string
+	}{
+		{
+			name:  "dockerhub_busybox",
+			image: "busybox:latest",
+		},
+		{
+			name:  "ghcr_nix",
+			image: "ghcr.io/avdv/nix-build@sha256:5f731adacf7290352fed6c1960dfb56ec3fdb31a376d0f2170961fbc96944d50",
+		},
+		{
+			name:  "executor_image",
+			image: "gcr.io/flame-public/buildbuddy-executor-enterprise:latest",
+		},
+		{
+			name:  "executor_docker",
+			image: "gcr.io/flame-public/executor-docker-default:enterprise-v1.6.0",
+		},
+		{
+			name:  "workflow_2004",
+			image: "gcr.io/flame-public/rbe-ubuntu20-04:latest",
+		},
+		{
+			name:  "workflow_2204",
+			image: "gcr.io/flame-public/rbe-ubuntu22-04:latest",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			layerDir := t.TempDir()
+			imgStore := ociruntime.NewImageStore(layerDir)
+
+			ctx := context.Background()
+			img, err := imgStore.Pull(ctx, tc.image, oci.Credentials{})
+			require.NoError(t, err)
+			require.NotNil(t, img)
+		})
+	}
+}