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

[33] Make dhcpd process run as the dhcpd user and group #34

Merged
merged 1 commit into from
Jul 6, 2022
Merged

[33] Make dhcpd process run as the dhcpd user and group #34

merged 1 commit into from
Jul 6, 2022

Conversation

yscialom
Copy link
Contributor

@yscialom yscialom commented Jul 5, 2022

Fixes issue #33.

Make the dhcpd process run as user and group dhcpd.

Additional Informations

Prior to running the process, the entrypoint script make sure those user & group have the same id as the user & group owning /data.

If /data is owned by the root user (or group), the dhcpd user (or group) will be modified to have UID 0 (or GID). This is allowed for two users (or groups) to share the same id, this is the purpose of the -o flag of usermod (or `groupmod).

Tests Cases

  • /data owned by 666:666
  • /data owned by 0:0
  • /data owned by 0:666

Test modus operandi

cd $(mktemp -d)
mkdir data
cat > data/dhcpd.conf <<EOF
subnet 192.168.1.0 netmask 255.255.255.0 {
    option routers                  192.168.1.254;
    option subnet-mask              255.255.255.0;
    option domain-name-servers      1.1.1.1;
    range                           192.168.1.1 192.168.1.253;
}
EOF
sudo chown --recursive 666:666 data # or other owner
docker run -d --rm --name dhcpd --init --net host -v "$(pwd)/data":/data networkboot/dhcpd:issue-33
sleep 10
find . -exec ls -l {} \;
docker exec dhcpd ps -ef
docker stop dhcpd
cd
sudo rm -rf -- $OLDPWD #BEWARE

Tests Results

666:666

$ find . -exec ls -l {} \;
total 4
drwxrwxr-x 2 666 666 4096 Jul  5 21:49 data
total 8
-rw-rw-r-- 1 666 666 255 Jul  5 21:48 dhcpd.conf
-rw-r--r-- 1 666 666 277 Jul  5 21:49 dhcpd.leases
-rw-r--r-- 1 666 666   0 Jul  5 21:49 dhcpd.leases~
-rw-r--r-- 1 666 666 277 Jul  5 21:49 ./data/dhcpd.leases
-rw-rw-r-- 1 666 666 255 Jul  5 21:48 ./data/dhcpd.conf
-rw-r--r-- 1 666 666 0 Jul  5 21:49 ./data/dhcpd.leases~
yscialom@ubuntu:/tmp/tmp.TcaQshAxmH$ docker exec dhcpd ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 21:49 ?        00:00:00 /sbin/docker-init -- /entrypoint.sh
root           6       1  0 21:49 ?        00:00:00 /usr/bin/dumb-init -- /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
dhcpd         31       6  0 21:49 ?        00:00:00 /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
root          35       0  0 21:49 ?        00:00:00 ps -ef

0:0

$ find . -exec ls -l {} \;
total 4
drwxrwxr-x 2 root root 4096 Jul  5 21:33 data
total 8
-rw-rw-r-- 1 root root 255 Jul  5 21:28 dhcpd.conf
-rw-r--r-- 1 root root 280 Jul  5 21:33 dhcpd.leases
-rw-r--r-- 1 root root   0 Jul  5 21:33 dhcpd.leases~
-rw-r--r-- 1 root root 280 Jul  5 21:33 ./data/dhcpd.leases
-rw-rw-r-- 1 root root 255 Jul  5 21:28 ./data/dhcpd.conf
-rw-r--r-- 1 root root 0 Jul  5 21:33 ./data/dhcpd.leases~
$ docker exec dhcpd ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 21:47 ?        00:00:00 /sbin/docker-init -- /entrypoint.sh
root           6       1  0 21:47 ?        00:00:00 /usr/bin/dumb-init -- /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
root          31       6  0 21:47 ?        00:00:00 /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
root          35       0  0 21:47 ?        00:00:00 ps -ef

0:666

$ find . -exec ls -l {} \;
total 4
drwxrwxr-x 2 root 666 4096 Jul  5 21:51 data
total 8
-rw-rw-r-- 1 root  666 255 Jul  5 21:50 dhcpd.conf
-rw-r--r-- 1 root root 280 Jul  5 21:51 dhcpd.leases
-rw-r--r-- 1 root  666   0 Jul  5 21:51 dhcpd.leases~
-rw-r--r-- 1 root root 280 Jul  5 21:51 ./data/dhcpd.leases
-rw-rw-r-- 1 root 666 255 Jul  5 21:50 ./data/dhcpd.conf
-rw-r--r-- 1 root 666 0 Jul  5 21:51 ./data/dhcpd.leases~
$ docker exec dhcpd ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 21:51 ?        00:00:00 /sbin/docker-init -- /entrypoint.sh
root           6       1  0 21:51 ?        00:00:00 /usr/bin/dumb-init -- /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
root          31       6  0 21:51 ?        00:00:00 /usr/sbin/dhcpd -4 -f -d --no-pid -cf /data/dhcpd.conf -lf /data/dhcpd.leases -user dhcpd -group dhcpd
root          35       0  0 21:51 ?        00:00:00 ps -ef

The mixed case is not passing. It looks like an issue with dhcpd itself. Opinions?

@robinsmidsrod
Copy link
Contributor

@yscialom Explicitly setting the uid/gid for dhcpd to 0 in /etc/passwd seems the wrong approach. I would instead keep the existing behavior, because that won't change permission/ownership on the data directory and its contents if running as root.

My approach would be to just add an else clause for the uid/gid checks and set two variables, e.g. USER_ARGS and GROUP_ARGS. Leave them as an empty string if running as root. Finally just add them as unquoted arguments to the final dhcpd call. That should be quite a minimal change with small chance of regressions.

@yscialom
Copy link
Contributor Author

yscialom commented Jul 6, 2022

Hello @robinsmidsrod, could you please elaborate on what "I would instead keep the existing behavior" means?

Currently, dhcpd runs as 0:0 whatever the owner of /data.

This PR makes it run as 0:0 if /data is owned by 0:0, or as uid:gui otherwise.

Now, running as root or running as dhcpd whose ids have been set to 0:0 is the same thing, but makes the entrypoint script much simpler as there is only one invocation of the dhcpd process.

I might have misunderstood your suggestion. Can you provide at least some pseudocode so we make sure we understand each other?

@robinsmidsrod
Copy link
Contributor

Something like this:

if [ $gid -ne 0 ]; then
    groupmod -g $gid dhcpd
    GROUP_ARGS="-group dhcpd"
else
    GROUP_ARGS=""
fi
if [ $uid -ne 0 ]; then
    usermod -u $uid dhcpd
    USER_ARGS="-user dhcpd"
else
    USER_ARGS=""
fi
...
$run dhcpd ... $USER_ARGS $GROUP_ARGS ...

The main reason for doing it this way is to not change ownership/permissions of more files than absolutely needed. I try to keep the behavior as close to what the .deb package does.

@yscialom
Copy link
Contributor Author

yscialom commented Jul 6, 2022

I fail to see any difference between your proposal and mine. Running as root or running as any other user with uid 0 is the same thing.

Files are owned by an uid, the named displayed by stat or ls -l are only for convenience.

Am I missing something?

@robinsmidsrod
Copy link
Contributor

By not specifying -user/-group in the dhcpd command line you get the default .deb run behavior (which could change over time). I actually thought the default behavior was to run as uid/gid dhcpd if nothing was specified.

But I have reconsidered and think your idea is good. Will merge.

@robinsmidsrod robinsmidsrod self-requested a review July 6, 2022 08:46
@robinsmidsrod robinsmidsrod merged commit 79eaa8b into networkboot:master Jul 6, 2022
@yscialom
Copy link
Contributor Author

yscialom commented Jul 6, 2022

"By not specifying -user/-group in the dhcpd command line you get the default .deb run behavior". Ok this is what I've missed, yeah it makes sense.

Let me double check if I can find any documentation on the guarantee the developers of dhcpd would have maid that -user/-group for running as root (uid 0) will be supported long time.

Thank you for the merge. As I'd like it to be available on dockerhub in the short future (days or a couple weeks), I'll work with the author of the other PR to automate it :)

KTGW

@yscialom yscialom deleted the feature/33-make-dhcpd-run-as-dhcpd-user branch July 6, 2022 09:01
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