libpod: support idmap for --rootfs #17274

giuseppe · 2023-01-30T09:28:21Z

Does this PR introduce a user-facing change?

allows to specify the option ":idmap" to the --rootfs argument

Now --rootfs supports the ":idmap" option to create an idmapped mount on the rootfs directory.

openshift-ci · 2023-01-30T09:28:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-01-30T11:35:33Z

docs/source/markdown/options/rootfs.md

+
+    The `idmap` option if specified, create an idmapped mount to the target user
+namespace in the container.
+The idmap option supports a custom mapping that can be different than the user


Why do you want this? When would you want the idmapping to be differnt the User Namespace mapping?

some users are using it as a way to restrict access to the file system to certain IDs, we had a request for crun: containers/crun#873

rhatdan · 2023-01-30T11:36:21Z

go.mod

@@ -177,3 +177,5 @@ require (
 )

 replace github.com/opencontainers/runc => github.com/opencontainers/runc v1.1.1-0.20220617142545-8b9452f75cbc
+
+replace github.com/containers/storage => github.com/giuseppe/storage v1.19.2-0.20230130105941-634c638e5c48


Isn't this already merged?

it is failing on freebsd, so I'll need another patch

Luap99 · 2023-01-30T12:33:31Z

pkg/specgen/specgen.go

-		lastColonIndex := strings.LastIndex(csc.Rootfs, ":")
-		if lastColonIndex != -1 && lastColonIndex+1 < len(csc.Rootfs) && csc.Rootfs[lastColonIndex+1:] == "O" {
-			csc.RootfsOverlay = true
-			csc.Rootfs = csc.Rootfs[:lastColonIndex]
+		parts := strings.SplitN(arg, ":", 2)
+		if len(parts) == 2 {
+			for _, opt := range strings.Split(parts[1], ",") {
+				switch {
+				case (opt == "idmap" || strings.HasPrefix(opt, "idmap=")):
+					csc.RootfsMapping = &opt
+				case opt == "O":
+					csc.RootfsOverlay = true
+				default:
+					return nil, fmt.Errorf("unknown option %q", opt)
+				}
+			}
+			csc.Rootfs = parts[0]


This will break backwards compat, now it will fail if a path contains a colon. As I understand the old code carefully checked that only :O was interpreted as option and everything else considered a regular path.

I don't think we can do this if we a serious about backwards compat. Should we add a new flag for this?

yes I know we break that, but that is a bit weird corner case and we already use : as separator for other options (e.g. --volume). The previous code was not that much clearer where we only threated :O as an option and everything else as part of the path. I'd not worry about adding another option just to change this behavior.

How do you envision somebody using a path with a colon
#11913, we should not break them again!

We currently do not support --volume SOURCE or DEST with ':' so I do not see this as a breaking change especially where we have support for ':0'. Sure theoretically they could have a rootfs with a ':' but I don't think this is worth worrying about. We can release not it, and be done.

This is breaking and at least one user depends on it see the linked issue #11913.
For volumes there is --mount which allows paths to contain a colon.

should we just add another exception for "idmap" and avoid arguments?

@rhatdan what do you think?

supporting "idmap" and arguments is easy in the same way we handle "O" but it is a mess when we want to provide both options like :O,idmap

pkg/specgen/specgen.go

giuseppe · 2023-01-30T15:51:12Z

to get unblocked I'll need containers/storage#1489

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2023-02-01T20:24:55Z

ready for review

vrothberg

LGTM
@edsantiago PTAL

docs/source/markdown/options/rootfs.md

edsantiago · 2023-02-02T15:04:18Z

Test is failing on my f37 laptop, 6.0.9-300.fc37:

  # bin/podman image mount quay.io/libpod/testimage:20221018
   /var/lib/containers/storage/overlay/6c8641d4b8fa906ae3ef2a2f9a77421e2343f2e33364cb5ac3a95a2c4910e657/merged
   # bin/podman image unmount quay.io/libpod/testimage:20221018
   f5a99120db6452661930a1db3bf7390eec9b963f5f62c068fa32dc1d550afad3
   # bin/podman run --rm --userns=auto --rootfs /tmp/podman_bats.biwqmf/rootfs:idmap stat -L -c %u:%g /proc/self/exe
   Error: invalid argument

I can find nothing useful in the journal.

I have one more comment about cleanup; will write that soon

edsantiago · 2023-02-02T15:32:36Z

test/system/030-run.bats

+
+    # check if the underlying file system supports idmapped mounts
+    mkdir -p $PODMAN_TMPDIR/idmap-check
+    mount --bind -o X-mount.idmap='0:1:1' $PODMAN_TMPDIR $PODMAN_TMPDIR || skip "idmapped mounts not supported"


Suggestion, to avoid leaving behind a podman-mount:

- mount --bind -o X-mount.idmap='0:1:1' $PODMAN_TMPDIR $PODMAN_TMPDIR || skip "idmapped mounts not supported" + echo "$_LOG_PROMPT mount --bind -o X-mount.idmap=0:1:1 ..." + run mount --bind -o X-mount.idmap='0:1:1' $PODMAN_TMPDIR $PODMAN_TMPDIR + echo "$output" + if [[ $status -ne 0 ]]; then + run_podman image unmount $IMAGE + skip "idmapped mounts not supported" + fi +

It is not a "podman mount", it is a raw mount command. How can it leak (well assuming the script is not killed in the middle) if the next command is an "umount"?

I've moved the 'podman mount later, does the new version look fine to you?

Yes! That's a much better solution, thank you!

Test still fails on my laptop though. (Same EINVAL)

does the run mount --bind -o X-mount.idmap='0:1:1' $PODMAN_TMPDIR $PODMAN_TMPDIR command work fine on your laptop?

ah, mount will silently ignore the option if not supported :/

I need to figure out a different way to check the feature

I've not found any way to test it, so I am reusing the podman command and check its error message

edsantiago

Blocking until test-failure issue is resolved

add a new option idmap to --rootfs that works in the same way as it does for volumes. Signed-off-by: Giuseppe Scrivano <[email protected]>

edsantiago · 2023-02-02T22:40:39Z

LGTM this time - at least, it skips properly. Thanks.

(Sigh... we've gone three days without a cdn03 flake, but it just happened here.)

rhatdan · 2023-02-03T03:09:21Z

/lgtm

openshift-ci bot added the release-note label Jan 30, 2023

giuseppe changed the title ~~libpod: support idmap for --rootfs~~ [WIP] libpod: support idmap for --rootfs Jan 30, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 30, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2023

giuseppe force-pushed the idmap-rootfs branch 2 times, most recently from de3b4f9 to 54c88ef Compare January 30, 2023 11:06

rhatdan reviewed Jan 30, 2023

View reviewed changes

giuseppe force-pushed the idmap-rootfs branch 3 times, most recently from 12b31be to f76a114 Compare January 30, 2023 12:21

Luap99 reviewed Jan 30, 2023

View reviewed changes

giuseppe force-pushed the idmap-rootfs branch from f76a114 to caeaeaa Compare January 30, 2023 13:00

giuseppe force-pushed the idmap-rootfs branch from caeaeaa to 1f157d3 Compare January 30, 2023 20:26

github-actions bot removed the kind/api-change Change to remote API; merits scrutiny label Jan 30, 2023

giuseppe force-pushed the idmap-rootfs branch 2 times, most recently from ec06220 to f82893a Compare January 31, 2023 15:32

giuseppe added 3 commits February 1, 2023 09:26

vendor: update containers/storage

427d69c

Signed-off-by: Giuseppe Scrivano <[email protected]>

libpod: use GraphRoot for overlay upper dir

0e949bf

Signed-off-by: Giuseppe Scrivano <[email protected]>

utils: new conversion method

cbb45a6

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the idmap-rootfs branch from f82893a to 0de548e Compare February 1, 2023 08:54

giuseppe changed the title ~~[WIP] libpod: support idmap for --rootfs~~ libpod: support idmap for --rootfs Feb 1, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023

giuseppe force-pushed the idmap-rootfs branch from 0de548e to 881f9b7 Compare February 1, 2023 17:00

vrothberg reviewed Feb 2, 2023

View reviewed changes

docs/source/markdown/options/rootfs.md Outdated Show resolved Hide resolved

edsantiago reviewed Feb 2, 2023

View reviewed changes

edsantiago suggested changes Feb 2, 2023

View reviewed changes

giuseppe force-pushed the idmap-rootfs branch 2 times, most recently from 87f623d to 0251de7 Compare February 2, 2023 16:56

libpod: support idmap for --rootfs

2bb4c7c

add a new option idmap to --rootfs that works in the same way as it does for volumes. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the idmap-rootfs branch from 0251de7 to 2bb4c7c Compare February 2, 2023 21:35

openshift-ci bot assigned rhatdan Feb 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2023

openshift-merge-robot merged commit 78458e0 into containers:main Feb 3, 2023

This was referenced Feb 7, 2023

copying system image from manifest list: writing/storing blob: happened during read: unexpected EOF #17193

Open

placeholder issue for quay.io flakes #16973

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 12, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libpod: support idmap for --rootfs #17274

libpod: support idmap for --rootfs #17274

giuseppe commented Jan 30, 2023

openshift-ci bot commented Jan 30, 2023

rhatdan Jan 30, 2023

giuseppe Jan 30, 2023

rhatdan Jan 30, 2023

giuseppe Jan 30, 2023

Luap99 Jan 30, 2023

giuseppe Jan 30, 2023

Luap99 Jan 30, 2023

rhatdan Jan 30, 2023

Luap99 Jan 30, 2023

giuseppe Jan 30, 2023

giuseppe Jan 30, 2023

giuseppe commented Jan 30, 2023

giuseppe commented Feb 1, 2023

vrothberg left a comment

edsantiago commented Feb 2, 2023 •

edited

Loading

edsantiago Feb 2, 2023

giuseppe Feb 2, 2023

giuseppe Feb 2, 2023

edsantiago Feb 2, 2023

giuseppe Feb 2, 2023

giuseppe Feb 2, 2023

giuseppe Feb 2, 2023

edsantiago left a comment

edsantiago commented Feb 2, 2023

rhatdan commented Feb 3, 2023

libpod: support idmap for --rootfs #17274

libpod: support idmap for --rootfs #17274

Conversation

giuseppe commented Jan 30, 2023

Does this PR introduce a user-facing change?

openshift-ci bot commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Jan 30, 2023

giuseppe commented Feb 1, 2023

vrothberg left a comment

Choose a reason for hiding this comment

edsantiago commented Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago commented Feb 2, 2023

rhatdan commented Feb 3, 2023

edsantiago commented Feb 2, 2023 •

edited

Loading