Suggest optional security hardening in systemd service#15079
Conversation
Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
|
Hey! Thanks for this PR! |
|
Hey @skjnldsv All of the settings are explained in systemd documentation, i.e. https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html Regardless, I'm attaching screenshot of
|
In edge case, if user put nextcloud files under home of the selected user, this setting would cause them to be read-only. Normally for such cases we could use ReadWritePaths, but then we'd need to hardcode placement of the nextcloud files, which is unwanted. Better to remove this setting and it leave it up to the system administrators if they want to add further protection, the setting is not as critical as other anyway. Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
|
@JustArchi then please add that link to the docs. Our documentation should be as straightforward as possible. An admin should not have to search deeper on other docs to understand why we do X or y (ideally of course) |
Then a bit more explanations is still warranted I think. Even if we don't add that to the systemd file, we need to be more transparent |
Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
|
Sure, done. |
This allows RW operations without specifying read write paths, for as long as user did not pick crucial directories for operation, and they should not. Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
Now, this is currently not required for notify_push functionality to the best of my knowledge. However, in case notify_push would somehow write to the nextcloud files in the future, it won't hurt to account for that case, as full system protection should be good enough for what we're aiming for. See nextcloud/documentation#15079 (comment) Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
skjnldsv
left a comment
There was a problem hiding this comment.
So, I think we're getting close to approval, but there is still potentially breaking options that I think warrant a .. warning::
These options will silently break common Nextcloud setups:
PrivateMounts=yesisolates the mount namespace like we discussed. Any external storage (NFS, SMB, local mounts via config.php) that the cron job tries to scan will be invisible. This is a real regression for many admins, not just edge-case.PrivateUsers=yescreates a user namespace. File permission checks on the host filesystem (e.g., data directory owned by www-data) can fail because the UID mapping may not carry through to bind-mounted paths.UMask=0077creates files/dirs as 600/700. If cron writes temp files or cache that the web server process reads (both running as www-data but potentially different forks), this can cause permission errors. Also breaks setups where the data dir needs group access.
Signed-off-by: Łukasz Domeradzki <JustArchi@JustArchi.net>
|
@skjnldsv I've agreed with and done the following:
Still wondering if
Because this can happen only in one very specific edge case:
Only in this case nginx will not be able to serve the file directly, will resort to php engine, and that one will resolve the file correctly. I ran But this is appdata, data directory, nginx does not serve files from there directly, ever, and cron should never write inside nextcloud's root directory to the best of my knowledge (it can even be read-only, so this UMask will be the last problem in this case, as a lot of setups have those files owned by e.g. Therefore, I believe current version is correct in regards to your review - feel free to give it another shot 👍 |

☑️ Resolves
This is similar to pull request nextcloud/notify_push#704 that I opened for notify_push. I didn't find any existing issue opened that this PR resolves.
In particular, this PR adds a few additional
[Service]systemd entries that are wanted for people that want to ensure additional security bulletproofing on their system.I verified that nextcloud's cron service works properly with no errors or warnings upon applying. I've been using this configuration on my own machine since a few weeks now and everything is working properly - most importantly, nextcloud reports new version as available for updating, which proves that the most crucial cron functionality works properly.
I believe this is worthy addition. If you want to make it truly optional, I can also comment out all of those entries, leaving them up to the user to enable. Considering the fact that it doesn't create any apparent issues however, I believe they should be enabled by default.
Thanks in advance for considering this PR.
🖼️ Screenshots
✅ Checklist
codespellor similar and addressed any spelling issues