pkg/archive: bail if setting xattrs is unsupported

Extended attributes are set on files in container images for a reason.
Fail to unpack if extended attributes are present in a layer and setting
the attributes on the unpacked files fails for any reason.

Add an option to the vfs graph driver to opt into the old behaviour
where ENOTSUPP and EPERM errors encountered when setting extended
attributes are ignored. Make it abundantly clear to users and anyone
triaging their bug reports that they are shooting themselves in the
foot by enabling this option.

Signed-off-by: Cory Snider <csnider@mirantis.com>

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)
@@ -760,11 +778,7 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L
 	var xattrErrs []string
 	for key, value := range hdr.Xattrs {
 		if err := system.Lsetxattr(path, key, []byte(value), 0); err != nil {
-			if errors.Is(err, syscall.ENOTSUP) || errors.Is(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).
 				xattrErrs = append(xattrErrs, err.Error())
@@ -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
 			}