From 7c366f160213e70aa874a5b201b23d9ff58793bd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Jan 2017 11:42:36 +0100 Subject: [PATCH 1/2] Filter out sensitive appconfig values Signed-off-by: Joas Schilling --- core/Command/Config/ListConfigs.php | 19 ++++++++-- lib/private/AllConfig.php | 2 +- lib/private/AppConfig.php | 55 ++++++++++++++++++++++++----- lib/public/IAppConfig.php | 9 +++++ 4 files changed, 74 insertions(+), 11 deletions(-) diff --git a/core/Command/Config/ListConfigs.php b/core/Command/Config/ListConfigs.php index 2737bc2cea..94b493c924 100644 --- a/core/Command/Config/ListConfigs.php +++ b/core/Command/Config/ListConfigs.php @@ -89,14 +89,14 @@ class ListConfigs extends Base { 'apps' => [], ]; foreach ($apps as $appName) { - $configs['apps'][$appName] = $this->appConfig->getValues($appName, false); + $configs['apps'][$appName] = $this->getAppConfigs($appName, $noSensitiveValues); } break; default: $configs = [ 'apps' => [ - $app => $this->appConfig->getValues($app, false), + $app => $this->getAppConfigs($app, $noSensitiveValues), ], ]; } @@ -129,6 +129,21 @@ class ListConfigs extends Base { return $configs; } + /** + * Get the app configs + * + * @param string $app + * @param bool $noSensitiveValues + * @return array + */ + protected function getAppConfigs($app, $noSensitiveValues) { + if ($noSensitiveValues) { + return $this->appConfig->getFilteredValues($app, false); + } else { + return $this->appConfig->getValues($app, false); + } + } + /** * @param string $argumentName * @param CompletionContext $context diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index 4e13d70371..fc1f862cd0 100644 --- a/lib/private/AllConfig.php +++ b/lib/private/AllConfig.php @@ -68,7 +68,7 @@ class AllConfig implements \OCP\IConfig { /** * @param SystemConfig $systemConfig */ - function __construct(SystemConfig $systemConfig) { + public function __construct(SystemConfig $systemConfig) { $this->userCache = new CappedMemoryCache(); $this->systemConfig = $systemConfig; } diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index d92e8965b5..06e760e86f 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -29,7 +29,9 @@ namespace OC; +use OC\DB\OracleConnection; use OCP\IAppConfig; +use OCP\IConfig; use OCP\IDBConnection; /** @@ -37,12 +39,22 @@ use OCP\IDBConnection; * database. */ class AppConfig implements IAppConfig { - /** - * @var \OCP\IDBConnection $conn - */ + + /** @var array[] */ + protected $sensitiveValues = [ + 'user_ldap' => [ + 'ldap_agent_password', + ], + ]; + + /** @var \OCP\IDBConnection */ protected $conn; - private $cache = array(); + /** @var array[] */ + private $cache = []; + + /** @var bool */ + private $configLoaded = false; /** * @param IDBConnection $conn @@ -85,6 +97,7 @@ class AppConfig implements IAppConfig { * * @param string $app the app we are looking for * @return array an array of key names + * @deprecated 8.0.0 use method getAppKeys of \OCP\IConfig * * This function gets all keys of an app. Please note that the values are * not returned. @@ -112,6 +125,7 @@ class AppConfig implements IAppConfig { * @param string $key key * @param string $default = null, default value if the key does not exist * @return string the value or $default + * @deprecated 8.0.0 use method getAppValue of \OCP\IConfig * * This function gets a value from the appconfig table. If the key does * not exist the default value will be returned @@ -146,6 +160,7 @@ class AppConfig implements IAppConfig { * @param string $key key * @param string|float|int $value value * @return bool True if the value was inserted or updated, false if the value was the same + * @deprecated 8.0.0 use method setAppValue of \OCP\IConfig */ public function setValue($app, $key, $value) { if (!$this->hasKey($app, $key)) { @@ -182,7 +197,7 @@ class AppConfig implements IAppConfig { * http://docs.oracle.com/cd/E11882_01/server.112/e26088/conditions002.htm#i1033286 * > Large objects (LOBs) are not supported in comparison conditions. */ - if (!($this->conn instanceof \OC\DB\OracleConnection)) { + if (!($this->conn instanceof OracleConnection)) { // Only update the value when it is not the same $sql->andWhere($sql->expr()->neq('configvalue', $sql->createParameter('configvalue'))) ->setParameter('configvalue', $value); @@ -200,7 +215,8 @@ class AppConfig implements IAppConfig { * * @param string $app app * @param string $key key - * @return boolean|null + * @return boolean + * @deprecated 8.0.0 use method deleteAppValue of \OCP\IConfig */ public function deleteKey($app, $key) { $this->loadConfigValues(); @@ -214,13 +230,15 @@ class AppConfig implements IAppConfig { $sql->execute(); unset($this->cache[$app][$key]); + return false; } /** * Remove app from appconfig * * @param string $app app - * @return boolean|null + * @return boolean + * @deprecated 8.0.0 use method deleteAppValue of \OCP\IConfig * * Removes all keys in appconfig belonging to the app. */ @@ -234,6 +252,7 @@ class AppConfig implements IAppConfig { $sql->execute(); unset($this->cache[$app]); + return false; } /** @@ -261,11 +280,31 @@ class AppConfig implements IAppConfig { } } + /** + * get all values of the app or and filters out sensitive data + * + * @param string $app + * @return array + */ + public function getFilteredValues($app) { + $values = $this->getValues($app, false); + + foreach ($this->sensitiveValues[$app] as $sensitiveKey) { + if (isset($values[$sensitiveKey])) { + $values[$sensitiveKey] = IConfig::SENSITIVE_VALUE; + } + } + + return $values; + } + /** * Load all the app config values */ protected function loadConfigValues() { - if ($this->configLoaded) return; + if ($this->configLoaded) { + return; + } $this->cache = []; diff --git a/lib/public/IAppConfig.php b/lib/public/IAppConfig.php index 01aca47ad8..4a92a22484 100644 --- a/lib/public/IAppConfig.php +++ b/lib/public/IAppConfig.php @@ -86,6 +86,15 @@ interface IAppConfig { */ public function getValues($app, $key); + /** + * get all values of the app or and filters out sensitive data + * + * @param string $app + * @return array + * @since 12.0.0 + */ + public function getFilteredValues($app); + /** * sets a value in the appconfig * @param string $app app From cf849e621b378b87d92845f67fe0281770292269 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 12 Jan 2017 10:49:22 +0100 Subject: [PATCH 2/2] Fix tests Signed-off-by: Joas Schilling --- lib/private/SystemConfig.php | 1 - tests/Core/Command/Config/ListConfigsTest.php | 6 +++++ tests/lib/AppConfigTest.php | 23 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/private/SystemConfig.php b/lib/private/SystemConfig.php index 1029a6619f..e5f1adaf00 100644 --- a/lib/private/SystemConfig.php +++ b/lib/private/SystemConfig.php @@ -44,7 +44,6 @@ class SystemConfig { 'passwordsalt' => true, 'secret' => true, 'updater.secret' => true, - 'ldap_agent_password' => true, 'proxyuserpwd' => true, 'log.condition' => [ 'shared_secret' => true, diff --git a/tests/Core/Command/Config/ListConfigsTest.php b/tests/Core/Command/Config/ListConfigsTest.php index 0f170cee84..861c1f59d5 100644 --- a/tests/Core/Command/Config/ListConfigsTest.php +++ b/tests/Core/Command/Config/ListConfigsTest.php @@ -285,10 +285,16 @@ class ListConfigsTest extends TestCase { $this->systemConfig->expects($this->any()) ->method('getValue') ->willReturnMap($systemConfigMap); + $this->appConfig->expects($this->any()) + ->method('getValues') + ->willReturnMap($appConfig); } else { $this->systemConfig->expects($this->any()) ->method('getFilteredValue') ->willReturnMap($systemConfigMap); + $this->appConfig->expects($this->any()) + ->method('getFilteredValues') + ->willReturnMap($appConfig); } $this->appConfig->expects($this->any()) diff --git a/tests/lib/AppConfigTest.php b/tests/lib/AppConfigTest.php index c4da750775..fed929352d 100644 --- a/tests/lib/AppConfigTest.php +++ b/tests/lib/AppConfigTest.php @@ -8,6 +8,7 @@ */ namespace Test; +use OCP\IConfig; /** * Class AppConfigTest @@ -305,6 +306,28 @@ class AppConfigTest extends TestCase { $this->assertEquals($expected, $values); } + public function testGetFilteredValues() { + /** @var \OC\AppConfig|\PHPUnit_Framework_MockObject_MockObject $config */ + $config = $this->getMockBuilder(\OC\AppConfig::class) + ->setConstructorArgs([\OC::$server->getDatabaseConnection()]) + ->setMethods(['getValues']) + ->getMock(); + + $config->expects($this->once()) + ->method('getValues') + ->with('user_ldap', false) + ->willReturn([ + 'ldap_agent_password' => 'secret', + 'ldap_dn' => 'dn', + ]); + + $values = $config->getFilteredValues('user_ldap'); + $this->assertEquals([ + 'ldap_agent_password' => IConfig::SENSITIVE_VALUE, + 'ldap_dn' => 'dn', + ], $values); + } + public function testSettingConfigParallel() { $appConfig1 = new \OC\AppConfig(\OC::$server->getDatabaseConnection()); $appConfig2 = new \OC\AppConfig(\OC::$server->getDatabaseConnection());