pi-hole / FTL

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

Limit app password permissions #1995

Closed DL6ER closed 1 day ago

DL6ER commented 2 weeks ago

What does this implement/fix?

Limit app password permissions by default. Add new webserver.api.app_sudo mode for users to remove this new limitation if they really need to

grafik


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:

DL6ER commented 2 weeks ago

grafik

yubiuser commented 2 weeks ago

I think I found a bug here. Login via app password and try a config change. Won't work. Enabled app_sudo > config change will work. Disable app_sudo > Config change will still work

(Also the web interface won't show this active session as coming from an app password)

chrko@ThinkPad-X230:~$ curl -k -XPOST "https://pi.hole/api/auth" --data '{"password":"nDgnVeJVko5buF1HMA/zfZ7KbTDPAbOpv4zvlDkIq8I="}'
{
    "session":  {
        "valid":    true,
        "totp": false,
        "sid":  "pNMUGNtG42aiQAIwtYT5ng=",
        "csrf": "eKihaYo8VP9sNHfJLg6syA=",
        "validity": 300,
        "message":  "app-password correct"
    },
    "took": 1.1221466064453125
}chrko@ThinkPad-X230:~$ curl -k -X UT "https://pi.hole/api/config/dns/upstreams/1.1.1.1" --data '{"sid":"pNMUGNtG42aiQAIwtYT5ng="}'
{
    "error":    {
        "key":  "forbidden",
        "message":  "Unable to change configuration (read-only)",
        "hint": "The current app session is not allowed to modify Pi-hole config settings (webserver.api.app_sudo is false)"
    },
    "took": 0.00031328201293945312
}chrko@ThinkPad-X230:~$ curl -k -X PUT "https://pi.hole/api/config/dns/upstreams/1.1.1.1" --data '{"sid":"pNMUGNtG42aiQAIwtYT5ng="}'
{
    "took": 0.050773859024047852
}chrko@ThinkPad-X230:~$ curl -k -X PUT "https://pi.hole/api/config/dns/upstreams/2.2.2.2" --data '{"sid":"pNMUGNtG42aiQAIwtYT5ng="}'
{
    "took": 0.057279586791992188
DL6ER commented 2 weeks ago

Ah, yes. Your case is special. The reason is that FTL restarted in between (to add the new DNS server) and sessions restoring from the database had a small copy-paste bug causing the app status not to be saved to the database and, hence, all app sessions backed up during a restart became full-blown sessions after the restart and the setting became ineffective by design for them.

DL6ER commented 2 days ago

send_json_error() uses cJSON to create the output. If you have webserver.api.prettyJSON = false, the output will be most compact, if the value is true, the expected separator between between keys and values is \t so just as in your example.

Have a look at https://github.com/pi-hole/FTL/blob/0367117cad371bb9cbe41dfd0ead69c39e8e5192/src/webserver/http-common.c#L23-L47 where you can also see that the expected (formatted) output has tabs as separators.

yubiuser commented 2 days ago

I have webserver.api.prettyJSON = true and was only surprised of the different distances between key and value

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.