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

Ensure pihole-FTL can write to all files in /etc/pihole, /run/pihole and /var/log/pihole #5356

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Aug 3, 2023

What does this PR aim to accomplish?:

In v5 we create custom.list with owner root.

# Install empty custom.list file if it does not exist
if [[ ! -r "${PI_HOLE_CONFIG_DIR}/custom.list" ]]; then
if ! install -o root -m 644 /dev/null "${PI_HOLE_CONFIG_DIR}/custom.list" &>/dev/null; then
printf " %b Error: Unable to initialize configuration file %s/custom.list\\n" "${COL_LIGHT_RED}" "${PI_HOLE_CONFIG_DIR}"
return 1
fi
fi

However, in v6 FTL needs to write to the file after dropping priviledges/as user pihole.

2023-08-03 20:20:48.970 [9285M] INFO: FTL version: vDev-a9d4771
2023-08-03 20:20:48.970 [9285M] INFO: FTL commit: a9d47713
2023-08-03 20:20:48.970 [9285M] INFO: FTL date: 2023-07-28 23:46:26 +0200
2023-08-03 20:20:48.971 [9285M] INFO: FTL user: pihole
2023-08-03 20:20:48.971 [9285M] INFO: Compiled for linux/arm64/v8 (compiled on CI) using cc (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924
2023-08-03 20:20:49.012 [9285M] INFO: Writing config file
2023-08-03 20:20:49.044 [9285M] ERR: Cannot open /etc/pihole/custom.list for writing, unable to update custom.list: Permission denied

This PR sets the owner on installation to pihole and ensures that before FTL is started the owner is set to pihole for existing files.

P.S. We could also set a transition path v5 -> v6 to change owner of all files in /etc/pihole/ to pihole instead of root.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested a review from a team August 3, 2023 19:16
@DL6ER
Copy link
Member

DL6ER commented Aug 4, 2023

We should add a -R. Any file in /etc/pihole should be writable by pihole:pihole and doing so before each FTL start ensures permission issues possibly created by users manually tinkering around as root can easily be fixed by restarting FTL

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser changed the title Ensure pihole-FTL can write custom.list Ensure pihole-FTL can write to all files in /etc/pihole /run/pihole and /var/log/pihole Aug 4, 2023
@yubiuser
Copy link
Member Author

yubiuser commented Aug 4, 2023

I added a second commit that will set pihole:pihole for all files/folders in /etc/pihole, /var/log/pihole and /run/pihole

@yubiuser yubiuser requested a review from DL6ER August 4, 2023 17:34
@yubiuser yubiuser changed the title Ensure pihole-FTL can write to all files in /etc/pihole /run/pihole and /var/log/pihole Ensure pihole-FTL can write to all files in /etc/pihole, /run/pihole and /var/log/pihole Aug 4, 2023
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

Thanks for your change. I wonder though (this is entirely up for discussion!) if we should drop those chmods as well (read as: they are dropped here, but I think they shouldn't).

My initial idea would be that we should set 640 (u+rw,g+r) on the log files but something more generous on the config files (like 660 -> u+rw,g+rw) so that other users manually added the the group pihole can edit such files.

What we currently do (granting read permissions for all) on the

  • macvendor.db,
  • dhcp.leases, and
  • FTL.log

shouldn't be needed.

If FTL.log would contain query information (only if debug.queries = true), one might very well even consider this a privacy issue - we should definitely have the last digit 0 for such files.

Summary: I don't think there is anything (also not custom.list) that would need read permissions for all. 660 seems the way to go.

@yubiuser
Copy link
Member Author

yubiuser commented Aug 4, 2023

I agree with the requested changes in re-adding file permission. Having 640for the log files and 660 for the config files seems to be a good balance. See also #4760

Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested review from DL6ER and a team August 5, 2023 12:58
advanced/Templates/pihole-FTL-prestart.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
advanced/Templates/pihole-FTL-prestart.sh Outdated Show resolved Hide resolved
chmod -R 0640 /var/log/pihole
chmod -R 0660 /etc/pihole /run/pihole
# allow all users to enter der directories
chmod 0755 /etc/pihole /var/log/pihole
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for allowing other users from being able to access the directories when if they aren't allowed to read any files in there?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was: others can access the dir and get the names of the files within but not the actual content of the files.

Co-authored-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested a review from DL6ER August 6, 2023 10:05
@yubiuser yubiuser requested a review from a team August 13, 2023 20:05
Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

Looks good to me, but as @DL6ER last requested changes, will await his approval/not to merge

@DL6ER DL6ER merged commit 43ddfcf into development-v6 Sep 26, 2023
15 checks passed
@DL6ER DL6ER deleted the fix/custom.list branch September 26, 2023 17:59
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

3 participants