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

Use CHAOS TXT record to construct FTL's API URL #376

Merged
merged 6 commits into from
Jan 7, 2024
Merged

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Dec 2, 2023

What does this PR aim to accomplish?:

Use CHAOS TXT record provided by FTL (pi-hole/FTL#1793) to construct the API URL. This adds HTTPS support to PADD as well.

Uses the same logic implemented by pi-hole/pi-hole#5499

Supersedes #375


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)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

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

Signed-off-by: Christian König <[email protected]>
@yubiuser yubiuser mentioned this pull request Dec 2, 2023
1 task
@yubiuser yubiuser requested review from DL6ER and a team December 2, 2023 21:13
@DL6ER
Copy link
Member

DL6ER commented Dec 2, 2023

Seems to work as expected when running in the device itself. I tested without --server and then again with localhost, ryzen (my device's hostname) and pi.hole. Then again with a non-existing hostname (--server local) and got

                                              PADDmini  

                                              START UP =====================
dig: couldn't get address for 'local': not found
API not available. Please check connectivity

which does not look overly nice but does the job, I guess. Not sure if you want to improve on this or not.


Reading through the code, I was wondering if you want to include the explicit --server localhost case in

if [ -z "${SERVER}" ]; then

as the other branch may fail here. It just didn't fail in my case because FTL is the local resolver of my Pi-hole (this isn't standard).


Now the test result from a different machine:


           __      __  __   
          |__) /\ |  \|  \  Pi-hole® Ad Detection Display
          |   /--\|__/|__/  A client for Pi-hole

          START UP ===================================================
curl: (6) Could not resolve host: ;;auth
curl: (6) Could not resolve host: communicationsauth
curl: (6) Could not resolve host: errorauth
curl: (6) Could not resolve host: toauth
curl: (7) Failed to connect to 127.0.0.1 port 80 after 0 ms: Connection refused
curl: (6) Could not resolve host: connectionauth
curl: (3) URL using bad/illegal format or missing URL
curl: (6) Could not resolve host: communicationsauth
curl: (6) Could not resolve host: errorauth
curl: (6) Could not resolve host: toauth
curl: (7) Failed to connect to 127.0.0.1 port 80 after 0 ms: Connection refused
curl: (6) Could not resolve host: connectionauth
curl: (3) URL using bad/illegal format or missing URL
curl: (6) Could not resolve host: communicationsauth
curl: (6) Could not resolve host: errorauth
curl: (6) Could not resolve host: toauth
curl: (7) Failed to connect to 127.0.0.1 port 80 after 0 ms: Connection refused
curl: (6) Could not resolve host: connectionauth
curl: (3) URL using bad/illegal format or missing URL
curl: (6) Could not resolve host: noauth
curl: (6) Could not resolve host: serversauth
curl: (6) Could not resolve host: couldauth
curl: (6) Could not resolve host: beauth
curl: (6) Could not resolve host: reachedauth
          Establishing connection with FTL...
curl: (6) Could not resolve host: reachedauth
          No response from FTL server. Please check connectivity and use the options to set the API URL
          Usage: padd.sh [--server <domain|IP>]

This does not look right. Running with --server pi.hole worked fine without any issues!

Signed-off-by: Christian König <[email protected]>
Signed-off-by: Christian König <[email protected]>
@yubiuser
Copy link
Member Author

yubiuser commented Dec 5, 2023

Thank for your review. I addressed both remarks. localhost is now considered, error handling is improved. I re-used the code from core where we perform the check for valid OS and get the list from our nameservers. Only detour is here that we're POSIX and can't use `$'..' to separate the lines.

@yubiuser
Copy link
Member Author

yubiuser commented Jan 6, 2024

Pinging @DL6ER

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected, I just noticed one unrelated thing:

grafik

The API is sending this null because the information is not available on my embedded miniserver:
grafik

Signed-off-by: Christian König <[email protected]>
@yubiuser
Copy link
Member Author

yubiuser commented Jan 7, 2024

I added 2 commits: first only moves the output, second should address the "null" device.

@yubiuser yubiuser requested a review from DL6ER January 7, 2024 11:57
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grafik

@yubiuser yubiuser merged commit b625aca into PADD_FTLv6 Jan 7, 2024
5 checks passed
@yubiuser yubiuser deleted the chaos branch January 7, 2024 14:45
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.

2 participants