Dishonoured capabilities test by EricMountain · Pull Request #41740 · moby/moby

EricMountain · 2020-12-02T13:58:06Z

- What I did

Tests #41723 so that the fix in #41724 can be verified.

- How I did it

Adds debian:bullseye as a frozen image: debian:buster doesn't support the -n option to getcap.

The test would be much harder to write otherwise, and much uglier.

As it stands:

We kick off a user-namespaced daemon.
Build an image in it with a file granted some capabilities and the effective bit set.
Save the image.
Start a new non-user-namespaced daemon.
Load the image.
Run it.

Since the fix isn't yet merged, the test is inverted to fail if the capabilities are found to be in V2 format (which is what we expect of the fix), and allow V3 (the current situation).

- How to verify it

TEST_FILTER=TestBuildUserNamespaceValidateCapabilitiesAreV2 make test-integration

- Description for the changelog

Tests #41723 - verifies capability format version for builds under a user-namespaced daemon.

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

@AkihiroSuda , @thaJeztah here is an integration test for #41724.

EricMountain · 2020-12-02T16:53:16Z

Back to the drawing board. I hadn't realised integration-cli was deprecated.

EricMountain · 2020-12-07T22:49:21Z

@AkihiroSuda , @thaJeztah here is an integration test for #41724... implemented in the right integration folder this time :)

thaJeztah · 2020-12-15T16:37:44Z

Dockerfile

        debian:buster@sha256:46d659005ca1151087efa997f1039ae45a7bf7a2cbbe2d17d3dcbda632a3ee9a \
+        debian:bullseye@sha256:7190e972ab16aefea4d758ebe42a293f4e5c5be63595f4d03a5b9bf6839a4344 \


Wondering if we should replace the :buster variant (and switch the buildpack-deps image accordingly as well)

@tianon wdyt?

Depends on how we use this image -- if we install packages, Bullseye is still a pretty heavily moving target (whereas Buster is "stable"), so using Bullseye will be more likely to introduce unrelated failures.

Looks like we don't typically invoke apt-get, so it'd probably be fine to update to bullseye for both, since if the image upstream built successfully there's a high chance it'll work for what's used here (which will be pinned by digest anyhow).

I added a commit to replace use of debian:buster with debian:bullseye.

thaJeztah · 2020-12-15T16:38:09Z

TESTING.md


+### Writing new integration tests
+
+Note the `integration-cli` tests are deprecated; new tests will be rejected by


thaJeztah · 2020-12-15T16:46:41Z

testutil/environment/environment.go

 }

+// IsUserNamespaceInKernel returns whether the kernel supports user namespaces
+func (e *Execution) IsUserNamespaceInKernel() bool {


Wondering; was IsUserNamespace() not working for this case?

IIUC IsUserNamespace() checks if we are running with a user-namespaced daemon right now. Whereas for the test I've written I'm interested in knowing if it's possible to run a user-namespaced daemon in the current test environment.

Ah, right.

Was also wondering if there wasn't an existing utility for this, but looks indeed that the existing ones check if it's enabled (not if it's supported); looks like the daemon does not actually check for that;

moby/daemon/daemon_unix.go

Lines 1167 to 1170 in cf31b96

func setupRemappedRoot(config *config.Config) (*idtools.IdentityMapping, error) {

if runtime.GOOS != "linux" && config.RemappedRoot != "" {

return nil, fmt.Errorf("User namespaces are only supported on Linux")

}

(perhaps something we should add)

For reference; this was copied from the equivalent utility in integration-cli;

moby/integration-cli/requirements_test.go

Lines 136 to 139 in 46cdcd2

func UserNamespaceInKernel() bool {

if _, err := os.Stat("/proc/self/uid_map"); os.IsNotExist(err) {

/*

* This kernel-provided file only exists if user namespaces are

( we should look at moving those remaining tests to integration as well)

tianon

LGTM 👍

(I think we should probably also update from buildpack-deps:buster to buildpack-deps:bullseye for consistency, but IMO that's orthogonal to this - the debian:buster -> debian:bullseye change was related. 😅)

Dockerfile.e2e

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

Capabilities are serialised in VFS_CAP_REVISION_3 when an image is built in a user-namespaced daemon, instead of VFS_CAP_REVISION_2. This adds a test for this, though it's currently wired to fail if the capabilities are serialised in VFS_CAP_REVISION_2 instead in this situation, since this is unexpected. Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

EricMountain · 2020-12-19T21:36:24Z

LGTM 👍

Thanks!

(I think we should probably also update from buildpack-deps:buster to buildpack-deps:bullseye for consistency, but IMO that's orthogonal to this - the debian:buster -> debian:bullseye change was related. 😅)

Actually considered including that, however indeed seemed too orthogonal to this PR.

thaJeztah

LGTM, thanks!

Looks like we don't typically invoke apt-get, so it'd probably be fine to update to bullseye for both, since if the image upstream built successfully there's a high chance it'll work for what's used here (which will be pinned by digest anyhow).

Actually considered including that, however indeed seemed too orthogonal to this PR.

Yes, we can do that in a follow-up; would probably be good to do, so that we can share the image-layers between the two (slightly faster building of the Dockerfile)

thaJeztah · 2020-12-22T12:16:09Z

testutil/environment/environment.go

 }

+// IsUserNamespaceInKernel returns whether the kernel supports user namespaces
+func (e *Execution) IsUserNamespaceInKernel() bool {


Ah, right.

Was also wondering if there wasn't an existing utility for this, but looks indeed that the existing ones check if it's enabled (not if it's supported); looks like the daemon does not actually check for that;

moby/daemon/daemon_unix.go

Lines 1167 to 1170 in cf31b96

func setupRemappedRoot(config *config.Config) (*idtools.IdentityMapping, error) {

if runtime.GOOS != "linux" && config.RemappedRoot != "" {

return nil, fmt.Errorf("User namespaces are only supported on Linux")

}

(perhaps something we should add)

For reference; this was copied from the equivalent utility in integration-cli;

moby/integration-cli/requirements_test.go

Lines 136 to 139 in 46cdcd2

func UserNamespaceInKernel() bool {

if _, err := os.Stat("/proc/self/uid_map"); os.IsNotExist(err) {

/*

* This kernel-provided file only exists if user namespaces are

( we should look at moving those remaining tests to integration as well)

EricMountain mentioned this pull request Dec 2, 2020

Use v2 capabilities in layer archives #41724

Merged

EricMountain force-pushed the dishonoured-capabilities-test branch from 15dfcf5 to 778bb9b Compare December 2, 2020 14:18

EricMountain marked this pull request as draft December 2, 2020 16:53

EricMountain force-pushed the dishonoured-capabilities-test branch 3 times, most recently from 1274be6 to 8d78fcf Compare December 7, 2020 21:04

EricMountain marked this pull request as ready for review December 7, 2020 21:05

EricMountain force-pushed the dishonoured-capabilities-test branch 2 times, most recently from 07bfc82 to 141d54c Compare December 7, 2020 21:21

thaJeztah reviewed Dec 15, 2020

View reviewed changes

tianon approved these changes Dec 17, 2020

View reviewed changes

Dockerfile.e2e Outdated Show resolved Hide resolved

EricMountain force-pushed the dishonoured-capabilities-test branch from 362a669 to d9aa70e Compare December 17, 2020 22:02

EricMountain added 5 commits December 18, 2020 07:51

Add debian:bullseye to frozen images

58a4c03

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

Add IsUserNamespaceInKernel() test environment condition

b314e90

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

TESTING.md: Fix path the test environment conditions

a0fc7a4

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

TESTING.md: note that integration-cli is deprecated

7ba1af3

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

EricMountain force-pushed the dishonoured-capabilities-test branch from d9aa70e to d5f09c7 Compare December 18, 2020 06:52

Move use of debian:buster frozen image to debian:bullseye

1c5806c

Signed-off-by: Eric Mountain <eric.mountain@datadoghq.com>

EricMountain force-pushed the dishonoured-capabilities-test branch from d5f09c7 to 1c5806c Compare December 19, 2020 16:59

thaJeztah added area/security/userns area/testing status/2-code-review labels Dec 22, 2020

thaJeztah approved these changes Dec 22, 2020

View reviewed changes

thaJeztah merged commit 4d6bc59 into moby:master Dec 22, 2020

thaJeztah added this to the 20.10.2 milestone Dec 22, 2020

EricMountain deleted the dishonoured-capabilities-test branch December 22, 2020 13:46

thaJeztah mentioned this pull request Jan 8, 2021

integration: fix TestBuildUserNamespaceValidateCapabilitiesAreV2 not using frozen image #41865

Merged

		debian:buster@sha256:46d659005ca1151087efa997f1039ae45a7bf7a2cbbe2d17d3dcbda632a3ee9a \
		debian:bullseye@sha256:7190e972ab16aefea4d758ebe42a293f4e5c5be63595f4d03a5b9bf6839a4344 \


		### Writing new integration tests

		Note the `integration-cli` tests are deprecated; new tests will be rejected by

	func setupRemappedRoot(config config.Config) (idtools.IdentityMapping, error) {
	if runtime.GOOS != "linux" && config.RemappedRoot != "" {
	return nil, fmt.Errorf("User namespaces are only supported on Linux")
	}

	func UserNamespaceInKernel() bool {
	if _, err := os.Stat("/proc/self/uid_map"); os.IsNotExist(err) {
	/*
	* This kernel-provided file only exists if user namespaces are

Conversation

EricMountain commented Dec 2, 2020

Uh oh!

EricMountain commented Dec 2, 2020

Uh oh!

EricMountain commented Dec 2, 2020

Uh oh!

EricMountain commented Dec 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EricMountain commented Dec 19, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants