Migrate config.php to proper config format or move config to DB

LukasReschke commented 9 years ago

Currently the ownCloud config is stored within config/config.php within a PHP array, the files has to be r/w.

While this is a pragmatic solution there are several problems with that:

The file has to be writable
The file is in an executable format

From a security PoV it is desirable to make the whole ownCloud folder read-only, if an attacker gained read-only access to the FS he cannot escalate his privileges further. (which might be handy in combination with the server-side encryption app)

Furthermore, using PHP to store the config means that an attacker can execute any arbitrary PHP code as soon as he gained full FS access of the ownCloud folder.

Therefore I'd recommend one of the following approaches:

Migrate config.php to YAML (or JSON or whatever)
- Pro: File is not executable
- Pro: Would still be easy to modify with a terminal
- Contra: File would still be writeable and allow other attack vectors
- Contra: Improper protection allows access to config
Migrate config values to DB and only store DB credentials within config.php
- Pro: Attacker cannot read any config data in case the DB is not reachable from outside
- Pro: Makes it possible to make config.php read-only
- Contra: Not that easy to modify configuration values with a terminal.
  - Can be fixed with using console commands as pointed out below.

LukasReschke commented 9 years ago

To keep backwards-compatibility:

Read settings from config.php
Write new settings to DB which have a higher priority than config.php

That way we would not need to worry about migration too much and could easily make it a read-only file.

PVince81 commented 9 years ago

JSON sounds nice. :wink:

DeepDiver1975 commented 9 years ago

JSON sounds nice. :wink:

I'd vote for JSON over YAML as well - JSON can be read/written with PHP without further components - YAML would need an additional component like symfony/yaml

Contra: Not that easy to modify configuration values with a terminal.

We could provide console commands for this purpose as well as an config editing page if necessary - the console should be enough from my pov

Migrate config values to DB and only store DB credentials within config.php

What we need to keep in mind with this approach is that we carefully need to analyse config settings which belong to the server and which belong to the system. e.g. the version number in config.php is bound to the server and not to the system.

But anyway - I'd vote for tiny steps and go for the json approach first

karlitschek commented 9 years ago

The configfile format could be change of needed. I´m actually not sure if JSON is a format that provides the optimal readability for users. So not sure about that. The other thing is that this file still needs to be writable by ownCloud for changing settings via the GUI, inital setup, support for remote configuration of a server via API, and so on. So I´m not exactly sure how this could help exactly. I see the desire to make everything readonly but not sure this passes the reality check.

LukasReschke commented 9 years ago

The other thing is that this file still needs to be writable by ownCloud for changing settings via the GUI, inital setup, support for remote configuration of a server via API, and so on.

Well, RW on PHP file is bad, RW on a JSON file is not that bad ;-)

If we want to have RO config files we could go with the alternative approach suggested by me:

Store initial config in config.php (including DB data)
New config options are stored in the DB
Config seen by ownCloud is then config.php + config data stored within the DB

We would in this case be able to make the config RO after installation. - This would have the advantage that a wrongly configured webserver will not by accident leak the config.json.

So, there are a lot of possibilities to solve this ;-)

LukasReschke commented 9 years ago

tl;dr: Still dataloss when using IIS and ownCloud. Switching to another config format would fix that and increase security.

I discussed this with @PVince81 on IRC a little bit since this is in my opinion quite an important decision that we need at some point to do. Doing this properly can increase the system hardening for ownCloud installations enormously and shouldn’t be neglected. For example the recent Local file inclusion in core (oC-SA-2014-018) wouldn’t have been possible to exploit in most environments.

But this is not the only reason to consider changing to a read-only config and moving settings to the DB: We still have a very critical bug when running ownCloud with IIS which pretty certainly lead to data-loss or even worse people stealing personal data. It is the good old: “Race-condition when reading and writing config and then borking everything resulting in complete configuration loss and an angry mob that wants to lynch us developers”-bug. While we fixed https://github.com/owncloud/core/issues/12214, the patch is only effective when running on *NIX systems. On Windows IIS servers the patch is pretty much useless and there is certainly a risk of bumping into this bug and then having the configuration lost.

What makes this even more worse is that then an attacker might be able to reinstall the ownCloud instance. Our users are trusting us with one of their most important stuff: Their data. Our goal is to protect those data and such bugs are really bad for that.

Sure, there might be a way to fix this issue but fixing and debugging this for *NIX systems took already a lot of time and I’m pretty sure fixing this might just lead us into other bugs.

I see the point that sysadmins might want to be able to modify the settings with an editor such as vim directly in config.php but considering the move to systems such as systemd where you use a binary utility to even read the logs a command line utility to change the settings instead of a plain config file might be a fair choice.

What I am suggesting is:

Define a set of config options that need to stay in config.php (maintenance/DB config/installed/etc…)
Create a table in the database “systemconfig” (or let’s use oc_appconfig) (tricky point: how to handle arrays etc… - maybe convert to JSON?)
Copy all other settings from the config.php into the “systemconfig” table with a repair step
Create an utility “occ config set” which allows to modify the configuration options (tricky point: how to handle arrays etc… - maybe convert to JSON)
Write new settings only to the database
Read settings from database and the “required” ones (maintenance etc.) from the config.php

We could even do this on an evolutionary base, such as migrating every single setting on it’s own.

From my point of view this would offer us the following advantages:

More security: The config can be made read-only and therefore many attack vectors are gone.
More stability: Bugs such as the race-condition would be gone. (furthermore config.php is opcode instead of data which can result in problems with caches)
Easier deployment: Deploying across multiple application servers is easier as you don’t have to update all the config.php’s on every server. (that said, I actually know of one university that had some data loss as they updated one config.php (“datadirectory”) but not the other one - great if you notice this much later)

First steps would be to progressively move every setting one by one to the new settings table (or oc_appconfig).

nickvergessen commented 9 years ago

Just putting this in here:

one disadvantage of moving it to a table is you need to interact with the db to read/change configs instead of opening the file and the comments from the sample config get lost, unless we store them in the db aswell

bantu commented 9 years ago

"Contra: Not that easy to modify configuration values with a terminal." is a non-issue. You should have console commands for that anyway. You probably also have to flush a cache or two when making changes. See e.g. https://github.com/phpbb/phpbb/tree/develop-ascraeus/phpBB/phpbb/console/command/config

Edit: DeepDiver already mentioned that.

bantu commented 9 years ago

As a follow up to how config values are stored: The config API should be changed for a better support of default values. Specifically, repeating config value defaults all over the code is pointless. There should be one file holding default values as defined by ownCloud and then there should be one file for user specific values that may overwrite the defaults. This way default values can be easily changed with software updates without having to think or care about changes made by the user. As far as I remember the Symfony config component supports all of that, so I would recommend just using that or at least looking into it. http://symfony.com/doc/current/components/config/index.html

LukasReschke commented 9 years ago

Bug report about IIS and config: https://github.com/owncloud/core/issues/12263

LukasReschke commented 9 years ago

Another bug report about NFS and the config: https://github.com/owncloud/core/issues/12300

(Please note that flock is not supported on NFS to my knowledge)

LukasReschke commented 9 years ago

Work-around for @DeepDiver1975 which allows a read-only config is at https://github.com/owncloud/core/pull/12419 … - Still requires a long-term fix aka "moving settings to the database".

butonic commented 9 years ago

From a deployment perspective having a minimal config is desirable. Optimally, only the db config should be necessary. The rest should be stored in the db. This makes administration of several web servers a lot easier, because only the db connection needs to be specified. Then the file would contain maybe five settings related to the database. We could go so far as to only store the DSN (Data Source Name) in a config file: http://doctrine.readthedocs.org/en/latest/en/manual/introduction-to-connections.html Something like mysql://user@unix(/path/to/socket)/pear or pgsql://user:pass@tcp(localhost:5555)/pear.

karlitschek commented 9 years ago

Hmmm. In my opinion everything that is "system configuration" should be in the config file. this makes automatic deployment and setup, remote configuration, config diff and other things a lot easier. This is best practices for most unix services for a long time. Everything that is user config or data should go into the database. The config file (over-)write problem are a bug in our code. This has to be fixed by us. The config file format is a tricky question. the benefit of a .php file is that it will never be readable via the browser. Maybe we switch to a different config file format but keep it a .php file with a "<?php exit; " in the first line.

PVince81 commented 9 years ago

The alternative would be to only have read-only values in the config.php and move all read-write settings to the database. This means that everything that can be configured in the admin page (ex: mail settings) must go to the database. This would solve the overwrite issue.

Or if we still want to go the "everything in the DB except the DB settings" route, then we can provide a CLI tool ./occ deploy-config configfilewithupdatedsettings that will import/update the settings into the database. We wanted to move the external storage settings from mount.json into the DB too at some point, and had the same discussion with @MTRichards about providing a way to deploy from the CLI.

I'm not sure it is worth to spend an additional man-month to try and fight against (the absence of) windows proper file locking... that man-month could be spent to implement the suggestion above.

PVince81 commented 9 years ago

and then if you really want the config settings updates to auto-deploy to the DB, you could have process that observes that "configfilewithupdatesettings" whenever it has changed and use that CLI tool to auto-import them, which gives you almost the same admin experience you had before.

DeepDiver1975 commented 9 years ago

For most of the configuration setting they are fine to stay in the config file as we have it today as they are basic system setup and will not change during regular operations.

With a few exception they are already kind of read-only.

Issues start as soon as we write to the config file - e.g. because of the admin changes some setting in the admin page. This is a critical circumstance in a distributed environment because the write operation will be executed only on ONE web server on the cluster. Other config files will remain unchanged.

On the other hand in a single web server setup (which is the most common setup in the the non-enterprise world) changing the config via the web is almost required because accessing the files on the web server might not be that easy (thinking about web hosters where one gets ftp access only).

This is a circumstance we need to have an answer to.

Proposal:

Introduce a deployment mode switch (deployment.mode = {single, cluster})
In case the mode is 'single' we allow changing the configuration via the web browser
In case the mode is 'cluster' the config file is read only and changes have to be done to the files directly

Furthermore we closely review any write operations to the system config and reevaluate if this is the right approach.

ghost commented 9 years ago

@MTRichards @karlitschek I'd love to catch up on this for a later release (either way, explicitly or implicitly make a decision)

MTRichards commented 9 years ago

Definitely worthy of a discussion, there are (as usual) preferences for people based on their use case and deployment scenario...and preferred deployment tools.

karlitschek commented 9 years ago

Yes. A lot of scenarios and cases to consider. Happy to discuss.

jerrac commented 8 years ago

I'd love to see a read-only configuration file that I can template with a config management tool like Ansible.

I would say that some of the stuff in the config file should only be in the config file, and some should only be in the database.

Like the data dir path would only be in the file, while the mail settings would be in the db.

Installs would have the user make the file rw, then ro after the install was complete.

jnweiger commented 8 years ago

With overwrite.cli.url we need a writable .htaccess too. 9.0.1 offers shortend URLs by means of adding a rewrite rule to .htaccess like this

ErrorDocument 403 /owncloud/core/templates/403.php
ErrorDocument 404 /owncloud/core/templates/404.php
<IfModule mod_rewrite.c>
  RewriteRule . index.php [PT,E=PATH_INFO:$1]
  RewriteBase /owncloud
  <IfModule mod_env.c>
    SetEnv front_controller_active true
    <IfModule mod_dir.c>
      DirectorySlash off
    </IfModule>
  </IfModule>
</IfModule>

This feature needs a review for improved hardening.

PVince81 commented 8 years ago

@butonic @DeepDiver1975 reopen the discussion about having a read-only config.php and move all non-database options to the database ?

DeepDiver1975 commented 8 years ago

@butonic @DeepDiver1975 reopen the discussion about having a read-only config.php and move all non-database options to the database ?

absolutly :heavy_plus_sign: :100:

butonic commented 8 years ago

➕ 💯 for readonly config

cc @cdamken @felixboehm @pako81 @michaelstingl @dercorn @Kawohl @CaptionF @RealRancor @tflidd

butonic commented 8 years ago

cc @pmaier1

PVince81 commented 8 years ago

[ ] decide what format to use for config (JSON, YAML, ...)
[ ] find out what settings are currently writable by OC and must be moved to the database
- [ ] mail settings
- [ ] trusted domains (when accessing from another domain, a popup suggests this)
- [ ] maintenance mode
- [ ] theme name (when disabling at update time)
- [ ] version (at update time)
- [ ] data-fingerprint (when running occ maintenance:data-fingerprint)
[ ] move them to the database from the code's perspective
[ ] implement migration/repair step for moving the config to the database
[ ] adjust release scripts / makefiles ?

butonic commented 8 years ago

9.2?

PVince81 commented 8 years ago

even if the whole thing isn't complete by 9.2, we can still start working on first migrating the read-write settings.

ppaysant commented 7 years ago

Hi, config.php is an easy way to store config at install time. You get the app source files, put them in [owncloud]/apps/, copy/paste the conf proposed in the readme inside the config.php, adapt it quickly to your context, then reload the UI in your browser.

This "copy/paste/adapt" thing is the point.

If the conf must be stored inside the DB, won't it be more difficult for the admin, to set the config options for his apps ? For instance, if the app must have in conf a multi dimensionnal array of contact emails, that seems easier to create that array in a php file than in a db field. (Look at https://github.com/CNRS-DSI-Dev/user_files_migrate configuration section of README for an example of a conf I'm speaking of).

A JSON file or a YAML file could be an alternative to the php one, but a file seems very usefull IMHO.

felixboehm commented 7 years ago

A file needs to be transferred to every webserver in a scaleout, while the db is just there. A file config should only contain informations needed on install, then being readonly, write other configs to DB.

PVince81 commented 7 years ago

@ppaysant there is a command occ config:* to modify database settings. But it might not work with multi-dimensional array. Maybe such arrays need to be converted to a flat list instead.

PVince81 commented 7 years ago

@ppaysant to give more context, the reason for this idea is because currently ownCloud also updates config.php which requires this file to be writable. The idea is to have at least all read-write settings in the database and the read-only ones could stay in config.php if needed.

nickvergessen commented 7 years ago

But it might not work with multi-dimensional array

This was fixed for 9.1

PVince81 commented 7 years ago

backlog for now, need to schedule time for this

felixheidecke commented 7 years ago

Classic case of too long didn't read … 😝 I vote for JSON because JavaScript!!

owncloud / core

Migrate config.php to proper config format or move config to DB #11843

Proposal: