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

[Experimental] Add ability to change UID / GID for www-data and pihole user. #982

Merged
merged 6 commits into from
Jan 20, 2022

Conversation

edgd1er
Copy link
Contributor

@edgd1er edgd1er commented Jan 13, 2022

When mounting volumes, specific rights may be needed to write on mounted volumes on the host.

Description

05-changer-uid-gid.sh added to cont-init .d ( s6 container initialisation). based on env vars if set, www-data UID / GID and pihole UID and GID are changed. Files owned by these two users are chowned to be sure

Motivation and Context

as per #328 and personnal need, I had to be able to write on host's mounted volumes.
In order to be able to write on mounted volumes, pihole and www-data user have to be mapped to an existing/known user on host system.

#328

How Has This Been Tested?

I can now update blacklist/whitelist db, gravity db, ....

env vars set:

root@pihole:/# echo vars pihole:${PIHOLE_UID}/${PIHOLE_GID} www-data: ${WEB_UID}/${WEB_GID} 
vars pihole:1001/116 www-data: 1001/116

system uid/gid

root@pihole:/# id -a pihole
uid=1001(www-data) gid=116(www-data) groups=116(www-data),999(pihole)
root@pihole:/# id -a www-data
uid=1001(www-data) gid=116(www-data) groups=116(www-data),999(pihole)

just added a script in :

root@pihole:/etc/cont-init.d# ls -l
-rwxr-xr-x 1 root root  941 Jan  9 09:03 05-changer-uid-gid.sh
-rwxr-xr-x 1 root root 1785 Jan  9 09:03 20-start.sh

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

README.md Outdated Show resolved Hide resolved
@PromoFaux
Copy link
Member

Thank you for taking the time to submit this - I will try to look it over as soon as I can. One thing to note, please sign off your commit per the instructions here : https://github.com/pi-hole/docker-pi-hole/pull/982/checks?check_run_id=4800034886

Thanks!

When mounting volumes, specific rights may be needed to write on mounted volumes on the host.

Signed-off-by: edgd1er <edgd1er@hotmail.com>
Signed-off-by: edgd1er <edgd1er@hotmail.com>
@PromoFaux
Copy link
Member

Confirmed doing exactly what it says on the tin. Just playing with the output a bit and then I'll merge to dev

@PromoFaux
Copy link
Member

PromoFaux commented Jan 17, 2022

Of course, I spoke to soon. The script is exiting with error 125 on a fresh container.... investigating

@edgd1er
Copy link
Contributor Author

edgd1er commented Jan 17, 2022

Of course, I spoke to soon. The script is exiting with error 125 on a fresh container.... investigating

Arf, that's unfortunate. Do you have a way to reproduce ?

What I've done is:

  • tried both mounted volumes and docker volumes.
  • set www-data and pihole as 131/140
    docker compose down -v
    docker compose build
    docker compose up
docker-pi-hole-pihole-1  | [cont-init.d] executing container initialization scripts...
docker-pi-hole-pihole-1  | [cont-init.d] 05-changer-uid-gid.sh: executing... 
docker-pi-hole-pihole-1  | user www-data 33 => 131
docker-pi-hole-pihole-1  | group www-data 33 => 140
docker-pi-hole-pihole-1  | user pihole 999 => 131
docker-pi-hole-pihole-1  | [cont-init.d] 05-changer-uid-gid.sh: exited 0.
docker-pi-hole-pihole-1  | [cont-init.d] 20-start.sh: executing...
uid=131(www-data) gid=140(www-data) groups=140(www-data),999(pihole)
root@8318fa7de998:/etc/pihole# id -a pihole  
uid=131(www-data) gid=140(www-data) groups=140(www-data),999(pihole)

@PromoFaux
Copy link
Member

seemingly just docker-compose down && docker-compose up -d is enough. From my "change something, see what happens" testing so far, it appears to be rooted in the chown/chgrp commands.

On a pihole/pihole:lastest image I ran the following:

root@1e8cf9cfa13a:/# usermod -o -u 1000 pihole
root@1e8cf9cfa13a:/# find / -user 999 -print0 2>/dev/null | xargs -0 -n1 chown -h pihole 2>/dev/null
root@1e8cf9cfa13a:/# echo $?
123

@edgd1er
Copy link
Contributor Author

edgd1er commented Jan 17, 2022

seemingly just docker-compose down && docker-compose up -d is enough. From my "change something, see what happens" testing so far, it appears to be rooted in the chown/chgrp commands.

On a pihole/pihole:lastest image I ran the following:

root@1e8cf9cfa13a:/# usermod -o -u 1000 pihole
root@1e8cf9cfa13a:/# find / -user 999 -print0 2>/dev/null | xargs -0 -n1 chown -h pihole 2>/dev/null
root@1e8cf9cfa13a:/# echo $?
123

the find command may need to be completed to ignore /dev /sys /srv, if that's the problem...

find / \( -path /srv -prune -o -path /dev -prune -o -path /proc -prune \) -o -user 131 -print0

@PromoFaux
Copy link
Member

I'm looking at making a few tweaks, including removing the find command from this file. Firstly - there is already a section of bash_functions called from start.sh (prepare_configs) that is designed to fix permissions of mounted files. Secondly - it might be wise to use fix-attrs.d for this case

@PromoFaux
Copy link
Member

Scratch that:

After fixing attributes (through /etc/fix-attrs.d/) and just before starting user provided services up (through /etc/services.d) our overlay will execute all the scripts found in /etc/cont-init.d, for example:

…run` script. remove pihole-FTL test from test-config because nothing exists until it has started for the first time.

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member

@edgd1er - apologies for stepping on your PR - we can roll back commits and I'll do them separately if you'd prefer - I mostly wanted to tweak a few things after some extended testing and figured it makes sense to wrap them into the same PR as they're related

@edgd1er
Copy link
Contributor Author

edgd1er commented Jan 18, 2022

No hard feelings. Let s keep it simple, same subject, same pr. I ll rebase the forked project when merged.

@edgd1er
Copy link
Contributor Author

edgd1er commented Jan 20, 2022

@PromoFaux , v3 of s6-overlay has been released.

Changelog says it is a complete rewrite and could fix many problems especially the fix-attrs.
Do you think this would be a reasonable test case to test v3 ? https://github.com/just-containers/s6-overlay/releases/tag/v3.0.0.0

@PromoFaux
Copy link
Member

PromoFaux commented Jan 20, 2022

It is certainly worth a look to see! I actually had to go back on my thought about moving some of the permission setting things to the fix-attrs.d, because it runs before cont-init.d, it could be that some of the files don't actually exist yet.

Also from the readme:

This section describes a functionality from the versions of s6-overlay that are anterior to 3.0.0.0. fix-attrs is still supported in 3.0.0.0, but is deprecated, for several reasons: one of them is that it's generally not good policy to change ownership dynamically when it can be done statically.

Edit: Looks like quite a few changes to S^, so probably worth looking at in another PR

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux PromoFaux changed the title change UID / GID for www-data and pihole user. Add ability to change UID / GID for www-data and pihole user. Jan 20, 2022
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux PromoFaux changed the title Add ability to change UID / GID for www-data and pihole user. [Experimental] Add ability to change UID / GID for www-data and pihole user. Jan 20, 2022
@PromoFaux PromoFaux merged commit 25539a9 into pi-hole:dev Jan 20, 2022
@PromoFaux PromoFaux mentioned this pull request Jan 20, 2022
7 tasks
@edgd1er
Copy link
Contributor Author

edgd1er commented Jan 20, 2022

thanks for merging. S--overlay has a new installation process that need to be elaborated and tested:

s6-overlay-noarch-3.0.0.0.tar.xz: this tarball contains the scripts implementing the overlay. We call it "noarch" because it is architecture- independent: it only contains scripts and other text files. Everyone who wants to run s6-overlay needs to extract this tarball.
s6-overlay-x86_64-3.0.0.0.tar.xz: replace x86_64 with your system's architecture. This tarball contains all the necessary binaries from the s6 ecosystem, all linked statically and out of the way of your image's binaries. Unless you know for sure that your image already comes with all the packages providing the binaries used in the overlay, you need to extract this tarball.

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.

None yet

2 participants