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

Implement UpdateRemoteUserID handling #1287

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aacebedo
Copy link
Contributor

@aacebedo aacebedo commented Sep 27, 2024

This PR is a tentative implementation of the UpdateRemoteUserID handling.

The reference implementation uses an additional dockerfile (https://github.com/devcontainers/cli/blob/main/scripts/updateUID.Dockerfile) to create a new image using the container image created previously.

I don't think this approach works well with prebuilding feature of devpod. So I decided to amend the
container after it has been created.

In some case, this modification may make the same uid gid to be used twice for 2 users and 2 groups but in the end it shall not impact the way the container work in the end.

This also solves #1284.

@pascalbreuninger
Copy link
Member

@aacebedo Thanks for the PR! Let's run the test suite real quick and see if it works out.
We've been working on this a while ago as well, although without using the UpdateRemoteUserID option. See this PR.
At the time we've abandonded it because of potential breaking changes and we'd need to invest more time figuring out the consequences.

As for your PR I think scoping it only to linux, as per the spec, would already limit the potential blast radius. I'll still need to test the change with a number of provider/workspace combinations

@aacebedo aacebedo force-pushed the aacebedo/implement_updateremoteuseruid branch from 57c95a3 to b07caec Compare September 29, 2024 18:18
@aacebedo
Copy link
Contributor Author

aacebedo commented Sep 29, 2024

fixed the lint issue
added the detection of windows and if the uids are root

@aacebedo aacebedo force-pushed the aacebedo/implement_updateremoteuseruid branch from b07caec to f6f5d0f Compare September 29, 2024 20:50
pkg/driver/docker/docker.go Outdated Show resolved Hide resolved
pkg/driver/docker/docker.go Outdated Show resolved Hide resolved
@aacebedo aacebedo force-pushed the aacebedo/implement_updateremoteuseruid branch from f6f5d0f to ea68d64 Compare October 3, 2024 19:57
pkg/driver/docker/docker.go Outdated Show resolved Hide resolved
@aacebedo aacebedo force-pushed the aacebedo/implement_updateremoteuseruid branch from ea68d64 to 4f419f7 Compare October 4, 2024 19:05
@aacebedo
Copy link
Contributor Author

aacebedo commented Oct 8, 2024

If everything is OK can we merge?

@aacebedo aacebedo force-pushed the aacebedo/implement_updateremoteuseruid branch from 4f419f7 to f46464d Compare October 8, 2024 19:40
@aacebedo
Copy link
Contributor Author

aacebedo commented Oct 9, 2024

rebased on main and test E2E tests. It is now working

@pascalbreuninger
Copy link
Member

@aacebedo I haven't forgotten this PR, I'm still thinking through it and weigh against other options. Thanks for the work you've put in, bear with me :)

@aacebedo
Copy link
Contributor Author

Thanks!

I have some users who are facing the issue and it is very annoying for them.

Do you have an idea of an ETA? Can I help?

@bkneis
Copy link
Contributor

bkneis commented Oct 17, 2024

@aacebedo I've tested this locally but did not get the result I thought I would. Using the following devcontainer.json and Dockerfile, I toggled updateRemoteUserUID from true to false. Here are the resulting passwd files

{
  "name": "Basic Python",
  "build": { "dockerfile": "Dockerfile" },
  "remoteUser": "vscode",
  "containerUser": "vscode",
  "updateRemoteUserUID": false,
  "customizations": {
    "vscode": {
      "extensions": ["ms-python.python"]
    }
  }
}
FROM library/python:3.11-bookworm

ARG USER=vscode
ARG DEBIAN_FRONTEND=noninteractive
RUN apt update \
    && apt install -y --no-install-recommends sudo \
    && apt autoremove -y \
    && rm -rf /var/lib/apt/lists/* \
    && useradd -m -s /usr/bin/bash ${USER} \
    && echo "${USER} ALL=(ALL) NOPASSWD: ALL" >/etc/sudoers.d/${USER} \
    && chmod 0440 /etc/sudoers.d/${USER}

When updateRemoteUserUID is true

root:x:0:
daemon:x:1:
...
vscode:x:1000:

When updateRemoteUserUID is false

root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
...
vscode:x:1000:1000::/home/vscode:/usr/bin/bash

Should the false entry have dropped the extra info? Also what is the current issue you are facing? When I set this to false, I thought I would have permission issues but can still mount my workspace

@aacebedo
Copy link
Contributor Author

aacebedo commented Oct 17, 2024

@aacebedo I've tested this locally but did not get the result I thought I would. Using the following devcontainer.json and Dockerfile, I toggled updateRemoteUserUID from true to false. Here are the resulting passwd files

{
  "name": "Basic Python",
  "build": { "dockerfile": "Dockerfile" },
  "remoteUser": "vscode",
  "containerUser": "vscode",
  "updateRemoteUserUID": false,
  "customizations": {
    "vscode": {
      "extensions": ["ms-python.python"]
    }
  }
}
FROM library/python:3.11-bookworm

ARG USER=vscode
ARG DEBIAN_FRONTEND=noninteractive
RUN apt update \
    && apt install -y --no-install-recommends sudo \
    && apt autoremove -y \
    && rm -rf /var/lib/apt/lists/* \
    && useradd -m -s /usr/bin/bash ${USER} \
    && echo "${USER} ALL=(ALL) NOPASSWD: ALL" >/etc/sudoers.d/${USER} \
    && chmod 0440 /etc/sudoers.d/${USER}

When updateRemoteUserUID is true

root:x:0:
daemon:x:1:
...
vscode:x:1000:

When updateRemoteUserUID is false

root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
...
vscode:x:1000:1000::/home/vscode:/usr/bin/bash

Should the false entry have dropped the extra info? Also what is the current issue you are facing? When I set this to false, I thought I would have permission issues but can still mount my workspace

Hi @bkneis

You printed the /etc/passwd of the container right? I'll check

For the issue I am facing, it is described in #1284. Ensure that the UID GID of the user in the container is not the same as the one on your host.

@bkneis
Copy link
Contributor

bkneis commented Oct 18, 2024

@aacebedo Thanks for clarifying. Yes these two files were from the container. My host user has UID 1000 so I was expecting a different UID when setting to true. I am not using podman, but I don't think this should make a difference for this test? As it should still parse my /etc/passwd and check the UID? Just want to make sure the PR is working as expected before merging

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.

3 participants