From b2bcbaa708532866c08aa8aba8d979ad9f3c77fd Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 11:34:21 +0100 Subject: [PATCH 01/10] keymgr: Bump ssh-key to 0.6.0. This brings in the changes from #936. --- Cargo.lock | 194 ++++++++++---------------- crates/tor-keymgr/Cargo.toml | 2 +- crates/tor-keymgr/src/key_type/ssh.rs | 11 +- 3 files changed, 82 insertions(+), 125 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d72665936..241a9e3b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -627,9 +627,9 @@ dependencies = [ [[package]] name = "base16ct" -version = "0.1.1" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "349a06037c7bf932dd7e7d1f653678b2038b9ad46a74102f1fc7bd7872678cce" +checksum = "4c7f02d4ea65f2c1853089ffd8d2787bdbc63de2f0d29dedbcf8ccdfa0ccd4cf" [[package]] name = "base64" @@ -935,9 +935,9 @@ checksum = "7a81dae078cea95a014a339291cec439d2f232ebe854a9d672b796c6afafa9b7" [[package]] name = "crypto-bigint" -version = "0.4.9" +version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef2b4b23cddf68b89b8f8069890e8c270d54e2d5fe1b143820234805e4cb17ef" +checksum = "cf4c2f4e1afd912bc40bfd6fed5d9dc1f288e0ba01bfcc835cc5bc3eb13efe15" dependencies = [ "generic-array", "rand_core 0.6.4", @@ -1076,17 +1076,6 @@ version = "2.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2e66c9d817f1720209181c316d28635c050fa304f9c79e47a520882661b7308" -[[package]] -name = "der" -version = "0.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f1a467a65c5e759bce6e65eaf91cc29f466cdc57cb65777bd646872a8a1fd4de" -dependencies = [ - "const-oid", - "pem-rfc7468 0.6.0", - "zeroize", -] - [[package]] name = "der" version = "0.7.7" @@ -1094,7 +1083,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0c7ed52955ce76b1554f509074bb357d3fb8ac9b51288a65a3fd480d1dfba946" dependencies = [ "const-oid", - "pem-rfc7468 0.7.0", + "pem-rfc7468", "zeroize", ] @@ -1292,14 +1281,16 @@ dependencies = [ [[package]] name = "ecdsa" -version = "0.14.8" +version = "0.16.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "413301934810f597c1d19ca71c8710e99a3f1ba28a0d2ebc01551a2daeea3c5c" +checksum = "a4b1e0c257a9e9f25f90ff76d7a68360ed497ee519c8e428d1825ef0000799d4" dependencies = [ - "der 0.6.1", + "der", + "digest 0.10.7", "elliptic-curve", "rfc6979", - "signature 1.6.4", + "signature 2.1.0", + "spki", ] [[package]] @@ -1346,17 +1337,17 @@ checksum = "a26ae43d7bcc3b814de94796a5e736d4029efb0ee900c12e2d54c993ad1a1e07" [[package]] name = "elliptic-curve" -version = "0.12.3" +version = "0.13.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e7bb888ab5300a19b8e5bceef25ac745ad065f3c9f7efc6de1b91958110891d3" +checksum = "968405c8fdc9b3bf4df0a6638858cc0b52462836ab6b1c87377785dd09cf1c0b" dependencies = [ "base16ct", "crypto-bigint", - "der 0.6.1", "digest 0.10.7", "ff", "generic-array", "group", + "pkcs8", "rand_core 0.6.4", "sec1", "subtle", @@ -1485,9 +1476,9 @@ checksum = "6999dc1837253364c2ebb0704ba97994bd874e8f195d665c50b7548f6ea92764" [[package]] name = "ff" -version = "0.12.1" +version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d013fc25338cc558c5c2cfbad646908fb23591e2404481826742b651c9af7160" +checksum = "ded41244b729663b1e574f1b4fb731469f69f79c17667b5d776b16cda0479449" dependencies = [ "rand_core 0.6.4", "subtle", @@ -1742,6 +1733,7 @@ checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" dependencies = [ "typenum", "version_check", + "zeroize", ] [[package]] @@ -1788,9 +1780,9 @@ dependencies = [ [[package]] name = "group" -version = "0.12.1" +version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5dfbfb3a6cfbd390d5c9564ab283a0349b9b9fcd46a706c1eb10e0db70bfbac7" +checksum = "f0f9ef7462f7c099f518d754361858f86d8a07af53ba9af0fe635bbccb151a63" dependencies = [ "ff", "rand_core 0.6.4", @@ -2639,23 +2631,25 @@ checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" [[package]] name = "p256" -version = "0.11.1" +version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "51f44edd08f51e2ade572f141051021c5af22677e42b7dd28a88155151c33594" +checksum = "c9863ad85fa8f4460f9c48cb909d38a0d689dba1f6f6988a5e3e0d31071bcd4b" dependencies = [ "ecdsa", "elliptic-curve", + "primeorder", "sha2 0.10.7", ] [[package]] name = "p384" -version = "0.11.2" +version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dfc8c5bf642dde52bb9e87c0ecd8ca5a76faac2eeed98dedb7c717997e1080aa" +checksum = "70786f51bcc69f6a4c0360e063a4cac5419ef7c5cd5b3c99ad70f3be5ba79209" dependencies = [ "ecdsa", "elliptic-curve", + "primeorder", "sha2 0.10.7", ] @@ -2711,15 +2705,6 @@ dependencies = [ "regex", ] -[[package]] -name = "pem-rfc7468" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "24d159833a9105500e0398934e205e0773f0b27529557134ecfc51c27646adac" -dependencies = [ - "base64ct", -] - [[package]] name = "pem-rfc7468" version = "0.7.0" @@ -2815,37 +2800,15 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" -[[package]] -name = "pkcs1" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eff33bdbdfc54cc98a2eca766ebdec3e1b8fb7387523d5c9c9a2891da856f719" -dependencies = [ - "der 0.6.1", - "pkcs8 0.9.0", - "spki 0.6.0", - "zeroize", -] - [[package]] name = "pkcs1" version = "0.7.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c8ffb9f10fa047879315e6625af03c164b16962a5368d724ed16323b68ace47f" dependencies = [ - "der 0.7.7", - "pkcs8 0.10.2", - "spki 0.7.2", -] - -[[package]] -name = "pkcs8" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9eca2c590a5f85da82668fa685c09ce2888b9430e83299debf1f34b65fd4a4ba" -dependencies = [ - "der 0.6.1", - "spki 0.6.0", + "der", + "pkcs8", + "spki", ] [[package]] @@ -2854,8 +2817,8 @@ version = "0.10.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f950b2377845cebe5cf8b5165cb3cc1a5e0fa5cfa3e1f7f55707d8fd82e0a7b7" dependencies = [ - "der 0.7.7", - "spki 0.7.2", + "der", + "spki", ] [[package]] @@ -2901,6 +2864,15 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5b40af805b3121feab8a3c29f04d8ad262fa8e0561883e7653e024ae4479e6de" +[[package]] +name = "primeorder" +version = "0.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c2fcef82c0ec6eefcc179b978446c399b3cdf73c392c35604e399eee6df1ee3" +dependencies = [ + "elliptic-curve", +] + [[package]] name = "proc-macro-crate" version = "1.3.1" @@ -3154,13 +3126,12 @@ dependencies = [ [[package]] name = "rfc6979" -version = "0.3.1" +version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7743f17af12fa0b03b803ba12cd6a8d9483a587e89c69445e3909655c0b9fabb" +checksum = "f8dd2a808d456c4a54e300a23e9f5a67e122c3024119acbfd73e3bf664491cb2" dependencies = [ - "crypto-bigint", "hmac", - "zeroize", + "subtle", ] [[package]] @@ -3209,27 +3180,6 @@ dependencies = [ "serde", ] -[[package]] -name = "rsa" -version = "0.7.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "094052d5470cbcef561cb848a7209968c9f12dfa6d668f4bca048ac5de51099c" -dependencies = [ - "byteorder", - "digest 0.10.7", - "num-bigint-dig", - "num-integer", - "num-iter", - "num-traits", - "pkcs1 0.4.1", - "pkcs8 0.9.0", - "rand_core 0.6.4", - "signature 1.6.4", - "smallvec", - "subtle", - "zeroize", -] - [[package]] name = "rsa" version = "0.9.2" @@ -3243,11 +3193,12 @@ dependencies = [ "num-integer", "num-iter", "num-traits", - "pkcs1 0.7.5", - "pkcs8 0.10.2", + "pkcs1", + "pkcs8", "rand_core 0.6.4", + "sha2 0.10.7", "signature 2.1.0", - "spki 0.7.2", + "spki", "subtle", "zeroize", ] @@ -3414,14 +3365,14 @@ dependencies = [ [[package]] name = "sec1" -version = "0.3.0" +version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3be24c1842290c45df0a7bf069e0c268a747ad05a192f2fd7dcfdbc1cba40928" +checksum = "d3e97a565f76233a6003f9f5c54be1d9c5bdfa3eccfb189469f11ec4901c47dc" dependencies = [ "base16ct", - "der 0.6.1", + "der", "generic-array", - "pkcs8 0.9.0", + "pkcs8", "subtle", "zeroize", ] @@ -3700,10 +3651,6 @@ name = "signature" version = "1.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "74233d3b3b2f6d4b006dc19dee745e73e2a6bfb6f93607cd3b02bd5b00797d7c" -dependencies = [ - "digest 0.10.7", - "rand_core 0.6.4", -] [[package]] name = "signature" @@ -3786,16 +3733,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" -[[package]] -name = "spki" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "67cf02bbac7a337dc36e4f5a693db6c21e7863f45070f7064577eb4367a3212b" -dependencies = [ - "base64ct", - "der 0.6.1", -] - [[package]] name = "spki" version = "0.7.2" @@ -3803,35 +3740,46 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9d1e996ef02c474957d681f1b05213dfb0abab947b446a62d37770b23500184a" dependencies = [ "base64ct", - "der 0.7.7", + "der", +] + +[[package]] +name = "ssh-cipher" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "caac132742f0d33c3af65bfcde7f6aa8f62f0e991d80db99149eb9d44708784f" +dependencies = [ + "cipher", + "ssh-encoding", ] [[package]] name = "ssh-encoding" -version = "0.1.0" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "19cfdc32e0199062113edf41f344fbf784b8205a94600233c84eb838f45191e1" +checksum = "eb9242b9ef4108a78e8cd1a2c98e193ef372437f8c22be363075233321dd4a15" dependencies = [ "base64ct", - "pem-rfc7468 0.6.0", + "pem-rfc7468", "sha2 0.10.7", ] [[package]] name = "ssh-key" -version = "0.5.1" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "288d8f5562af5a3be4bda308dd374b2c807b940ac370b5efa1c99311da91d9a1" +checksum = "4b0a17fec6ea344bfa1cda3aed2f0696fddc6295cfcc8c454a3bf58b8ffaabeb" dependencies = [ - "ed25519-dalek", "p256", "p384", "rand_core 0.6.4", - "rsa 0.7.2", + "rsa", "sec1", "sha2 0.10.7", - "signature 1.6.4", + "signature 2.1.0", + "ssh-cipher", "ssh-encoding", + "subtle", "zeroize", ] @@ -4765,7 +4713,7 @@ dependencies = [ "rand 0.8.5", "rand_core 0.5.1", "rand_core 0.6.4", - "rsa 0.9.2", + "rsa", "safelog", "serde", "serde_test", diff --git a/crates/tor-keymgr/Cargo.toml b/crates/tor-keymgr/Cargo.toml index 9404fa64b..0fd3959b4 100644 --- a/crates/tor-keymgr/Cargo.toml +++ b/crates/tor-keymgr/Cargo.toml @@ -32,7 +32,7 @@ fs-mistrust = { path = "../fs-mistrust", version = "0.7.3", features = ["serde", itertools = "0.11.0" rand = "0.8" serde = { version = "1.0.103", features = ["derive"] } -ssh-key = { version = "0.5.1", features = ["std"] } +ssh-key = { version = "0.6.0", features = ["std"] } thiserror = "1" tor-config = { path = "../tor-config", version = "0.9.3" } tor-error = { path = "../tor-error", version = "0.5.3" } diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index ddff9c211..b58493889 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -15,6 +15,12 @@ use zeroize::Zeroizing; use std::path::PathBuf; use std::sync::Arc; +/// The algorithm string for x25519 SSH keys. +// +// TODO HSS: start a protocol name registry in the torspec repo and document the usage and purpose +// of this "protocol" name. +pub(crate) const X25519_ALGORITHM_NAME: &str = "x25519@torproject.org"; + /// An unparsed OpenSSH key. /// /// Note: This is a wrapper around the contents of a file we think is an OpenSSH key. The inner @@ -43,7 +49,7 @@ impl UnparsedOpenSshKey { /// SSH key algorithms. // // Note: this contains all the types supported by ssh_key, plus X25519. -#[derive(Copy, Clone, Debug, PartialEq, derive_more::Display)] +#[derive(Clone, Debug, PartialEq, derive_more::Display)] pub(crate) enum SshKeyAlgorithm { /// Digital Signature Algorithm Dsa, @@ -72,6 +78,9 @@ impl From for SshKeyAlgorithm { Algorithm::Rsa { .. } => SshKeyAlgorithm::Rsa, Algorithm::SkEcdsaSha2NistP256 => SshKeyAlgorithm::SkEcdsaSha2NistP256, Algorithm::SkEd25519 => SshKeyAlgorithm::SkEd25519, + Algorithm::Other(name) if name.as_str() == X25519_ALGORITHM_NAME => { + SshKeyAlgorithm::X25519 + } // Note: ssh_key::Algorithm is non_exhaustive, so we need this catch-all variant _ => SshKeyAlgorithm::Unknown(algo), } From 17d965e8941397b8c362d88df8c90e6a2632bd52 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 11:51:34 +0100 Subject: [PATCH 02/10] 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 From fade75ae16e1b5350b2f437379c53b46f179e131 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 15 Aug 2023 02:11:16 +0100 Subject: [PATCH 03/10] tor-keymgr: Test x25519 key parsing. --- crates/tor-keymgr/src/key_type/ssh.rs | 30 +++++++++++++++++++ .../testdata/x25519_openssh.private | 7 +++++ .../x25519_openssh_unknown_algorithm.private | 7 +++++ 3 files changed, 44 insertions(+) create mode 100644 crates/tor-keymgr/testdata/x25519_openssh.private create mode 100644 crates/tor-keymgr/testdata/x25519_openssh_unknown_algorithm.private diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index ac0d1b27c..f80b9b868 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -241,6 +241,9 @@ mod tests { const OPENSSH_ED25519: &[u8] = include_bytes!("../../testdata/ed25519_openssh.private"); const OPENSSH_ED25519_BAD: &[u8] = include_bytes!("../../testdata/ed25519_openssh_bad.private"); const OPENSSH_DSA: &[u8] = include_bytes!("../../testdata/dsa_openssh.private"); + const OPENSSH_X25519: &[u8] = include_bytes!("../../testdata/x25519_openssh.private"); + const OPENSSH_X25519_UNKNOWN_ALGORITHM: &[u8] = + include_bytes!("../../testdata/x25519_openssh_unknown_algorithm.private"); #[test] fn wrong_key_type() { @@ -284,4 +287,31 @@ mod tests { assert!(erased_key.downcast::().is_ok()); } + + #[test] + fn x25519_key() { + let key_type = KeyType::X25519StaticSecret; + let key = UnparsedOpenSshKey::new(OPENSSH_X25519.into(), PathBuf::from("/dummy/path")); + let erased_key = key_type.parse_ssh_format_erased(key).unwrap(); + + assert!(erased_key.downcast::().is_ok()); + } + + #[test] + fn invalid_x25519_key() { + let key_type = KeyType::X25519StaticSecret; + let key = UnparsedOpenSshKey::new( + OPENSSH_X25519_UNKNOWN_ALGORITHM.into(), + PathBuf::from("/dummy/path"), + ); + let err = key_type + .parse_ssh_format_erased(key) + .map(|_| "") + .unwrap_err(); + + assert_eq!( + err.to_string(), + "Unexpected OpenSSH key type: wanted X25519, found pangolin@torproject.org" + ); + } } diff --git a/crates/tor-keymgr/testdata/x25519_openssh.private b/crates/tor-keymgr/testdata/x25519_openssh.private new file mode 100644 index 000000000..33552585f --- /dev/null +++ b/crates/tor-keymgr/testdata/x25519_openssh.private @@ -0,0 +1,7 @@ +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAPQAAABV4MjU1MT +lAdG9ycHJvamVjdC5vcmcAAAAgD3MDTeSLGUGD1Re2s6aF6g73KSyiteXjV1SL+pu6ASUA +AAB4wA/uXMAP7lwAAAAVeDI1NTE5QHRvcnByb2plY3Qub3JnAAAAIA9zA03kixlBg9UXtr +OmheoO9yksorXl41dUi/qbugElAAAAIDCADBQ5fbboMIMbmOgHMfHZEaIcdm0TF2Kt8bHc +pz5zAAAACHRlc3Qta2V5AQID +-----END OPENSSH PRIVATE KEY----- diff --git a/crates/tor-keymgr/testdata/x25519_openssh_unknown_algorithm.private b/crates/tor-keymgr/testdata/x25519_openssh_unknown_algorithm.private new file mode 100644 index 000000000..a048a9b70 --- /dev/null +++ b/crates/tor-keymgr/testdata/x25519_openssh_unknown_algorithm.private @@ -0,0 +1,7 @@ +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAPwAAABdwYW5nb2 +xpbkB0b3Jwcm9qZWN0Lm9yZwAAACBFTBevfAXZy6Ut6i9PSHeXHUpaJ6+NKabGzeoT6rFV +DgAAAHjiJEYk4iRGJAAAABdwYW5nb2xpbkB0b3Jwcm9qZWN0Lm9yZwAAACBFTBevfAXZy6 +Ut6i9PSHeXHUpaJ6+NKabGzeoT6rFVDgAAACBo1W5vk9S8KAG7icAnKfq0NasfGTLO2+MJ +ifSnMVvXSgAAAAh0ZXN0LWtleQE= +-----END OPENSSH PRIVATE KEY----- From 9d8c28c639bd9fad8e7e65cc25530970a931de8f Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 14 Aug 2023 17:20:09 +0100 Subject: [PATCH 04/10] keymgr: Remove unused helper. This helper is no longer needed (the logic from `parse_ssh_format_erased` changed). --- crates/tor-keymgr/src/key_type/ssh.rs | 33 --------------------------- 1 file changed, 33 deletions(-) diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index f80b9b868..b1b414905 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -122,39 +122,6 @@ impl HasKind for SshKeyError { } } -/// A helper for reading Ed25519 OpenSSH private keys from disk. -fn read_ed25519_keypair(key_type: KeyType, key: UnparsedOpenSshKey) -> Result { - 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(), - })?; - - // Build the expected key type (i.e. convert ssh_key key types to the key types - // we're using internally). - let key = match sk.key_data() { - KeypairData::Ed25519(key) => { - ed25519::Keypair::from_bytes(&key.to_bytes()).map_err(|_| { - tor_error::internal!("failed to build ed25519 key out of ed25519 OpenSSH key") - })? - } - _ => { - return Err(SshKeyError::UnexpectedSshKeyType { - path: key.path, - wanted_key_algo: key_type.ssh_algorithm(), - found_key_algo: sk.algorithm().into(), - } - .boxed()); - } - }; - - Ok(key) -} - impl KeyType { /// Get the algorithm of this key type. pub(crate) fn ssh_algorithm(&self) -> SshKeyAlgorithm { From 0b109f3ee8e600b86ca2d092b7b61348e91cc3c4 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 11:53:44 +0100 Subject: [PATCH 05/10] keymgr: Import internal!. --- crates/tor-keymgr/src/key_type/ssh.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index b1b414905..3e8076c60 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -8,7 +8,7 @@ use ssh_key::Algorithm; use crate::{ErasedKey, KeyType, KeystoreError, Result}; -use tor_error::{ErrorKind, HasKind}; +use tor_error::{ErrorKind, HasKind, internal}; use tor_llcrypto::pk::{curve25519, ed25519}; use zeroize::Zeroizing; @@ -166,7 +166,7 @@ impl KeyType { 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") + internal!("failed to build ed25519 key out of ed25519 OpenSSH key") }) .map(Box::new)?), KeypairData::Other(key) @@ -176,7 +176,7 @@ impl KeyType { .private .as_ref() .try_into() - .map_err(|_| tor_error::internal!("invalid x25519 private key"))?; + .map_err(|_| internal!("invalid x25519 private key"))?; Ok(Box::new(curve25519::StaticSecret::from(key))) } From abf83ecfa6f262a9e34e69bc03f3f56de60a207a Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 11:53:59 +0100 Subject: [PATCH 06/10] keymgr: Import internal! (fmt). --- crates/tor-keymgr/src/key_type/ssh.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index 3e8076c60..8854f28f8 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -8,7 +8,7 @@ use ssh_key::Algorithm; use crate::{ErasedKey, KeyType, KeystoreError, Result}; -use tor_error::{ErrorKind, HasKind, internal}; +use tor_error::{internal, ErrorKind, HasKind}; use tor_llcrypto::pk::{curve25519, ed25519}; use zeroize::Zeroizing; @@ -165,9 +165,7 @@ impl KeyType { // we're using internally). match sk.key_data() { KeypairData::Ed25519(key) => Ok(ed25519::Keypair::from_bytes(&key.to_bytes()) - .map_err(|_| { - internal!("failed to build ed25519 key out of ed25519 OpenSSH key") - }) + .map_err(|_| internal!("failed to build ed25519 key out of ed25519 OpenSSH key")) .map(Box::new)?), KeypairData::Other(key) if SshKeyAlgorithm::from(key.algorithm()) == SshKeyAlgorithm::X25519 => From f07651807b158967f763b218a7c9d4870ed01311 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Jul 2023 12:17:51 +0100 Subject: [PATCH 07/10] keymgr: Implement as_ssh_keypair_data for curve25519 keys. --- crates/tor-keymgr/src/keystore.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/tor-keymgr/src/keystore.rs b/crates/tor-keymgr/src/keystore.rs index 90cc1a1e9..75989e262 100644 --- a/crates/tor-keymgr/src/keystore.rs +++ b/crates/tor-keymgr/src/keystore.rs @@ -3,14 +3,16 @@ pub(crate) mod arti; pub use ssh_key::private::KeypairData as SshKeypairData; +use ssh_key::{Algorithm, AlgorithmName}; use rand::{CryptoRng, RngCore}; -use ssh_key::private::{Ed25519Keypair, Ed25519PrivateKey}; -use ssh_key::public::Ed25519PublicKey; +use ssh_key::private::{Ed25519Keypair, Ed25519PrivateKey, OpaqueKeypair}; +use ssh_key::public::{Ed25519PublicKey, OpaquePublicKey}; use tor_error::internal; use tor_hscrypto::pk::{HsClientDescEncSecretKey, HsClientIntroAuthKeypair}; use tor_llcrypto::pk::{curve25519, ed25519}; +use crate::key_type::ssh::X25519_ALGORITHM_NAME; use crate::key_type::KeyType; use crate::{KeySpecifier, KeystoreId, Result}; @@ -113,8 +115,15 @@ impl EncodableKey for curve25519::StaticSecret { } fn as_ssh_keypair_data(&self) -> Result { - // TODO HSS: implement when the ssh-key changes from #936 are released. - Err(internal!("not implemented").into()) + let algorithm_name = AlgorithmName::new(X25519_ALGORITHM_NAME) + .map_err(|_| internal!("invalid algorithm name"))?; + + let public = curve25519::PublicKey::from(self); + let ssh_public = + OpaquePublicKey::new(public.to_bytes().to_vec(), Algorithm::Other(algorithm_name)); + let keypair = OpaqueKeypair::new(self.to_bytes().to_vec(), ssh_public); + + Ok(SshKeypairData::Other(keypair)) } } From c8999f230bd00ba92d8482fe76e99547ccb98d46 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 15 Aug 2023 01:39:56 +0100 Subject: [PATCH 08/10] tor-keymgr: Re-export ssh-key. The `KeypairData` type from [ssh-key] at some point leaked into the keymgr API (via the `EncodableKey` trait). Instead of re-exporting just `KeypairData`, let's re-export the entire `ssh_key` crate (`EncodableKey` implementors would need additional types from `ssh_key` to construct a `KeypairData` object anyway). [ssh-key]: https://crates.io/crates/ssh-key --- crates/tor-keymgr/semver.md | 2 ++ crates/tor-keymgr/src/keystore.rs | 16 +++++++--------- crates/tor-keymgr/src/lib.rs | 3 ++- crates/tor-keymgr/src/mgr.rs | 8 +++++--- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/tor-keymgr/semver.md b/crates/tor-keymgr/semver.md index 049fd9ef3..85bd805af 100644 --- a/crates/tor-keymgr/semver.md +++ b/crates/tor-keymgr/semver.md @@ -1,3 +1,5 @@ ADDED: `EncodableKey::as_ssh_keypair_data` ADDED: `SshKeypairData` (re-exported from `ssh-key`) REMOVED: `KeyType::to_ssh_format` +BREAKING: re-export `ssh_key` rather than just `ssh_key::private::KeypairData` +BREAKING: ssh-key is bumped to 0.6.0 (we re-export `ssh_key`) diff --git a/crates/tor-keymgr/src/keystore.rs b/crates/tor-keymgr/src/keystore.rs index 75989e262..23795024b 100644 --- a/crates/tor-keymgr/src/keystore.rs +++ b/crates/tor-keymgr/src/keystore.rs @@ -2,12 +2,10 @@ pub(crate) mod arti; -pub use ssh_key::private::KeypairData as SshKeypairData; -use ssh_key::{Algorithm, AlgorithmName}; - use rand::{CryptoRng, RngCore}; use ssh_key::private::{Ed25519Keypair, Ed25519PrivateKey, OpaqueKeypair}; use ssh_key::public::{Ed25519PublicKey, OpaquePublicKey}; +use ssh_key::{Algorithm, AlgorithmName}; use tor_error::internal; use tor_hscrypto::pk::{HsClientDescEncSecretKey, HsClientIntroAuthKeypair}; use tor_llcrypto::pk::{curve25519, ed25519}; @@ -93,8 +91,8 @@ pub trait EncodableKey: Downcast { where Self: Sized; - /// Return the [`SshKeypairData`][crate::SshKeypairData] of this key. - fn as_ssh_keypair_data(&self) -> Result; + /// Return the [`KeypairData`][ssh_key::private::KeypairData] of this key. + fn as_ssh_keypair_data(&self) -> Result; } impl_downcast!(EncodableKey); @@ -114,7 +112,7 @@ impl EncodableKey for curve25519::StaticSecret { Ok(curve25519::StaticSecret::new(rng)) } - fn as_ssh_keypair_data(&self) -> Result { + fn as_ssh_keypair_data(&self) -> Result { let algorithm_name = AlgorithmName::new(X25519_ALGORITHM_NAME) .map_err(|_| internal!("invalid algorithm name"))?; @@ -123,7 +121,7 @@ impl EncodableKey for curve25519::StaticSecret { OpaquePublicKey::new(public.to_bytes().to_vec(), Algorithm::Other(algorithm_name)); let keypair = OpaqueKeypair::new(self.to_bytes().to_vec(), ssh_public); - Ok(SshKeypairData::Other(keypair)) + Ok(ssh_key::private::KeypairData::Other(keypair)) } } @@ -144,13 +142,13 @@ impl EncodableKey for ed25519::Keypair { Ok(ed25519::Keypair::generate(&mut rng.rng_compat())) } - fn as_ssh_keypair_data(&self) -> Result { + fn as_ssh_keypair_data(&self) -> Result { let keypair = Ed25519Keypair { public: Ed25519PublicKey(self.public.to_bytes()), private: Ed25519PrivateKey::from_bytes(self.secret.as_bytes()), }; - Ok(SshKeypairData::Ed25519(keypair)) + Ok(ssh_key::private::KeypairData::Ed25519(keypair)) } } diff --git a/crates/tor-keymgr/src/lib.rs b/crates/tor-keymgr/src/lib.rs index c32da8deb..758158041 100644 --- a/crates/tor-keymgr/src/lib.rs +++ b/crates/tor-keymgr/src/lib.rs @@ -65,8 +65,9 @@ pub use key_specifier::{ArtiPath, ArtiPathComponent, CTorPath, KeySpecifier}; pub use { key_type::KeyType, keystore::arti::ArtiNativeKeystore, - keystore::{EncodableKey, ErasedKey, KeygenRng, Keystore, SshKeypairData, ToEncodableKey}, + keystore::{EncodableKey, ErasedKey, KeygenRng, Keystore, ToEncodableKey}, mgr::KeyMgr, + ssh_key, }; #[cfg(not(feature = "keymgr"))] diff --git a/crates/tor-keymgr/src/mgr.rs b/crates/tor-keymgr/src/mgr.rs index 20df58e3a..c28c7a487 100644 --- a/crates/tor-keymgr/src/mgr.rs +++ b/crates/tor-keymgr/src/mgr.rs @@ -192,7 +192,7 @@ mod tests { #![allow(clippy::useless_vec)] //! use super::*; - use crate::{ArtiPath, ErasedKey, KeyType, SshKeypairData}; + use crate::{ArtiPath, ErasedKey, KeyType}; use std::collections::HashMap; use std::str::FromStr; use std::sync::RwLock; @@ -217,9 +217,11 @@ mod tests { Ok("generated_test_key".into()) } - fn as_ssh_keypair_data(&self) -> Result { + fn as_ssh_keypair_data(&self) -> Result { // (Ab)use the encrypted variant for testing purposes - Ok(SshKeypairData::Encrypted(self.as_bytes().to_vec())) + Ok(ssh_key::private::KeypairData::Encrypted( + self.as_bytes().to_vec(), + )) } } From 4b72da73b376185298011652dd71d5876248bb66 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 15 Aug 2023 15:44:49 +0100 Subject: [PATCH 09/10] tor-keymgr: Add sec1 0.7.3 dependency. --- Cargo.lock | 1 + crates/tor-keymgr/Cargo.toml | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 241a9e3b5..71c551551 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4655,6 +4655,7 @@ dependencies = [ "fs-mistrust", "itertools", "rand 0.8.5", + "sec1", "serde", "ssh-key", "tempfile", diff --git a/crates/tor-keymgr/Cargo.toml b/crates/tor-keymgr/Cargo.toml index 0fd3959b4..ba70168c2 100644 --- a/crates/tor-keymgr/Cargo.toml +++ b/crates/tor-keymgr/Cargo.toml @@ -31,6 +31,16 @@ dyn-clone = "1.0.11" fs-mistrust = { path = "../fs-mistrust", version = "0.7.3", features = ["serde", "walkdir"] } itertools = "0.11.0" rand = "0.8" +# TODO HSS: this is not actually a direct dependency of tor-keymgr. It is added +# here to satisfy the minimal-versions check, which would otherwise fail, as +# ssh-key has a hard dependency on 0.7.3. + +# Adding `-p sec1:0.7.3` to the update command from in +# maint/downgrade_dependencies doesn't work because cargo isn't won't be able +# select sec1@0.7.3 after downgrading the dependencies. +# +# See https://github.com/RustCrypto/SSH/issues/153 +sec1 = "0.7.3" serde = { version = "1.0.103", features = ["derive"] } ssh-key = { version = "0.6.0", features = ["std"] } thiserror = "1" From 1e002b14c9c7911140af69cea5999e26a1263815 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 15 Aug 2023 20:59:30 +0100 Subject: [PATCH 10/10] keymgr: Write a registry sketch. This comment will form the basis for the protocol name registry. --- crates/tor-keymgr/src/key_type/ssh.rs | 51 +++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/crates/tor-keymgr/src/key_type/ssh.rs b/crates/tor-keymgr/src/key_type/ssh.rs index 8854f28f8..4e035d69b 100644 --- a/crates/tor-keymgr/src/key_type/ssh.rs +++ b/crates/tor-keymgr/src/key_type/ssh.rs @@ -16,9 +16,56 @@ use std::path::PathBuf; use std::sync::Arc; /// The algorithm string for x25519 SSH keys. -// +/// // TODO HSS: start a protocol name registry in the torspec repo and document the usage and purpose -// of this "protocol" name. +// of this "protocol" name: +// +// ### Assigned Additional Algorithm Names +// +// #### Registration Procedure(s) +// +// TODO +// +// #### NOTE +// +// The algorithm names MUST meet the criteria for additional algorithm names described in [RFC4251 +// § 6]. +// +// We reserve the following custom OpenSSH key types: +// +// +---------------------------+--------------------+---------------------+------------------------+ +// | Public Key Algorithm Name | Public Key Format | Private Key Format | Purpose | +// |---------------------------|--------------------|---------------------|------------------------| +// | x25519@torproject.org | [TODO link to spec | [TODO link to spec | Arti keystore storage | +// | | describing the key | describing the key | format | +// | | format] | format] | | +// | | | | | +// +---------------------------+--------------------+---------------------+------------------------+ +// +// [RFC4251 § 6]: https://www.rfc-editor.org/rfc/rfc4251.html#section-6 +// +// +// +// # x25519@torproject.org OpenSSH Keys +// +// ## Introduction +// +// X25519 keys do not have a predefined SSH key algorithm name in [IANA's Secure Shell(SSH) +// Protocol Parameters], so in order to be able to store this type of key in OpenSSH format, +// we need to define a custom OpenSSH key type. +// +// ## Key Format +// +// An x25519@torproject.org public key file is encoded in the format specified in +// [RFC4716 § 3.4]. +// +// Private keys use the format specified in [PROTOCOL.key]. +// +// TODO: flesh out the RFC and write down a concrete example for clarity. +// +// [IANA's Secure Shell(SSH) Protocol Parameters]: https://www.iana.org/assignments/ssh-parameters/ssh-parameters.xhtml#ssh-parameters-19 +// [RFC4716 § 3.4]: https://datatracker.ietf.org/doc/html/rfc4716#section-3.4 +// [PROTOCOL.key]: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.key?annotate=HEAD pub(crate) const X25519_ALGORITHM_NAME: &str = "x25519@torproject.org"; /// An unparsed OpenSSH key.