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

ilab-wrapper: Run podman with sudo #713

Merged
merged 1 commit into from
Aug 2, 2024
Merged

Conversation

omertuc
Copy link
Contributor

@omertuc omertuc commented Aug 1, 2024

Solves RHELAI-740

Background

The ilab command is wrapped by an ilab script which launches ilab inside a podman container.

Issue

Since the ilab container image is pulled during the bootc image build process using the root user, the image is not accessible to non-root users.

Solution

We run the container as sudo in order to be able to access the root container storage. But for security reasons we map root UID 0 inside the container to the current user's UID (and all the other subuids to the user's /etc/subuid range) so that we're effectively running the container as the current user.

Additional changes

Changed "--env" "HOME" to "--env" "HOME=$HOME" to pass the HOME environment variable from the current shell and not from the sudo environment.

Future work

In the future, we will run podman as the current user, once we figure a reasonable way for the current user to access the root's user container storage

@omertuc omertuc force-pushed the sudo branch 4 times, most recently from 8d5fcf3 to dc2d390 Compare August 1, 2024 12:50
@omertuc omertuc marked this pull request as ready for review August 1, 2024 14:03
/etc/subuid)

# TODO: Handle multiple subuid ranges, for now, hard fail
if [[ $(echo "$CURRENT_USER_SUBUID_RANGE" | wc -l) != 1 ]]; then
Copy link
Member

@rhatdan rhatdan Aug 1, 2024

Choose a reason for hiding this comment

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

$(wc -l <<< ${CURRENT_USER_SUBUID_RANGE})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI this doesn't work because Here-Strings always have a newline appended to them

if [[ -z "$CURRENT_USER_SUBUID_RANGE" ]]; then
echo "No subuid range found for user $CURRENT_USER_NAME ($UID)"
else
echo "Multiple subuid ranges found for user $CURRENT_USER_NAME ($UID):"
Copy link
Member

Choose a reason for hiding this comment

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

Might want to use printf rather then echo.

Copy link
Member

Choose a reason for hiding this comment

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

Similar to above this should go to stderr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular reason you think we should use printf?

Copy link
Member

Choose a reason for hiding this comment

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

Makes writing multi line messages easier. But It is not a big deal.

training/ilab-wrapper/ilab Outdated Show resolved Hide resolved
@@ -8,21 +8,54 @@ export ENTRYPOINT="/opt/python3.11/venv/bin/ilab"
export PARAMS=("$@")

for dir in "$HOME/.cache" "$HOME/.config" "$HOME/.local"; do
Copy link
Member

Choose a reason for hiding this comment

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

Why is this duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhatdan
Copy link
Member

rhatdan commented Aug 2, 2024

Needs rebase.

# Background

The ilab command is wrapped by an `ilab` script which launches ilab
inside a podman container.

# Issue

Since the ilab container image is pulled during the bootc image build
process using the root user, the image is not accessible to non-root
users.

# Solution

We run the container as sudo in order to be able to access the root
container storage. But for security reasons we map root UID 0 inside the
container to the current user's UID (and all the other subuids to the
user's /etc/subuid range) so that we're effectively running the
container as the current user.

# Additional changes

Changed `"--env" "HOME"` to `"--env" "HOME=$HOME"` to pass the HOME
environment variable from the current shell and not from the sudo
environment.

# Future work

In the future, we will run podman as the current user, once we figure a
reasonable way for the current user to access the root's user container
storage

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

omertuc commented Aug 2, 2024

Force pushed for rebase & resolving review comments

@rhatdan
Copy link
Member

rhatdan commented Aug 2, 2024

LGTM

@rhatdan rhatdan merged commit 7eae618 into containers:main Aug 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants