New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oci: ignore system.nfs4_acl and extend forbidden-xattr handling #252

Merged
merged 4 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@cyphar
Copy link
Member

cyphar commented Aug 29, 2018

It turns out that system.nfs4_acl is yet another xattr that we shouldn't
be touching, but also that we should not be clearing (while we have
permission to do so, clearing it results in NFS permissions breaking).
So as an extension we now now use ignoreXattrs for both unpack and
repack operations.

Closes #248
Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar cyphar force-pushed the cyphar:nfs-acl-xattr branch 8 times, most recently from 23704f4 to aa8200d Aug 29, 2018

@cyphar

This comment has been minimized.

Copy link
Member Author

cyphar commented Aug 31, 2018

Alright, this should fix the issue. @besnardjb did you want to test this?

@besnardjb

This comment has been minimized.

Copy link

besnardjb commented Aug 31, 2018

Hi @cyphar,

Thank you very much for the pull request, it is indeed much cleaner.

I've made a quick test on the Centos OCI image (from docker) and the NFS part is solved for me when extracting on shared FS with your configurable filter.

However, I've encountered another attribute (user.rootlesscontainers) in this same image. As a consequence, I still fail on lsetxattr as attributes are not supported on the target file-system (details below).

Here is how it failed and how I got there:

$ mount | grep raid
(...):/media/raid on /media/raid type nfs4 (rw,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=10.19.213.42,local_lock=none,addr=10.19.213.127)
$ pwd
/media/raid/phomes/jbbesnard/(...)/umoci
$ git describe                                                                                           
v0.4.1-19-gaa8200d
$ skopeo copy docker://centos oci:centos:latest
(...)
$ LANG=us strace -f -e trace=lsetxattr ./umoci --log debug  unpack --rootless --image ./centos ./centos_bundle
(...)
   • rootless{run/} ignoring forbidden xattr: "system.nfs4_acl"
   • unpacking entry           path=run/lock root=centos_bundle/rootfs type=53
   • rootless{run/} ignoring forbidden xattr: "system.nfs4_acl"
   • unpacking entry           path=run/lock/lockdev root=centos_bundle/rootfs type=53
[pid  4563] lsetxattr("centos_bundle/rootfs/run/lock/lockdev", "user.rootlesscontainers", "\10\377\377\377\377\17\0206", 8, 0) = -1 EOPNOTSUPP (Operation not supported)
   • rootless{lock/} ignoring forbidden xattr: "system.nfs4_acl"
   ⨯ create runtime bundle: unpack rootfs: unpack layer: unpack entry: run/lock/lockdev: apply hdr metadata: restore xattr metadata: centos_bundle/rootfs/run/lock/lockdev: unpriv.lsetxattr: operation not supported
(...)
+++ exited with 1 +++
#Same on alpine:
[pid  4956] lsetxattr("alpine_bundle/rootfs/etc/shadow", "user.rootlesscontainers", "\10\377\377\377\377\17\20*", 8, 0) = -1 EOPNOTSUPP (Operation not supported)

As my FS returns EOPNOTSUPP on such calls, I fail on extracting the image. In fact, it seems that exrtended attributes are not supported on NFS v4.

So (and to my knowledge) I see some ways here:

  • I manually add "user.rootlesscontainers" to the ban list as I'm on NFS but it would surely be useful on systems supporting it;
  • Add a flag basically telling "do your best I WANT this rootfs at all costs" 🤣 -- permissive;
  • As the target FS says that it does not support xattrs (EOPNOTSUPP) umoci emits a warning that information is being lost and just keep going on.

The later was my strategy here (besnardjb@612c1d2) although the implementation is clearly disputable 😄 !

// On NFS altering extended attrs may not be supported
if errors.Cause(err).Error() == "operation not supported" {
    continue
}

Thanks for your great work !

I remain available.

Cheers.

@cyphar

This comment has been minimized.

Copy link
Member Author

cyphar commented Aug 31, 2018

Ah okay. Yeah, we should ignore (but print a warning) on ENOTSUP. The string comparison is not the best way of doing it -- but I do understand what you mean. I'll work on an updated patch for that.

@cyphar cyphar force-pushed the cyphar:nfs-acl-xattr branch from aa8200d to aaafa0b Aug 31, 2018

@cyphar

This comment has been minimized.

Copy link
Member Author

cyphar commented Aug 31, 2018

Okay, I've pushed a new patch that should fix it. Ignore the test failure, I'm working on that separately.

@besnardjb

This comment has been minimized.

Copy link

besnardjb commented Sep 1, 2018

I confirm this solved my issue on both images for an NFS extract. Thanks!

@cyphar

This comment has been minimized.

Copy link
Member Author

cyphar commented Sep 2, 2018

Cool. I will merge this as soon as I've bumped the test coverage enough to make the tests succeed again. I'll also make a new release soon (sometime next week hopefully).

vendor: switch to github.com/rootless-containers/proto@v0.1.0
This obsoletes the need for a dedicated pkg/rootlesscontainers-proto
(because I effectively just copied the sources). This allows us to
increase our test coverage as well as remove the need to keep
rootlesscontainers.proto in sync with upstream (we get that for free
because we now use the canonical repository for the .proto).

Signed-off-by: Aleksa Sarai <asarai@suse.de>

@cyphar cyphar force-pushed the cyphar:nfs-acl-xattr branch 2 times, most recently from f4a302a to 6a28d78 Sep 3, 2018

cyphar added some commits Aug 29, 2018

oci: ignore system.nfs4_acl and extend forbidden-xattr handling
It turns out that system.nfs4_acl is yet another xattr that we shouldn't
be touching, but also that we should not be clearing (while we have
permission to do so, clearing it results in NFS permissions breaking).
So as an extension we now now use ignoreXattrs for both unpack and
repack operations.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
test: add a few more unpack tests
This is to bump coverage and to make sure we test things that not all
distributions ship (also include some fixes to previous tests -- in a
future patchset I will add some further systemic fixes to avoid
$BUNDLE_* bugs which appear to be common).

Signed-off-by: Aleksa Sarai <asarai@suse.de>

@cyphar cyphar force-pushed the cyphar:nfs-acl-xattr branch from 6a28d78 to 1428960 Sep 3, 2018

layer: ignore ENOTSUP on setxattr
This mirrors existing behaviour within Docker or similar tools, where we
ignore lsetxattr(2) failures because there's not much you can do if the
filesystem doesn't support it.

Signed-off-by: Aleksa Sarai <asarai@suse.de>

@cyphar cyphar force-pushed the cyphar:nfs-acl-xattr branch from 1428960 to 88e46ad Sep 3, 2018

@cyphar

This comment has been minimized.

Copy link
Member Author

cyphar commented Sep 3, 2018

LGTM.

@cyphar cyphar merged commit 88e46ad into openSUSE:master Sep 3, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details

cyphar added a commit that referenced this pull request Sep 3, 2018

merge branch 'pr-252'
  layer: ignore ENOTSUP on setxattr
  oci: ignore system.nfs4_acl and extend forbidden-xattr handling
  test: add a few more unpack tests
  vendor: switch to github.com/rootless-containers/proto@v0.1.0

LGTMs: @cyphar
Closes #252

@cyphar cyphar deleted the cyphar:nfs-acl-xattr branch Sep 3, 2018

@cyphar cyphar added this to the 0.4.2 milestone Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment