From 0f9776fb58e268a119225bae59143231ae229122 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Wed, 10 Apr 2019 15:26:04 -0700 Subject: [PATCH] Add a workflow and a new config option for locking authentication providers Summary: Ref T7667. Adds new flows `bin/auth lock` and `bin/auth unlock` to prevent compromised administrator accounts from doing additional damage by altering the authentication provider configuration. Note that this currently doesn't actually do anything because we aren't checking this config key in any of the edit controllers yet. Test Plan: Ran `lock` and `unlock`, checked for correct DB state, observed expected setup warning. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T7667 Differential Revision: https://secure.phabricator.com/D20394 --- src/__phutil_library_map__.php | 4 ++ .../PhabricatorAuthManagementLockWorkflow.php | 32 ++++++++++++++++ ...habricatorAuthManagementUnlockWorkflow.php | 33 +++++++++++++++++ .../check/PhabricatorAuthSetupCheck.php | 37 +++++++++++++++++++ ...PhabricatorAuthenticationConfigOptions.php | 20 ++++++++++ 5 files changed, 126 insertions(+) create mode 100644 src/applications/auth/management/PhabricatorAuthManagementLockWorkflow.php create mode 100644 src/applications/auth/management/PhabricatorAuthManagementUnlockWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 037344c09f..28cb330c53 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2306,12 +2306,14 @@ phutil_register_library_map(array( 'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php', 'PhabricatorAuthManagementListFactorsWorkflow' => 'applications/auth/management/PhabricatorAuthManagementListFactorsWorkflow.php', 'PhabricatorAuthManagementListMFAProvidersWorkflow' => 'applications/auth/management/PhabricatorAuthManagementListMFAProvidersWorkflow.php', + 'PhabricatorAuthManagementLockWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLockWorkflow.php', 'PhabricatorAuthManagementRecoverWorkflow' => 'applications/auth/management/PhabricatorAuthManagementRecoverWorkflow.php', 'PhabricatorAuthManagementRefreshWorkflow' => 'applications/auth/management/PhabricatorAuthManagementRefreshWorkflow.php', 'PhabricatorAuthManagementRevokeWorkflow' => 'applications/auth/management/PhabricatorAuthManagementRevokeWorkflow.php', 'PhabricatorAuthManagementStripWorkflow' => 'applications/auth/management/PhabricatorAuthManagementStripWorkflow.php', 'PhabricatorAuthManagementTrustOAuthClientWorkflow' => 'applications/auth/management/PhabricatorAuthManagementTrustOAuthClientWorkflow.php', 'PhabricatorAuthManagementUnlimitWorkflow' => 'applications/auth/management/PhabricatorAuthManagementUnlimitWorkflow.php', + 'PhabricatorAuthManagementUnlockWorkflow' => 'applications/auth/management/PhabricatorAuthManagementUnlockWorkflow.php', 'PhabricatorAuthManagementUntrustOAuthClientWorkflow' => 'applications/auth/management/PhabricatorAuthManagementUntrustOAuthClientWorkflow.php', 'PhabricatorAuthManagementVerifyWorkflow' => 'applications/auth/management/PhabricatorAuthManagementVerifyWorkflow.php', 'PhabricatorAuthManagementWorkflow' => 'applications/auth/management/PhabricatorAuthManagementWorkflow.php', @@ -8174,12 +8176,14 @@ phutil_register_library_map(array( 'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementListFactorsWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementListMFAProvidersWorkflow' => 'PhabricatorAuthManagementWorkflow', + 'PhabricatorAuthManagementLockWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementRecoverWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementRefreshWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementRevokeWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementStripWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementTrustOAuthClientWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementUnlimitWorkflow' => 'PhabricatorAuthManagementWorkflow', + 'PhabricatorAuthManagementUnlockWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementUntrustOAuthClientWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementVerifyWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementWorkflow' => 'PhabricatorManagementWorkflow', diff --git a/src/applications/auth/management/PhabricatorAuthManagementLockWorkflow.php b/src/applications/auth/management/PhabricatorAuthManagementLockWorkflow.php new file mode 100644 index 0000000000..e15069f775 --- /dev/null +++ b/src/applications/auth/management/PhabricatorAuthManagementLockWorkflow.php @@ -0,0 +1,32 @@ +setName('lock') + ->setExamples('**lock**') + ->setSynopsis( + pht( + 'Lock authentication provider config, to prevent changes to '. + 'the config without doing **bin/auth unlock**.')); + } + + public function execute(PhutilArgumentParser $args) { + $console = PhutilConsole::getConsole(); + + $key = 'auth.lock-config'; + $config_entry = PhabricatorConfigEntry::loadConfigEntry($key); + $config_entry->setValue(true); + + // If the entry has been deleted, resurrect it. + $config_entry->setIsDeleted(0); + + $config_entry->save(); + + echo tsprintf( + "%s\n", + pht('Locked the authentication provider configuration.')); + } +} diff --git a/src/applications/auth/management/PhabricatorAuthManagementUnlockWorkflow.php b/src/applications/auth/management/PhabricatorAuthManagementUnlockWorkflow.php new file mode 100644 index 0000000000..bcca83f65e --- /dev/null +++ b/src/applications/auth/management/PhabricatorAuthManagementUnlockWorkflow.php @@ -0,0 +1,33 @@ +setName('unlock') + ->setExamples('**unlock**') + ->setSynopsis( + pht( + 'Unlock the authentication provider config, to make it possible '. + 'to edit the config using the web UI. Make sure to do '. + '**bin/auth lock** when done editing the configuration.')); + } + + public function execute(PhutilArgumentParser $args) { + $console = PhutilConsole::getConsole(); + + $key = 'auth.lock-config'; + $config_entry = PhabricatorConfigEntry::loadConfigEntry($key); + $config_entry->setValue(false); + + // If the entry has been deleted, resurrect it. + $config_entry->setIsDeleted(0); + + $config_entry->save(); + + echo tsprintf( + "%s\n", + pht('Unlocked the authentication provider configuration.')); + } +} diff --git a/src/applications/config/check/PhabricatorAuthSetupCheck.php b/src/applications/config/check/PhabricatorAuthSetupCheck.php index 43fab77eb7..59bc17ecce 100644 --- a/src/applications/config/check/PhabricatorAuthSetupCheck.php +++ b/src/applications/config/check/PhabricatorAuthSetupCheck.php @@ -22,6 +22,7 @@ final class PhabricatorAuthSetupCheck extends PhabricatorSetupCheck { ->setViewer(PhabricatorUser::getOmnipotentUser()) ->execute(); + $did_warn = false; if (!$configs) { $message = pht( 'You have not configured any authentication providers yet. You '. @@ -35,6 +36,42 @@ final class PhabricatorAuthSetupCheck extends PhabricatorSetupCheck { ->setName(pht('No Authentication Providers Configured')) ->setMessage($message) ->addLink('/auth/', pht('Auth Application')); + + $did_warn = true; + } + + // This check is meant for new administrators, but we don't want to + // show both this warning and the "No Auth Providers" warning. Also, + // show this as a reminder to go back and do a `bin/auth lock` after + // they make their desired changes. + $is_locked = PhabricatorEnv::getEnvConfig('auth.lock-config'); + if (!$is_locked && !$did_warn) { + $message = pht( + 'Your authentication provider configuration is unlocked. Once you '. + 'finish setting up or modifying authentication, you should lock the '. + 'configuration to prevent unauthorized changes.'. + "\n\n". + 'Leaving your authentication provider configuration unlocked '. + 'increases the damage that a compromised administrator account can '. + 'do to your install, by, for example, changing the authentication '. + 'provider to a server they control and intercepting usernames and '. + 'passwords.'. + "\n\n". + 'To prevent this attack, you should configure your authentication '. + 'providers, and then lock the configuration by doing `%s` '. + 'from the command line. This will prevent changing the '. + 'authentication provider config without first doing `%s`.', + 'bin/auth lock', + 'bin/auth unlock'); + $this + ->newIssue('auth.config-unlocked') + ->setShortName(pht('Auth Config Unlocked')) + ->setName(pht('Authenticaton Provider Configuration Unlocked')) + ->setMessage($message) + ->addRelatedPhabricatorConfig('auth.lock-config') + ->addCommand( + hsprintf( + 'phabricator/ $ ./bin/auth lock')); } } } diff --git a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php index 1440714bf7..55e7223da5 100644 --- a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php +++ b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php @@ -73,6 +73,26 @@ final class PhabricatorAuthenticationConfigOptions ->addExample( "yourcompany.com\nmail.yourcompany.com", pht('Valid Setting')), + $this->newOption('auth.lock-config', 'bool', false) + ->setBoolOptions( + array( + pht('Auth provider config must be unlocked before editing'), + pht('Auth provider config can be edited without unlocking'), + )) + ->setSummary( + pht( + 'Require administrators to unlock the authentication provider '. + 'configuration from the CLI before it can be edited.')) + ->setDescription( + pht( + 'Normally, administrators configure authentication providers only '. + 'once, immediately after instance creation. To further secure '. + 'your instance, you can set this configuration option to `true`, '. + 'which will require an adminstrator with CLI access to run '. + '`bin/auth unlock` to make any later changes to authentication '. + "provider configuration.\n\nAfter changing the config, you should ". + 'run `bin/auth lock` again from the CLI.')) + ->setLocked(true), $this->newOption('account.editable', 'bool', true) ->setBoolOptions( array(