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

@aacebedo
Copy link
Contributor Author

aacebedo commented Oct 25, 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

@bkneis I tried your example with my branch and did not see this issue, in my case the /etc/passwd file is correct disregarding the value updateRemoteUserUID (it is not truncated). Can you recheck please (I used your example)?
If your host user and container is the same, uid is not going to change as the updateRemoteUserUID sync it. If you have a UID that is diffient it will update it.

@aacebedo
Copy link
Contributor Author

@bkneis could you give me more details about the commands to reproduce?

@bkneis
Copy link
Contributor

bkneis commented Oct 29, 2024

@aacebedo I just retested with your branch but could not reproduce my issue. Toggling updateRemoteUserUID both have

vscode@0c8cb50a3aa3:/workspaces/ac4$ cat /etc/passwd
root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
bin:x:2:2:bin:/bin:/usr/sbin/nologin
sys:x:3:3:sys:/dev:/usr/sbin/nologin
sync:x:4:65534:sync:/bin:/bin/sync
games:x:5:60:games:/usr/games:/usr/sbin/nologin
man:x:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:x:8:8:mail:/var/mail:/usr/sbin/nologin
news:x:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:x:13:13:proxy:/bin:/usr/sbin/nologin
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
backup:x:34:34:backup:/var/backups:/usr/sbin/nologin
list:x:38:38:Mailing List Manager:/var/list:/usr/sbin/nologin
irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
_apt:x:42:65534::/nonexistent:/usr/sbin/nologin
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
vscode:x:1000:1000::/home/vscode:/usr/bin/bash

when the UID is the same, which is expected. So it must have been finger trouble my end

@aacebedo
Copy link
Contributor Author

@aacebedo I just retested with your branch but could not reproduce my issue. Toggling updateRemoteUserUID both have

vscode@0c8cb50a3aa3:/workspaces/ac4$ cat /etc/passwd
root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
bin:x:2:2:bin:/bin:/usr/sbin/nologin
sys:x:3:3:sys:/dev:/usr/sbin/nologin
sync:x:4:65534:sync:/bin:/bin/sync
games:x:5:60:games:/usr/games:/usr/sbin/nologin
man:x:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:x:8:8:mail:/var/mail:/usr/sbin/nologin
news:x:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:x:13:13:proxy:/bin:/usr/sbin/nologin
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
backup:x:34:34:backup:/var/backups:/usr/sbin/nologin
list:x:38:38:Mailing List Manager:/var/list:/usr/sbin/nologin
irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
_apt:x:42:65534::/nonexistent:/usr/sbin/nologin
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
vscode:x:1000:1000::/home/vscode:/usr/bin/bash

when the UID is the same, which is expected. So it must have been finger trouble my end

Great so we can pass the tests on it and merge it then?

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