keymgr: Do not expect x25519 keys to be stored as ed25519 ssh keys.

Previously, the Arti key store would store x25519 secret keys as ed25519
OpenSSH keys, which it would convert to x25519 upon loading (using the
conversion function added in !1297 (merged)). This approach isn't good
enough though: most people will probably want to bring their existing
x25519 keys, and in order to store those in OpenSSH format, we'd need
convert them to ed25519, which is impossible (because the secret part of
an x25519 key contains a SHA512'd secret, whereas the corresponding,
"un-expanded", ed25519 secret key contains the secret itself rather than
the SHA).

Now that `ssh-key` has support for ssh keys with [custom algorithm
names], we can store x25519 in OpenSSH format directly. This commit
changes the storage format used by the keymgr for x25519 client auth
keys (from ed25519-ssh to our own custom key type with an algorithm name
of `"x25519@torproject.org"`).

Closes #936

[custom algorithm names]: https://github.com/RustCrypto/SSH/pull/136

crates/tor-keymgr/src/key_type/ssh.rs

@@ -9,7 +9,7 @@ use ssh_key::Algorithm;
 use crate::{ErasedKey, KeyType, KeystoreError, Result};
 
 use tor_error::{ErrorKind, HasKind};
-use tor_llcrypto::pk::{ed25519, keymanip};
+use tor_llcrypto::pk::{curve25519, ed25519};
 use zeroize::Zeroizing;
 
 use std::path::PathBuf;
@@ -170,20 +170,55 @@ impl KeyType {
     /// The caller is expected to downcast the value returned to a concrete type.
     pub(crate) fn parse_ssh_format_erased(&self, key: UnparsedOpenSshKey) -> Result<ErasedKey> {
         // TODO HSS: perhaps this needs to be a method on EncodableKey instead?
-        match self {
-            KeyType::Ed25519Keypair => {
-                read_ed25519_keypair(*self, key).map(|key| Box::new(key) as ErasedKey)
-            }
-            KeyType::X25519StaticSecret => {
-                // NOTE: The ssh-key crate doesn't support storing curve25519 keys in OpenSSH
-                // format. As a workaround, this type of key will be stored as ssh-ed25519 and
-                // converted back to x25519 upon retrieval.
-                let key = read_ed25519_keypair(KeyType::Ed25519Keypair, key)?;
 
-                Ok(Box::new(keymanip::convert_ed25519_to_curve25519_private(
-                    &key,
-                )))
+        let key_type = *self;
+        let sk = ssh_key::PrivateKey::from_openssh(&*key.inner).map_err(|e| {
+            SshKeyError::SshKeyParse {
+                // TODO: rust thinks this clone is necessary because key.path is also used below (but
+                // if we get to this point, we're going to return an error and never reach the other
+                // error handling branches where we use key.path).
+                path: key.path.clone(),
+                key_type,
+                err: e.into(),
             }
+        })?;
+
+        let wanted_key_algo = key_type.ssh_algorithm();
+
+        if SshKeyAlgorithm::from(sk.algorithm()) != wanted_key_algo {
+            return Err(SshKeyError::UnexpectedSshKeyType {
+                path: key.path,
+                wanted_key_algo,
+                found_key_algo: sk.algorithm().into(),
+            }
+            .boxed());
+        }
+
+        // Build the expected key type (i.e. convert ssh_key key types to the key types
+        // we're using internally).
+        match sk.key_data() {
+            KeypairData::Ed25519(key) => Ok(ed25519::Keypair::from_bytes(&key.to_bytes())
+                .map_err(|_| {
+                    tor_error::internal!("failed to build ed25519 key out of ed25519 OpenSSH key")
+                })
+                .map(Box::new)?),
+            KeypairData::Other(key)
+                if SshKeyAlgorithm::from(key.algorithm()) == SshKeyAlgorithm::X25519 =>
+            {
+                let key: [u8; 32] = key
+                    .private
+                    .as_ref()
+                    .try_into()
+                    .map_err(|_| tor_error::internal!("invalid x25519 private key"))?;
+
+                Ok(Box::new(curve25519::StaticSecret::from(key)))
+            }
+            _ => Err(SshKeyError::UnexpectedSshKeyType {
+                path: key.path,
+                wanted_key_algo,
+                found_key_algo: sk.algorithm().into(),
+            }
+            .boxed()),
         }
     }
 }

tests/shadow/shadow.data.template/hosts/articlient-onion-auth/keystore/client/default/yr4tcjsgag3l7ar4kt5j2gav22nfs5uaktys5yy2yuuisjreu7ty6aad.onion/KP_hsc_desc_enc.x25519_private

@@ -1,7 +1,7 @@
 -----BEGIN OPENSSH PRIVATE KEY-----
-b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
-QyNTUxOQAAACD4Tjq19rMz8G49hAAqsAP4MjQUwvU1mYPWxsmVLigDAgAAAJBopPdlaKT3
-ZQAAAAtzc2gtZWQyNTUxOQAAACD4Tjq19rMz8G49hAAqsAP4MjQUwvU1mYPWxsmVLigDAg
-AAAEAIsbCNVF+6EdbfJtuw5ehopIUpQtKCQzCDpoaTp9Hfq/hOOrX2szPwbj2EACqwA/gy
-NBTC9TWZg9bGyZUuKAMCAAAACHRlc3Qta2V5AQIDBAU=
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAPQAAABV4MjU1MT
+lAdG9ycHJvamVjdC5vcmcAAAAgct8Xu0pzmH8ODOtxWfiHcMf504eIwnsAgvD7C6/+7nUA
+AAB419he5tfYXuYAAAAVeDI1NTE5QHRvcnByb2plY3Qub3JnAAAAIHLfF7tKc5h/DgzrcV
+n4h3DH+dOHiMJ7AILw+wuv/u51AAAAIKCrmmcBaNqttEiJg5mzr3dlk/RQvGcuMGYctsjk
+CFRwAAAACHRlc3Qta2V5AQID
 -----END OPENSSH PRIVATE KEY-----

tests/shadow/shadow.data.template/hosts/fileserver-onion-auth/hs/authorized_clients/default.auth

@@ -1,2 +1,2 @@
-descriptor:x25519:IOKTNZA7YBQYJSTZR4V7NSPIPVAGWO3QGTRCJKOVKCMYFTXDQJIA
+descriptor:x25519:OLPRPO2KOOMH6DQM5NYVT6EHODD7TU4HRDBHWAEC6D5QXL765Z2Q