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

server_url() broken in Docker image #879

Closed
smuth4 opened this issue May 24, 2017 · 12 comments
Closed

server_url() broken in Docker image #879

smuth4 opened this issue May 24, 2017 · 12 comments
Labels
bug it's broken! docker containers & cloud server

Comments

@smuth4
Copy link

smuth4 commented May 24, 2017

Features that use the server_url function break inside the official docker container (I'm running development), due to $_SERVER['SERVER_NAME'] defaulting to "" since none is defined in nginx.conf. I think that falling back to HTTP_HOST if SERVER_NAME isn't set is a good compromise, but wanted to check before starting on any work

@ArthurHoaro ArthurHoaro added bug it's broken! server labels May 25, 2017
@ArthurHoaro
Copy link
Member

Thanks for the report! The server name should be set in the Docker container. @virtualtam do you want to take a look at it?
As for HTTP_HOST, it can be spoofed, so I don't think it's a very good idea to rely on it (it was actually one of my first PR on Shaarli! #98).

@smuth4
Copy link
Author

smuth4 commented May 25, 2017

Though an environment variable? I can do that myself if it's what's wanted, sounded a bit hacky though.

@virtualtam virtualtam added the docker containers & cloud label May 25, 2017
@virtualtam
Copy link
Member

virtualtam commented May 25, 2017

Hi!

Thanks @smuth4 for the feedback, there hasn't been much regarding our Docker images (either they work really well, or very few people use them ^^)

There is a full rewrite using an Alpine Linux base under work at #843 & #846 , I'm currently looking for a sane way to automate unit testing Shaarli in a containerized environment.


Nginx's server_name setting can contain a FQDN or a domain wildcard:

This setting cannot be changed at runtime, and cannot be parameterized using an environment variable:

As server_url() is used to return public-facing URLs, it should rely on HTTP proxy headers in a container context, assuming:

  • the Docker container is behind a reverse proxy (haproxy, Træfik, Nginx, Apache HTTPD, etc...)
  • the appropriate HTTP headers are passed from the reverse proxy to the Shaarli Nginx backend
  • server_url() uses the proxied values to build public-facing URLs

Relevant documentation:

@smuth4
Copy link
Author

smuth4 commented May 25, 2017

This would not have been caught in the unit tests as they are now.

This setting cannot be changed at runtime, and cannot be parameterized using an environment variable:

You could have the startup script run a quick sed on the config. Hacky, like I said, and something I'd only do as a last resort.

How would you like me to proceed? I can fall back to HTTP_X_FORWARDED_HOST instead of HTTP_HOST and make sure nginx sets it to $proxy_host correctly.

@virtualtam
Copy link
Member

@smuth4 Let's start with fixing support for proxy-forwarded host settings; as I'd rather avoid adding complexity to Dockerfiles and startup scripts.

@ArthurHoaro Regarding spoofing: though it's not mentioned in our Docker usage docs (for now), containers should run and be served/routed behind a reverse proxy to ensure:

  • proper SSL termination for secure connections
  • appropriate HTTP headers are set/forwarded to webapps running within containers

@ArthurHoaro
Copy link
Member

Yes that's what I'm doing, even though I'm not using the official image (I add theme, plugins, and stuff), but I don't see any relation with spoofing. What I was saying is that, if I'm not mistaken, HTTP_HOST is based on the user request.

@smuth4
Copy link
Author

smuth4 commented Jun 2, 2017

What I was saying is that, if I'm not mistaken, HTTP_HOST is based on the user request.

By default nginx actually set this to proxy_host instead: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_set_header, and HTTP_X_FORWARDED_HOST can be set by the user just like HTTP_HOST if nothing is reversing proxying (not saying HTTP_HOST is the better option, just pointing out behavior).

@virtualtam I'm willing to help if need be, but I'm not sure what, if anything, I should be doing in regards to "fixing support for proxy-forwarded host settings".

smuth4 pushed a commit to smuth4/Shaarli that referenced this issue Jul 7, 2017
alongside PROTO and PORT
Fixes shaarli#879
smuth4 added a commit to smuth4/Shaarli that referenced this issue Jul 8, 2017
alongside _PORT and _PROTO
Fixes shaarli#879
@nbud
Copy link

nbud commented Oct 6, 2017

Same issue as @smuth4 but #899 didn't fix the issue for me because HTTP_X_FORWARDED_HOST is not set up in my config. Could server_url rely on $_SERVER["HTTP_HOST"] if $_SERVER["SERVER_NAME"] is not set?

@ArthurHoaro
Copy link
Member

Is there any issue preventing you to add X-Forwarded-* directives to your reverse proxy?

@nbud
Copy link

nbud commented Oct 7, 2017

No real issue except that I don't know yet how to do it. However, it seems to me that supporting $_SERVER["HTTP_HOST"] as a fallback would have no downside but would the life of some users (like myself) easier.

@nbud
Copy link

nbud commented Oct 30, 2017

I fixed my reverse proxy issue by adding to the nginx configuration:

proxy_set_header X-Forwarded-Host $host;

I use jwilder/nginx-proxy, a very popular reverse proxy for Docker. Because it is widely used, I think it would be an improvement that the shaarli image worked out of the box with it. The only needed change would be to make server_url() use $_SERVER["HTTP_HOST"] if $_SERVER["SERVER_NAME"] is not set. I can PR it if you want.

@virtualtam
Copy link
Member

@nbud I've raised #1010 to provide a working example + documentation for Compose + Nginx reverse proxy

I suspect most remaining issues are related to the involved proxies' configuration (esp. headers) rather than the code dealing with HTTP headers; feel free to open new issues though, should you see any specific point to be addressed ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug it's broken! docker containers & cloud server
Projects
None yet
Development

No branches or pull requests

4 participants