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

Support special webserver.port ports ending in "s" (secure) and "r" (redirect) #5499

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Nov 23, 2023

What does this implement/fix?

Add support for special webserver.port ports ending in "s" (secure) and "r" (redirect)

Even when it made the code somewhat more bulky (especially the last-character-is-check), all my additions are POSIX-compliant.


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:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

…redirect)

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from a team November 23, 2023 21:20
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

The redirect part is tricky

nanopi@nanopi:~$ pihole -q flurry.com
curl: (6) Could not resolve host: nanopi.lan
curl: (6) Could not resolve host: nanopi.lan
Found  domains exactly matching 'flurry.com'.

/opt/pihole/query.sh: 66: [: Illegal number: 
Found  adlists exactly matching 'flurry.com'.

/opt/pihole/query.sh: 79: [: Illegal number: 
++ pihole-FTL --config webserver.port
+ ports=80r,443s
+ port=80r
+ '[' r = s ']'
+ '[' r = r ']'
+ API_PROT=http
+ API_PORT=80
+ API_URL=http://localhost:80/api
++ curl -skSL -o /dev/null -w '%{http_code}' http://localhost:80/api/auth
curl: (6) Could not resolve host: nanopi.lan
+ availabilityResonse=308

nanopi@nanopi:~$ curl -vL -o /dev/null -w '%{http_code}' http://localhost:80/api/auth
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying 127.0.0.1:80...
* Connected to localhost (127.0.0.1) port 80 (#0)
> GET /api/auth HTTP/1.1
> Host: localhost
> User-Agent: curl/7.88.1
> Accept: */*
> 
< HTTP/1.1 308 Permanent Redirect
< Location: https://nanopi.lan/api/auth
< Cache-Control: max-age=3600
< Content-Security-Policy: default-src 'self' 'unsafe-inline';
< X-Frame-Options: DENY
< X-XSS-Protection: 0
< X-Content-Type-Options: nosniff
< Referrer-Policy: strict-origin-when-cross-origin
< Content-Length: 0
< Date: Sun, 26 Nov 2023 20:42:53 GMT
< Connection: close
< 
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
* Closing connection 0
* Clear auth, redirects to port from 80 to 443
* Issue another request to this URL: 'https://nanopi.lan/api/auth'
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0* Could not resolve host: nanopi.lan
* Closing connection 1
curl: (6) Could not resolve host: nanopi.lan
308

Do you see what's happening here: FTL returning the hostname instead of localhost for the redirect, but the device does not use FTL as upstream do it does not know about the hostname.....

@yubiuser
Copy link
Member

/etc/hosts did contain the hostname, but without domain suffix.

@DL6ER
Copy link
Member Author

DL6ER commented Nov 26, 2023

We discussed at some place that redirects should go to webserver.domain instead of whatever IP, etc. is used to ensure TLS certificates will work as expected. But I do see where this may be problematic. Any good suggestions? I will keep thinking about this myself but, yeah, I did not factor in the possibility that the device running Pi-hole cannot resolve its own domain.

How does it look like if you configure webserver.domain to be nanopi without lan?

@yubiuser
Copy link
Member

How does it look like if you configure webserver.domain to be nanopi without lan?

This works.


Any good suggestions?

I still love your idea of FTL offering a TXT record where to find the (final) API.

@DL6ER
Copy link
Member Author

DL6ER commented Nov 26, 2023

I still love your idea of FTL offering a TXT record where to find the (final) API.

But it suffers from the same illness. It'd also report something like http://<webserver.domain>:<port>/api and when the host does not know how to resolve webserver.domain we'd be in exactly the same scenario as we are now (with much less code)

Signed-off-by: DL6ER <dl6er@dl6er.de>
advanced/Scripts/api.sh Outdated Show resolved Hide resolved
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from yubiuser November 28, 2023 22:00
@yubiuser
Copy link
Member

yubiuser commented Nov 30, 2023

Should we keep the approach to use pihole-FTL --config or use the feature implemented in pi-hole/FTL#1793?

…onfig webserver.ports

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Dec 1, 2023

Let's go for the simpler option...

advanced/Scripts/api.sh Outdated Show resolved Hide resolved
advanced/Scripts/api.sh Outdated Show resolved Hide resolved
advanced/Scripts/api.sh Outdated Show resolved Hide resolved
advanced/Scripts/api.sh Outdated Show resolved Hide resolved
@yubiuser yubiuser linked an issue Dec 2, 2023 that may be closed by this pull request
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from yubiuser December 2, 2023 21:43
@DL6ER DL6ER merged commit 7f7ec13 into development-v6 Dec 4, 2023
15 checks passed
@DL6ER DL6ER deleted the tweak/api_ports branch December 4, 2023 22:48
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.

api.sh does not handle https connections
2 participants