-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add CLI password generation #1999
Conversation
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
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! |
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: |
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. |
Coming back to a few points raised here: Security of this change compared to v5.0In v5.0 you'd need to have This is now changed. Your user either needs to be member of group Debug logsThis is not changed. You need Opt-out vs. opt-inEven 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm convinced
Currently, the cli property is not backed up after FTL restart. Is this fixed once 0ed7f6d has been merged or does it require more changes? |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: DL6ER <[email protected]>
Conflicts have been resolved. |
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: |
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 grouppihole
. 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:
git rebase
)Checklist:
developmental
branch.