Skip to content

Conversation

@phisch
Copy link
Contributor

@phisch phisch commented Nov 7, 2016

Description

This enables you to override configuration values with a corresponding environment variable.
Environment variables need to be prefixed with OC_. For example to override the dbname value, simply set the environment variable OC_dbname.

Related Issue

Motivation and Context

It is very convenient to be able to store sensitive data like database or mail credentials in an environment variable.

How Has This Been Tested?

Has been tested manually by using php-webserver like this:

OC_dbname=owncloud_database_name php -d variables_order=EGPCS -S localhost:8080

and with PhpUnit, tests included.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mention-bot
Copy link

@PhilippSchaffrath, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @nickvergessen and @LukasReschke to be potential reviewers.

@phisch phisch changed the title added functionality to override config.php values with 'OC_' prefixed… Override config.php values through ENV variables Nov 7, 2016
* @return mixed the value or $default
*/
public function getValue($key, $default = null) {
$envKey = self::ENV_PREFIX . $key;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use getenv()? makes it case insensitive and searches $_SERVER and $_ENV (which might not be populated). See http://stackoverflow.com/a/27077452

Copy link
Contributor Author

@phisch phisch Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! Fixed that and the tests.
Seems like environment variables set by Apache are case insensitive, those set by putenv are not. Just php things...

@PVince81
Copy link
Contributor

PVince81 commented Nov 8, 2016

I guess this is only ever used for testing purposes ? I don't see any use case for this in a productive environment.

@butonic
Copy link
Member

butonic commented Nov 8, 2016

actually deployment becomes easier when the credentials don't have to be added to the config.php I think @felixboehm may have more experience with cases like that.

It also allows us to pass weird security constraints where the password should not be written in plaintext in the config.php (which only prevents exposing the password when the php handler is disabled but otherwise does not increase security IMO).

@felixboehm
Copy link
Contributor

Perfect for production systems, using .env files and having different environments.

@phisch
Copy link
Contributor Author

phisch commented Nov 14, 2016

We have to make sure that cronjobs and occ commands still work.

@DeepDiver1975
Copy link
Member

We have to make sure that cronjobs and occ commands still work.

this is the biggest problem I see. We are using different environments for web, cli and cron execution.
There are always issues popping up because people use different php runtimes/version.

I guess you get my point.

Simple overwritting is not sufficient. I suggest to have this exclusive to make sure that in case a value is missing the process throws an exception and fails.

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Nov 21, 2016
@butonic
Copy link
Member

butonic commented Nov 21, 2016

@phisch
Copy link
Contributor Author

phisch commented Nov 22, 2016

@butonic i would ignore AWS for now.

@DeepDiver1975 I don't see this change happening because of the cron / occ problems. The advantage behind the idea was not setting credentials in the config.php but through environment variables. Doing so will make the occ and cron scripts not work anymore. You could do some tricks to get them to work, but that would mean adding credentials in crontab and every time you run an occ command, which is a very bad user experience and behavior.

I would close this PR if you guys don't see another solution for this!

@DeepDiver1975
Copy link
Member

I would close this PR if you guys don't see another solution for this!

yes - let's close this - THX

@phisch phisch closed this Nov 22, 2016
@phisch
Copy link
Contributor Author

phisch commented Nov 22, 2016

Deleting branch tomorrow unless someone has some more feedback.

@butonic
Copy link
Member

butonic commented Nov 24, 2016

we could define the environment variables in /etc/owncloud/env.php:

<?php
putenv('OC_dbuser=owncloud');
putenv('OC_dbpass=secret');
putenv('OC_dbhost=10.7.23.42');
// or whatever, eg oracle stuff
putenv('ORACLE_HOME=/opt/instantclient_11_2');
putenv('LD_LIBRARY_PATH=/opt/instantclient_11_2');
getenv('TNS_ADMIN=/opt/tnsadmin');

Then in eg /etc/php/5.6/mods-available/owncloud.ini use auto_prepend_file='/etc/owncloud/env.php' and enable the config with phpenmod owncloud. Also see http://php.net/manual/de/ini.core.php#ini.auto-prepend-file

It works like including a php file ... AND works for cron, occ and mod_php / php-fpm because phpenmod takes care of that.

It's still not exactly the same as having the env in the apache config because then only root would be able to read the file ... duh.

@phisch
Copy link
Contributor Author

phisch commented Dec 13, 2016

Reopening this, as it is still a requested feature and the discussion came up again.

As long as we document how to overwrite config values through environment variables for cronjobs (which is possible), there should be nothing against this.

@phisch phisch reopened this Dec 13, 2016
@phisch phisch force-pushed the environment-override-config branch from d23c2f3 to d755908 Compare December 13, 2016 10:08
@phisch
Copy link
Contributor Author

phisch commented Dec 13, 2016

rebased

@phisch
Copy link
Contributor Author

phisch commented Dec 19, 2016

I don't think we can improve this any further. At least this enables users to configure through environment variables.

  • We should provide examples for cron and cli environment variable injection in our documentation to prevent unnecessary confusion. I am totally fine with this PR as long as this is given.

@DeepDiver1975 can you poke jenkins?

@butonic
Copy link
Member

butonic commented Dec 19, 2016

👍

@phisch phisch force-pushed the environment-override-config branch from d755908 to 7b0e6b9 Compare January 24, 2017 08:43
@phisch
Copy link
Contributor Author

phisch commented Jan 24, 2017

rebased again, hoping jenkins will finally not randomly fail

@phisch phisch merged commit 994e2c0 into master Jan 24, 2017
@phisch phisch deleted the environment-override-config branch January 24, 2017 11:18
@PVince81
Copy link
Contributor

is a backport required ? (green ticket, etc...)

@phisch
Copy link
Contributor Author

phisch commented Jan 24, 2017

@PVince81 hmm, maybe 😅 i'll check and create them if necessary.

edit: backport for 9.1 needed

phisch added a commit that referenced this pull request Jan 24, 2017
* added functionality to override config.php values with 'OC_' prefixed environment variables

* use getenv to read environment variables since apache does not set $_ENV variables, fixed test
@mmattel
Copy link
Contributor

mmattel commented Jan 24, 2017

Question: is there a documentation update to describe the changes? Else this stays hidden and gets forgotten...

@PVince81
Copy link
Contributor

@PhilippSchaffrath please open a documentation ticket and describe how to use it.

CC @settermjd

@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants