search

pi-hole / FTL

The Pi-hole FTL engine
https://pi-hole.net
Other
1.34k stars 187 forks source link

Add CLI password generation #1999

Closed DL6ER closed 1 day ago

DL6ER commented 1 week ago

What does this implement/fix?

This PR adds a new feature that ensures the pihole command can be used locally without authentication for all users belonging to the group pihole. This is achieved by generating temporary passwords once on every (re)start of FTL and placing this password in /etc/pihole protected by suitable permissions.

The feature is opt-out, if users decide to disable it, the CLI will still work just fine, however, they will have to enter the password whenever changes are made.

[!NOTE]
This PR is to be used in combination with the following PRs, make sure you have checked out all corresponding branches:

To ease review, this PR is created on top of the existing #1995 containing changes necessary for session details being available in the API processing. It's preferential to have the other PR (#1995) reviewed + merged first, followed by changing this PR's base branch to development-v6

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A

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.
  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)

Checklist:

PromoFaux commented 1 week ago

The feature is opt-out,

Not begun testing it yet, but just reading through the various PRs. I wonder... should this feature rather be opt-in? My thinking is that it effectively increases the convenience for the user, but at the cost of security. To me, this is something the user should decide they are comfortable with.

I just feel the optics might be better on having it default to opt-in.

Just my opinion, and maybe I'm wrong - would encourage @pi-hole/core-maintainers / @pi-hole/ftl-maintainers / @pi-hole/debug to weigh in with opinions!

rdwebdesign commented 1 week ago

I still don't have a defined opinion.

I'm might be wrong, but I think enabling this option as default will work more or less the same way as Pi-hole v5.

Another way to think is: Disabling this option can be considered as adding an extra security level.

clwgh commented 1 week ago

I've read various entries linked from this, and if I'm understanding correctly, this change works as follows (please correct me if I'm wrong):

At the moment – anyone with access to the terminal can run the Pi-hole command and make changes. Access is effectively handled by whatever grants access to the terminal, whether that be SSH or physical access to the device with a keyboard and display.

After this change – the above access control remains in place, and an additional level of access control is introduced specifically for Pi-hole. The user, who has access to the terminal, now needs to be in the pihole group to make changes with the pihole command.

If this is enabled by default (opt-out) then the new behaviour enhances security. If it is disabled by default (opt-in) then the new behaviour leaves security the same as it is now.

In terms of security then, I think it is best to enable it by default (opt-out), since security is improved, and at worst is not weakened.

In terms of consistency, some users may be running scripts with the pihole command, and having it enabled by default could break those scripts. My feeling from the Discourse forum is that this is quite rare and people are mainly using the pihole command interactively. Do you have a sense of how reddit users are using it?

On balance it feels like enabling this new feature by default is worth doing.

How will debug logs work? It seems like they will work from the web admin interface (if the system makes the relevant group tweaks under the hood) but running from the command line will require the user to be in the pihole group, leading to an inconsistent experience depending on which method was used for the same function. The debug logs are presumably protected by the new behaviour because they are a means to extract a lot of the same information that the new behaviour would protect.

DL6ER commented 1 week ago

Coming back to a few points raised here:

Security of this change compared to v5.0

In v5.0 you'd need to have sudo powers to make changes to the configuration or the database. Most were not aware because Raspberry Pi systems and friends are often configured with password-less sudo for the main user, i.e., you have just never seen this happening behind the scenes. On "normal" systems, you'd have to enter your sudo password for every pihole command execution.

This is now changed. Your user either needs to be member of group pihole or already be root as I removed the automatic sudo for most parts (actually, I will remove it for all). So the user will have to run sudo pihole ... if they are not part of group pihole. I think this is consistent.

Debug logs

This is not changed. You need sudo-powers in v5, you need them as well in v6. As the web interface is now entirely unprivileged (for very good reasons!), there will be no debug logs from the web.

Opt-out vs. opt-in

Even when the feature is opt-out in the sense, that the CLI password is generated by default, this entire change could also be seen as opt-in due to intentional (new) restrictions caused by the CLI password's file permissions. There is no automatism and even users with password-less sudo will be asked to enter their API password as you'd have to either run pihole with sudo or add your user manually to group pihole.

yubiuser commented 2 days ago

Currently, the cli property is not backed up after FTL restart. Is this fixed once https://github.com/pi-hole/FTL/pull/1995/commits/0ed7f6dde19465bedfd3fee535b59e3f5ef748bc has been merged or does it require more changes?

github-actions[bot] commented 1 day ago

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] commented 1 day ago

Conflicts have been resolved.

yubiuser commented 1 day ago

Mhh.. still does not work.. Screenshot at 2024-06-30 09-34-17

__

The strange thing is: The table seems empty to FTL, but when I copy the whole database the property is set.

nanopi@nanopi:~$ pihole-FTL sqlite3 /etc/pihole/pihole-FTL.db "Select * from session"
nanopi@nanopi:~$

Screenshot at 2024-06-30 09-36-11