From 17d965e8941397b8c362d88df8c90e6a2632bd52 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 11:51:34 +0100 Subject: [PATCH] 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 | 61 +++++++++++++++---- .../KP_hsc_desc_enc.x25519_private | 10 +-- .../hs/authorized_clients/default.auth | 2 +- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index b58493889..ac0d1b27c 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/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 { // 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()), } } } diff --git a/tests/shadow/shadow.data.template/hosts/articlient-onion-auth/keystore/client/default/yr4tcjsgag3l7ar4kt5j2gav22nfs5uaktys5yy2yuuisjreu7ty6aad.onion/KP_hsc_desc_enc.x25519_private b/tests/shadow/shadow.data.template/hosts/articlient-onion-auth/keystore/client/default/yr4tcjsgag3l7ar4kt5j2gav22nfs5uaktys5yy2yuuisjreu7ty6aad.onion/KP_hsc_desc_enc.x25519_private index 5c6846eb9..362199f6a 100644 --- a/tests/shadow/shadow.data.template/hosts/articlient-onion-auth/keystore/client/default/yr4tcjsgag3l7ar4kt5j2gav22nfs5uaktys5yy2yuuisjreu7ty6aad.onion/KP_hsc_desc_enc.x25519_private +++ b/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----- diff --git a/tests/shadow/shadow.data.template/hosts/fileserver-onion-auth/hs/authorized_clients/default.auth b/tests/shadow/shadow.data.template/hosts/fileserver-onion-auth/hs/authorized_clients/default.auth index e54034ec3..f59e6edb0 100644 --- a/tests/shadow/shadow.data.template/hosts/fileserver-onion-auth/hs/authorized_clients/default.auth +++ b/tests/shadow/shadow.data.template/hosts/fileserver-onion-auth/hs/authorized_clients/default.auth @@ -1,2 +1,2 @@ -descriptor:x25519:IOKTNZA7YBQYJSTZR4V7NSPIPVAGWO3QGTRCJKOVKCMYFTXDQJIA +descriptor:x25519:OLPRPO2KOOMH6DQM5NYVT6EHODD7TU4HRDBHWAEC6D5QXL765Z2Q