Extend xattrs support for security.* and user.pax.*#40855
Extend xattrs support for security.* and user.pax.*#40855kstruczy wants to merge 3 commits intomoby:masterfrom
Conversation
Copy whitelisted extended attributes to the tar header. Before this patch, only security.capability xattrs were copied. After this patch, all xattrs from user.pax. and security. namespaces are copied. addresses moby#35699 Before this patch: DOCKER_BUILDKIT=0 docker build --no-cache -<<'EOF' FROM alpine RUN apk add --no-cache attr RUN touch /testfile \ && setfattr -n user.pax.flags -v ok /testfile RUN test "$(getfattr -n user.pax.flags --only-values testfile)" = "ok" EOF getfattr: testdir/testfile: No such file or directory The command '/bin/sh -c test "$(getfattr -n user.pax.flags --only-values testdir/testfile)" = "ok"' returned a non-zero code: 1 With this patch: DOCKER_BUILDKIT=0 docker build --no-cache -<<'EOF' FROM alpine RUN apk add --no-cache attr RUN touch /testfile \ && setfattr -n user.pax.flags -v ok /testfile RUN test "$(getfattr -n user.pax.flags --only-values testfile)" = "ok" EOF Successfully built 5ecde85b467d Fixes moby#35699 Fixes moby#40375 Co-developed-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Copy extended attributes in CopyFileWithTar(). This function is used
e.g. by the builder. After this patch, xattrs of the host file copied to
the image are preserved.
Before this patch:
touch ./testfile
setfattr -n user.pax.flags -v ok ./testfile
DOCKER_BUILDKIT=0 docker build --no-cache -<<'EOF'
FROM ubuntu
RUN apt update && apt install attr
COPY testfile /testfile
RUN test "$(getfattr -n user.pax.flags --only-values /testfile)" = "ok"
EOF
/testfile: user.pax.flags: No such attribute
The command '/bin/sh -c test "$(getfattr -n user.pax.flags --only-values /testfile)" = "ok"' returned a non-zero code: 1
With this patch:
touch ./testfile
setfattr -n user.pax.flags -v ok ./testfile
DOCKER_BUILDKIT=0 docker build --no-cache -<<'EOF'
FROM ubuntu
RUN apt update && apt install attr
COPY testfile /testfile
RUN test "$(getfattr -n user.pax.flags --only-values /testfile)" = "ok"
EOF
Successfully built 9b71baf4bb4e
Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Store extended attributes in the PAXRecords in the SCHILY.xattr namespace in the tar header. Xattr field in the tar header is deprecated, but if it exists it takes precedence over PAXRecords. Save xattrs in both places for backward compatibility. Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
f8d4035 to
41b3009
Compare
|
Applying |
|
That's a good point @AkihiroSuda . Do you mean EPERM error, e.g. if user has no SYS_ADMIN capabilities or something more? EPERM is already handled in the archiver (logged and ignored), it would probably be good to correct warning message though. Thanks for looking at this. |
|
I guess EINVAL may also happen under some circumstances |
|
I'd like to have @tonistiigi to have some eyes on this as well; we were discussing the changes in a meeting recently, and he had some concerns about having a "wildcard" |
| ) | ||
|
|
||
| const ( | ||
| paxSchilyXattr = "SCHILY.xattrs." |
There was a problem hiding this comment.
@kstruczy according to golang src code - archive/tar/common.go#L110, I think we should use "SCHILY.xattr." but not "SCHILY.xattrs." here?
There was a problem hiding this comment.
That's a good spot, I will fix it. Thanks
|
@AkihiroSuda I'm not sure when exactly EINVAL may be returned when it shouldn't, but I guess in principle you are right, if it's not EINVAL it can be ERANGE etc. Maybe a better way is to define a whitelist of xattrs that are expected to be supported/portable (if any), and a separate list of xattrs that are optional and copied on demand. That would be more consistent with cp, tar etc. in linux, where preserving xattrs is optional. That may involve adding a flag to all commands that may need it and there are many: image push/pull, save/load, import/export, commit, etc. I'm not sure if it's feasible? Maybe better a whitelist of such optional xattrs can be defined by the user e.g. in the docker daemon's config file? At least it's manageable at the host level. @tonistiigi , @thaJeztah do you have any thoughts on that? Thanks. |
Fixes #35699
Fixes #40375
Based on [WIP] PR from @thaJeztah :
#40087
- What I did
- How I did it
- How to verify it
Run TestBuildPreserveXattrs
Run test described here:
#40375
More information in commit messages
- Description for the changelog
Added archiver's support for xattrs other than security.capability
- A picture of a cute animal (not mandatory but encouraged)