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.

Sign up for GitHub

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

Jump to bottom

Require auth for more API endpoints #2411

Merged
merged 2 commits into from Nov 18, 2022
Merged

Require auth for more API endpoints #2411

merged 2 commits into from Nov 18, 2022

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Oct 21, 2022 •
edited

What does this PR aim to accomplish?:

When connecting to the API, most endpoints required already authentication. However, some did not (or were forgotten). This PR adds authentication as a requirement to the missing endpoints, except:

versions, type and version

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

@yubiuser yubiuser added the PR: Approval Required Open Pull Request, needs approval label Oct 21, 2022
@yubiuser yubiuser requested a review from a team October 21, 2022 19:56
@yubiuser
Copy link
Member Author

One can surely discuss if auth is required for getMaxlogage and dns-port

@DL6ER
Copy link
Member

DL6ER commented Oct 28, 2022

Without wanting to discuss these in particular, but it this yet another breaking change? Have they really been forgotten or were they just deemed not worth being "protected"? If so, who made the decision (please say it wasn't me... I don't recall)?

@yubiuser
Copy link
Member Author

Without wanting to discuss these in particular, but it this yet another breaking change?

Maybe, esp. for summary. Third-party apps might use that endpoint. However, If I had my Pi-hole password protected, I wouldn't want an app access to the data without token.

Have they really been forgotten or were they just deemed not worth being "protected"? If so, who made the decision (please say it wasn't me... I don't recall)?

Both can be true. I don't recall any discussion around this.

rdwebdesign
rdwebdesign requested changes Nov 3, 2022
View reviewed changes
api_FTL.php Outdated
@@ -32,7 +32,7 @@
}
}

if (isset($_GET['summary']) || isset($_GET['summaryRaw']) || !count($_GET)) {
if (isset($_GET['summary']) || isset($_GET['summaryRaw']) || !count($_GET) && $auth) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the intended behavior:

Here, you only need !count($_GET) && $auth to be true if the first 2 values were false.
If any other value is true, you don't need to be authenticated.

Example:

(true || false || false && false) == true

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add (...)

@yubiuser
Require auth for more API endpoints
08a6b46 
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser
Add missing Parentheses
97200b8 
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser added PR: Approved Open Pull Request, Approved by required number of reviewers and removed PR: Approval Required Open Pull Request, needs approval labels Nov 4, 2022
@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/upcoming-changes-authentication-for-more-api-endpoints-required/59315/1

@yubiuser yubiuser merged commit c00d127 into devel Nov 18, 2022
@yubiuser yubiuser deleted the auth branch November 18, 2022 07:45
@yubiuser yubiuser mentioned this pull request Dec 3, 2022
Closed
@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/pi-hole-ftl-v5-20-and-web-v5-18-released/59959/1

@DL6ER DL6ER mentioned this pull request Dec 22, 2022
Closed
This was referenced Dec 22, 2022
Closed
Closed
This was referenced Dec 23, 2022
Merged
Open
@BlackWolfWoof
Copy link

For people who were also using
curl http://192.168.178.10:8080/admin/api.php before and wondering why just adding auth still returns you nothing, you now have to use summaryRaw ?summaryRaw&auth=URAUTHTOKENHERE too.
Example: http://192.168.178.10:8080/admin/api.php?summaryRaw&auth=RAUtQziuM9uCj4i28NpFkhmeQaUMHMmVzAAebDfvLwD6SpoUC6rYYKNxBZVKF2S
Took me a few minutes and a look at sterrenb/flutterhole#165 to figure this one out.
Probably better for me to read the documentation next hehe

@yubiuser
Copy link
Member Author

Thanks for pointing this out. This is an unintended side-effect of the changes. There is no "default" response anymore if a password is set and no endpoint provided. We discuss internally how to proceed - but chances are high we'll also remove the "fall-back" option if no password is set to make both situation alike.

@Wiggum127 Wiggum127 mentioned this pull request Dec 27, 2022
Closed
@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/api-php-most-functions-do-not-work/60104/2

doublehelix added a commit to doublehelix/inky-pihole that referenced this pull request Jan 2, 2023
@doublehelix
Added mandatory API Token command line arguments for latest version o…
c7526fb 
…f API (which now has mandatory authentication)

See: https://pi-hole.net/blog/2022/12/21/pi-hole-ftl-v5-20-and-web-v5-18-released/#page-content
and: pi-hole/web#2411
WillPresley added a commit to WillPresley/pihello that referenced this pull request Jan 4, 2023
@WillPresley
Added support for Pi-hole API Token. Fix requests.
f002dc1 
Necessary due to recent planned changes with the Pi-hole web interface that have now gone into effect: https://pi-hole.net/blog/2022/11/17/upcoming-changes-authentication-for-more-api-endpoints-required/ and pi-hole/web#2411.

Most API calls, including 'status' and 'summary', now require the Pi-hole API token, which can be obtained through the Settings > API page.

Based on the 2nd link above, there may be some further changes in the future here.
@WillPresley WillPresley mentioned this pull request Jan 4, 2023
Merged
tomswartz07 added a commit to tomswartz07/pihole-stats-postgres that referenced this pull request Jan 17, 2023
@tomswartz07
Fix breaking change with the upstream API
b34b203 
Fixes issue changed in PiHole API
https://pi-hole.net/blog/2022/11/17/upcoming-changes-authentication-for-more-api-endpoints-required/

Actual change is listed here: pi-hole/web#2411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdwebdesign rdwebdesign rdwebdesign approved these changes

Assignees
No one assigned
Labels
Discussion PR: Approved Open Pull Request, Approved by required number of reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yubiuser @DL6ER @pralor-bot @BlackWolfWoof @rdwebdesign