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

Using basic auth credentials to access /dev/check still triggers the site login #92

Closed
tim-mediasuite opened this issue Aug 10, 2023 · 4 comments

Comments

@tim-mediasuite
Copy link

Since 2.6, /health/check doesn't show full check results - /dev/check does, but requires authentication when in live mode - that was an intended change. https://github.com/silverstripe/silverstripe-environmentcheck#authentication discusses configuring HTTP Basic Auth for this as a way for automated monitoring to access /dev/check.

This doesn't seem to work however - on a fresh SS5 installation (and also in our SS4 installs) configuring the ENV variables and loading /dev/check still redirects the user to the login flow instead of serving a 401. After logging in and being redirected back to /dev/check, we get the 401 and http auth flow as expected.

Working on debugging this as I suspect it's to do with the bypass added in yaml to avoid the /dev/confirm redirect. Oddly, copying over the rule below from environmentcheck to my test site's mysite.yml means this starts working as expected - I can't see why this workaround works though.

SilverStripe\Control\Director:
  rules:
    'dev/check/$Suite': 'Silverstripe\EnvironmentCheck\Controllers\DevCheckController'

Before:

➜  silverstripe-5-template git:(main) ✗ curl -I https://site-template.localhost.direct/dev/check -u "test:password"
HTTP/2 302 
[..]
location: https://site-template.localhost.direct/Security/login?BackURL=%2Fdev%2Fcheck

After:

➜  silverstripe-5-template git:(main) ✗ curl -I https://site-template.localhost.direct/dev/check -u "test:password"
HTTP/2 200 
[...]
@tim-mediasuite tim-mediasuite changed the title Basic auth configuration for /dev/check still requires login Using basic auth credentials to access /dev/check still triggers the site login Aug 10, 2023
@NightJar
Copy link
Contributor

This appears to be a mismatch in expectations, or that security has evolved over time and this module hasn't kept up.

As with most layered routes in Silverstripe there is a parent controller that must be cleared first - DevelopmentAdmin, in this case.
The route for dev/check has no requsite to be included before or after coreroutes which means it most likely comes afterwards, leaving dev to match first.

DevelopmentAdmin requires ADMIN permission on init (i.e. before delegation to a sub-controller), hard coded in both major versions of Silverstripe. No amount of subsequent checking or bypassing will circumvent this as it is called before delegation to handleAction.

Though the power of YML you could define the dev/check to be encountered in route matching before dev within your project to work around this for now. But I would be cautious to ensure other security checks reach parity between the two controllers (DevCheckController && DevelopmentAdmin).

There is an interesting debate to be had here about whether or not dev is the most appropriate place for this kind of check system to live. It feels weird to circumvent dev in order to provide the functionality that is seemingly dev related. But at the same time, URLs are a kind of taxonomy where dev fits this application... to an extent. I have no opinions really, not sure what to think.

@tim-mediasuite
Copy link
Author

Yeah another place this component's in a weird place is during installation - because it's tagged testing somewhere, you get asked to install it as a dev dependency. But if you're using this for monitoring you definitely want it in prod.

Thanks for the explanation as to why that workaround worked.

@NightJar
Copy link
Contributor

NightJar commented Aug 10, 2023

Thanks for the explanation as to why that workaround worked.

To be completely clear - to avoid the workaround working based on luck* alone, the YML section header should contain a Before: coreroutes key.

*ordering is otherwise undefined.

---
Name: devcheckfix
Before: coreroutes
---
SilverStripe\Control\Director:
  rules:
    'dev/check': 'Silverstripe\EnvironmentCheck\Controllers\DevCheckController'

@GuySartorelli
Copy link
Member

This has been fixed by #119 which will be included in the April minor release.

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

No branches or pull requests

3 participants