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

Change FTL database permission to 664 #1162

Merged
merged 4 commits into from
Sep 26, 2021
Merged

Change FTL database permission to 664 #1162

merged 4 commits into from
Sep 26, 2021

Conversation

yubiuser
Copy link
Member

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: {replace this text with a number from 1 to 10, with 1 being not familiar, and 10 being very familiar}

1


Sets the file permission of the pihole-FTL.db to 664 to allow write permission to the pihole group. This is necessary for pi-hole/web#1886 as it allows the web interface write permission to delete items from the message table

@yubiuser
Copy link
Member Author

I've been thinking about how to change permissions on already existing databases. My idea would be to move the whole section

FTL/src/database/common.c

Lines 172 to 175 in b90ab8b

// Explicitly set permissions to 0644
// 644 = u+w u+r g+r o+r
const mode_t mode = S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH;
chmod_file(FTLfiles.FTL_db, mode);

from db_create to db_init right before the database is going to be opened

sqlite3 *db = dbopen(false);

This would set the permissions every time the database in initialized (same for newly created db or existing databases). What do you think?

@DL6ER
Copy link
Member

DL6ER commented Sep 12, 2021

Yes, this sounds like a workable solution. There should be no harm involved in (re)setting the permissions once on every FTL start. FTL may not be root at the point where you want to do the chown, however, this shouldn't be a concern here because FTL has CAP_CHOWN which should give it the power of changing ownership of files even when not root.

edit 1

Thinking again about this, we might as well want to do it here:

https://github.com/pi-hole/pi-hole/blob/2673c2c0720fb3871a4c3cae6a3f38f9ccede24e/advanced/Templates/pihole-FTL.service#L29

because we never ensure the permissions are right after the file has been created.

@yubiuser
Copy link
Member Author

Like that pi-hole/pi-hole#4328?

@DL6ER DL6ER self-requested a review September 23, 2021 06:31
src/database/common.c Outdated Show resolved Hide resolved
DL6ER
DL6ER previously approved these changes Sep 23, 2021
@DL6ER DL6ER enabled auto-merge September 23, 2021 18:53
Signed-off-by: DL6ER <[email protected]>
@DL6ER
Copy link
Member

DL6ER commented Sep 26, 2021

Signed commits locally and updated the fork branch as discussed with @yubiuser

@DL6ER DL6ER changed the title Change database permission to 664 Change FTL database permission to 664 Sep 26, 2021
@DL6ER DL6ER merged commit 5ab7bf7 into pi-hole:development Sep 26, 2021
@yubiuser yubiuser deleted the db_permissions branch October 4, 2021 20:39
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