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.

Sign up for GitHub

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

Jump to bottom

userns: add new option --userns=keep-id #3196

Merged
merged 3 commits into from
May 25, 2019
Merged

userns: add new option --userns=keep-id #3196

merged 3 commits into from
May 25, 2019

Conversation

giuseppe
Copy link
Member

it creates a namespace where the current UID:GID on the host is mapped to the same UID:GID in the container.

$ id -u; id -g
1000
1000
$ podman run --userns=keep-id --rm fedora cat /proc/self/uid_map
         0          1       1000
      1000          0          1
      1001       1001      64536

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

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2019
@openshift-ci-robot
Copy link
Collaborator

[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:

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels May 23, 2019
@giuseppe
Copy link
Member Author

@debarshiray @rhatdan

@giuseppe giuseppe changed the title [WIP] userns: add new option --userns=keep-id userns: add new option --userns=keep-id May 24, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2019
@giuseppe
Copy link
Member Author

@debarshiray if you have a chance, could you try if it helps with fedora-toolbox?

@rhatdan
Copy link
Member

rhatdan commented May 24, 2019

Can you add an environment variable to make this the default.
PODMAN_USERNS_KEEPUID=1
Or something like that.

@giuseppe
Copy link
Member Author

Can you add an environment variable to make this the default.
PODMAN_USERNS_KEEPUID=1
Or something like that.

could it be in the configuration file?

@rhatdan
Copy link
Member

rhatdan commented May 24, 2019

Podman does not have a configuration file, (Well not one I want to expose.) Currently we change defaults via environment variables.

@giuseppe
Copy link
Member Author

Podman does not have a configuration file, (Well not one I want to expose.) Currently we change defaults via environment variables.

I've added a patch where you can override the --userns default value from the environment variable:

$ PODMAN_USERNS=keep-id ./bin/podman run --rm fedora cat /proc/self/uid_map
         0          1       1000
      1000          0          1
      1001       1001      64536

debarshiray
debarshiray reviewed May 24, 2019
View reviewed changes
cmd/podman/shared/create.go Outdated
@@ -450,6 +451,12 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.

// USER
user := c.String("user")
if usernsMode.IsKeepID() {
if user != "" {
return nil, errors.Errorf("cannot specify --user with --userns=keep-id")
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary?

Currently the toolbox script uses podman create --user root:root --userns=keep-id .... The --user root:root is for the container's entry point process, which calls useradd, sets up the symlinks for /etc/resolv.conf, etc. and needs to run as root inside the container.. The --userns=keep-id is used so that an interactive shell spawned via podman exec runs as the same UID as the host.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm no, I will relax this limitation if it is causing problems to toolbox

Copy link
Member Author

Choose a reason for hiding this comment

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

can you check the new version?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's fine now. Thanks.

TomSweeneyRedHat
TomSweeneyRedHat reviewed May 24, 2019
View reviewed changes
docs/podman-run.1.md
**--userns**=ns:my_namespace

Set the user namespace mode for the container. The use of userns is disabled by default.
Set the user namespace mode for the container. It defaults to the **PODMAN_USERNS** environment variable. An empty value means user namespaces are disabled.
Copy link
Member

Choose a reason for hiding this comment

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

if you take create comments, ditto here.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed both

@TomSweeneyRedHat
Copy link
Member

Rootless tests look like they need some love still.

giuseppe added 2 commits May 24, 2019 17:34
@giuseppe
userns: add new option --userns=keep-id
f09370c 
it creates a namespace where the current UID:GID on the host is mapped
to the same UID:GID in the container.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
podman: honor env variable PODMAN_USERNS
5eb321a 
Signed-off-by: Giuseppe Scrivano <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented May 25, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2019
@rhatdan
Copy link
Member

rhatdan commented May 25, 2019

@giuseppe what happens when you do a podman build with this set? Does it continue to work correctly?

@openshift-merge-robot openshift-merge-robot merged commit b1d590b into containers:master May 25, 2019
@rh-atomic-bot rh-atomic-bot mentioned this pull request May 25, 2019
Merged
@mheon mheon mentioned this pull request May 29, 2019
Closed
@debarshiray debarshiray mentioned this pull request Sep 2, 2019
Closed
@jdoss jdoss mentioned this pull request May 12, 2020
Closed
@kriansa kriansa mentioned this pull request Aug 29, 2021
Closed
@Trass3r Trass3r mentioned this pull request Sep 3, 2021
Closed
@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 Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@debarshiray debarshiray debarshiray left review comments

@baude baude Awaiting requested review from baude

@vrothberg vrothberg Awaiting requested review from vrothberg

Assignees

@rhatdan rhatdan

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@giuseppe @openshift-ci-robot @rhatdan @TomSweeneyRedHat @debarshiray @openshift-merge-robot