Raise a setup warning when locked configuration has a configuration value stored in the database

Summary:
Ref T13249. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>.

Today, when a configuration value is "locked", we prevent //writes// to the database. However, we still perform reads. When you upgrade, we generally don't want a bunch of your configuration to change by surprise.

Some day, I'd like to stop reading locked configuration from the database. This would defuse an escalation where an attacker finds a way to write to locked configuration despite safeguards, e.g. through SQL injection or policy bypass. Today, they could write to `cluster.mailers` or similar and substantially escalate access. A better behavior would be to ignore database values for `cluster.mailers` and other locked config, so that these impermissible writes have no effect.

Doing this today would break a lot of installs, but we can warn them about it now and then make the change at a later date.

Test Plan:
  - Forced a `phd.taskmasters` config value into the database.
  - Saw setup warning.
  - Used `bin/config delete --database phd.taskmasters` to clear the warning.
  - Reviewed documentation changes.
  - Reviewed `phd.taskmasters` documentation adjustment.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20159

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -15,6 +15,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
 
     $defined_keys = PhabricatorApplicationConfigOptions::loadAllOptions();
 
+    $stack = PhabricatorEnv::getConfigSourceStack();
+    $stack = $stack->getStack();
+
     foreach ($all_keys as $key) {
       if (isset($defined_keys[$key])) {
         continue;
@@ -48,9 +51,6 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
         ->setName($name)
         ->setSummary($summary);
 
-      $stack = PhabricatorEnv::getConfigSourceStack();
-      $stack = $stack->getStack();
-
       $found = array();
       $found_local = false;
       $found_database = false;
@@ -85,6 +85,101 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       }
     }
 
+    $options = PhabricatorApplicationConfigOptions::loadAllOptions();
+    foreach ($defined_keys as $key => $value) {
+      $option = idx($options, $key);
+      if (!$option) {
+        continue;
+      }
+
+      if (!$option->getLocked()) {
+        continue;
+      }
+
+      $found_database = false;
+      foreach ($stack as $source_key => $source) {
+        $value = $source->getKeys(array($key));
+        if ($value) {
+          if ($source instanceof PhabricatorConfigDatabaseSource) {
+            $found_database = true;
+            break;
+          }
+        }
+      }
+
+      if (!$found_database) {
+        continue;
+      }
+
+      // NOTE: These are values which we don't let you edit directly, but edit
+      // via other UI workflows. For now, don't raise this warning about them.
+      // In the future, before we stop reading database configuration for
+      // locked values, we either need to add a flag which lets these values
+      // continue reading from the database or move them to some other storage
+      // mechanism.
+      $soft_locks = array(
+        'phabricator.uninstalled-applications',
+        'phabricator.application-settings',
+        'config.ignore-issues',
+      );
+      $soft_locks = array_fuse($soft_locks);
+      if (isset($soft_locks[$key])) {
+        continue;
+      }
+
+      $doc_name = 'Configuration Guide: Locked and Hidden Configuration';
+      $doc_href = PhabricatorEnv::getDoclink($doc_name);
+
+      $set_command = phutil_tag(
+        'tt',
+        array(),
+        csprintf(
+          'bin/config set %R <value>',
+          $key));
+
+      $summary = pht(
+        'Configuration value "%s" is locked, but has a value in the database.',
+        $key);
+      $message = pht(
+        'The configuration value "%s" is locked (so it can not be edited '.
+        'from the web UI), but has a database value. Usually, this means '.
+        'that it was previously not locked, you set it using the web UI, '.
+        'and it later became locked.'.
+        "\n\n".
+        'You should copy this configuration value in a local configuration '.
+        'source (usually by using %s) and then remove it from the database '.
+        'with the command below.'.
+        "\n\n".
+        'For more information on locked and hidden configuration, including '.
+        'details about this setup issue, see %s.'.
+        "\n\n".
+        'This database value is currently respected, but a future version '.
+        'of Phabricator will stop respecting database values for locked '.
+        'configuration options.',
+        $key,
+        $set_command,
+        phutil_tag(
+          'a',
+          array(
+            'href' => $doc_href,
+            'target' => '_blank',
+          ),
+          $doc_name));
+      $command = csprintf(
+        'phabricator/ $ ./bin/config delete --database %R',
+        $key);
+
+      $this->newIssue('config.locked.'.$key)
+        ->setShortName(pht('Deprecated Config Source'))
+        ->setName(
+          pht(
+            'Locked Configuration Option "%s" Has Database Value',
+            $key))
+        ->setSummary($summary)
+        ->setMessage($message)
+        ->addCommand($command)
+        ->addPhabricatorConfig($key);
+    }
 
     if (PhabricatorEnv::getEnvConfig('feed.http-hooks')) {
       $this->newIssue('config.deprecated.feed.http-hooks')

src/applications/config/option/PhabricatorPHDConfigOptions.php

@@ -41,7 +41,12 @@ final class PhabricatorPHDConfigOptions
             "If you are running a cluster, this limit applies separately ".
             "to each instance of `phd`. For example, if this limit is set ".
             "to `4` and you have three hosts running daemons, the effective ".
-            "global limit will be 12.")),
+            "global limit will be 12.".
+            "\n\n".
+            "After changing this value, you must restart the daemons. Most ".
+            "configuration changes are picked up by the daemons ".
+            "automatically, but pool sizes can not be changed without a ".
+            "restart.")),
       $this->newOption('phd.verbose', 'bool', false)
         ->setLocked(true)
         ->setBoolOptions(

src/docs/user/configuration/configuration_locked.diviner

@@ -111,6 +111,55 @@ phabricator/ $ ./bin/config set <key> <value>
 ```
 
 
+Locked Configuration With Database Values
+=========================================
+
+You may receive a setup issue warning you that a locked configuration key has a
+value set in the database. Most commonly, this is because:
+
+  - In some earlier version of Phabricator, this configuration was not locked.
+  - In the past, you or some other administrator used the web UI to set a
+    value. This value was written to the database.
+  - In a later version of the software, the value became locked.
+
+When Phabricator was originally released, locked configuration did not yet
+exist. Locked configuration was introduced later, and then configuration options
+were gradually locked for a long time after that.
+
+In some cases the meaning of a value changed and it became possible to use it
+to break an install or the configuration became a security risk. In other
+cases, we identified an existing security risk or arrived at some other reason
+to lock the value.
+
+Locking values was more common in the past, and it is now relatively rare for
+an unlocked value to become locked: when new values are introduced, they are
+generally locked or hidden appropriately. In most cases, this setup issue only
+affects installs that have used Phabricator for a long time.
+
+At time of writing (February 2019), Phabricator currently respects these old
+database values. However, some future version of Phabricator will refuse to
+read locked configuration from the database, because this improves security if
+an attacker manages to find a way to bypass restrictions on editing locked
+configuration from the web UI.
+
+To clear this setup warning and avoid surprise behavioral changes in the future,
+you should move these configuration values from the database to a local config
+file. Usually, you'll do this by first copying the value from the database:
+
+```
+phabricator/ $ ./bin/config set <key> <value>
+```
+
+...and then removing the database value:
+
+```
+phabricator/ $ ./bin/config delete --database <key>
+```
+
+See @{Configuration User Guide: Advanced Configuration} for some more detailed
+discussion of different configuration sources.
+
+
 Next Steps
 ==========