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

Documentation missing for 0.7.0 removing Proxy => HTTP_PROXY mitigation #1101

Open
dominics opened this issue Aug 3, 2017 · 2 comments
Open

Comments

@dominics
Copy link

dominics commented Aug 3, 2017

I note that when using 0.7.0 of this module, the new fastcgi params no longer block a Proxy header from being sent through as HTTP_PROXY to a fastcgi application. (i.e. it reverts #835)

I understand the decision to ship upstream versions in #862 / #1076, but this should at the very least be mentioned specifically in the CHANGELOG. "Use nginx defaults for fastcgi_params" is probably not specific enough to call attention to this issue for PHP sysadmins.

There is no legitimate use for the 'Proxy' header in HTTP. OTOH, I'm sure there are PHP users out there still using e.g. Guzzle 6.2.0 and an older version of PHP. They should probably be told to add an HTTP_PROXY => '""' entry to their fastcgi_param parameter on any location that talks fastcgi to PHP, otherwise upgrading to 0.7.0 will make them vulnerable again. (This fix matches Nginx's public documentation on setting up fastcgi for PHP, even if it doesn't match the upstream fastcgi_params source)

@wyardley
Copy link
Collaborator

wyardley commented Aug 3, 2017

Yes, agree it might make sense to document this in a bit stronger terms.

That said, my reading is that the default behavior in #835 is still in place, and handles adding HTTP_PROXY => '""' in the location block (which is what the existing test is for)? It appears that those changes are still in place, just not the line in fastcgi.conf / fastcgi_params.
or am I reading this wrong in:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/init.pp#L116-L120
https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_location_spec.rb#L575-L580

Separately, might be worth checking with the nginx mailing list and see why they haven't included that change in their shipped default in
http://hg.nginx.org/nginx/file/tip/conf/fastcgi.conf or
http://hg.nginx.org/nginx/file/tip/conf/fastcgi_params

@dominics
Copy link
Author

dominics commented Aug 4, 2017

Yeah, I think that might be reading it wrong. To me, it looks like:

  • The proxy_set_header fix is still in place, but only protects reverse-proxied downstream services, not e.g. PHP running on the same box
  • The test is just testing that if the user adds an explicit 'HTTP_PROXY' => '""' to their $fastcgi_params, that it will be reflected in the final config (I don't think it's testing that one is added to location blocks automatically)

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

No branches or pull requests

2 participants