if folder is not owned by www-data, nginx cannot write to it, as its …

[KlavsKlavsen]

…childs is the ones who log.

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

[bastelfreak]

Hi @KlavsKlavsen, thanks for the PR. Can you add an acceptance test that verifies that this is now working (and was a bug before)?

[ekohl]

Then we need to look at a better fix because this was a CVE. d47ab8b introduced this to mitigate DSA-3701 / CVE-2016-1247 and should match the default setup on Debian.

[KlavsKlavsen]

Hi @KlavsKlavsen, thanks for the PR. Can you add an acceptance test that verifies that this is now working (and was a bug before)?

Then I would have to start up nginx, setup a vhost - and curl to that.. and then check its error log to see if it logs it cannot log to accesslog ? I haven't looked at the current tests.. I hope there is already examples of doing that ?

[dhoppe]

@KlavsKlavsen I think you just need to change the value of the following line:

puppet-nginx/spec/classes/nginx_spec.rb

Line 363 in 705c19b

owner: 'root',

[ekohl]

@KlavsKlavsen could you describe where you see this fail on your systems? This should match the OS defaults

[KlavsKlavsen]

could you describe where you see this fail on your systems? This should match the OS defaults

Its on Ubuntu 16.04. I tried installing nginx on a vanilla Ubuntu and can verify the perms nginx module has IS the OS defaults.. Which IS a bug..
I can find tonnes of people having perms issues for nginx logs and fixes it manually.

If you read the nginx docs its pretty clear:
http://nginx.org/en/docs/http/ngx_http_log_module.html

the user whose credentials are used by worker processes should have permissions to create files in a directory with such logs;

And puppet nginx module uses same "user" as default - ie. worker runs as www-data.
I

[bastelfreak]

I still think that this at least needs an acceptance test to verify it was broken before and is fixed now.

[ekohl]

On my Debian 9 box this is not a problem. Nginx still has one master process as root and that allows it to log. Because this was a CVE I'm very hesitant to change this.

Are you using OS packages or nginx provided packages?

[KlavsKlavsen]

Using OS (Ubuntu) packages for nginx.

[KlavsKlavsen]

@ekohl If you read the docs I linked to -it clearly states that the logfiles MUST be writeable by the user the worker process nginx uses.. It does NOT help that there is a root process listening on port 80.

[KlavsKlavsen]

@KlavsKlavsen I think you just need to change the value of the following line:

puppet-nginx/spec/classes/nginx_spec.rb

Line 363 in 705c19b
owner: 'root',

Thank you @dhoppe - that has now been changed to reflect the change to www-data.

[dhoppe]

@bastelfreak, @ekohl Do you have any objections or can we merge this pull request?

[bastelfreak]

I wasn't able to verify this issue on my Ubuntu 16.04 box.

[ekohl]

Given this was introduced to close a CVE where Ubuntu was explicitly mentioned, I'm still strongly against merging this. We're following the OS defaults and I'd take it up with the OS maintainers if there's a problem with this. https://legalhackers.com/advisories/Nginx-Exploit-Deb-Root-PrivEsc-CVE-2016-1247.html describes how this can lead to a root privilege escalation which I take very seriously.

[bastelfreak]

I agree with @ekohl here.

[bastelfreak]

Hi,
I'm going to close this PR due to inactivity. Please reopen and response if you still think this should be merged.