-
-
Notifications
You must be signed in to change notification settings - Fork 579
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
Ensure monitoring is accessible also with basic authentication #3247
Conversation
Artifacts to test this will be available at https://github.com/drud/ddev/actions/runs/1242263546 in a couple of minutes. This probably needs a couple of container-level tests, or maybe just the one will do it. Perhaps something added to https://github.com/drud/ddev/blob/master/containers/ddev-webserver/tests/ddev-webserver/php_webserver.bats, which would get all permutations. It's not easy to add a mount for one test and not the others though. But putting it there would get all the permutations. I think maybe just writing the config and reloading internally would be fine, then make sure to remove the config. (I can do it) |
I have currently no idea about the container tests, if you already know what's to do feel free to do so. I would first have to invest some time to it. But if you do and once it's finished I like to learn from your changes, so please ping me at Slack... |
Tested now and everything works like expected so far. |
@gilbertsoft would appreciate if you could review this one as it is now, thanks. |
If basic authentication is enabled the monitoring endpoints are not longer accessible and this cannot be changed in a custom configuration because the locations are already defined. To avoid this problem this patch set `auth_basic` always to `off`.
Artifacts for testing are at https://github.com/drud/ddev/actions/runs/1246233873 Or you can use gitpod, https://gitpod.io/#https://github.com/drud/ddev/pull/3247 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! One little suggestion and two questions...
containers/ddev-webserver/tests/ddev-webserver/testdata/nginx/auth.conf
Outdated
Show resolved
Hide resolved
Co-authored-by: Gilbertsoft <25326036+gilbertsoft@users.noreply.github.com>
The Problem/Issue/Bug:
With Nginx if basic authentication is enabled the monitoring endpoints are not longer accessible and this cannot be changed in a custom configuration because the locations are already defined.
For Apache this is not an issue because an alias is used for
/phpstatus
.In addition, this need uncovered problems in how apache is run via supervisord, and those had to be fixed. This was originally #3249 but for practicality had to be rolled in here.
How this PR Solves The Problem:
auth_basic
always tooff
for phpstatuschild_exit_monitor entered FATAL state, too many start retries too quickly
showed up in the logs at startup in v1.18 (using nonexistent python (python2) in /usr/local/bin/kill_supervisor.py)Manual Testing Instructions:
Enable auth_basic with a custom config
.ddev/nginx/auth.conf
:Create a
.ddev/nginx/htpasswd
file using apache2-utils see https://docs.nginx.com/nginx/admin-guide/security-controls/configuring-http-basic-authentication/.Access the webroot with
ddev launch
and an authentication dialog should be shown, abort the authentication. Now check the two monitoring end points to be accessible without authentication. ATTENTION: the browser remembers a basic auth so once authenticated you are not able to log off without closing the browser.Also check the logs for errors with
ddev logs
.Automated Testing Overview:
Added a full extra stanza to container testing for this situation, which covers both apache and nginx.
Related Issue Link(s):
Some of what gets fixed here was first attempted in #3249, but unfortunately they were too closely related to keep them separate.