Skip to content
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

Support lookup of intermediate ID for uidmapping and gidmapping in --userns=auto #20827

Merged

Conversation

kaivol
Copy link
Contributor

@kaivol kaivol commented Nov 29, 2023

In rootless mode, the host IDs in the uidmapping and gidmapping options of --userns=auto are mapped from an intermediate namespace, just like when using the --uidmap and --gidmap options.
This means that for a given host ID, the user first has to look up the intermediate ID manually.

The --uidmap and --gidmap options also support prefixing the host ID in the mapping with the @ symbol, which means that podman will look up the intermediate ID corresponding to the given host ID and it will map the found intermediate ID to the given container ID.

This PR adds the functionality mentioned above to the uidmapping and gidmapping options in --userns=auto.

`uidmapping` and `gidmapping` options of `--userns=auto` in rootless mode: 
Added the possibility to map actual host IDs (instead of intermediate IDs) by prefixing the host ID 
with the `@` symbol. 

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@giuseppe
Copy link
Member

the build is failing

@kaivol kaivol force-pushed the userns-auto-intermediate-id-lookup branch 2 times, most recently from 4b19bb4 to a2793ae Compare November 29, 2023 17:42
@kaivol kaivol force-pushed the userns-auto-intermediate-id-lookup branch from a2793ae to d5cf46e Compare November 29, 2023 18:04
@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

@giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, but can we please add a system test?

You can add more tests along the lines of:

diff --git a/test/system/170-run-userns.bats b/test/system/170-run-userns.bats
index 667648e5f..c4fcac3ed 100644
--- a/test/system/170-run-userns.bats
+++ b/test/system/170-run-userns.bats
@@ -147,3 +147,12 @@ EOF
     is "${output}" "$user" "Container should run as the current user"
     run_podman rmi -f $(pause_image)
 }
+
+@test "podman userns=auto with id mapping" {
+    skip_if_not_rootless
+    run_podman unshare awk '{if(NR == 2){print $2}}' /proc/self/uid_map
+    first_id=$output
+    mapping=1:@$first_id:1
+    run_podman run --rm --userns=auto:uidmapping=$mapping $IMAGE awk '{if($1 == 1){print $2}}' /proc/self/uid_map
+    assert "$output" == 1
+}

to run it, you can use the command:

$ PODMAN=bin/podman bats -f "podman userns=auto with id mapping" test/system/170-run-userns.bats

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023
@edsantiago
Copy link
Member

to run it, you can use the command:

$ PODMAN=bin/podman bats -f "podman userns=auto with id mapping" test/system/170-run-userns.bats

...or, even quicker/safer/better:

$ hack/bats 170:"auto with id mapping"

Better for infinite reasons, among them: it runs as root & rootless, and does a lot of invisible setup. Also obviously much more friendly.

Signed-off-by: kaivol <[email protected]>
@kaivol
Copy link
Contributor Author

kaivol commented Dec 10, 2023

Added system test as suggested by @giuseppe.
I'm not sure if I should add more tests..?

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

openshift-ci bot commented Dec 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, kaivol

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:

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

@rhatdan
Copy link
Member

rhatdan commented Dec 11, 2023

/lgtm
Thanks @kaivol

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 611ba2f into containers:main Dec 11, 2023
85 of 90 checks passed
@edsantiago
Copy link
Member

This broke CI.

edsantiago added a commit to edsantiago/libpod that referenced this pull request Dec 11, 2023
Broken by containers#20827.

Signed-off-by: Ed Santiago <[email protected]>
@kaivol
Copy link
Contributor Author

kaivol commented Dec 11, 2023

Just for my understanding, it was only the test that was erroneous, and that was fixed in #20979, right?

@edsantiago
Copy link
Member

@kaivol yes, absolutely. You did nothing wrong, and the problem is (I think) now fixed. I spoke hastily this morning because I was rushing to a commitment. I apologize for leaving you in suspense like that.

Thank you again for your PR and your patience.

@github-actions 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 Mar 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants