merge #609 into opencontainers/umoci:main

Aleksa Sarai (1):
  tests: fix failures on SELinux-enabled systems

LGTMs: cyphar

Author: cyphar

internal/system/xattr_unix_test.go

@@ -28,6 +28,16 @@ import (
 	"golang.org/x/sys/unix"
 )
 
+// specialXattrs come from the OCI xattr remapping logic in oci/layer/xattr.go.
+// These are xattrs that will be auto-set by the system and we should ignore
+// their existence when doing xattr-related operations (for our tests, this
+// especially means to take care of them when checking sets of xattrs that
+// exist on the filesystem).
+var specialXattrs = map[string]struct{}{
+	"security.selinux": {},
+	"system.nfs4_acl":  {},
+}
+
 func TestClearxattrFilter(t *testing.T) {
 	dir := t.TempDir()
 
@@ -38,6 +48,12 @@ func TestClearxattrFilter(t *testing.T) {
 	path := file.Name()
 	defer os.RemoveAll(path) //nolint:errcheck
 
+	autosetXattrs, err := Llistxattr(path)
+	require.NoErrorf(t, err, "llistxattr %q", path)
+	for _, xattr := range autosetXattrs {
+		require.Contains(t, specialXattrs, xattr, "auto-set xattrs must be part of special list")
+	}
+
 	xattrs := []struct {
 		name, value string
 		forbidden   bool
@@ -65,11 +81,16 @@ func TestClearxattrFilter(t *testing.T) {
 		}
 		require.NoErrorf(t, err, "lsetxattr %q=%q on %q", xattr.name, xattr.value, path)
 	}
+	// If we are running on an SELinux-enabled system, all new files get a
+	// security.selinux xattr that gets auto-set and cannot be removed so we
+	// need to include it in the expected set.
+	expectAllXattrNames := append(setXattrNames, autosetXattrs...)
+	expectRemainingXattrNames := append(forbiddenXattrNames, autosetXattrs...)
 
 	// Check they're all present.
 	allXattrList, err := Llistxattr(path)
 	require.NoErrorf(t, err, "llistxattr %q", path)
-	assert.ElementsMatch(t, setXattrNames, allXattrList, "all xattrs should be present after setting")
+	assert.ElementsMatch(t, expectAllXattrNames, allXattrList, "all xattrs should be present after setting")
 
 	// Now clear them.
 	err = Lclearxattrs(path, func(xattrName string) bool {
@@ -79,9 +100,9 @@ func TestClearxattrFilter(t *testing.T) {
 	require.NoErrorf(t, err, "lclearxattrs %q (forbidden=%v)", path, forbiddenXattrs)
 
 	// Check that only the forbidden ones remain.
-	forbiddenXattrList, err := Llistxattr(path)
+	remainingXattrList, err := Llistxattr(path)
 	require.NoErrorf(t, err, "llistxattr %q", path)
-	assert.NotElementsMatch(t, setXattrNames, forbiddenXattrList, "there should be a different set of xattrs after clearing")
-	assert.ElementsMatch(t, forbiddenXattrNames, forbiddenXattrList, "only explicitly forbidden xattrs should be allowed to remain after clearing")
-	assert.NotEmpty(t, forbiddenXattrList, "there should be some remaining xattrs after clearing")
+	assert.NotElementsMatch(t, expectAllXattrNames, remainingXattrList, "there should be a different set of xattrs after clearing")
+	assert.ElementsMatch(t, expectRemainingXattrNames, remainingXattrList, "only explicitly forbidden xattrs should be allowed to remain after clearing")
+	assert.NotEmpty(t, remainingXattrList, "there should be some remaining xattrs after clearing")
 }

oci/layer/xattr_linux_test.go

@@ -37,6 +37,11 @@ import (
 	"github.com/opencontainers/umoci/pkg/fseval"
 )
 
+func isForbiddenXattr(xattrName string) bool {
+	found, ok := getXattrFilter(xattrName)
+	return ok && found == forbiddenXattrFilter{}
+}
+
 func getAllXattrs(t *testing.T, path string) map[string]string {
 	names, err := system.Llistxattr(path)
 	require.NoErrorf(t, err, "fetch all xattrs for %q", path)
@@ -201,6 +206,14 @@ func testUnpackGenerateRoundTrip_ComplexXattr_OverlayfsRootfs(t *testing.T, tarX
 				fullPath := filepath.Join(dir, path)
 
 				xattrs := getAllXattrs(t, fullPath)
+				// Strip out any forbidden xattrs from the returned set.
+				// TODO: This is a little ugly -- ideally we would create
+				// copies of the necessary maps with special xattrs added.
+				for name := range xattrs {
+					if isForbiddenXattr(name) {
+						delete(xattrs, name)
+					}
+				}
 
 				if test.expectRemap {
 					assert.Equalf(t, de.remapXattrs, xattrs, "UnpackEntry(%q): expected to see %#v remapped properly", path, de.xattrs)
@@ -212,7 +225,6 @@ func testUnpackGenerateRoundTrip_ComplexXattr_OverlayfsRootfs(t *testing.T, tarX
 					require.NoErrorf(t, err, "isOverlayWhiteout(%q)", path)
 					assert.Falsef(t, isWo, "isOverlayWhiteout(%q): regular entries with overlayfs xattrs should not end up being unpacked with overlayfs whiteout xattrs", path)
 				} else {
-					xattrs := getAllXattrs(t, fullPath)
 					assert.Equalf(t, de.xattrs, xattrs, "UnpackEntry(%q): expected to see %#v not be remapped", path, de.xattrs)
 					assert.NotEqualf(t, de.remapXattrs, xattrs, "UnpackEntry(%q): expected to see %#v not be remapped", path, de.xattrs)
 				}
@@ -348,6 +360,14 @@ func TestUnpackGenerateRoundTrip_MockedSELinux(t *testing.T) {
 				if value, ok := de.autoXattrs[forbiddenTestXattr]; ok {
 					wantXattrs[forbiddenTestXattr] = value
 				}
+				// If we are running on an SELinux system (or even more
+				// unlikely, on top of nfsv4) we need to also allow any masked
+				// xattrs auto-applied by the system.
+				for gotXattr, value := range xattrs {
+					if isForbiddenXattr(gotXattr) {
+						wantXattrs[gotXattr] = value
+					}
+				}
 				assert.Equalf(t, wantXattrs, xattrs, "UnpackEntry(%q): expected to only see specific subset of applied xattrs", path)
 			}