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

uid/gidmapping #145

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

uid/gidmapping #145

wants to merge 22 commits into from

Conversation

ezrizhu
Copy link
Collaborator

@ezrizhu ezrizhu commented Feb 17, 2024

[1] due to regular files not being tracked in try (tangent #150), we're also not setting the permissions correctly. e.g., if /test is owned by another user, this will not be reflected in try. This is likely outside the scope of this PR. However we can probably track this in another issue, #90 could also fix it.

Four tiers of uid/gid mapping

  1. no gid mapping: Files are only accessible if the ownership is set to both current user's uid and gid.
  2. unprivileged gid mapping: groups will look exactly the same inside and outside the container, the try will have access to things owned by things that one of user's group has access to aswell
  3. privileged gid mapping: both a file's user and group ownership will also look and act exactly the same inside and outside try. (needs sudo, and try will have access to everything)
  4. privileged uid mapping with -u set: id result will look the same.
# no mapping
ezri@pashsn3:~/try$ ./try ls -l try
-rwxrwxr-x 1 root root 21884 Feb 17 10:39 try

# unprivileged mapping
ezri@pashsn3:~/try$ ./try ls -l try
-rwxrwxr-x 1 root ezri 21884 Feb 17 10:39 try

# privileged mapping
ezri@pashsn3:~/try$ sudo ./try ls -l try
-rwxrwxr-x 1 ezri ezri 21884 Feb 17 10:39 try

# shell
ezri@pashsn3:~/try$ ls -l try
-rwxrwxr-x 1 ezri ezri 20263 Feb 17 10:42 try
# no mapping
ezri@pashsn3:~/try$ ./try id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)

# unprivileged mapping
ezri@pashsn3:~/try$ ./try id
uid=0(root) gid=1001(ezri) groups=1001(ezri),27(sudo)

# privileged mapping without user set
ezri@pashsn3:~/try$ sudo ./try id
uid=0(root) gid=0(root) groups=0(root)

# privileged mapping with user set
ezri@pashsn3:~/try$ sudo ./try -u ezri id
uid=1001(ezri) gid=1001(ezri) groups=1001(ezri),27(sudo)

# shell
ezri@pashsn3:~/try$ id
uid=1001(ezri) gid=1001(ezri) groups=1001(ezri),27(sudo)

closes #140
closes #143
closes #6

@ezrizhu ezrizhu self-assigned this Feb 17, 2024
@ezrizhu ezrizhu changed the title [WIP] gidmapping support [WIP] gidmapping + docker support Feb 17, 2024
@ezrizhu ezrizhu changed the title [WIP] gidmapping + docker support [WIP] uid/gidmapping + docker support Feb 17, 2024
@ezrizhu ezrizhu mentioned this pull request Feb 17, 2024
3 tasks
@ezrizhu ezrizhu marked this pull request as ready for review February 17, 2024 10:49
@ezrizhu ezrizhu changed the title [WIP] uid/gidmapping + docker support uid/gidmapping + docker support Feb 20, 2024
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

I understand... most of this. A few more questions. I think moving the mapper into this repo is important from a supply-chain security perspective.

setup.sh Show resolved Hide resolved
try
@@ -159,6 +166,12 @@ autodetect_union_helper() {
fi
}

# notify the mapper that we're up
echo "a" > "$SOCKET"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use different sentinels when sending to and receiving from the mapper---we should check that the sentinels are what we expect, too.

try
then
error "need root for -u" 2
fi
EUSER="$OPTARG"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to unconditionally clear EUSER at startup, so that it can't be invoked as EUSER=foo try .... I suggest using a more specific name, like TRYUSER.

try
. "$script_to_execute"
if [ "$EUSER" ]
then
su - $EUSER -c "cd "$START_DIR" && sh $script_to_execute"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we confident that all of the other variables we care about are marked as export?

This should probably be exec su ..., since we have no other work afterwards. In fact, we could probably do cd "$START_DIR" && exec su - ....

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test to ensure that:

(a) normal (non-sudo) use of try doesn't let you read files you should not have
(b) sudo within the normal try container doesn't leak privileges
(c) we need to carefully articulate how we lose privileges after having run --map-root-user

try
# Change uid/gid mapping
################################################################################

mapper() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a more informative name, like uid_gid_mapper()?

try
mapper() {
cat "$SOCKET" > /dev/null
# Get the pid of the unshare process with current pid as parent
pid=$(pgrep -P $$ -f unshare)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a new external dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we're adding libcap2-bin and procps/procps-ng. readme will be updated appropriately.
libcap2-bin for setup to do addcap to the gidmapper.
procps for pgrep to look for unshare thread id, was hoping for a non-external-dependency needed option, perhaps we can have this be built into the c gidmapper impl.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's write the process id on the socket, save ourselves the trouble.

Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

notes from meeting

we need to understand the security implications before moving forward

try
mapper() {
cat "$SOCKET" > /dev/null
# Get the pid of the unshare process with current pid as parent
pid=$(pgrep -P $$ -f unshare)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's write the process id on the socket, save ourselves the trouble.

try
gidmapper "$pid" 0 0 65535 0 0 65535
else
# If not running as root, we can only mount the caller user
gidmapper "$pid" 0 "$(id -u)" 1 0 0 65535
Copy link
Contributor

Choose a reason for hiding this comment

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

investigate gidmapper "$pid" 0 "$(id -u)" 1 0 "$(id -g)" 1? why map all groups?

try
. "$script_to_execute"
if [ "$EUSER" ]
then
su - $EUSER -c "cd "$START_DIR" && sh $script_to_execute"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test to ensure that:

(a) normal (non-sudo) use of try doesn't let you read files you should not have
(b) sudo within the normal try container doesn't leak privileges
(c) we need to carefully articulate how we lose privileges after having run --map-root-user

try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@mgree
Copy link
Contributor

mgree commented Feb 27, 2024

One other test to run: can a valid sudoer use sudo inside the container? (Feels good for running scripts that need to install things, but mostly are unprivileged.)

@ezrizhu ezrizhu changed the title uid/gidmapping + docker support uid/gidmapping Feb 29, 2024
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.

Understand gid mapping; build utility for it future: sync Running apt
2 participants