Merge pull request #45464 from corhere/xattrs-matter-in-images

Fail unpacking images with xattrs to filesystems without xattr support

daemon/graphdriver/fsdiff.go

@@ -23,7 +23,12 @@ var (
 // on the exported NewNaiveDiffDriver function below.
 type NaiveDiffDriver struct {
 	ProtoDriver
-	idMap idtools.IdentityMapping
+	IDMap idtools.IdentityMapping
+	// If true, allow ApplyDiff to succeed in spite of failures to set
+	// extended attributes on the unpacked files due to the destination
+	// filesystem not supporting them or a lack of permissions. The
+	// resulting unpacked layer may be subtly broken.
+	BestEffortXattrs bool
 }
 
 // NewNaiveDiffDriver returns a fully functional driver that wraps the
@@ -36,7 +41,7 @@ type NaiveDiffDriver struct {
 //	DiffSize(id, parent string) (size int64, err error)
 func NewNaiveDiffDriver(driver ProtoDriver, idMap idtools.IdentityMapping) Driver {
 	return &NaiveDiffDriver{ProtoDriver: driver,
-		idMap: idMap}
+		IDMap: idMap}
 }
 
 // Diff produces an archive of the changes between the specified
@@ -80,7 +85,7 @@ func (gdw *NaiveDiffDriver) Diff(id, parent string) (arch io.ReadCloser, err err
 		return nil, err
 	}
 
-	archive, err := archive.ExportChanges(layerFs, changes, gdw.idMap)
+	archive, err := archive.ExportChanges(layerFs, changes, gdw.IDMap)
 	if err != nil {
 		return nil, err
 	}
@@ -136,7 +141,7 @@ func (gdw *NaiveDiffDriver) ApplyDiff(id, parent string, diff io.Reader) (size i
 	defer driver.Put(id)
 
 	layerFs := layerRootFs
-	options := &archive.TarOptions{IDMap: gdw.idMap}
+	options := &archive.TarOptions{IDMap: gdw.IDMap, BestEffortXattrs: gdw.BestEffortXattrs}
 	start := time.Now().UTC()
 	logrus.WithField("id", id).Debug("Start untar layer")
 	if size, err = ApplyUncompressedLayer(layerFs, diff, options); err != nil {

daemon/graphdriver/vfs/driver.go

@@ -16,6 +16,11 @@ import (
 	"github.com/pkg/errors"
 )
 
+const (
+	xattrsStorageOpt         = "vfs.xattrs"
+	bestEffortXattrsOptValue = "i_want_broken_containers"
+)
+
 var (
 	// CopyDir defines the copy method to use.
 	CopyDir = dirCopy
@@ -51,7 +56,11 @@ func Init(home string, options []string, idMap idtools.IdentityMapping) (graphdr
 		return nil, quota.ErrQuotaNotSupported
 	}
 
-	return graphdriver.NewNaiveDiffDriver(d, d.idMapping), nil
+	return &graphdriver.NaiveDiffDriver{
+		ProtoDriver:      d,
+		IDMap:            d.idMapping,
+		BestEffortXattrs: d.bestEffortXattrs,
+	}, nil
 }
 
 // Driver holds information about the driver, home directory of the driver.
@@ -60,16 +69,24 @@ func Init(home string, options []string, idMap idtools.IdentityMapping) (graphdr
 // Driver must be wrapped in NaiveDiffDriver to be used as a graphdriver.Driver
 type Driver struct {
 	driverQuota
-	home      string
-	idMapping idtools.IdentityMapping
+	home             string
+	idMapping        idtools.IdentityMapping
+	bestEffortXattrs bool
 }
 
 func (d *Driver) String() string {
 	return "vfs"
 }
 
-// Status is used for implementing the graphdriver.ProtoDriver interface. VFS does not currently have any status information.
+// Status is used for implementing the graphdriver.ProtoDriver interface.
 func (d *Driver) Status() [][2]string {
+	if d.bestEffortXattrs {
+		return [][2]string{
+			// These strings are looked for in daemon/info_unix.go:fillDriverWarnings()
+			// because plumbing is hard and temporary is forever. Forgive me.
+			{"Extended file attributes", "best-effort"},
+		}
+	}
 	return nil
 }
 
@@ -98,6 +115,11 @@ func (d *Driver) parseOptions(options []string) error {
 			if err = d.setQuotaOpt(uint64(size)); err != nil {
 				return errdefs.InvalidParameter(errors.Wrap(err, "failed to set option size for vfs"))
 			}
+		case xattrsStorageOpt:
+			if val != bestEffortXattrsOptValue {
+				return errdefs.InvalidParameter(errors.Errorf("do not set the " + xattrsStorageOpt + " option unless you are willing to accept the consequences"))
+			}
+			d.bestEffortXattrs = true
 		default:
 			return errdefs.InvalidParameter(errors.Errorf("unknown option %s for vfs", key))
 		}

daemon/graphdriver/vfs/vfs_test.go

@@ -4,8 +4,19 @@
 package vfs // import "github.com/docker/docker/daemon/graphdriver/vfs"
 
 import (
+	"archive/tar"
+	"bytes"
+	"errors"
+	"io"
+	"os"
+	"path/filepath"
+	"syscall"
 	"testing"
 
+	"github.com/moby/sys/mount"
+	"gotest.tools/v3/assert"
+
+	"github.com/docker/docker/daemon/graphdriver"
 	"github.com/docker/docker/daemon/graphdriver/graphtest"
 )
 
@@ -34,3 +45,71 @@ func TestVfsSetQuota(t *testing.T) {
 func TestVfsTeardown(t *testing.T) {
 	graphtest.PutDriver(t)
 }
+
+func TestXattrUnsupportedByBackingFS(t *testing.T) {
+	rootdir := t.TempDir()
+	// The ramfs filesystem is unconditionally compiled into the kernel,
+	// and does not support extended attributes.
+	err := mount.Mount("ramfs", rootdir, "ramfs", "")
+	if errors.Is(err, syscall.EPERM) {
+		t.Skip("test requires the ability to mount a filesystem")
+	}
+	assert.NilError(t, err)
+	defer mount.Unmount(rootdir)
+
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)
+	const (
+		filename = "test.txt"
+		content  = "hello world\n"
+	)
+	assert.NilError(t, tw.WriteHeader(&tar.Header{
+		Name: filename,
+		Mode: 0o644,
+		Size: int64(len(content)),
+		PAXRecords: map[string]string{
+			"SCHILY.xattr.user.test": "helloxattr",
+		},
+	}))
+	_, err = io.WriteString(tw, content)
+	assert.NilError(t, err)
+	assert.NilError(t, tw.Close())
+	testlayer := buf.Bytes()
+
+	for _, tt := range []struct {
+		name        string
+		opts        []string
+		expectErrIs error
+	}{
+		{
+			name:        "Default",
+			expectErrIs: syscall.EOPNOTSUPP,
+		},
+		{
+			name: "xattrs=i_want_broken_containers",
+			opts: []string{"xattrs=i_want_broken_containers"},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			subdir := filepath.Join(rootdir, tt.name)
+			assert.NilError(t, os.Mkdir(subdir, 0o755))
+			d, err := graphdriver.GetDriver("vfs", nil,
+				graphdriver.Options{DriverOptions: tt.opts, Root: subdir})
+			assert.NilError(t, err)
+			defer d.Cleanup()
+
+			assert.NilError(t, d.Create("test", "", nil))
+			_, err = d.ApplyDiff("test", "", bytes.NewReader(testlayer))
+			assert.ErrorIs(t, err, tt.expectErrIs)
+
+			if err == nil {
+				path, err := d.Get("test", "")
+				assert.NilError(t, err)
+				defer d.Put("test")
+				actual, err := os.ReadFile(filepath.Join(path, filename))
+				assert.NilError(t, err)
+				assert.Equal(t, string(actual), content)
+			}
+		})
+	}
+}

daemon/info_unix.go

@@ -290,6 +290,15 @@ func fillDriverWarnings(v *types.Info) {
 			}
 			msg += "         Running without d_type support will not be supported in future releases."
 
+			v.Warnings = append(v.Warnings, msg)
+			continue
+		}
+		if pair[0] == "Extended file attributes" && pair[1] == "best-effort" {
+			msg := fmt.Sprintf("WARNING: %s: extended file attributes from container images "+
+				"will be silently discarded if the backing filesystem does not support them.\n"+
+				"         CONTAINERS MAY MALFUNCTION IF EXTENDED ATTRIBUTES ARE MISSING.\n"+
+				"         This is an UNSUPPORTABLE configuration for which no bug reports will be accepted.\n", v.Driver)
+
 			v.Warnings = append(v.Warnings, msg)
 			continue
 		}

pkg/archive/archive.go

@@ -70,6 +70,12 @@ type (
 		// replaced with the matching name from this map.
 		RebaseNames map[string]string
 		InUserNS    bool
+		// Allow unpacking to succeed in spite of failures to set extended
+		// attributes on the unpacked files due to the destination filesystem
+		// not supporting them or a lack of permissions. Extended attributes
+		// were probably in the archive for a reason, so set this option at
+		// your own peril.
+		BestEffortXattrs bool
 	}
 )
 
@@ -666,7 +672,19 @@ func (ta *tarAppender) addTarFile(path, name string) error {
 	return nil
 }
 
-func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, Lchown bool, chownOpts *idtools.Identity, inUserns bool) error {
+func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, opts *TarOptions) error {
+	var (
+		Lchown                     = true
+		inUserns, bestEffortXattrs bool
+		chownOpts                  *idtools.Identity
+	)
+	if opts != nil {
+		Lchown = !opts.NoLchown
+		inUserns = opts.InUserNS
+		chownOpts = opts.ChownOpts
+		bestEffortXattrs = opts.BestEffortXattrs
+	}
+
 	// hdr.Mode is in linux format, which we can use for sycalls,
 	// but for os.Foo() calls we need the mode converted to os.FileMode,
 	// so use hdrInfo.Mode() (they differ for e.g. setuid bits)
@@ -757,26 +775,22 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L
 		}
 	}
 
-	var errors []string
+	var xattrErrs []string
 	for key, value := range hdr.Xattrs {
 		if err := system.Lsetxattr(path, key, []byte(value), 0); err != nil {
-			if err == syscall.ENOTSUP || err == syscall.EPERM {
-				// We ignore errors here because not all graphdrivers support
-				// xattrs *cough* old versions of AUFS *cough*. However only
-				// ENOTSUP should be emitted in that case, otherwise we still
-				// bail.
+			if bestEffortXattrs && errors.Is(err, syscall.ENOTSUP) || errors.Is(err, syscall.EPERM) {
 				// EPERM occurs if modifying xattrs is not allowed. This can
 				// happen when running in userns with restrictions (ChromeOS).
-				errors = append(errors, err.Error())
+				xattrErrs = append(xattrErrs, err.Error())
 				continue
 			}
 			return err
 		}
 	}
 
-	if len(errors) > 0 {
+	if len(xattrErrs) > 0 {
 		logrus.WithFields(logrus.Fields{
-			"errors": errors,
+			"errors": xattrErrs,
 		}).Warn("ignored xattrs in archive: underlying filesystem doesn't support them")
 	}
 
@@ -1158,7 +1172,7 @@ loop:
 			}
 		}
 
-		if err := createTarFile(path, dest, hdr, trBuf, !options.NoLchown, options.ChownOpts, options.InUserNS); err != nil {
+		if err := createTarFile(path, dest, hdr, trBuf, options); err != nil {
 			return err
 		}
 

pkg/archive/archive_test.go

@@ -863,7 +863,7 @@ func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(tmpDir)
-	err = createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, true, nil, false)
+	err = createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, nil)
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/archive/diff.go

@@ -92,7 +92,7 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 					}
 					defer os.RemoveAll(aufsTempdir)
 				}
-				if err := createTarFile(filepath.Join(aufsTempdir, basename), dest, hdr, tr, true, nil, options.InUserNS); err != nil {
+				if err := createTarFile(filepath.Join(aufsTempdir, basename), dest, hdr, tr, options); err != nil {
 					return 0, err
 				}
 			}
@@ -183,7 +183,7 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 				return 0, err
 			}
 
-			if err := createTarFile(path, dest, srcHdr, srcData, !options.NoLchown, nil, options.InUserNS); err != nil {
+			if err := createTarFile(path, dest, srcHdr, srcData, options); err != nil {
 				return 0, err
 			}
 

pkg/system/xattrs_linux.go

@@ -1,11 +1,19 @@
 package system // import "github.com/docker/docker/pkg/system"
 
-import "golang.org/x/sys/unix"
+import (
+	"io/fs"
+
+	"golang.org/x/sys/unix"
+)
 
 // Lgetxattr retrieves the value of the extended attribute identified by attr
 // and associated with the given path in the file system.
 // It will returns a nil slice and nil error if the xattr is not set.
 func Lgetxattr(path string, attr string) ([]byte, error) {
+	pathErr := func(err error) ([]byte, error) {
+		return nil, &fs.PathError{Op: "lgetxattr", Path: path, Err: err}
+	}
+
 	// Start with a 128 length byte array
 	dest := make([]byte, 128)
 	sz, errno := unix.Lgetxattr(path, attr, dest)
@@ -14,7 +22,7 @@ func Lgetxattr(path string, attr string) ([]byte, error) {
 		// Buffer too small, use zero-sized buffer to get the actual size
 		sz, errno = unix.Lgetxattr(path, attr, []byte{})
 		if errno != nil {
-			return nil, errno
+			return pathErr(errno)
 		}
 		dest = make([]byte, sz)
 		sz, errno = unix.Lgetxattr(path, attr, dest)
@@ -24,7 +32,7 @@ func Lgetxattr(path string, attr string) ([]byte, error) {
 	case errno == unix.ENODATA:
 		return nil, nil
 	case errno != nil:
-		return nil, errno
+		return pathErr(errno)
 	}
 
 	return dest[:sz], nil
@@ -33,5 +41,9 @@ func Lgetxattr(path string, attr string) ([]byte, error) {
 // Lsetxattr sets the value of the extended attribute identified by attr
 // and associated with the given path in the file system.
 func Lsetxattr(path string, attr string, data []byte, flags int) error {
-	return unix.Lsetxattr(path, attr, data, flags)
+	err := unix.Lsetxattr(path, attr, data, flags)
+	if err != nil {
+		return &fs.PathError{Op: "lsetxattr", Path: path, Err: err}
+	}
+	return nil
 }