merge branch 'pr-259'

  oci: layer: skip forbidden xattrs if unchanged
  system: skip xattr tests if unsupported

LGTMs: @cyphar
Closes #259

CHANGELOG.md

@@ -26,6 +26,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   spec, in the interest of playing nice, we can just follow their new
   restriction (even though it is not supported by the spec). This also makes
   our layers *slightly* smaller. openSUSE/umoci#254
+- `umoci unpack` now no longer erases `system.nfs4_acl` and also has some more
+  sophisticated handling of forbidden xattrs. openSUSE/umoci#252
+  openSUSE/umoci#248
+- `umoci unpack` now appears to work correctly on SELinux-enabled systems
+  (previously we had various issues where `umoci` wouldn't like it when it was
+  trying to ensure the filesystem was reproducibly generated and SELinux xattrs
+  would act strangely). To fix this, now `umoci unpack` will only cause errors
+  if it has been asked to change a forbidden xattr to a value different than
+  it's current on-disk value. openSUSE/umoci#235 openSUSE/umoci#259
 
 ## [0.4.1] - 2018-08-16
 ### Added

oci/layer/tar_extract.go

@@ -19,6 +19,7 @@ package layer
 
 import (
 	"archive/tar"
+	"bytes"
 	"fmt"
 	"io"
 	"os"
@@ -134,14 +135,31 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
 		return errors.Wrapf(err, "clear xattr metadata: %s", path)
 	}
 	for name, value := range hdr.Xattrs {
+		value := []byte(value)
+
+		// Forbidden xattrs should never be touched.
 		if _, skip := ignoreXattrs[name]; skip {
+			// If the xattr is already set to the requested value, don't bail.
+			// The reason for this logic is kinda convoluted, but effectively
+			// because restoreMetadata is called with the *on-disk* metadata we
+			// run the risk of things like "security.selinux" being included in
+			// that metadata (and thus tripping the forbidden xattr error). By
+			// only touching xattrs that have a different value we are somewhat
+			// more efficient and we don't have to special case parent restore.
+			// Of course this will only ever impact ignoreXattrs.
+			if oldValue, err := te.fsEval.Lgetxattr(path, name); err == nil {
+				if bytes.Equal(value, oldValue) {
+					log.Debugf("restore xattr metadata: skipping already-set xattr %q: %s", name, hdr.Name)
+					continue
+				}
+			}
 			if te.partialRootless {
 				log.Warnf("rootless{%s} ignoring forbidden xattr: %q", hdr.Name, name)
 				continue
 			}
 			return errors.Errorf("restore xattr metadata: saw forbidden xattr %q: %s", name, hdr.Name)
 		}
-		if err := te.fsEval.Lsetxattr(path, name, []byte(value), 0); err != nil {
+		if err := te.fsEval.Lsetxattr(path, name, value, 0); err != nil {
 			// In rootless mode, some xattrs will fail (security.capability).
 			// This is _fine_ as long as we're not running as root (in which
 			// case we shouldn't be ignoring xattrs that we were told to set).

pkg/system/xattr_linux_test.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 )
 
@@ -57,6 +58,9 @@ func TestClearxattrFilter(t *testing.T) {
 		}
 
 		if err := unix.Lsetxattr(path, xattr.name, []byte(xattr.value), 0); err != nil {
+			if errors.Cause(err) == unix.ENOTSUP {
+				t.Skip("xattrs unsupported on backing filesystem")
+			}
 			t.Fatalf("unexpected error setting %v=%v on %v: %v", xattr.name, xattr.value, path, err)
 		}
 	}