diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2439,6 +2439,14 @@ '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', @@ -8679,6 +8687,14 @@ '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( diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyException.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyException.php @@ -0,0 +1,9 @@ + + } + + 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; + } + +} diff --git a/src/applications/passphrase/controller/PassphraseCredentialEditController.php b/src/applications/passphrase/controller/PassphraseCredentialEditController.php --- a/src/applications/passphrase/controller/PassphraseCredentialEditController.php +++ b/src/applications/passphrase/controller/PassphraseCredentialEditController.php @@ -80,6 +80,7 @@ $validation_exception = null; $errors = array(); $e_password = null; + $e_secret = null; if ($request->isFormPost()) { $v_name = $request->getStr('name'); @@ -97,22 +98,36 @@ $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'); + } + if ($ex->isPassphraseException()) { + $e_password = pht('Invalid'); } - } else { - $e_password = pht('Required'); - $errors[] = pht( - 'This key requires a password. You must provide the password '. - 'for the key.'); } } @@ -166,13 +181,14 @@ ->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()) @@ -182,6 +198,7 @@ $new_secret = id(new PassphraseSecret()) ->setSecretData($v_decrypt) ->save(); + $xactions[] = id(new PassphraseCredentialTransaction()) ->setTransactionType($type_secret_id) ->setNewValue($new_secret->getID()); @@ -287,7 +304,8 @@ ->setName('secret') ->setLabel($type->getSecretLabel()) ->setDisabled($credential_is_locked) - ->setValue($v_secret)); + ->setValue($v_secret) + ->setError($e_secret)); if ($type->shouldShowPasswordField()) { $form->appendChild( diff --git a/src/applications/passphrase/credentialtype/PassphraseCredentialType.php b/src/applications/passphrase/credentialtype/PassphraseCredentialType.php --- a/src/applications/passphrase/credentialtype/PassphraseCredentialType.php +++ b/src/applications/passphrase/credentialtype/PassphraseCredentialType.php @@ -102,35 +102,6 @@ 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; } diff --git a/src/applications/passphrase/credentialtype/PassphraseSSHPrivateKeyTextCredentialType.php b/src/applications/passphrase/credentialtype/PassphraseSSHPrivateKeyTextCredentialType.php --- a/src/applications/passphrase/credentialtype/PassphraseSSHPrivateKeyTextCredentialType.php +++ b/src/applications/passphrase/credentialtype/PassphraseSSHPrivateKeyTextCredentialType.php @@ -29,40 +29,4 @@ 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)); - } - } - }