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

Disabling user auth also disables the API token requirement for /metrics #4628

Open
1 task done
thielj opened this issue Mar 29, 2024 · 6 comments · Fixed by #4723
Open
1 task done

Disabling user auth also disables the API token requirement for /metrics #4628

thielj opened this issue Mar 29, 2024 · 6 comments · Fixed by #4723
Labels
area:documentation Improvements or additions to documentation area:settings Related to Settings page and application configration good first issue Good for newcomers help wanted May need your help to test or answer

Comments

@thielj
Copy link

thielj commented Mar 29, 2024

📑 I have found these related issues/pull requests

-/-

🛡️ Security Policy

Description

I have an API token to access /metrics which worked well.

I have now disabled user authentication and added Authelia as a middleware, with both the /metrics and /api/push endpoints configured as 'bypass', with everything else requiring authentication.

To my surprise. the API token is no longer required anymore to access /metrics.

👟 Reproduction steps

see above

👀 Expected behavior

I expected that the /metrics endpoint still requires an API token. According to the docs,

By default, HTTP basic authentication is used to secure access to the Prometheus metrics endpoint. As soon as you add your first API key, the use of basic authentication for the endpoint will be permanently disabled.

😓 Actual Behavior

/metrics was unprotected

🐻 Uptime-Kuma Version

1.23.11

💻 Operating System and Arch

louislam/uptime-kuma:alpine (x64)

🌐 Browser

n/a

🖥️ Deployment Environment

n/a

📝 Relevant log output

No response

@thielj thielj added the bug Something isn't working label Mar 29, 2024
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 29, 2024

Disabling auth disables auth, so far this is not a bug from my standpoint.

I agree that we might want to draw more attention to what this does here (and in the docs)
image

If you/somebody wants to create a PR for this change, we would be open to this ^^

@CommanderStorm CommanderStorm added area:documentation Improvements or additions to documentation help wanted May need your help to test or answer good first issue Good for newcomers area:settings Related to Settings page and application configration labels Mar 29, 2024
@thielj
Copy link
Author

thielj commented Apr 7, 2024

Thanks. As /metrics can disclose a lot of private details, the documentation and GUI should be very clear and explicit about this.

I would suggest to allow disabling user/GUI auth separately from service/API auth and/or requiring deletion of all API keys before the built-in API auth is disabled.

Feel free to close the issue or keep it as a heads-up for other users as long as necessary.

@CommanderStorm
Copy link
Collaborator

Changing the helptexts in the disable-auth popup seems sufficient to me. No need to introduce more complexity than needed.

@thielj
Copy link
Author

thielj commented Apr 7, 2024

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Apr 7, 2024

Where/How?
I think that disabling auth disables auth entirely has the logical cause that no auth is present any longer. => don't think this needs to be added on that docs page.

@thielj
Copy link
Author

thielj commented Apr 7, 2024

Oh well, I tried ¯\_(ツ)_/¯

@thielj thielj closed this as completed Apr 7, 2024
@CommanderStorm CommanderStorm reopened this Apr 7, 2024
@CommanderStorm CommanderStorm removed the bug Something isn't working label Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation Improvements or additions to documentation area:settings Related to Settings page and application configration good first issue Good for newcomers help wanted May need your help to test or answer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants