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

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

Merged
merged 1 commit into from May 9, 2024

Conversation

PromoFaux
Copy link
Member

What does this PR aim to accomplish?:

Fixes #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)

  • I have read the above and my PR is ready for review. Check this box to confirm

@PromoFaux PromoFaux requested a review from a team May 8, 2024 20:54
@rdwebdesign
Copy link
Member

rdwebdesign commented May 8, 2024

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
Copy link
Member Author

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

src/Dockerfile Outdated Show resolved Hide resolved
…grep

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux PromoFaux merged commit d2e46f3 into development-v6 May 9, 2024
7 checks passed
@PromoFaux PromoFaux deleted the fix/grep-version branch May 9, 2024 07:42
@yubiuser
Copy link
Member

yubiuser commented May 9, 2024

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
Copy link
Member Author

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

@DL6ER
Copy link
Member

DL6ER commented May 10, 2024

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
Copy link
Member

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
Copy link
Member

DL6ER commented May 10, 2024

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

DL6ER commented May 10, 2024

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
Copy link
Member Author

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
Copy link
Member

DL6ER commented May 10, 2024

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

@dschaper
Copy link
Member

dschaper commented May 10, 2024

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

image

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.

None yet

5 participants