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

Restore audit wildcard-support #727

Merged
merged 2 commits into from
Apr 9, 2020
Merged

Restore audit wildcard-support #727

merged 2 commits into from
Apr 9, 2020

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Apr 7, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


With Pi-hole v4.x we had wildcard-support in the audit log. When migrating this list into the database, we lost this ability. This PR is the successor of #722 which aimed at restoring this feature. A few changes were necessary that exceeded Github's ability to suggest changes so I moved this into a new PR, preserving the OP's authorship of the first commit.

This PR is milestoned for still being included in the v5.0 release.

silibum and others added 2 commits April 7, 2020 11:22
Signed-off-by: Kevin 'silibum' Böhme <kboehme@silibum.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER added this to the v5.0 milestone Apr 7, 2020
@DL6ER
Copy link
Member Author

DL6ER commented Apr 7, 2020

@silibum Please review my proposed changes.

You can even test this yourself when running

pihole checkout ftl new/audit_wildcards

(note that this does only work when you are already on the beta testing branches).

I changed three things in here:

  1. Added indentation and comments for clarity (always a good thing)
  2. Changed from numbered to names SQL parameters (this allows us to only have to bind once), I added a comment about how this works further down in the same file, where the domain is actually bound to the prepared statement
  3. I fixed the ELSE clause from substr(:input, - length(domain)) to just :input.
    Otherwise, it would cause wildcard-behavior also for non-* domains.
    Let me give an example:
    1. local in the database
    2. something.local as input (domain to be checked against the audit table)
    3. The string manipulation you had in there caused the input domain to be truncated from the right to (length(domain) = length('local') = 5), i.e. "something.local" -> "local"
    4. the result (stored in matcher) is identical to domain and leads to a false positive

@DL6ER DL6ER mentioned this pull request Apr 7, 2020
5 tasks
@silibum
Copy link
Contributor

silibum commented Apr 7, 2020

Hey,

looks good. Compiled and running without any issue - I "missed" to test the non prefixed version: I have 122 entries in my table and just noticed that are 0 non prefixed... I don't use the AdminLTE for adding to the list (now I use phpLiteAdmin and before I edited the .list-file) - Wrong testing 👎

Comments - Yeah not everyone knows what that query does - just thought it was self explaining enough but better to comment that query a bit.

Thanks for the fixing/improving - looks solid. Credits aren't important to me (specially because this wasn't 100% tested before submitting) - I (and I'm sure many others) just like to retain wildcard-support with v5.x ;-)

@DL6ER
Copy link
Member Author

DL6ER commented Apr 8, 2020

Then this is ready for review + approval + merge.

Git can automatically preserve authorship when you know how to do it, see the details of the first commit in this PR:
Screenshot from 2020-04-08 10-32-56

@DL6ER DL6ER requested a review from a team April 8, 2020 08:34
@DL6ER DL6ER merged commit 2a90bb7 into release/v5.0 Apr 9, 2020
@DL6ER DL6ER deleted the new/audit_wildcards branch April 9, 2020 07:13
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/wildcard-auditing/6080/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants