Detect and prompt for passwords on SSH private keys, then strip them

Summary: Fixes T4356. Currently, if users add a passworded private key to the Passphrase application, we never ask for the password and can not use it later. This makes several changes: - Prompt for the password. - Detect passworded private keys, and don't accept them until we can decrypt them. - Try to decrypt passworded private keys, and tell the user if the password is missing or incorrect. - Stop further creation of path-based private keys, which are really just for compatibility. We can't do anything reasonable about passwords with these, since users can change the files. Test Plan: Created a private key with a password, was prompted to provide it, tried empty/bad passwords, provided the correct password and had the key decrypted for use. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4356 Differential Revision: https://secure.phabricator.com/D8102
2024-11-22 23:02:42 +01:00 · 2014-01-30 11:43:06 -08:00 · 2014-01-30 11:43:06 -08:00 · eb397a48b4
commit eb397a48b4
parent 3bfa54819e
5 changed files with 249 additions and 71 deletions
--- a/src/applications/passphrase/controller/PassphraseCredentialCreateController.php
+++ b/src/applications/passphrase/controller/PassphraseCredentialCreateController.php
@ -6,7 +6,7 @@ final class PassphraseCredentialCreateController extends PassphraseController {
    $request = $this->getRequest();
    $viewer = $request->getUser();

-    $types = PassphraseCredentialType::getAllTypes();
+    $types = PassphraseCredentialType::getAllCreateableTypes();
    $types = mpull($types, null, 'getCredentialType');
    $types = msort($types, 'getCredentialTypeName');

--- a/src/applications/passphrase/controller/PassphraseCredentialEditController.php
+++ b/src/applications/passphrase/controller/PassphraseCredentialEditController.php
@ -32,6 +32,11 @@ final class PassphraseCredentialEditController extends PassphraseController {
        throw new Exception(pht('Credential has invalid type "%s"!', $type));
      }

+      if (!$type->isCreateable()) {
+        throw new Exception(
+          pht('Credential has noncreateable type "%s"!', $type));
+      }
+
      $is_new = false;
    } else {
      $type_const = $request->getStr('type');
@ -65,93 +70,123 @@ final class PassphraseCredentialEditController extends PassphraseController {
    $v_secret = $credential->getSecretID() ? str_repeat($bullet, 32) : null;

    $validation_exception = null;
+    $errors = array();
+    $e_password = null;
    if ($request->isFormPost()) {
+
      $v_name = $request->getStr('name');
      $v_desc = $request->getStr('description');
      $v_username = $request->getStr('username');
-      $v_secret = $request->getStr('secret');
      $v_view_policy = $request->getStr('viewPolicy');
      $v_edit_policy = $request->getStr('editPolicy');

-      $type_name = PassphraseCredentialTransaction::TYPE_NAME;
-      $type_desc = PassphraseCredentialTransaction::TYPE_DESCRIPTION;
-      $type_username = PassphraseCredentialTransaction::TYPE_USERNAME;
-      $type_destroy = PassphraseCredentialTransaction::TYPE_DESTROY;
-      $type_secret_id = PassphraseCredentialTransaction::TYPE_SECRET_ID;
-      $type_view_policy = PhabricatorTransactions::TYPE_VIEW_POLICY;
-      $type_edit_policy = PhabricatorTransactions::TYPE_EDIT_POLICY;
+      $v_secret = $request->getStr('secret');
+      $v_password = $request->getStr('password');
+      $v_decrypt = $v_secret;

-      $xactions = array();
+      $env_secret = new PhutilOpaqueEnvelope($v_secret);
+      $env_password = new PhutilOpaqueEnvelope($v_password);

-      $xactions[] = id(new PassphraseCredentialTransaction())
-        ->setTransactionType($type_name)
-        ->setNewValue($v_name);
-
-      $xactions[] = id(new PassphraseCredentialTransaction())
-        ->setTransactionType($type_desc)
-        ->setNewValue($v_desc);
-
-      $xactions[] = id(new PassphraseCredentialTransaction())
-        ->setTransactionType($type_username)
-        ->setNewValue($v_username);
-
-      $xactions[] = id(new PassphraseCredentialTransaction())
-        ->setTransactionType($type_view_policy)
-        ->setNewValue($v_view_policy);
-
-      $xactions[] = id(new PassphraseCredentialTransaction())
-        ->setTransactionType($type_edit_policy)
-        ->setNewValue($v_edit_policy);
-
-      // Open a transaction in case we're writing a new secret; this limits
-      // the amount of code which handles secret plaintexts.
-      $credential->openTransaction();
-
-      $min_secret = str_replace($bullet, '', trim($v_secret));
-      if (strlen($min_secret)) {
-        // If the credential was previously destroyed, restore it when it is
-        // edited if a secret is provided.
-        $xactions[] = id(new PassphraseCredentialTransaction())
-          ->setTransactionType($type_destroy)
-          ->setNewValue(0);
-
-        $new_secret = id(new PassphraseSecret())
-          ->setSecretData($v_secret)
-          ->save();
-        $xactions[] = id(new PassphraseCredentialTransaction())
-          ->setTransactionType($type_secret_id)
-          ->setNewValue($new_secret->getID());
+      if ($type->requiresPassword($env_secret)) {
+        if (strlen($v_password)) {
+          $v_decrypt = $type->decryptSecret($env_secret, $env_password);
+          if ($v_decrypt === null) {
+            $e_password = pht('Incorrect');
+            $errors[] = pht(
+              'This key requires a password, but the password you provided '.
+              'is incorrect.');
+          } else {
+            $v_decrypt = $v_decrypt->openEnvelope();
+          }
+        } else {
+          $e_password = pht('Required');
+          $errors[] = pht(
+            'This key requires a password. You must provide the password '.
+            'for the key.');
+        }
      }

-      try {
-        $editor = id(new PassphraseCredentialTransactionEditor())
-          ->setActor($viewer)
-          ->setContinueOnNoEffect(true)
-          ->setContentSourceFromRequest($request)
-          ->applyTransactions($credential, $xactions);
+      if (!$errors) {
+        $type_name = PassphraseCredentialTransaction::TYPE_NAME;
+        $type_desc = PassphraseCredentialTransaction::TYPE_DESCRIPTION;
+        $type_username = PassphraseCredentialTransaction::TYPE_USERNAME;
+        $type_destroy = PassphraseCredentialTransaction::TYPE_DESTROY;
+        $type_secret_id = PassphraseCredentialTransaction::TYPE_SECRET_ID;
+        $type_view_policy = PhabricatorTransactions::TYPE_VIEW_POLICY;
+        $type_edit_policy = PhabricatorTransactions::TYPE_EDIT_POLICY;

-        $credential->saveTransaction();
+        $xactions = array();

-        if ($request->isAjax()) {
-          return id(new AphrontAjaxResponse())->setContent(
-            array(
-              'phid' => $credential->getPHID(),
-              'name' => 'K'.$credential->getID().' '.$credential->getName(),
-            ));
-        } else {
-          return id(new AphrontRedirectResponse())
-            ->setURI('/K'.$credential->getID());
+        $xactions[] = id(new PassphraseCredentialTransaction())
+          ->setTransactionType($type_name)
+          ->setNewValue($v_name);
+
+        $xactions[] = id(new PassphraseCredentialTransaction())
+          ->setTransactionType($type_desc)
+          ->setNewValue($v_desc);
+
+        $xactions[] = id(new PassphraseCredentialTransaction())
+          ->setTransactionType($type_username)
+          ->setNewValue($v_username);
+
+        $xactions[] = id(new PassphraseCredentialTransaction())
+          ->setTransactionType($type_view_policy)
+          ->setNewValue($v_view_policy);
+
+        $xactions[] = id(new PassphraseCredentialTransaction())
+          ->setTransactionType($type_edit_policy)
+          ->setNewValue($v_edit_policy);
+
+        // Open a transaction in case we're writing a new secret; this limits
+        // the amount of code which handles secret plaintexts.
+        $credential->openTransaction();
+
+        $min_secret = str_replace($bullet, '', trim($v_decrypt));
+        if (strlen($min_secret)) {
+          // If the credential was previously destroyed, restore it when it is
+          // edited if a secret is provided.
+          $xactions[] = id(new PassphraseCredentialTransaction())
+            ->setTransactionType($type_destroy)
+            ->setNewValue(0);
+
+          $new_secret = id(new PassphraseSecret())
+            ->setSecretData($v_decrypt)
+            ->save();
+          $xactions[] = id(new PassphraseCredentialTransaction())
+            ->setTransactionType($type_secret_id)
+            ->setNewValue($new_secret->getID());
        }
-      } catch (PhabricatorApplicationTransactionValidationException $ex) {
-        $credential->killTransaction();

-        $validation_exception = $ex;
+        try {
+          $editor = id(new PassphraseCredentialTransactionEditor())
+            ->setActor($viewer)
+            ->setContinueOnNoEffect(true)
+            ->setContentSourceFromRequest($request)
+            ->applyTransactions($credential, $xactions);

-        $e_name = $ex->getShortMessage($type_name);
-        $e_username = $ex->getShortMessage($type_username);
+          $credential->saveTransaction();

-        $credential->setViewPolicy($v_view_policy);
-        $credential->setEditPolicy($v_edit_policy);
+          if ($request->isAjax()) {
+            return id(new AphrontAjaxResponse())->setContent(
+              array(
+                'phid' => $credential->getPHID(),
+                'name' => 'K'.$credential->getID().' '.$credential->getName(),
+              ));
+          } else {
+            return id(new AphrontRedirectResponse())
+              ->setURI('/K'.$credential->getID());
+          }
+        } catch (PhabricatorApplicationTransactionValidationException $ex) {
+          $credential->killTransaction();
+
+          $validation_exception = $ex;
+
+          $e_name = $ex->getShortMessage($type_name);
+          $e_username = $ex->getShortMessage($type_username);
+
+          $credential->setViewPolicy($v_view_policy);
+          $credential->setEditPolicy($v_edit_policy);
+        }
      }
    }

@ -214,6 +249,14 @@ final class PassphraseCredentialEditController extends PassphraseController {
          ->setLabel($type->getSecretLabel())
          ->setValue($v_secret));

+    if ($type->shouldShowPasswordField()) {
+      $form->appendChild(
+        id(new AphrontFormPasswordControl())
+          ->setName('password')
+          ->setLabel($type->getPasswordLabel())
+          ->setError($e_password));
+    }
+
    $crumbs = $this->buildApplicationCrumbs();

    if ($is_new) {
@ -230,10 +273,13 @@ final class PassphraseCredentialEditController extends PassphraseController {
    }

    if ($request->isAjax()) {
+      $errors = id(new AphrontErrorView())->setErrors($errors);
+
      $dialog = id(new AphrontDialogView())
        ->setUser($viewer)
        ->setWidth(AphrontDialogView::WIDTH_FORM)
        ->setTitle($title)
+        ->appendChild($errors)
        ->appendChild($form)
        ->addSubmitButton(pht('Create Credential'))
        ->addCancelButton($this->getApplicationURI());
@ -248,6 +294,7 @@ final class PassphraseCredentialEditController extends PassphraseController {

    $box = id(new PHUIObjectBoxView())
      ->setHeaderText($header)
+      ->setFormErrors($errors)
      ->setValidationException($validation_exception)
      ->setForm($form);

--- a/src/applications/passphrase/credentialtype/PassphraseCredentialType.php
+++ b/src/applications/passphrase/credentialtype/PassphraseCredentialType.php
@ -1,5 +1,8 @@
 <?php

+/**
+ * @task password   Managing Encryption Passwords
+ */
 abstract class PassphraseCredentialType extends Phobject {

  abstract public function getCredentialType();
@ -19,10 +22,88 @@ abstract class PassphraseCredentialType extends Phobject {
    return $types;
  }

+  public static function getAllCreateableTypes() {
+    $types = self::getAllTypes();
+    foreach ($types as $key => $type) {
+      if (!$type->isCreateable()) {
+        unset($types[$key]);
+      }
+    }
+
+    return $types;
+  }
+
  public static function getTypeByConstant($constant) {
    $all = self::getAllTypes();
    $all = mpull($all, null, 'getCredentialType');
    return idx($all, $constant);
  }

+
+  /**
+   * Can users create new credentials of this type?
+   *
+   * @return bool True if new credentials of this type can be created.
+   */
+  public function isCreateable() {
+    return true;
+  }
+
+
+/* -(  Passwords  )---------------------------------------------------------- */
+
+
+  /**
+   * Return true to show an additional "Password" field. This is used by
+   * SSH credentials to strip passwords off private keys.
+   *
+   * @return bool True if a password field should be shown to the user.
+   *
+   * @task password
+   */
+  public function shouldShowPasswordField() {
+    return false;
+  }
+
+
+  /**
+   * Return the label for the password field, if one is shown.
+   *
+   * @return string   Human-readable field label.
+   *
+   * @task password
+   */
+  public function getPasswordLabel() {
+    return pht('Password');
+  }
+
+
+  /**
+   * Return true if the provided credental requires a password to decrypt.
+   *
+   * @param PhutilOpaqueEnvelope  Credential secret value.
+   * @return bool                 True if the credential needs a password.
+   *
+   * @task password
+   */
+  public function requiresPassword(PhutilOpaqueEnvelope $secret) {
+    return false;
+  }
+
+
+  /**
+   * Return the decrypted credential secret, or `null` if the password does
+   * not decrypt the credential.
+   *
+   * @param PhutilOpaqueEnvelope  Credential secret value.
+   * @param PhutilOpaqueEnvelope  Credential password.
+   * @return
+   * @task password
+   */
+  public function decryptSecret(
+    PhutilOpaqueEnvelope $secret,
+    PhutilOpaqueEnvelope $password) {
+    return $secret;
+  }
+
 }
--- a/src/applications/passphrase/credentialtype/PassphraseCredentialTypeSSHPrivateKeyFile.php
+++ b/src/applications/passphrase/credentialtype/PassphraseCredentialTypeSSHPrivateKeyFile.php
@ -25,4 +25,12 @@ final class PassphraseCredentialTypeSSHPrivateKeyFile
    return new AphrontFormTextControl();
  }

+  public function isCreateable() {
+    // This credential type exists to support historic repository configuration.
+    // We don't support creating new credentials with this type, since it does
+    // not scale and managing passwords is much more difficult than if we have
+    // the key text.
+    return false;
+  }
+
 }
--- a/src/applications/passphrase/credentialtype/PassphraseCredentialTypeSSHPrivateKeyText.php
+++ b/src/applications/passphrase/credentialtype/PassphraseCredentialTypeSSHPrivateKeyText.php
@ -21,4 +21,46 @@ final class PassphraseCredentialTypeSSHPrivateKeyText
    return pht('Private Key');
  }

+  public function shouldShowPasswordField() {
+    return true;
+  }
+
+  public function getPasswordLabel() {
+    return pht('Password for Key');
+  }
+
+  public function requiresPassword(PhutilOpaqueEnvelope $secret) {
+    // According to the internet, this is the canonical test for an SSH private
+    // key with a password.
+    return preg_match('/ENCRYPTED/', $secret->openEnvelope());
+  }
+
+  public function decryptSecret(
+    PhutilOpaqueEnvelope $secret,
+    PhutilOpaqueEnvelope $password) {
+
+    $tmp = new TempFile();
+    Filesystem::writeFile($tmp, $secret->openEnvelope());
+
+    if (!Filesystem::binaryExists('ssh-keygen')) {
+      throw new Exception(
+        pht(
+          'Decrypting SSH keys requires the `ssh-keygen` binary, but it '.
+          'is not available in PATH. Either make it available or strip the '.
+          'password fromt his SSH key manually before uploading it.'));
+    }
+
+    list($err, $stdout, $stderr) = exec_manual(
+      'ssh-keygen -p -P %P -N %s -f %s',
+      $password,
+      '',
+      (string)$tmp);
+
+    if ($err) {
+      return null;
+    } else {
+      return new PhutilOpaqueEnvelope(Filesystem::readFile($tmp));
+    }
+  }
+
 }