/moby

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

Implement capabilitiy bit storage in layers #3845

Merged
merged 4 commits into from Feb 20, 2014

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@alexlarsson @crosbymichael @tianon @vieux @unclejack @creack
@alexlarsson
Contributor

alexlarsson commented Jan 30, 2014

This adds support for storing the capability bits of files in docker layers.
The capability bits are stored in the security.capabilites extended attributes, so i added support for xattrs to tar/archive, which is needed for this. The upstream patch is here:
https://codereview.appspot.com/54570043/
And the pull request for github.com/dotcloud/tar which is used for now is here:
https://github.com/dotcloud/tar/pull/2

This is important for fedora, that use capability bits in many rpms to avoid setuid binaries.
A simple way to verify this is to do:

# docker run -t -i fedora sh
# yum install -y attr httpd
# attr -l /usr/sbin/suexec 
Attribute "selinux" has a 32 byte value for /usr/sbin/suexec
Attribute "capability" has a 20 byte value for /usr/sbin/suexec
# exit
# docker commit $id httpd-caps
# docker run -t -i httpd-caps sh
# attr -l /usr/sbin/suexec 
Attribute "selinux" has a 32 byte value for /usr/sbin/suexec
Attribute "capability" has a 20 byte value for /usr/sbin/suexec
# exit

This verifies that the installation of httpd created a /usr/sbin/suexec with a capability bit set, and commiting the container to an image and then launching that kept the capabilities.
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 30, 2014

Contributor

We need to wait to merge this after we figure out a solution with the distro packaging maintainers. I'll keep things updated here.

cc @tianon
Contributor

crosbymichael commented Jan 30, 2014

We need to wait to merge this after we figure out a solution with the distro packaging maintainers. I'll keep things updated here.

cc @tianon
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jan 30, 2014

Contributor

Upstream tar issue is: https://code.google.com/p/go/issues/detail?id=7154
Contributor

alexlarsson commented Jan 30, 2014

Upstream tar issue is: https://code.google.com/p/go/issues/detail?id=7154

@tianon tianon referenced this pull request Feb 13, 2014

Merged

Add vendored archive/tar that includes xattrs patch #4131

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 14, 2014

Contributor

I rebased this to include the new vendored archive/tar from #4131
Contributor

alexlarsson commented Feb 14, 2014

I rebased this to include the new vendored archive/tar from #4131
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 14, 2014

Contributor

(Note that the vendoring doesn't quite seem to work yet)
Contributor

alexlarsson commented Feb 14, 2014

(Note that the vendoring doesn't quite seem to work yet)
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 17, 2014

Contributor

rebased on the now landed, working vendored archive/tar
Contributor

alexlarsson commented Feb 17, 2014

rebased on the now landed, working vendored archive/tar
@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Feb 17, 2014

Member

Looks like "archive/stat_linux.go" needs a gofmt -w -s.
Member

tianon commented Feb 17, 2014

Looks like "archive/stat_linux.go" needs a gofmt -w -s.
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 17, 2014

Contributor

fixed
Contributor

alexlarsson commented Feb 17, 2014

fixed
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Feb 19, 2014

Contributor

ping @vieux
Contributor

crosbymichael commented Feb 19, 2014

ping @vieux
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Feb 19, 2014

Contributor

@vieux can you take a look at this?
Contributor

crosbymichael commented Feb 19, 2014

@vieux can you take a look at this?
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Feb 19, 2014

Collaborator

@alexlarsson I used your example:

instead of:

Attribute "selinux" has a 32 byte value for /usr/sbin/suexec
Attribute "capability" has a 20 byte value for /usr/sbin/suexec

I get Attribute "capability" has a 20 byte value for /usr/sbin/suexec in the first image.
and in the second Attribute "capability" has a 128 byte value for /usr/sbin/suexec

Is that normal ? (20/128)
Collaborator

vieux commented Feb 19, 2014

@alexlarsson I used your example:

instead of:

Attribute "selinux" has a 32 byte value for /usr/sbin/suexec
Attribute "capability" has a 20 byte value for /usr/sbin/suexec

I get Attribute "capability" has a 20 byte value for /usr/sbin/suexec in the first image.
and in the second Attribute "capability" has a 128 byte value for /usr/sbin/suexec

Is that normal ? (20/128)

alexlarsson added some commits Jan 15, 2014

@alexlarsson
archive: extract xattrs from tarfiles 
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
c8428d7
@alexlarsson
archive: Detect file changes to capability bits 
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
87ca750
@alexlarsson
archive: Handle capabilities in tar files 
If a file has a security.capability set, we push this to the tar file.
This is important to handle in e.g. layer files or when copying files
to containers, as some distros (e.g. Fedora) use capability bits as
a more finegrained version of setuid bits, and thus if the capabilites
are stripped (and setuid is not set) the binaries will fail to work.

Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
3b99539
@alexlarsson
Archive: Add Add Lgetxattr and Lsetxattr implementations 
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
d19e998
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 20, 2014

Contributor

@vieux Gah, sorry about that. I made a change in the getxattr call in the rebase which i didn't test enough.
Should work now.
Contributor

alexlarsson commented Feb 20, 2014

@vieux Gah, sorry about that. I made a change in the getxattr call in the rebase which i didn't test enough.
Should work now.
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Feb 20, 2014

Collaborator

@alexlarsson I still don't have the Attribute "selinux"... but I guess it's normal.

At least before/after I have the same thing => LGTM
Collaborator

vieux commented Feb 20, 2014

@alexlarsson I still don't have the Attribute "selinux"... but I guess it's normal.

At least before/after I have the same thing => LGTM
@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Feb 20, 2014

Contributor

Attribute "capability" has a 20 byte value for /usr/sbin/suexec is what I got before and after commit.

LGTM
Contributor

unclejack commented Feb 20, 2014

Attribute "capability" has a 20 byte value for /usr/sbin/suexec is what I got before and after commit.

LGTM
@creack

This comment has been minimized.

Show comment
Hide comment
@creack

creack Feb 20, 2014

Contributor

LGTM
Contributor

creack commented Feb 20, 2014

LGTM
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Feb 20, 2014

Contributor

@vieux well, you don't have the selinux context because you don't use selinux. No worries, I won't tell Dan about it...
Contributor

alexlarsson commented Feb 20, 2014

@vieux well, you don't have the selinux context because you don't use selinux. No worries, I won't tell Dan about it...

vieux added a commit that referenced this pull request Feb 20, 2014

@vieux
Merge pull request #3845 from alexlarsson/tar-caps 
Implement capabilitiy bit storage in layers
a690908

@vieux vieux merged commit a690908 into moby:master Feb 20, 2014
1 check passed

1 check passed

default The Travis CI build passed
Details

@alexlarsson alexlarsson deleted the alexlarsson:tar-caps branch Mar 3, 2014

@gopherbot gopherbot referenced this pull request Dec 8, 2014

Closed

archive/tar: Add support for extended attributes #7154

@thaJeztah thaJeztah referenced this pull request Dec 5, 2017

Open

docker build does not preserve xattrs in the generated image #35699

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