Use v2 capabilities in layer archives by EricMountain · Pull Request #41724 · moby/moby

EricMountain · 2020-11-27T15:41:10Z

- What I did

When building images in a user-namespaced container, v3 capabilities are
stored including the root UID of the creator of the user-namespace.

This UID does not make sense outside the build environment however. If
the image is run in a non-user-namespaced runtime, or if a user-namespaced
runtime uses a different UID, the capabilities requested by the effective
bit will not be honoured by execve(2) due to this mismatch.

Instead, we convert v3 capabilities to v2, dropping the root UID on the
fly.

- How I did it

Patched ReadSecurityXattrToTarHeader() to automatically convert v3 capabilities to v2 by switching the version identifier and dropping the root UID data.

- How to verify it

This reproducer can be used.

Compared with the output in issue #41723 this produces:

    default: + docker run --rm capabilities-built-with-no-userns:1.0 /bin/bash -c '(/usr/local/bin/sleep-test infinity & ); sleep 1; grep Cap /proc/$(pgrep sleep-test)/status'
    default: CapInh:	00000000a80425fb
    default: CapPrm:	0000000000000400
    default: CapEff:	0000000000000400
    default: CapBnd:	00000000a80425fb
    default: CapAmb:	0000000000000000
    default: + docker run --rm capabilities-built-with-userns:1.0 /bin/bash -c '(/usr/local/bin/sleep-test infinity & ); sleep 1; grep Cap /proc/$(pgrep sleep-test)/status'
    default: CapInh:	00000000a80425fb
    default: CapPrm:	0000000000000400
    default: CapEff:	0000000000000400
    default: CapBnd:	00000000a80425fb
    default: CapAmb:	0000000000000000

So we see that in the 2nd case also execve(2) honoured the effective bit.

- Description for the changelog

Capabilities in image layers are stored in v2 format even when built inside a non-root user-namespace.

- A picture of a cute animal (not mandatory but encouraged)

justincormack · 2020-11-27T15:59:38Z

Hi, I would rather this went via buildkit first as that is where we are doing the work in build. We might port it to the legacy builder, but this is not where we are largely doing build development work.

EricMountain · 2020-11-27T16:43:30Z

@justincormack , thanks I'll take a look. Would you be able to point me to where in buildkit this should go?

Trying to get to grips with the buildkit code: IIUC buildkit leverages containerd to write out image layers, correct? Would this be the right place to make the equivalent change?

EricMountain · 2020-11-30T17:27:13Z

@justincormack I've come to realise BuildKit is using the same block of code to build the tarballs, so this PR is also the fix for BuildKit. Or am I missing something?

thaJeztah · 2020-11-30T17:44:05Z

@tonistiigi PTAL

AkihiroSuda · 2020-12-01T00:04:12Z

Could you add integration tests?

thaJeztah · 2020-12-01T09:48:40Z

@dmcgowan @estesp would containerd's archive package also need similar changes? https://github.com/containerd/containerd/tree/master/archive

EricMountain · 2020-12-01T09:59:38Z

@AkihiroSuda is there an image I can use in the integration tests that would support the getcap -n option? I checked debian:buster, one of the frozen images already used in integration tests, however it doesn't support -n.

I might be able to get away without this, however it would be a lot uglier: basically setcap in a build, then tar the file in a run (hoping debian:buster tar knows about serialising security.capabibilities) and check the version in the serialised data.

thaJeztah · 2020-12-01T12:01:31Z

Alpine's libcap looks to have it, but don't think we include it in the frozen images.

EricMountain · 2020-12-01T12:14:02Z

@thaJeztah in integration tests, are we supposed to use only the frozen images, or is it considered OK to use e.g. Alpine for cases such as this? From what I've seen, integration tests only use scratch or frozen images, so my guess was I'm not supposed to use anything else.

And if I should only use frozen images, would it be acceptable for me to add Alpine to the list?

EricMountain · 2020-12-02T14:12:12Z

Integration test implemented in #41740. Switching this PR to draft for now as it will require an update to the test once that has merged.

When building images in a user-namespaced container, v3 capabilities are stored including the root UID of the creator of the user-namespace. This UID does not make sense outside the build environment however. If the image is run in a non-user-namespaced runtime, or if a user-namespaced runtime uses a different UID, the capabilities requested by the effective bit will not be honoured by `execve(2)` due to this mismatch. Instead, we convert v3 capabilities to v2, dropping the root UID on the fly. Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

EricMountain · 2020-12-23T13:35:02Z

Test in #41740 merged, the fix is now ready for review.

EricMountain · 2021-01-11T17:26:21Z

Hi @thaJeztah , @AkihiroSuda might I get a review now the integration test PR has merged? Thanks!

thaJeztah

LGTM

ping @AkihiroSuda @tonistiigi PTAL

thaJeztah · 2021-01-12T14:00:49Z

do we need similar changes in containerd as well, @AkihiroSuda ?

AkihiroSuda · 2021-01-13T06:54:20Z

do we need similar changes in containerd as well, @AkihiroSuda ?

Better to have, but not urgent

thaJeztah · 2021-01-13T09:40:50Z

do we need similar changes in containerd as well, @AkihiroSuda ?

Better to have, but not urgent

@EricMountain perhaps you're interested in contributing to containerd as well? (Or to open a tracking issue, describing the problem)?

EricMountain · 2021-01-13T10:36:38Z

@EricMountain perhaps you're interested in contributing to containerd as well? (Or to open a tracking issue, describing the problem)?

@thaJeztah sure, I'll take a look.

tonistiigi · 2021-02-11T19:41:11Z

@AkihiroSuda Do we need to cherry-pick this? Is this breaking existing users? Should we strip to v3 part on extraction if it is node specific.

thaJeztah · 2021-02-15T08:59:28Z

@AkihiroSuda ^ could you have a look?

AkihiroSuda · 2021-02-15T09:24:54Z

Seems safe to cherry-pick, but if we can have v21.xx soon, I guess cherry-picking is not necessary.

AkihiroSuda · 2021-05-05T12:51:46Z

Cherry-picked as #42352

thaJeztah added area/builder Build area/images Image Distribution status/2-code-review labels Dec 1, 2020

EricMountain mentioned this pull request Dec 2, 2020

Dishonoured capabilities test #41740

Merged

EricMountain force-pushed the dishonoured-capabilities branch from d07ded3 to 72bf86a Compare December 2, 2020 14:00

EricMountain marked this pull request as draft December 2, 2020 14:12

EricMountain force-pushed the dishonoured-capabilities branch from 72bf86a to 01d3a17 Compare December 22, 2020 14:41

EricMountain force-pushed the dishonoured-capabilities branch from 01d3a17 to 95eb490 Compare December 23, 2020 13:17

EricMountain marked this pull request as ready for review December 23, 2020 13:33

thaJeztah approved these changes Jan 12, 2021

View reviewed changes

AkihiroSuda approved these changes Jan 13, 2021

View reviewed changes

AkihiroSuda merged commit 327daef into moby:master Jan 13, 2021

thaJeztah added this to the 20.10.3 milestone Jan 13, 2021

EricMountain deleted the dishonoured-capabilities branch January 13, 2021 10:36

thaJeztah added the process/cherry-pick label Feb 2, 2021

thaJeztah modified the milestones: 20.10.3, 21.xx Feb 2, 2021

AkihiroSuda mentioned this pull request May 5, 2021

[20.10 backport] Use v2 capabilities in layer archives #42352

Merged

AkihiroSuda added process/cherry-picked and removed process/cherry-pick labels May 5, 2021

rumpl mentioned this pull request Dec 14, 2023

c8d: Fix image commit with userns mapping #46938

Closed

Conversation

EricMountain commented Nov 27, 2020

Uh oh!

justincormack commented Nov 27, 2020

Uh oh!

EricMountain commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricMountain commented Nov 30, 2020

Uh oh!

thaJeztah commented Nov 30, 2020

Uh oh!

AkihiroSuda commented Dec 1, 2020

Uh oh!

thaJeztah commented Dec 1, 2020

Uh oh!

EricMountain commented Dec 1, 2020

Uh oh!

thaJeztah commented Dec 1, 2020

Uh oh!

EricMountain commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricMountain commented Dec 2, 2020

Uh oh!

EricMountain commented Dec 23, 2020

Uh oh!

EricMountain commented Jan 11, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 12, 2021

Uh oh!

AkihiroSuda commented Jan 13, 2021

Uh oh!

thaJeztah commented Jan 13, 2021

Uh oh!

EricMountain commented Jan 13, 2021

Uh oh!

tonistiigi commented Feb 11, 2021

Uh oh!

thaJeztah commented Feb 15, 2021

Uh oh!

AkihiroSuda commented Feb 15, 2021

Uh oh!

AkihiroSuda commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EricMountain commented Nov 27, 2020 •

edited

Loading

EricMountain commented Dec 1, 2020 •

edited

Loading