Skip to content

Update Dev Spaces image to support sudo#734

Closed
cgruver wants to merge 2 commits into
ansible:mainfrom
cgruver:main
Closed

Update Dev Spaces image to support sudo#734
cgruver wants to merge 2 commits into
ansible:mainfrom
cgruver:main

Conversation

@cgruver
Copy link
Copy Markdown
Contributor

@cgruver cgruver commented Apr 20, 2026

fix: Fixes sudo execution in the Dev Spaces workspace image

Added logic to remove the user entry with uid 10001. This user is injected by the base image that this image is built from. The presence of that user entry results in duplicate entries in /etc/passwd and /etc/group. The duplicate entries prevent sudo from working properly since uid 1000 is not resolved as belonging to the wheel group.

Added an entry to /etc/shadow for the user. This prevents the PAM errors when executing sudo

Changed the USER entry in the Containerfile to 1000. This change has no runtime effect. It is for reference.

Added the env var ADT_CONTAINER_ENGINE=podman to the devfile.yaml. This enables using podman in the Dev Spaces workspace for tox

Signed-off-by: cgruver <cgruver@redhat.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the OpenShift Dev Spaces container image to make sudo work reliably by ensuring the intended non-root user is resolvable and has the expected group/shadow entries, and tweaks the devfile to prefer podman for ADT tooling.

Changes:

  • Update image build (setup.sh) to remove the base user account, recreate it with UID 1000 and wheel membership, and attempt to address PAM/shadow requirements for sudo.
  • Change the image default USER to 1000 in the Containerfile.
  • Set ADT_CONTAINER_ENGINE=podman in devfile.yaml to enable podman usage in workspaces (e.g., for tox).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
devspaces/context/setup.sh Adds user management + shadow handling to support sudo/group resolution.
devspaces/Containerfile Switches default runtime user to UID 1000 (documents SCC-enforced UID intent).
devfile.yaml Sets ADT container engine to podman for workspace tooling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread devspaces/context/setup.sh Outdated
# Add the user with the UID that the SCC will enforce
useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
chmod 600 /etc/shadow
echo "user:*:0:0:99999:7:::" >> /etc/shadow
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The shadow entry being added uses lastchg=0, which forces an immediate password change on first login on many distros and can trigger pam_unix/account-expired failures even when using NOPASSWD sudo. If you need a shadow entry just to satisfy PAM, set sane aging fields (or manage them via chage) while keeping the account locked.

Suggested change
echo "user:*:0:0:99999:7:::" >> /etc/shadow
usermod -L user
chage -d "$(date +%F)" -m 0 -M 99999 -W 7 user

Copilot uses AI. Check for mistakes.
Comment thread devspaces/context/setup.sh Outdated
Comment on lines +52 to +58
# Remove the base image entries for user
userdel user
# Add the user with the UID that the SCC will enforce
useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
chmod 600 /etc/shadow
echo "user:*:0:0:99999:7:::" >> /etc/shadow
chown -R user /home/user
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

This reintroduces the possibility of duplicate user entries at runtime: if the container runs under an arbitrary UID (the entrypoint supports this via the whoami check), entrypoint.sh will append a new user passwd entry for that UID while this image already contains user with UID 1000. Consider avoiding a fixed user entry, or adjust the entrypoint to update/replace an existing user line instead of appending when the username already exists.

Suggested change
# Remove the base image entries for user
userdel user
# Add the user with the UID that the SCC will enforce
useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
chmod 600 /etc/shadow
echo "user:*:0:0:99999:7:::" >> /etc/shadow
chown -R user /home/user
# Do not create a fixed `user` passwd entry here: the runtime entrypoint
# supports arbitrary UIDs and may add `user` dynamically when no passwd
# entry exists for the current UID.
mkdir -p /home/user
chown -R 1000:0 /home/user

Copilot uses AI. Check for mistakes.
Comment thread devspaces/Containerfile
Comment on lines +19 to +20
# Reflect the UID that the SCC will force the workspace to run as.
USER 1000
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The PR description says changing USER has “no runtime effect”, but it does change the default runtime user for anyone running the image outside an SCC-enforced environment (e.g., local podman/docker runs, CI, or other Kubernetes setups). If the intent is strictly documentation, consider leaving the image USER unchanged and documenting the expected UID elsewhere, or update the PR description to reflect the behavior change.

Copilot uses AI. Check for mistakes.
Comment thread devspaces/context/setup.sh Outdated
# Remove the base image entries for user
userdel user
# Add the user with the UID that the SCC will enforce
useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

This change is intended to make sudo usable, but there isn't an automated regression test covering it. Consider adding a container integration test that exercises sudo from the non-root user (e.g., sudo -n true / sudo -n id) so future image changes don't silently break PAM/sudo behavior again.

Copilot uses AI. Check for mistakes.
Comment thread devspaces/context/setup.sh Outdated
touch /etc/subgid /etc/subuid
chown 0:0 /etc/subgid /etc/subuid
# Remove the base image entries for user
userdel user
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

userdel user will cause the build to fail under set -e if the base image doesn't contain a user account (or if the account can't be deleted). Guard this with getent passwd user/id user before deleting, or make the delete non-fatal and log when the user wasn't present.

Suggested change
userdel user
if id user >/dev/null 2>&1; then
userdel user
else
echo "user account not present; skipping deletion"
fi

Copilot uses AI. Check for mistakes.
Comment thread devspaces/context/setup.sh Outdated
# Add the user with the UID that the SCC will enforce
useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
chmod 600 /etc/shadow
echo "user:*:0:0:99999:7:::" >> /etc/shadow
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

Appending a user: line to /etc/shadow after useradd will typically create a duplicate user entry (since useradd already writes /etc/shadow). Duplicate shadow entries can break PAM/sudo and reintroduce the same class of issue you're trying to fix. Prefer updating the existing entry (e.g., lock the password with usermod/passwd -l) or only add the line if it doesn't already exist.

Suggested change
echo "user:*:0:0:99999:7:::" >> /etc/shadow
passwd -l user

Copilot uses AI. Check for mistakes.
@leogallego
Copy link
Copy Markdown
Contributor

Hey @cgruver, great to see this land. Thanks for putting it together after our discussion on #729.

I and my friend Claude went through the diff and also reviewed Copilot's feedback on the PR. Here's what we found:

Duplicate /etc/shadow entry

useradd on line 56 already creates a shadow entry for user, and then the echo on line 57 appends a second one. Your reference image (base-image-with-sudo) worked because it appended the shadow entry for an already-existing user without running useradd first, so there was no duplicate. With useradd in the picture, you'd end up with two user lines in /etc/shadow, which can make PAM behave unpredictably. Copilot flagged this too and we agree. Probably just needs a passwd -l user or usermod -L user instead of the raw echo.

Password aging / lastchg=0

Related to the above: the appended entry has lastchg=0, which on RHEL-based systems means "password expired, must change on first login." PAM's account check can reject the session even with NOPASSWD sudo. Copilot caught this one as well. If you do need to set aging fields explicitly, chage -d "$(date +%F)" -m 0 -M 99999 -W 7 user would avoid that.

userdel guard

Also agree with Copilot here: userdel user will break the build under set -e if the base image ever drops the user account. A quick if id user >/dev/null 2>&1; then userdel user; fi guard would handle that.

Testing with hostUsers: false

Did you build and test this with hostUsers: false (user namespaces)? The chmod 600 /etc/shadow worked in your reference image, but under ID-mapped overlays we saw the kernel remap permissions back to 000 at runtime regardless of build-time chmod. If it's not working there, shipping a container-friendly /etc/pam.d/sudo with pam_permit.so for account management might still be needed as a fallback.

OCP version

One other thing to consider: we recently moved our cluster to OCP 4.21.9 and I recall there were fixes in this version that apply to our use case, specifically issues we were hitting in 4.21.5. For example, there was a Jira for "SELinux type container_engine_t is missing permissions to allow write to audit log" which directly relates to the CAP_AUDIT_WRITE failures we saw. Does this PR take any of those CRI-O / user namespace fixes into account, or is it meant to work independently of the cluster version? We also need to tackle the SCC changes separately for our CI/bootstrap.

Happy to help test once the shadow entry is sorted, we have the cluster and SCC config ready to go.

@cgruver
Copy link
Copy Markdown
Contributor Author

cgruver commented Apr 21, 2026

Thanks @leogallego

I will make a few tweaks and add them to this PR.

Hey @cgruver, great to see this land. Thanks for putting it together after our discussion on #729.

I and my friend Claude went through the diff and also reviewed Copilot's feedback on the PR. Here's what we found:

Duplicate /etc/shadow entry

useradd on line 56 already creates a shadow entry for user, and then the echo on line 57 appends a second one. Your reference image (base-image-with-sudo) worked because it appended the shadow entry for an already-existing user without running useradd first, so there was no duplicate. With useradd in the picture, you'd end up with two user lines in /etc/shadow, which can make PAM behave unpredictably. Copilot flagged this too and we agree. Probably just needs a passwd -l user or usermod -L user instead of the raw echo.

I added the /etc/shadow because the PAM error still existed with just the Useradd. I will remove the shadow file entry and try the passed -l user suggestion.

Password aging / lastchg=0

Related to the above: the appended entry has lastchg=0, which on RHEL-based systems means "password expired, must change on first login." PAM's account check can reject the session even with NOPASSWD sudo. Copilot caught this one as well. If you do need to set aging fields explicitly, chage -d "$(date +%F)" -m 0 -M 99999 -W 7 user would avoid that.

If the above change works, it should resolve this one too.

userdel guard

Also agree with Copilot here: userdel user will break the build under set -e if the base image ever drops the user account. A quick if id user >/dev/null 2>&1; then userdel user; fi guard would handle that.

Good suggestion.

Testing with hostUsers: false

Did you build and test this with hostUsers: false (user namespaces)? The chmod 600 /etc/shadow worked in your reference image, but under ID-mapped overlays we saw the kernel remap permissions back to 000 at runtime regardless of build-time chmod. If it's not working there, shipping a container-friendly /etc/pam.d/sudo with pam_permit.so for account management might still be needed as a fallback.

Yes, I tested with hostUsers: false that's the only secure way to enable sudo. If the above suggestion about passed -l user works, then the chown will not be necessary.

OCP version

One other thing to consider: we recently moved our cluster to OCP 4.21.9 and I recall there were fixes in this version that apply to our use case, specifically issues we were hitting in 4.21.5. For example, there was a Jira for "SELinux type container_engine_t is missing permissions to allow write to audit log" which directly relates to the CAP_AUDIT_WRITE failures we saw. Does this PR take any of those CRI-O / user namespace fixes into account, or is it meant to work independently of the cluster version? We also need to tackle the SCC changes separately for our CI/bootstrap.

This fix requires OCP 4.21.9 +, or the OCP 4.20 with the same fix.

Happy to help test once the shadow entry is sorted, we have the cluster and SCC config ready to go.

…avoid PAM error.

Signed-off-by: cgruver <cgruver@redhat.com>
@cgruver
Copy link
Copy Markdown
Contributor Author

cgruver commented Apr 21, 2026

I have modified the logic in setup.sh to account for code review comments.

Note that chmod 400 /etc/shadow is necessary to avoid PAM errors inside of the workspace container.

# Remove the base image entries for user
if id user >/dev/null 2>&1
then
  userdel user
  # Add the user with the UID that the SCC will enforce
  useradd -u 1000 -G wheel,root -d /home/user --shell /bin/bash -m user
  usermod -L user
  chmod 400 /etc/shadow
  chown -R user /home/user
fi

@cgruver
Copy link
Copy Markdown
Contributor Author

cgruver commented Apr 21, 2026

I'm going to have to close this PR. I cannot get gpg signing to work in my environment. Needs a bit more research...

@leogallego is going to create a PR with these changes.

@cgruver cgruver closed this Apr 21, 2026
shatakshiiii added a commit that referenced this pull request Apr 22, 2026
* fix: devspaces sudo support with correct UID and user setup

Fixes sudo execution in the Dev Spaces workspace image by removing the
injected base image user (uid 10001) and recreating it with uid 1000 to
match the SCC-enforced UID. Adds ADT_CONTAINER_ENGINE=podman to devfile.

Based on work by @cgruver in #734.

Co-Authored-By: cgruver <39659182+cgruver@users.noreply.github.com>

* fix tox -e lint

* fix: add error handling for useradd in devspaces setup

Co-Authored-By: alisonlhart <alisonlhart@users.noreply.github.com>

---------

Co-authored-by: cgruver <39659182+cgruver@users.noreply.github.com>
Co-authored-by: shatakshiiii <shatakshimishra01@gmail.com>
Co-authored-by: alisonlhart <alisonlhart@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants