Page MenuHomePhabricator
Files F15283621

D20905.diff
No OneTemporary
Actions

D20905.diff
View Options

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 @@
+<?php
+
+abstract class PhabricatorAuthSSHPrivateKeyException
+ extends Exception {
+
+ abstract public function isFormatException();
+ abstract public function isPassphraseException();
+
+}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyFormatException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyFormatException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyFormatException.php
@@ -0,0 +1,14 @@
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyFormatException
+ extends PhabricatorAuthSSHPrivateKeyException {
+
+ public function isFormatException() {
+ return true;
+ }
+
+ public function isPassphraseException() {
+ return false;
+ }
+
+}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException.php
@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyIncorrectPassphraseException
+ extends PhabricatorAuthSSHPrivateKeyPassphraseException {}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyMissingPassphraseException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyMissingPassphraseException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyMissingPassphraseException.php
@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyMissingPassphraseException
+ extends PhabricatorAuthSSHPrivateKeyPassphraseException {}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyPassphraseException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyPassphraseException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyPassphraseException.php
@@ -0,0 +1,14 @@
+<?php
+
+abstract class PhabricatorAuthSSHPrivateKeyPassphraseException
+ extends PhabricatorAuthSSHPrivateKeyException {
+
+ final public function isFormatException() {
+ return false;
+ }
+
+ final public function isPassphraseException() {
+ return true;
+ }
+
+}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeySurplusPassphraseException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeySurplusPassphraseException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeySurplusPassphraseException.php
@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorAuthSSHPrivateKeySurplusPassphraseException
+ extends PhabricatorAuthSSHPrivateKeyPassphraseException {}
diff --git a/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyUnknownException.php b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyUnknownException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/exception/privatekey/PhabricatorAuthSSHPrivateKeyUnknownException.php
@@ -0,0 +1,14 @@
+<?php
+
+final class PhabricatorAuthSSHPrivateKeyUnknownException
+ extends PhabricatorAuthSSHPrivateKeyException {
+
+ public function isFormatException() {
+ return true;
+ }
+
+ public function isPassphraseException() {
+ return true;
+ }
+
+}
diff --git a/src/applications/auth/sshkey/PhabricatorAuthSSHPrivateKey.php b/src/applications/auth/sshkey/PhabricatorAuthSSHPrivateKey.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/sshkey/PhabricatorAuthSSHPrivateKey.php
@@ -0,0 +1,210 @@
+<?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;
+ }
+
+}
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));
- }
- }
-
}

File Metadata

Mime Type
text/plain
Expires
Wed, Mar 5, 8:03 AM (1 w, 22 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7224079
Default Alt Text
D20905.diff (20 KB)

Event Timeline

Phabricator · Built by Phacility