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

libpod: do not parse --hostuser in base 8 #19816

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

giuseppe
Copy link
Member

fix the parsing of --hostuser to treat the input in base 10.

Closes: #19800

Does this PR introduce a user-facing change?

Now --hostuser correctly parse the user id in base 10, not in base 8

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
test/system/foo.bats Outdated Show resolved Hide resolved
fix the parsing of --hostuser to treat the input in base 10.

Closes: containers#19800

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

# find a user with a uid > 100 that is a valid octal
# Issue #19800
octal_user=$(awk -F\: '$1!="nobody" && $3>100 && $3~/^[0-7]+$/ {print $1 " " $3; exit}' /etc/passwd)
Copy link
Member

Choose a reason for hiding this comment

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

Can we just add/commit a user octal_user to a container image, are we guaranteed to have an image with such a user?

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't be able to run this test only with that image and cause it to fail on any other system where the user is not present?

Copy link
Member

Choose a reason for hiding this comment

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

I am saying you do it with a Containerfile

from $IMAGE
run useradd -u 8000 test-user

podman build -t testimage .
podman run --user test-user testimage

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried and it won't work with --hostuser, it fails with:

Error: creating temporary passwd file for container ff2b26bee3529ff2bd393b0a56b2f47ea98b9d134ed240ce43a91caa490cee25: user: unknown user test-user

Copy link
Member

Choose a reason for hiding this comment

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

Ok so the host user account must exist for this to work. So only a container within a container would work.

@antifuchs
Copy link

antifuchs commented Aug 31, 2023

Thanks for taking this on! I'm trying the fix now, but I have a suspicion that base 8 might still be used in writing the new /etc/passwd entry; will try to validate that real quick. strike that suspicion, it works perfectly - correctly translates the valid-octal user's ID and correctly writes the UID to /etc/passwd, as well as correctly translating the not-valid-octal user's ID.

@mheon
Copy link
Member

mheon commented Sep 1, 2023

LGTM

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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-merge-robot openshift-merge-robot merged commit 99a5347 into containers:main Sep 1, 2023
93 checks passed
@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 Dec 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
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.

UIDs and GID are handled in base 8 (incorrectly)
6 participants