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

Allow root owned config. #11075

Open
1 of 8 tasks
Dreamsorcerer opened this issue Sep 5, 2018 · 24 comments
Open
1 of 8 tasks

Allow root owned config. #11075

Dreamsorcerer opened this issue Sep 5, 2018 · 24 comments

Comments

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Sep 5, 2018

It would be good to fully support read-only configs, in order to be able to support a setup where the config is owned by root in /etc/nextcloud/ for example. This increases security, and better supports packaging Nextcloud into Linux distributions.

With some minor tweaks to the code, I have successfully managed to run Nextcloud as described, and have managed to perform an upgrade successfully by manually making changes to the config.

In order to accomplish this properly, these are the changes I believe are needed:

  • In order to run Nextcloud normally with a root owned config, Nextcloud should check for the owner of the datadirectory instead of the config file (given this is the only thing it should be editing). (Check datadirectory owner, not config owner. #27613, feat: allow to configure php.user #45307)
  • Regression in v24+: fix: regression with updating read-only config #44039
  • For upgrades, Nextcloud should use the database to store non-user options during the upgrade process, rather than editing the config file.
    • The loglevel could be set in the DB, and then perhaps the lowest value between the config and the DB is the level used. This way the upgrade process can set debuglevel to 0 in the DB, and then unset it after completion, leaving the user's setting alone.
    • maintenance could be set in the DB, with maintenance mode being enabled if the flag is found in either the config or DB. Again, the upgrade can then set this in the DB, and unset it after upgrade.
    • version could be set in the DB. Surely the release number is primarily used to ensure the DB is upgraded? Therefore the value belongs in the DB and not with the user config. (This makes me wonder what might go wrong if someone imports their database from a backup, but are still using the current config file with a newer release number and the imported DB is then in the wrong state.)
  • Additionally, Nextcloud should avoid writing default settings to the config. When upgrading, it wanted to write "'theme' = ''", which is the default value if not present.
  • If Nextcloud really does need to update a value in the config, it should pause the upgrade and tell the user what value needs to be changed/added and then allow the user to continue the upgrade after manually updating the config.
@nextcloud-bot
Copy link
Member

GitMate.io thinks possibly related issues are #6550 (Config 'trusted_proxies'. Allow networks with CIDR), #5117 (unsafe nginx config), #10120 (allow to disable encryption), #10121 (allow to disable encryption), and #10018 (Update root.vue).

@ct-zen
Copy link

ct-zen commented Sep 10, 2018

Neither config nor apps should NEED to be writable except when you want to make changes. At all other times those areas should be locked down to prevent the webserver software from making unwanted/unauthorized changes. This way you are protected from unauthorized changes that may come about through weaknesses in any of the systems involved (code, webserver, php, leaked/weak credentials, etc.).

@flokli
Copy link
Contributor

flokli commented Nov 18, 2018

Also very interested in support for a read-only config through the whole lifecycle of nextcloud (installation, running, upgrading).

We currently run into unwanted side-effects with having the occ maintenance:install and nextcloud itself writing config.php, while overriding some parts in override.config.php (see NixOS/nixpkgs#49783 ).

@Dreamsorcerer As for the issues you mentioned, I think it might help breaking them down to PRs, linking to this issue.

Thinking your ideas further, this would also imply the occ maintenance:install command by default reading database options etc. from the config file instead of command line arguments provided (and not writing to the config file on its own, as it doesn't change when options are being read from there obviously).

@Dreamsorcerer
Copy link
Contributor Author

@Dreamsorcerer As for the issues you mentioned, I think it might help breaking them down to PRs, linking to this issue.

Of course, but the 4 line patch I linked is the only work I've put into this so far. Certainly doing a separate PR for each step will be useful if someone does start to work on this seriously.

@skjnldsv skjnldsv added the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Aug 20, 2020
@Kixunil
Copy link

Kixunil commented Jan 28, 2021

I don't think any checks of owners are correct. Checking scripts should only check read access(). (But why check it at all and not just try to read it?) There should be no need for write access.

@Dreamsorcerer
Copy link
Contributor Author

Write access is needed to complete the upgrade, right? Because it will try to update the apps installed.

@Kixunil
Copy link

Kixunil commented Jan 28, 2021

I mean writes should go to DB. Besides upgrades could be handled by package managers instead. Then write should not be needed.

@Dreamsorcerer
Copy link
Contributor Author

But an app is a set of files downloaded on the system (in the datadir). To upgrade them, you must overwrite those files with the new app...

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

Apps can be stored in another directory and/or can also be handled by package managers.

@Dreamsorcerer
Copy link
Contributor Author

Dreamsorcerer commented Jan 29, 2021

It would still need write access to that directory in order to complete the upgrade, how would that be any different from how it works right now?

Package managers sounds like a bad idea, as now you need someone to package and update everything in the Nextcloud app store and keep up to date with all new submissions. You would also need to disable the app store in the Nextcloud interface as it would result in broken behaviour. My proposal is to make it easier to package Nextcloud (e.g. in Debian), not harder.

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

The question is what exactly does it need to write to finish the upgrade? Why is replacing the files (by package managers) not sufficient? Can the code be modified to no longer need it? If yes, then it gives the users/packagers more options, to choose from. They can still do it without packaging. It's not forcing people into it.

You enumerated disadvantages of packaging but forgot about advantages. A very similar situation arises with Python PIP. The software outside system package management can not have system-level dependencies or they must be installed manually. This is already the case with Nextcloud video conversion app which requires you to install ffmpeg manually. If it were Debian package for instance, this would be solved trivially.

Updates are obviously easier to perform using one tool rather than two tools.

System packaging also often gives you some guarantees about the packaged software: it's reviewed and signed. I didn't check Nextcloud apps deeply enough to tell but I'd be surprised if Nextcloud app store has security and reliability guarantees comparable to those of Debian.

Finally, the problems you describe can be mitigated by automating packaging (as is already done in Debian in cases of Python, Rust...) Disabling app store is already possible in config and it's non-issue if the administrator installed Nextcloud using package manger - he can install apps too.

I would personally like to see it the same as with Python PIP - package apps as .deb but also keep the option to use app store.

@Dreamsorcerer
Copy link
Contributor Author

This still sounds like a lot more work than I imagine a Debian maintainer would like to take on. My proposal here is to imitate the WordPress package in Debian. Note that there are not thousands of packages in Debian for every plugin and theme available in the WP store. Nextcloud uses a very similar design and I expect Debian maintainers would look to handle it in much the same way.

There are certainly user advantages to what you propose, I just don't think any maintainer will agree that those are worth the extra work though.

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

Depends, really. Some may be more willing than others. There's tons of python packages for instance. Also there's a few Wordpress themes and one plugin. It's not out of question. My point is make it possible, not mandatory.

I would personally like to take a closer look at it at some point.

@Dreamsorcerer
Copy link
Contributor Author

Dreamsorcerer commented Jan 29, 2021

The themes are the pre-installed ones for previous versions of WP, I suspect they are included for some kind of backwards-compatibility reason. Not sure about the random plugin.

Regardless, my point is that the user still has the choice to install plugins from the store. This requires write access to the plugins directory. In much the same way, if the user has the choice to install apps in Nextcloud, we need write access to the data directory.

I'm not even sure why we're arguing about this, because in what possible situation would the server not have write access to the data directory? This is only likely if you've misconfigured something and your install is therefore broken...
At worst, in your ideal world, this check is pointless, but should always pass. In any other case (where the user can install apps from the store) this is a useful sanity check before starting the upgrade.

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

Oh, you were talking about writing to data directory (/var/lib/nextcloud-server?), which must obviously be allowed for other reasons. I was thinking about /apps directory inside the source code (presumably /usr/share/nextcloud-server/apps). Ideally the default would be for the two to not conflict. Just as PIP doesn't conflict with system packages. (Although it does lead to surprising results so I wouldn't recommend it using same package from both.)

@Dreamsorcerer
Copy link
Contributor Author

Ah, I forgot that this is not the default behaviour. The way I have the config setup (which would be required for Debian packages) is with an app path in the data directory. It still picks up system installed apps just fine, but user installed apps go into the data directory.

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

Yes, you can have multiple paths and it should be used in packaging. :)
Not sure what happens if you install same app twice.

@Dreamsorcerer
Copy link
Contributor Author

But, basically, it's that path which needs to be writable, because Nextcloud will try to upgrade incompatible apps.
System apps in the read-only path are fine, they won't be touched.

With a couple of hacks I have this working already, with me manually updating the core nextcloud code as root, and then running the usual upgrade step in a browser.

At some point, I want to come back to this issue so I can implement it properly without hacks and make it user-friendly.

@Kixunil
Copy link

Kixunil commented Jan 29, 2021

Cool! I was looking into making maintenance:install read the config so that passwords don't have to be on command line. (Useful for dbconfig in Debian.) I have something somewhat working but I got stuck at login redirect loop (I tried passing various pathinfo/script names, nothing seems to help.). By digging into the code a lot it seems that /login route is lost somewhere but I can't understand where. If you happen to have an idea that'd be great. (I realize that this is off-topic but if anyone helps me, it frees up my time to look at the topic of this issue. :))

@Dreamsorcerer
Copy link
Contributor Author

No idea, sorry.

@szaimen
Copy link
Contributor

szaimen commented Jun 22, 2021

Do you mind creating a PR with this patch for further discussion?
Thank you!

@szaimen szaimen removed the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Jun 23, 2021
@Dreamsorcerer
Copy link
Contributor Author

First step has finally been merged. Next up is storing some of the options used during upgrade in the DB.

@Kixunil
Copy link

Kixunil commented Mar 1, 2024

Cool! FYI I have some initial packaging work done. It works mostly well except upgrades of NC versions are now broken. Also I've automated the apps packaging as I mentioned, it was pretty easy.

@Dreamsorcerer

This comment has been minimized.

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

No branches or pull requests

8 participants