Correctly identify more SSH private key problems as "formatting" or "passphrase" related

Summary:
Ref T13454. Fixes T13006. When a user provide us with an SSH private key and (possibly) a passphrase:

# Try to verify that they're correct by extracting the public key.
# If that fails, try to figure out why it didn't work.

Our success in step (2) will vary depending on what the problem is, and we may end up falling through to a very generic error, but the outcome should generally be better than the old approach.

Previously, we had a very unsophisticated test for the text "ENCRYPTED" in the key body and questionable handling of the results: for example, providing a passphrase when a key did not require one did not raise an error.

Test Plan:
Created and edited credentials with:

- Valid, passphrase-free keys.
- Valid, passphrased keys with the right passphrase.
- Valid, passphrase-free keys with a passphrase ("surplus passphrase" error).
- Valid, passphrased keys with no passphrase ("missing passphrase" error).
- Valid, passphrased keys with an invalid passphrase ("invalid passphrase" error).
- Invalid keys ("format" error).

The precision of these errors will vary depending on how helpful "ssh-keygen" is.

Maniphest Tasks: T13454, T13006

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

src/__phutil_library_map__.php

···
     'PhabricatorAuthSSHKeyTransaction' => 'applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php',
     'PhabricatorAuthSSHKeyTransactionQuery' => 'applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php',
     'PhabricatorAuthSSHKeyViewController' => 'applications/auth/controller/PhabricatorAuthSSHKeyViewController.php',
+    'PhabricatorAuthSSHPrivateKey' => 'applications/auth/sshkey/PhabricatorAuthSSHPrivateKey.php',
+    'PhabricatorAuthSSHPrivateKeyException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyException.php',
+    'PhabricatorAuthSSHPrivateKeyFormatException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyFormatException.php',
+    'PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException.php',
+    'PhabricatorAuthSSHPrivateKeyMissingPassphraseException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyMissingPassphraseException.php',
+    'PhabricatorAuthSSHPrivateKeyPassphraseException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyPassphraseException.php',
+    'PhabricatorAuthSSHPrivateKeySurplusPassphraseException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeySurplusPassphraseException.php',
+    'PhabricatorAuthSSHPrivateKeyUnknownException' => 'applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyUnknownException.php',
     'PhabricatorAuthSSHPublicKey' => 'applications/auth/sshkey/PhabricatorAuthSSHPublicKey.php',
     'PhabricatorAuthSSHRevoker' => 'applications/auth/revoker/PhabricatorAuthSSHRevoker.php',
     'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php',
···
     'PhabricatorAuthSSHKeyTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorAuthSSHKeyTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorAuthSSHKeyViewController' => 'PhabricatorAuthSSHKeyController',
+    'PhabricatorAuthSSHPrivateKey' => 'Phobject',
+    'PhabricatorAuthSSHPrivateKeyException' => 'Exception',
+    'PhabricatorAuthSSHPrivateKeyFormatException' => 'PhabricatorAuthSSHPrivateKeyException',
+    'PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException' => 'PhabricatorAuthSSHPrivateKeyPassphraseException',
+    'PhabricatorAuthSSHPrivateKeyMissingPassphraseException' => 'PhabricatorAuthSSHPrivateKeyPassphraseException',
+    'PhabricatorAuthSSHPrivateKeyPassphraseException' => 'PhabricatorAuthSSHPrivateKeyException',
+    'PhabricatorAuthSSHPrivateKeySurplusPassphraseException' => 'PhabricatorAuthSSHPrivateKeyPassphraseException',
+    'PhabricatorAuthSSHPrivateKeyUnknownException' => 'PhabricatorAuthSSHPrivateKeyException',
     'PhabricatorAuthSSHPublicKey' => 'Phobject',
     'PhabricatorAuthSSHRevoker' => 'PhabricatorAuthRevoker',
     'PhabricatorAuthSession' => array(

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyException.php

···
+<?php
+
+abstract class PhabricatorAuthSSHPrivateKeyException
+  extends Exception {
+
+  abstract public function isFormatException();
+  abstract public function isPassphraseException();
+
+}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyFormatException.php

···
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyFormatException
+  extends PhabricatorAuthSSHPrivateKeyException {
+
+  public function isFormatException() {
+    return true;
+  }
+
+  public function isPassphraseException() {
+    return false;
+  }
+
+}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException.php

···
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException
+  extends PhabricatorAuthSSHPrivateKeyPassphraseException {}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyMissingPassphraseException.php

···
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyMissingPassphraseException
+  extends PhabricatorAuthSSHPrivateKeyPassphraseException {}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyPassphraseException.php

···
+<?php
+
+abstract class PhabricatorAuthSSHPrivateKeyPassphraseException
+  extends PhabricatorAuthSSHPrivateKeyException {
+
+  final public function isFormatException() {
+    return false;
+  }
+
+  final public function isPassphraseException() {
+    return true;
+  }
+
+}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeySurplusPassphraseException.php

···
+<?php
+
+final class PhabricatorAuthSSHPrivateKeySurplusPassphraseException
+  extends PhabricatorAuthSSHPrivateKeyPassphraseException {}

src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyUnknownException.php

···
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyUnknownException
+  extends PhabricatorAuthSSHPrivateKeyException {
+
+  public function isFormatException() {
+    return true;
+  }
+
+  public function isPassphraseException() {
+    return true;
+  }
+
+}

src/applications/auth/sshkey/PhabricatorAuthSSHPrivateKey.php

···
+<?php
+
+/**
+ * Data structure representing a raw private key.
+ */
+final class PhabricatorAuthSSHPrivateKey extends Phobject {
+
+  private $body;
+  private $passphrase;
+
+  private function __construct() {
+    // <internal>
+  }
+
+  public function setPassphrase(PhutilOpaqueEnvelope $passphrase) {
+    $this->passphrase = $passphrase;
+    return $this;
+  }
+
+  public function getPassphrase() {
+    return $this->passphrase;
+  }
+
+  public static function newFromRawKey(PhutilOpaqueEnvelope $entire_key) {
+    $key = new self();
+
+    $key->body = $entire_key;
+
+    return $key;
+  }
+
+  public function getKeyBody() {
+    return $this->body;
+  }
+
+  public function newBarePrivateKey() {
+    if (!Filesystem::binaryExists('ssh-keygen')) {
+      throw new Exception(
+        pht(
+          'Analyzing or decrypting SSH keys requires the "ssh-keygen" binary, '.
+          'but it is not available in "$PATH". Make it available to work with '.
+          'SSH private keys.'));
+    }
+
+    $old_body = $this->body;
+
+    // Some versions of "ssh-keygen" are sensitive to trailing whitespace for
+    // some keys. Trim any trailing whitespace and replace it with a single
+    // newline.
+    $raw_body = $old_body->openEnvelope();
+    $raw_body = rtrim($raw_body)."\n";
+    $old_body = new PhutilOpaqueEnvelope($raw_body);
+
+    $tmp = $this->newTemporaryPrivateKeyFile($old_body);
+
+    // See T13454 for discussion of why this is so awkward. In broad strokes,
+    // we don't have a straightforward way to distinguish between keys with an
+    // invalid format and keys with a passphrase which we don't know.
+
+    // First, try to extract the public key from the file using the (possibly
+    // empty) passphrase we were given. If everything is in good shape, this
+    // should work.
+
+    $passphrase = $this->getPassphrase();
+    if ($passphrase) {
+      list($err, $stdout, $stderr) = exec_manual(
+        'ssh-keygen -y -P %P -f %R',
+        $passphrase,
+        $tmp);
+    } else {
+      list($err, $stdout, $stderr) = exec_manual(
+        'ssh-keygen -y -P %s -f %R',
+        '',
+        $tmp);
+    }
+
+    // If that worked, the key is good and the (possibly empty) passphrase is
+    // correct. Strip the passphrase if we have one, then return the bare key.
+
+    if (!$err) {
+      if ($passphrase) {
+        execx(
+          'ssh-keygen -y -P %P -N %s -f %R',
+          $passphrase,
+          '',
+          $tmp);
+
+        $new_body = new PhutilOpaqueEnvelope(Filesystem::readFile($tmp));
+        unset($tmp);
+      } else {
+        $new_body = $old_body;
+      }
+
+      return self::newFromRawKey($new_body);
+    }
+
+    // We were not able to extract the public key. Try to figure out why. The
+    // reasons we expect are:
+    //
+    //   - We were given a passphrase, but the key has no passphrase.
+    //   - We were given a passphrase, but the passphrase is wrong.
+    //   - We were not given a passphrase, but the key has a passphrase.
+    //   - The key format is invalid.
+    //
+    // Our ability to separate these cases varies a lot, particularly because
+    // some versions of "ssh-keygen" return very similar diagnostic messages
+    // for any error condition. Try our best.
+
+    if ($passphrase) {
+      // First, test for "we were given a passphrase, but the key has no
+      // passphrase", since this is a conclusive test.
+      list($err) = exec_manual(
+        'ssh-keygen -y -P %s -f %R',
+        '',
+        $tmp);
+      if (!$err) {
+        throw new PhabricatorAuthSSHPrivateKeySurplusPassphraseException(
+          pht(
+            'A passphrase was provided for this private key, but it does '.
+            'not require a passphrase. Check that you supplied the correct '.
+            'key, or omit the passphrase.'));
+      }
+    }
+
+    // We're out of conclusive tests, so try to guess why the error occurred.
+    // In some versions of "ssh-keygen", we get a usable diagnostic message. In
+    // other versions, not so much.
+
+    $reason_format = 'format';
+    $reason_passphrase = 'passphrase';
+    $reason_unknown = 'unknown';
+
+    $patterns = array(
+      // macOS 10.14.6
+      '/incorrect passphrase supplied to decrypt private key/'
+        => $reason_passphrase,
+
+      // macOS 10.14.6
+      '/invalid format/' => $reason_format,
+
+      // Ubuntu 14
+      '/load failed/' => $reason_unknown,
+    );
+
+    $reason = 'unknown';
+    foreach ($patterns as $pattern => $pattern_reason) {
+      $ok = preg_match($pattern, $stderr);
+
+      if ($ok === false) {
+        throw new Exception(
+          pht(
+            'Pattern "%s" is not valid.',
+            $pattern));
+      }
+
+      if ($ok) {
+        $reason = $pattern_reason;
+        break;
+      }
+    }
+
+    if ($reason === $reason_format) {
+      throw new PhabricatorAuthSSHPrivateKeyFormatException(
+        pht(
+          'This private key is not formatted correctly. Check that you '.
+          'have provided the complete text of a valid private key.'));
+    }
+
+    if ($reason === $reason_passphrase) {
+      if ($passphrase) {
+        throw new PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException(
+          pht(
+            'This private key requires a passphrase, but the wrong '.
+            'passphrase was provided. Check that you supplied the correct '.
+            'key and passphrase.'));
+      } else {
+        throw new PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException(
+          pht(
+            'This private key requires a passphrase, but no passphrase was '.
+            'provided. Check that you supplied the correct key, or provide '.
+            'the passphrase.'));
+      }
+    }
+
+    if ($passphrase) {
+      throw new PhabricatorAuthSSHPrivateKeyUnknownException(
+        pht(
+          'This private key could not be opened with the provided passphrase. '.
+          'This might mean that the passphrase is wrong or that the key is '.
+          'not formatted correctly. Check that you have supplied the '.
+          'complete text of a valid private key and the correct passphrase.'));
+    } else {
+      throw new PhabricatorAuthSSHPrivateKeyUnknownException(
+        pht(
+          'This private key could not be opened. This might mean that the '.
+          'key requires a passphrase, or might mean that the key is not '.
+          'formatted correctly. Check that you have supplied the complete '.
+          'text of a valid private key and the correct passphrase.'));
+    }
+  }
+
+  private function newTemporaryPrivateKeyFile(PhutilOpaqueEnvelope $key_body) {
+    $tmp = new TempFile();
+
+    Filesystem::writeFile($tmp, $key_body->openEnvelope());
+
+    return $tmp;
+  }
+
+}

src/applications/passphrase/controller/PassphraseCredentialEditController.php

···
     $validation_exception = null;
     $errors = array();
     $e_password = null;
+    $e_secret = null;
     if ($request->isFormPost()) {
 
       $v_name = $request->getStr('name');
···
       $env_secret = new PhutilOpaqueEnvelope($v_secret);
       $env_password = new PhutilOpaqueEnvelope($v_password);
 
-      if ($type->requiresPassword($env_secret)) {
+      $has_secret = !preg_match('/^('.$bullet.')+$/', trim($v_decrypt));
+
+      // Validate and repair SSH private keys, and apply passwords if they
+      // are provided. See T13454 for discussion.
+
+      // This should eventually be refactored to be modular rather than a
+      // hard-coded set of behaviors here in the Controller, but this is
+      // likely a fairly extensive change.
+
+      $is_ssh = ($type instanceof PassphraseSSHPrivateKeyTextCredentialType);
+
+      if ($is_ssh && $has_secret) {
+        $old_object = PhabricatorAuthSSHPrivateKey::newFromRawKey($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();
+          $old_object->setPassphrase($env_password);
+        }
+
+        try {
+          $new_object = $old_object->newBarePrivateKey();
+          $v_decrypt = $new_object->getKeyBody()->openEnvelope();
+        } catch (PhabricatorAuthSSHPrivateKeyException $ex) {
+          $errors[] = $ex->getMessage();
+
+          if ($ex->isFormatException()) {
+            $e_secret = pht('Invalid');
           }
-        } else {
-          $e_password = pht('Required');
-          $errors[] = pht(
-            'This key requires a password. You must provide the password '.
-            'for the key.');
+          if ($ex->isPassphraseException()) {
+            $e_password = pht('Invalid');
+          }
         }
       }
 
···
             ->setTransactionType($type_username)
             ->setNewValue($v_username);
           }
+
           // If some value other than a sequence of bullets was provided for
           // the credential, update it. In particular, note that we are
           // explicitly allowing empty secrets: one use case is HTTP auth where
           // the username is a secret token which covers both identity and
           // authentication.
 
-          if (!preg_match('/^('.$bullet.')+$/', trim($v_decrypt))) {
+          if ($has_secret) {
             // If the credential was previously destroyed, restore it when it is
             // edited if a secret is provided.
             $xactions[] = id(new PassphraseCredentialTransaction())
···
             $new_secret = id(new PassphraseSecret())
               ->setSecretData($v_decrypt)
               ->save();
+
             $xactions[] = id(new PassphraseCredentialTransaction())
               ->setTransactionType($type_secret_id)
               ->setNewValue($new_secret->getID());
···
         ->setName('secret')
         ->setLabel($type->getSecretLabel())
         ->setDisabled($credential_is_locked)
-        ->setValue($v_secret));
+        ->setValue($v_secret)
+        ->setError($e_secret));
 
     if ($type->shouldShowPasswordField()) {
       $form->appendChild(

src/applications/passphrase/credentialtype/PassphraseCredentialType.php

···
     return pht('Password');
   }
 
-
-  /**
-   * Return true if the provided credential 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;
-  }
-
   public function shouldRequireUsername() {
     return true;
   }

src/applications/passphrase/credentialtype/PassphraseSSHPrivateKeyTextCredentialType.php

···
     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 `%s` binary, but it '.
-          'is not available in %s. Either make it available or strip the '.
-          'password from this SSH key manually before uploading it.',
-          'ssh-keygen',
-          '$PATH'));
-    }
-
-    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));
-    }
-  }
-
 }