oci: layer: skip forbidden xattrs if unchanged #259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyphar
force-pushed
the
xattr-on-demand
branch
2 times, most recently
from
855b8ba to
7c73e71
Compare
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 <[email protected]>
cyphar
force-pushed
the
xattr-on-demand
branch
from
7c73e71 to
e5b152c
Compare
|
/cc @ganto |
It turns out that since SELinux auto-sets labels for every directory created, 6fd1e0e ("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 <[email protected]>
cyphar
force-pushed
the
xattr-on-demand
branch
from
e5b152c to
cc4461a
Compare
|
LGTM, after testing on Fedora 28 it all works. I'll do a release after this is merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out that since SELinux auto-sets labels for every directory
created, 6fd1e0e ("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.
Fixes #235
Signed-off-by: Aleksa Sarai [email protected]