nginx improvements #2473
nginx improvements #2473
Conversation
|
By analyzing the blame information on this pull request, we identified @carlaschroder to be a potential reviewer |
|
Does Are you sure adding your /data/.ocdata location doesn't allow browsing to that location now? Can you explain why owncloud is browsing here? |
|
I say stay with deny all which throws the correct 403 = Forbidden instead of the wrong 404 = Not found. The access to the .ocdata shouldn't be done by ownCloud via the Web so normally you shouldn't get a log message about that in your logs. |
|
To block the data directory with But there are two diffeences:
owncloud uses the access attempt via the browser to /data/.ocdata to check if the data directory is accessible or not. In case it is, it warns you on the admin page (your data directory is accessible from the internet...). This check is performed everytime you access or reload the admin page. The "bad" thing about that is, that everytime you get nginx error log entry because of the forbidden rule. Your statement regarding |
mmattel
force-pushed
the
nginx_location_update
branch
from
b200962 to
a270242
Compare
|
If the return and break is needed then it should be 403 and not 404 from my PoV. There is no need to do obfuscating here. For the test of the protected datadir the /data/htaccesstest.txt is used: where an hint at the docs already exists how to suppress that message. |
|
Then the Apache rule has to be changed. I just made that the webservers behave equal ! ./ocdata, just give this a try and you see that you get the result as I described it. I have this message since the day when .ocdata has been introduced. But this is anyway not the point. I want to supress the access denied message because it is only filling up the log unnecessarily. |
|
This is a policy decision; we should ask the OC security guy to weigh in, they might know why 404 is used here instead of 403. I vaguely remember someone mentioning a scanner that thinks 404's are more secure. We should confirm if that still matters and...
|
|
Can't quite remember why 403 or 404 is used here. Not an NGINX expert myself. Sorry :-) |
|
Thanks Luke, guess we gotta figure out where to contribute now, lol The decision isn't specific to nginx, we're just trying to figure out why it was done this way in apache, and make sure apache/nginx match. |
|
To consider:
Technically both are correct denying acces, but from a security POV, 404 is much better. @LukasReschke independent where you contribute to, I would like to hear your expertise 😄 |
There is no single call to the .ocdata via a GET request: https://github.com/owncloud/core/search?l=php&q=ocdata&utf8=%E2%9C%93 If you're getting this message (i don't get it btw. even without those rules) there is something fishy in your environment.
There is no security benefit from using 404. The structure of ownCloud are known and publicly available. |
|
@RealRancor Security is not definded by code or structures beeing public available. There are dev teams like us, experienced folks and ordinary users. 404 is correct for the users (majority) and therefore it is correct in .htaccess - nginx should just do the same. Opera 37: IE 11: Chrome 50: |
|
@mmattel If there is such an access via GET then we need to create a new bugreport in core. This access doesn't make any sense as mostly every webserver setup out there is blocking an access to "dot" files. |
|
Sorry - pls stop. Everything is fine with the attempt from core accessing /data/.ocdata via the browser because this is the security check for the data directory and it is done when accessing the admin page. You can see this clearly on the logs above. (htaccesstest.txt is legacy code. see lib/private/legacy/util.php) This PR is about aligning nginx to Apache and adds a documentation when an admin wants to suppress access denied .ocdata error log messages. |
|
Sorry, this is plainly wrong. The .ocdata is NOT used for the security check for the data directory als already mentioned above. It makes NO sense to access the .ocdata via the browser so this needs to be fixed in core if the access is done. |
|
If you are so sure, give it a try, allow access to the data directory, access the admin page and watch all the messages. Drill it down, allow access to /data but restrict to /data/.ocdata and watch again. |
|
If it is used then another bugreport is needed in core. Access to .ocdata for proofing if the datadir is protected is no proof if it is protected. If .ocdata is used this is a huge security issue. |
|
Feel free to investigate and write a core issue ... |
|
Already done: owncloud/core#24987 |
|
@mmattel The .ocdata part can be removed as oC 9.0.3+ is now correctly checking for htaccess.txt again: owncloud/core#25045 |
mmattel
force-pushed
the
nginx_location_update
branch
from
a270242 to
e5334f6
Compare
|
@RealRancor done. I did a reset to master because of other already merged PR´s and added the changes accordingly so things are clean now. Thanks for your support, ready to merge (from my pov). |
|
@mmattel Just stumbled over another thing:
http://nginx.org/en/docs/http/ngx_http_rewrite_module.html#return So is the break; after the return needed at all? |
removing break statement
mmattel
force-pushed
the
nginx_location_update
branch
from
e5334f6 to
7d63c82
Compare
|
@RealRancor When I was reading the docs I found the same, but I also found examples on google where folkes used break in addition. Originally I added this for beeing on the secure side. Now digging deeper and also did test with and without again, I removed the break statement and I can tell that everything is working as expected 😄 |
|
👍 from my side (but personally i keep the deny all in my config 😀 ) |
These are small improvements for the nginx setup:
deny;byreturn 404; break;which equals .htaccess[R=404,L].ocdataReferncing the discussion in owncloud/core#24968 (comment)
@josh4trunks Your feedback is more than welcomed 😄