From 37d495678eb54e50e5a3ec82803d902b764fcd70 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 11 Sep 2018 03:38:02 +1000 Subject: [PATCH 1/2] system: skip xattr tests if unsupported It looks like this causes test failures on Fedora. There's something odd going on with /tmp on their system, which results in xattrs not being able to be set. This means we cannot really do much (in the actual unpack code we ignore this error in the same way). Signed-off-by: Aleksa Sarai --- pkg/system/xattr_linux_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/system/xattr_linux_test.go b/pkg/system/xattr_linux_test.go index df18179c4..4d7107cd7 100644 --- a/pkg/system/xattr_linux_test.go +++ b/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) } } From cc4461a6ad88b9b1851714aaabe51d2770283bf0 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 11 Sep 2018 03:27:07 +1000 Subject: [PATCH 2/2] oci: layer: skip forbidden xattrs if unchanged It turns out that since SELinux auto-sets labels for every directory created, 6fd1e0e62645 ("oci: ignore system.nfs4_acl and extend forbidden-xattr handling") was far from sufficient to fix the overall issue. In particular, restoring of parent metadata broke on such systems because we try to re-apply the metadata we saw *on-disk*. Erroring out because a forbidden xattr was on-disk is silly (and is counter to what we actually want to do), so instead we can only error out if we've been asked to change the xattr from the on-disk value. This avoids us having to special-case restoreMetadat with on-disk data -- and instead we can make it slightly more general. This does mean that images which container "security.selinux" set "just right" will now be ignored, I think that this is a small price to pay. Signed-off-by: Aleksa Sarai --- CHANGELOG.md | 9 +++++++++ oci/layer/tar_extract.go | 20 +++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4283681d..1d83de5a9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/oci/layer/tar_extract.go b/oci/layer/tar_extract.go index f692c8aa0..9ac137756 100644 --- a/oci/layer/tar_extract.go +++ b/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).