Skip to content
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

cyphar
Copy link
Member

@cyphar 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 nfs-acl-xattr branch 8 times, most recently from 23704f4 to aa8200d Compare August 31, 2018 16:36
@cyphar
Copy link
Member Author

cyphar commented Aug 31, 2018

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

@besnardjb
Copy link

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
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
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
Copy link

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

@cyphar
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).

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 nfs-acl-xattr branch 2 times, most recently from f4a302a to 6a28d78 Compare September 3, 2018 17:35
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>
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>
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
Copy link
Member Author

cyphar commented Sep 3, 2018

LGTM.

@cyphar cyphar merged commit 88e46ad into opencontainers:master Sep 3, 2018
cyphar added a commit that referenced this pull request Sep 3, 2018
  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 nfs-acl-xattr branch September 3, 2018 18:44
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants