pi-hole / docker-pi-hole

Pi-hole in a docker container
https://pi-hole.net
Other
8.39k stars 1.12k forks source link

Install grep to avoid issues in pihole -w/b with the default busybox … #1576

Closed PromoFaux closed 4 months ago

PromoFaux commented 4 months ago

What does this PR aim to accomplish?:

Fixes https://github.com/pi-hole/docker-pi-hole/issues/1577 by replacing busybox's grep - which does not include the P option

Before:

dev-v6:/# pihole -b test.com
grep: unrecognized option: P
BusyBox v1.36.1 (2023-11-07 18:53:09 UTC) multi-call binary.

Usage: grep [-HhnlLoqvsrRiwFE] [-m N] [-A|B|C N] { PATTERN | -e PATTERN... | -f FILE... } [FILE]...

Search for PATTERN in FILEs (or stdin)

        -H      Add 'filename:' prefix
        -h      Do not add 'filename:' prefix
        -n      Add 'line_no:' prefix
        -l      Show only names of files that match
        -L      Show only names of files that don't match
        -c      Show only count of matching lines
        -o      Show only the matching part of line
        -q      Quiet. Return 0 if PATTERN is found, 1 otherwise
        -v      Select non-matching lines
        -s      Suppress open and read errors
        -r      Recurse
        -R      Recurse and dereference symlinks
        -i      Ignore case
        -w      Match whole words only
        -x      Match whole lines only
        -F      PATTERN is a literal (not regexp)
        -E      PATTERN is an extended regexp
        -m N    Match up to N times per file
        -A N    Print N lines of trailing context
        -B N    Print N lines of leading context
        -C N    Same as '-A N -B N'
        -e PTRN Pattern to match
        -f FILE Read pattern from file
grep: unrecognized option: P
BusyBox v1.36.1 (2023-11-07 18:53:09 UTC) multi-call binary.

Usage: grep [-HhnlLoqvsrRiwFE] [-m N] [-A|B|C N] { PATTERN | -e PATTERN... | -f FILE... } [FILE]...

Search for PATTERN in FILEs (or stdin)

        -H      Add 'filename:' prefix
        -h      Do not add 'filename:' prefix
        -n      Add 'line_no:' prefix
        -l      Show only names of files that match
        -L      Show only names of files that don't match
        -c      Show only count of matching lines
        -o      Show only the matching part of line
        -q      Quiet. Return 0 if PATTERN is found, 1 otherwise
        -v      Select non-matching lines
        -s      Suppress open and read errors
        -r      Recurse
        -R      Recurse and dereference symlinks
        -i      Ignore case
        -w      Match whole words only
        -x      Match whole lines only
        -F      PATTERN is a literal (not regexp)
        -E      PATTERN is an extended regexp
        -m N    Match up to N times per file
        -A N    Print N lines of trailing context
        -B N    Print N lines of leading context
        -C N    Same as '-A N -B N'
        -e PTRN Pattern to match
        -f FILE Read pattern from file
  [✗] test.com is not a valid argument or domain name!

After:

dev-v6:/# pihole -b test.com
  [i] Adding test.com to the blacklist...
  [✓] Reloading DNS lists

image


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)

rdwebdesign commented 4 months ago

Question: Why do we need to use the --upgrade option?

I tested adding grep to the list of packages to be installed and the result was the same.

pidev:/# grep -V
grep (GNU grep) 3.11
Copyright (C) 2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Mike Haertel and others; see
<https://git.savannah.gnu.org/cgit/grep.git/tree/AUTHORS>.

grep -P uses PCRE2 10.42 2022-12-11
PromoFaux commented 4 months ago

Maybe we do not... @yubiuser made the comment here that it did not work when he tried, and so I took the upgrade option from a comment you made elsewhere and it worked.

I didn't try it without upgrade before based on Yubi's comment, but having since tried it I can confirm it is now working

yubiuser commented 4 months ago

Something must have changed in between so that it is working now with only grep being available. I testet it with the container now and bosth -b/-w work.

P.S. Adding grep was the lazy way ;-) list.sh still uses the direct database access. At some point we should re-write it to use FTL and the new API :-)

PromoFaux commented 4 months ago

Yeah, absolutley agree on that - just going for the quickfix option for the time being :)

DL6ER commented 4 months ago

We have code for CLI lists via API ready for quite some time but there was an ongoing discussion because this change then introduces authentication for these CLI options while there was none before. We discussed this shortly back in the days but never came to a conclusion what we wanted to do IIRC. Eventually, it was forgotten.

My latest suggestion was to change the default of webserver.api.localAPIauth to false (it currently default to true) but this is also not the optimal solution effectively allowing the CLI to use the API at will without password.

CLI domain searching is also ported to the API and we added a separate option for it (webserver.api.searchAPIauth) - if we'd decide for my suggestion, this special option could be removed again - which I'd absolutely love to do!

yubiuser commented 4 months ago

I don't remember what my position was when we discussed this back then, but today I think we should not let the cli to bypass the authentication by default. We have https://github.com/pi-hole/pi-hole/blob/development-v6/advanced/Scripts/api.sh which can handle cli authentication so there should be no need to allow bypassing it. (I admit it is a bit cumber-stone to re-authenticate for each added black/whitelist entry - unless we store the SID somewhere and don't delete the session)

DL6ER commented 4 months ago

Yes, storing the session somewhere in the respective user's home should solve this. Maybe we create a directory ~/.pihole and store a file called api_sid in there. We can always throw this against the API and it will either accept or reject it, the latter forcing the user to re-login.

Users will complain, but probably not too hard. We could offer the possibility to create a file ~/.pihole/api_pw where users can enter either their Pi-hole's normal or application password to cause the CLI to auto-login.

PromoFaux commented 4 months ago

Storing the password could work, BUT how would we do that in such a way that it doesn't lead to the possibility of the password being compromised?

yubiuser commented 4 months ago

I wouldn't store the password. I think it's fine to request re-login after the session has expired. Saving the SID should make it very convenient already.

DL6ER commented 4 months ago

That's what the application password has been designed for. If they use (and store!) it in some Python scripts in their home directory programmatically interacting with the API, why not allow them to store it in an equally "safe" (that's why I said in /home/<user>) place?

PromoFaux commented 4 months ago

So maybe we just preface it with "you can do x but it is at your own risk"

SID maybe the better option, mind.

DL6ER commented 4 months ago

We'll do SID anyway but it will still require relogin after some inactivity.

dschaper commented 4 months ago

This is how the AWS CLI utility handles it. I think that's a pretty good model to use?

image