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

Set auth_request off for acme challenge location #570

Merged
merged 1 commit into from
Aug 20, 2019

Conversation

dsullivan
Copy link
Contributor

Issue #569. Simple change to ensure that auth_request is off for the acme challenge location, just as we do for auth_basic.

@buchdag
Copy link
Member

buchdag commented Aug 19, 2019

Seems good to me but I have one question:

Nginx doc for auth_request says that

This module is not built by default, it should be enabled with the --with-http_auth_request_module configuration parameter.

What are the consequences of including statements for a potentially unbuilt module ?

@dsullivan
Copy link
Contributor Author

dsullivan commented Aug 19, 2019

Argh.

I had to compile from source to find a nginx install without the auth_request module enabled. (It seems to be enabled for in the popular Docker images, Alpine and Ubuntu packages, etc.) But, yes, without the module enabled, nginx throws an error:

nginx: [emerg] unknown directive "auth_request" in /usr/local/nginx/conf/nginx.conf:38

And from what I understand, nginx doesn't have an equivalent of Apache's directive. There's probably no way for this container's scripts to interrogate the nginx container for available modules, either.

Although in practice I don't imagine anyone would run into problems, it's probably not worth the risk.

@buchdag
Copy link
Member

buchdag commented Aug 20, 2019

Is this Nginx doc really up to date ? Because none of the Nginx container Dockerfiles include --with-http_auth_request_module yet the auth_request module seems to be there.

@buchdag
Copy link
Member

buchdag commented Aug 20, 2019

Ok, bottom line is while Nginx is not built with the http_auth_request module by default, most distro packaged versions appears to include it (along with kinda useful stuff that isn't included by default either like the SSL module).

So the containerised versions of Nginx (either alpine or debian, and as far back as the tagged versions go) also include this module :

[buchdag@host ~]$ docker run nginx:1.7 nginx -V
nginx version: nginx/1.7.12
built by gcc 4.7.2 (Debian 4.7.2-5) 
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-http_ssl_module --with-http_realip_module --with-http_addition_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_stub_status_module --with-http_auth_request_module --with-mail --with-mail_ssl_module --with-file-aio --with-http_spdy_module --with-cc-opt='-g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2' --with-ld-opt='-Wl,-z,relro -Wl,--as-needed' --with-ipv6
[buchdag@host ~]$ docker run nginx:1.8-alpine nginx -V
nginx version: nginx/1.8.1
built by gcc 5.3.0 (Alpine 5.3.0) 
built with OpenSSL 1.0.2g  1 Mar 2016
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-http_ssl_module --with-http_realip_module --with-http_addition_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_stub_status_module --with-http_auth_request_module --with-mail --with-mail_ssl_module --with-file-aio --with-http_spdy_module --with-ipv6

I don't see why the http_auth_request module would disappear from either Alpine's or Debian's Nginx anytime soon, so I guess this can be safely merged after all.

What is your view on this ?

@dsullivan
Copy link
Contributor Author

I also was unable to find any packaged version of nginx that did not include the http_auth_request module. I had to compile from source myself to test the error condition.

I suspect that anyone who compiles nginx themselves probably has their own certificate management strategy, too. So I think this is safe to merge.

@buchdag
Copy link
Member

buchdag commented Aug 20, 2019

Agreed, merging. Thanks for the contribution.

@buchdag buchdag merged commit 7cc7f10 into nginx-proxy:master Aug 20, 2019
@buchdag buchdag added the kind/feature-request Issue requesting a new feature label Oct 10, 2019
buchdag added a commit to nginx-proxy/nginx-proxy that referenced this pull request Mar 20, 2020
Add the following to the Let's Encrypt ACME challenge "no redirection to HTTPS"
nginx-proxy/acme-companion#570
nginx-proxy/acme-companion#335
buchdag added a commit to buchdag/acme-companion that referenced this pull request May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants