From 0482f0ce0470c982c592779f055abe52d7aecdb2 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Thu, 13 Oct 2022 22:33:36 +0200 Subject: [PATCH] ssh: remove unused traits, simplify code Signed-off-by: Alexis Sellier --- radicle-ssh/src/agent/client.rs | 28 +++--- radicle-ssh/src/encoding.rs | 14 +-- radicle-ssh/src/key.rs | 26 ----- radicle-ssh/src/lib.rs | 1 - radicle/src/ssh.rs | 165 ++++++++++++++------------------ 5 files changed, 88 insertions(+), 146 deletions(-) delete mode 100644 radicle-ssh/src/key.rs diff --git a/radicle-ssh/src/agent/client.rs b/radicle-ssh/src/agent/client.rs index 55f0ca8d..14c27fe5 100644 --- a/radicle-ssh/src/agent/client.rs +++ b/radicle-ssh/src/agent/client.rs @@ -11,9 +11,8 @@ use zeroize::Zeroize as _; use crate::agent::msg; use crate::agent::Constraint; -use crate::encoding; +use crate::encoding::{self, Encodable}; use crate::encoding::{Buffer, Encoding, Reader}; -use crate::key::{Private, Public}; /// An ed25519 Signature. pub type Signature = [u8; 64]; @@ -87,7 +86,7 @@ impl AgentClient { /// to apply when using the key to sign. pub fn add_identity(&mut self, key: &K, constraints: &[Constraint]) -> Result<(), Error> where - K: Private, + K: Encodable, K::Error: std::error::Error + Send + Sync + 'static, { self.buf.zeroize(); @@ -98,8 +97,7 @@ impl AgentClient { } else { self.buf.push(msg::ADD_ID_CONSTRAINED) } - key.write(&mut self.buf) - .map_err(|err| Error::Private(Box::new(err)))?; + key.write(&mut self.buf); if !constraints.is_empty() { for cons in constraints { @@ -203,7 +201,7 @@ impl AgentClient { /// keys. pub fn request_identities(&mut self) -> Result, Error> where - K: Public, + K: Encodable, K::Error: std::error::Error + Send + Sync + 'static, { self.buf.zeroize(); @@ -224,7 +222,7 @@ impl AgentClient { let _ = r.read_string()?; let mut r = key.reader(0); - if let Some(pk) = K::read(&mut r).map_err(|err| Error::Public(Box::new(err)))? { + if let Ok(pk) = K::read(&mut r) { keys.push(pk); } } @@ -236,7 +234,7 @@ impl AgentClient { /// Ask the agent to sign the supplied piece of data. pub fn sign_request(&mut self, public: &K, data: Buffer) -> Result where - K: Public + fmt::Debug, + K: Encodable + fmt::Debug, { self.prepare_sign_request(public, &data); self.stream.read_response(&mut self.buf)?; @@ -255,7 +253,7 @@ impl AgentClient { fn prepare_sign_request(&mut self, public: &K, data: &[u8]) where - K: Public + fmt::Debug, + K: Encodable + fmt::Debug, { // byte SSH_AGENTC_SIGN_REQUEST // string key blob @@ -263,10 +261,9 @@ impl AgentClient { // uint32 flags let mut pk = Buffer::default(); - let n = public.write(&mut pk); - let total = 1 + n + 4 + data.len() + 4; + public.write(&mut pk); - debug_assert_eq!(n, pk.len()); + let total = 1 + pk.len() + 4 + data.len() + 4; self.buf.zeroize(); self.buf @@ -294,13 +291,12 @@ impl AgentClient { /// Ask the agent to remove a key from its memory. pub fn remove_identity(&mut self, public: &K) -> Result<(), Error> where - K: Public, + K: Encodable, { let mut pk: Buffer = Vec::new().into(); - let n = public.write(&mut pk); - let total = 1 + n; + public.write(&mut pk); - debug_assert_eq!(n, pk.len()); + let total = 1 + pk.len(); self.buf.zeroize(); self.buf.write_u32::(total as u32)?; diff --git a/radicle-ssh/src/encoding.rs b/radicle-ssh/src/encoding.rs index d6d18b1d..fc38e02e 100644 --- a/radicle-ssh/src/encoding.rs +++ b/radicle-ssh/src/encoding.rs @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. // -use std::mem::size_of; use std::ops::DerefMut; use byteorder::{BigEndian, ByteOrder, WriteBytesExt}; @@ -33,16 +32,15 @@ pub trait Encodable: Sized { type Error: std::error::Error + Send + Sync + 'static; /// Read from the SSH format. - fn read_ssh(reader: &mut Cursor) -> Result; - + fn read(reader: &mut Cursor) -> Result; /// Write to the SSH format. - fn write_ssh(&self, buf: &mut E); + fn write(&self, buf: &mut E); } /// Encode in the SSH format. pub trait Encoding { /// Push an SSH-encoded string to `self`. - fn extend_ssh_string(&mut self, s: &[u8]) -> usize; + fn extend_ssh_string(&mut self, s: &[u8]); /// Push an SSH-encoded blank string of length `s` to `self`. fn extend_ssh_string_blank(&mut self, s: usize) -> &mut [u8]; /// Push an SSH-encoded multiple-precision integer. @@ -67,11 +65,9 @@ pub fn mpint_len(s: &[u8]) -> usize { } impl Encoding for Vec { - fn extend_ssh_string(&mut self, s: &[u8]) -> usize { + fn extend_ssh_string(&mut self, s: &[u8]) { self.write_u32::(s.len() as u32).unwrap(); self.extend(s); - - size_of::() + s.len() } fn extend_ssh_string_blank(&mut self, len: usize) -> &mut [u8] { @@ -134,7 +130,7 @@ impl Encoding for Vec { } impl Encoding for Buffer { - fn extend_ssh_string(&mut self, s: &[u8]) -> usize { + fn extend_ssh_string(&mut self, s: &[u8]) { self.deref_mut().extend_ssh_string(s) } diff --git a/radicle-ssh/src/key.rs b/radicle-ssh/src/key.rs deleted file mode 100644 index 19914d73..00000000 --- a/radicle-ssh/src/key.rs +++ /dev/null @@ -1,26 +0,0 @@ -use std::error::Error; - -use crate::encoding::{Buffer, Cursor, Encoding}; - -/// A public SSH key. -pub trait Public: Sized { - type Error: Error + Send + Sync + 'static; - - /// Write the public key to the given buffer, in SSH "blob" format. - fn write(&self, buf: &mut E) -> usize; - /// Read the public key from the given reader. - fn read(reader: &mut Cursor) -> Result, Self::Error>; -} - -/// A private SSH key. -pub trait Private: Sized { - type Error: Error + Send + Sync + 'static; - - /// Read a private key from the given reader. - fn read(reader: &mut Cursor) -> Result, Self::Error>; - /// Write the key bytes to the supplied buffer. - fn write(&self, buf: &mut Buffer) -> Result<(), Self::Error>; - /// Sign the data and write the signature to the given buffer. - fn write_signature>(&self, data: T, buf: &mut Buffer) - -> Result<(), Self::Error>; -} diff --git a/radicle-ssh/src/lib.rs b/radicle-ssh/src/lib.rs index 96fce5a4..632188e7 100644 --- a/radicle-ssh/src/lib.rs +++ b/radicle-ssh/src/lib.rs @@ -1,3 +1,2 @@ pub mod agent; pub mod encoding; -pub mod key; diff --git a/radicle/src/ssh.rs b/radicle/src/ssh.rs index 97dc3830..2dcd2727 100644 --- a/radicle/src/ssh.rs +++ b/radicle/src/ssh.rs @@ -1,17 +1,13 @@ pub mod agent; use std::io; -use std::ops::DerefMut; -use byteorder::{BigEndian, WriteBytesExt}; use thiserror::Error; -use zeroize::Zeroizing; use radicle_ssh::encoding; use radicle_ssh::encoding::Encodable; use radicle_ssh::encoding::Encoding; use radicle_ssh::encoding::Reader; -use radicle_ssh::key::Public; use crate::crypto; use crate::crypto::PublicKey; @@ -90,7 +86,7 @@ pub enum SignatureError { impl Encodable for crypto::Signature { type Error = SignatureError; - fn read_ssh(r: &mut encoding::Cursor) -> Result { + fn read(r: &mut encoding::Cursor) -> Result { let buf = r.read_string()?; let mut inner_strs = buf.reader(0); @@ -105,7 +101,7 @@ impl Encodable for crypto::Signature { Ok(sig) } - fn write_ssh(&self, buf: &mut E) { + fn write(&self, buf: &mut E) { let mut inner_strs = Vec::new(); inner_strs.extend_ssh_string(b"ssh-ed25519"); inner_strs.extend_ssh_string(self.as_ref()); @@ -126,12 +122,10 @@ pub enum PublicKeyError { impl Encodable for PublicKey { type Error = PublicKeyError; - fn read_ssh(r: &mut encoding::Cursor) -> Result { - let buf = r.read_string()?; - let mut str_r = buf.reader(0); - match str_r.read_string()? { + fn read(r: &mut encoding::Cursor) -> Result { + match r.read_string()? { b"ssh-ed25519" => { - let s = str_r.read_string()?; + let s = r.read_string()?; let p = PublicKey::try_from(s)?; Ok(p) @@ -142,31 +136,11 @@ impl Encodable for PublicKey { } } - fn write_ssh(&self, w: &mut E) { - _ = self.write(w); - } -} - -impl Public for PublicKey { - type Error = PublicKeyError; - - fn read(r: &mut encoding::Cursor) -> Result, Self::Error> { - match r.read_string()? { - b"ssh-ed25519" => { - let s = r.read_string()?; - let p = PublicKey::try_from(s)?; - - Ok(Some(p)) - } - _ => Ok(None), - } - } - - fn write(&self, buf: &mut E) -> usize { + fn write(&self, w: &mut E) { let mut str_w: Vec = Vec::::new(); str_w.extend_ssh_string(b"ssh-ed25519"); str_w.extend_ssh_string(&self[..]); - buf.extend_ssh_string(&str_w) + w.extend_ssh_string(&str_w) } } @@ -188,14 +162,16 @@ pub enum SecretKeyError { Crypto(#[from] crypto::Error), #[error(transparent)] Io(#[from] io::Error), + #[error("unknown algorithm '{0}'")] + UnknownAlgorithm(String), #[error("public key does not match secret key")] Mismatch, } -impl radicle_ssh::key::Private for SecretKey { +impl Encodable for SecretKey { type Error = SecretKeyError; - fn read(r: &mut encoding::Cursor) -> Result, Self::Error> { + fn read(r: &mut encoding::Cursor) -> Result { match r.read_string()? { b"ssh-ed25519" => { let public = r.read_string()?; @@ -206,38 +182,21 @@ impl radicle_ssh::key::Private for SecretKey { if public != key.public_key().as_ref() { return Err(SecretKeyError::Mismatch); } - Ok(Some(SecretKey(key))) + Ok(SecretKey(key)) } - _ => Ok(None), + s => Err(SecretKeyError::UnknownAlgorithm( + String::from_utf8_lossy(s).to_string(), + )), } } - fn write(&self, buf: &mut Zeroizing>) -> Result<(), Self::Error> { + fn write(&self, buf: &mut E) { let public = self.0.public_key(); buf.extend_ssh_string(b"ssh-ed25519"); buf.extend_ssh_string(public.as_ref()); - buf.deref_mut().write_u32::(64)?; - buf.extend(&*self.0); + buf.extend_ssh_string(&*self.0); buf.extend_ssh_string(b"radicle"); - - Ok(()) - } - - fn write_signature>( - &self, - data: Bytes, - buf: &mut Zeroizing>, - ) -> Result<(), Self::Error> { - let name = "ssh-ed25519"; - let signature: [u8; 64] = *self.0.sign(data.as_ref(), None); - - buf.deref_mut() - .write_u32::((name.len() + signature.len() + 8) as u32)?; - buf.extend_ssh_string(name.as_bytes()); - buf.extend_ssh_string(&signature); - - Ok(()) } } @@ -253,12 +212,8 @@ pub enum ExtendedSignatureError { MissingHeader, #[error(transparent)] Encoding(#[from] encoding::Error), - #[error("public key encoding")] - PublicKeyEncoding, #[error(transparent)] - PublicKeyError(#[from] PublicKeyError), - #[error("signature encoding")] - SignatureEncoding, + PublicKey(#[from] PublicKeyError), #[error(transparent)] SignatureError(#[from] SignatureError), #[error("unsupported version '{0}'")] @@ -270,8 +225,8 @@ pub enum ExtendedSignatureError { /// See #[derive(Clone, Debug)] pub struct ExtendedSignature { - sig_version: u32, - publickey: crypto::PublicKey, + version: u32, + public_key: crypto::PublicKey, /// Unambigious interpretation domain to prevent cross-protocol attacks. namespace: Vec, reserved: Vec, @@ -283,31 +238,30 @@ pub struct ExtendedSignature { impl Encodable for ExtendedSignature { type Error = ExtendedSignatureError; - fn read_ssh(r: &mut encoding::Cursor) -> Result { + fn read(r: &mut encoding::Cursor) -> Result { let sig_version = r.read_u32()?; if sig_version > 1 { return Err(ExtendedSignatureError::UnsupportedVersion(sig_version)); } + let mut pk = r.read_string()?.reader(0); Ok(ExtendedSignature { - sig_version, - publickey: PublicKey::read_ssh(r) - .map_err(|_| ExtendedSignatureError::PublicKeyEncoding)?, + version: sig_version, + public_key: PublicKey::read(&mut pk)?, namespace: r.read_string()?.into(), reserved: r.read_string()?.into(), hash_algorithm: r.read_string()?.into(), - signature: crypto::Signature::read_ssh(r) - .map_err(|_| ExtendedSignatureError::PublicKeyEncoding)?, + signature: crypto::Signature::read(r)?, }) } - fn write_ssh(&self, buf: &mut E) { - buf.extend_u32(self.sig_version); - let _ = &self.publickey.write_ssh(buf); + fn write(&self, buf: &mut E) { + buf.extend_u32(self.version); + let _ = &self.public_key.write(buf); buf.extend_ssh_string(&self.namespace); buf.extend_ssh_string(&self.reserved); buf.extend_ssh_string(&self.hash_algorithm); - let _ = &self.signature.write_ssh(buf); + let _ = &self.signature.write(buf); } } @@ -334,27 +288,24 @@ impl ExtendedSignature { return Err(ExtendedSignatureError::MagicPreamble(preamble)); } - let sig = ExtendedSignature::read_ssh(&mut reader)?; + let sig = ExtendedSignature::read(&mut reader)?; Ok(sig) } pub fn to_armored(&self) -> Vec { - let mut v = encoding::Buffer::default(); - - v.extend(Self::MAGIC_PREAMBLE); - self.write_ssh(&mut v); + let mut buf = encoding::Buffer::from(Self::MAGIC_PREAMBLE.to_vec()); + self.write(&mut buf); let mut armored = Self::ARMORED_HEADER.to_vec(); armored.push(b'\n'); - let body: Vec = base64::encode(v).into(); - for line in body.chunks(Self::ARMORED_WIDTH) { - armored.extend(line.to_vec()); + let body = base64::encode(buf); + for line in body.as_bytes().chunks(Self::ARMORED_WIDTH) { + armored.extend(line); armored.push(b'\n'); } armored.extend(Self::ARMORED_FOOTER); - armored } } @@ -366,13 +317,12 @@ mod test { use quickcheck_macros::quickcheck; use zeroize::Zeroizing; - use super::{ExtendedSignature, SecretKey}; - use crate::crypto; + use super::{fmt, ExtendedSignature, SecretKey}; use crate::crypto::PublicKey; + use crate::crypto::{self}; use crate::test::arbitrary::ByteArray; use radicle_ssh::agent::client::{AgentClient, ClientStream, Error}; - use radicle_ssh::encoding::Reader; - use radicle_ssh::key::Private as _; + use radicle_ssh::encoding::*; #[derive(Clone, Default)] struct DummyStream { @@ -396,12 +346,12 @@ mod test { #[quickcheck] fn prop_encode_decode_sk(input: ByteArray<64>) { - let mut buf = Vec::new().into(); + let mut buf = Buffer::default(); let sk = crypto::SecretKey::new(input.into_inner()); - SecretKey(sk).write(&mut buf).unwrap(); + SecretKey(sk).write(&mut buf); let mut cursor = buf.reader(0); - let output = SecretKey::read(&mut cursor).unwrap().unwrap(); + let output = SecretKey::read(&mut cursor).unwrap(); assert_eq!(sk, output.0); } @@ -469,16 +419,43 @@ mod test { let armored: &[u8] = b"-----BEGIN SSH SIGNATURE----- U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgvjrQogRxxLjzzWns8+mKJAGzEX 4fm2ALoN7pyvD2ttQAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5 -AAAAQJ759x+pFz0z2yM13S/sqeOOSgTE3fhoJG54dotNTk17dQEPKnH4S4N5jjA+pxM1mb -oejZ0WJ0cQtBjWZ7JEBQM= +AAAAQI84aPZsXxlQigpy1/Y/iJSmHSS//CIgvqvUMQIb/TM2vhCKruduH0cK02k9G8wOI+ +EUMf2bSDyxbJyZThOEiAs= -----END SSH SIGNATURE-----"; + let public_key = "AAAAC3NzaC1lZDI1NTE5AAAAIL460KIEccS4881p7PPpiiQBsxF+H5tgC6De6crw9rbU"; let signature = ExtendedSignature::from_armored(armored).unwrap(); - assert_eq!(1, signature.sig_version); + + assert_eq!(signature.version, 1); + assert_eq!(fmt::key(&signature.public_key), public_key); assert_eq!( String::from_utf8(armored.to_vec()), String::from_utf8(signature.to_armored()), "signature should remain unaltered after decoding" ); } + + #[test] + fn test_signature_verify() { + let seed = crypto::Seed::new([1; 32]); + let pair = crypto::KeyPair::from_seed(seed); + let message = &[0xff]; + let sig = pair.sk.sign(message, None); + let esig = ExtendedSignature { + version: 1, + public_key: pair.pk.into(), + signature: sig.into(), + hash_algorithm: vec![], + namespace: vec![], + reserved: vec![], + }; + + let armored = esig.to_armored(); + let unarmored = ExtendedSignature::from_armored(&armored).unwrap(); + + unarmored + .public_key + .verify(message, &unarmored.signature) + .unwrap(); + } }