From f5212405c52a1390b3f0af63b4b482deeba84761 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Mon, 31 Oct 2022 14:36:14 +0100 Subject: [PATCH] ssh: simplify `radicle-ssh` crate implementation Remove the internal buffer and simplify code around that. Signed-off-by: Alexis Sellier --- radicle-crypto/src/ssh.rs | 18 +-- radicle-ssh/src/agent/client.rs | 252 ++++++++++++++++---------------- radicle-tools/src/rad-agent.rs | 2 +- 3 files changed, 133 insertions(+), 139 deletions(-) diff --git a/radicle-crypto/src/ssh.rs b/radicle-crypto/src/ssh.rs index d27d57ec..b06c9a6f 100644 --- a/radicle-crypto/src/ssh.rs +++ b/radicle-crypto/src/ssh.rs @@ -289,8 +289,7 @@ impl ExtendedSignature { return Err(ExtendedSignatureError::MagicPreamble(preamble)); } - let sig = ExtendedSignature::read(&mut reader)?; - Ok(sig) + ExtendedSignature::read(&mut reader) } pub fn to_armored(&self) -> Vec { @@ -316,7 +315,6 @@ mod test { use std::sync::{Arc, Mutex}; use quickcheck_macros::quickcheck; - use zeroize::Zeroizing; use super::{fmt, ExtendedSignature, SecretKey}; use crate as crypto; @@ -327,21 +325,21 @@ mod test { #[derive(Clone, Default)] struct DummyStream { - incoming: Arc>>>, + incoming: Arc>>, } impl ClientStream for DummyStream { - fn connect_socket

(_path: P) -> Result, Error> + fn connect

(_path: P) -> Result, Error> where P: AsRef + Send, { panic!("This function should never be called!") } - fn read_response(&mut self, buf: &mut Zeroizing>) -> Result<(), Error> { - *self.incoming.lock().unwrap() = buf.clone(); + fn request(&mut self, buf: &[u8]) -> Result { + *self.incoming.lock().unwrap() = buf.to_vec(); - Ok(()) + Ok(Buffer::default()) } } @@ -405,9 +403,9 @@ mod test { let stream = DummyStream::default(); let mut agent = AgentClient::connect(stream.clone()); - let data: Zeroizing> = vec![1, 2, 3, 4, 5, 6, 7, 8, 9].into(); + let data: Vec = vec![1, 2, 3, 4, 5, 6, 7, 8, 9]; - agent.sign_request(&pk, data).ok(); + agent.sign(&pk, &data).ok(); assert_eq!( stream.incoming.lock().unwrap().as_slice(), diff --git a/radicle-ssh/src/agent/client.rs b/radicle-ssh/src/agent/client.rs index 73196b4a..ef0897f4 100644 --- a/radicle-ssh/src/agent/client.rs +++ b/radicle-ssh/src/agent/client.rs @@ -43,26 +43,22 @@ pub enum Error { /// SSH agent client. pub struct AgentClient { stream: S, - buf: Buffer, } // https://tools.ietf.org/html/draft-miller-ssh-agent-00#section-4.1 impl AgentClient { /// Connect to an SSH agent via the provided stream (on Unix, usually a Unix-domain socket). pub fn connect(stream: S) -> Self { - AgentClient { - stream, - buf: Vec::new().into(), - } + AgentClient { stream } } } pub trait ClientStream: Sized + Send + Sync { - /// How to read the response from the stream - fn read_response(&mut self, buf: &mut Buffer) -> Result<(), Error>; + /// Send an agent request to through the stream and read the response. + fn request(&mut self, req: &[u8]) -> Result; /// How to connect the streaming socket - fn connect_socket

(path: P) -> Result, Error> + fn connect

(path: P) -> Result, Error> where P: AsRef + Send; @@ -72,11 +68,11 @@ pub trait ClientStream: Sized + Send + Sync { } else { return Err(Error::EnvVar("SSH_AUTH_SOCK")); }; - match Self::connect_socket(var) { + match Self::connect(var) { Err(Error::Io(io_err)) if io_err.kind() == std::io::ErrorKind::NotFound => { Err(Error::BadAuthSock) } - owise => owise, + err => err, } } } @@ -89,37 +85,38 @@ impl AgentClient { K: Encodable, K::Error: std::error::Error + Send + Sync + 'static, { - self.buf.zeroize(); - self.buf.resize(4, 0); + let mut buf = Buffer::default(); + + buf.resize(4, 0); if constraints.is_empty() { - self.buf.push(msg::ADD_IDENTITY) + buf.push(msg::ADD_IDENTITY) } else { - self.buf.push(msg::ADD_ID_CONSTRAINED) + buf.push(msg::ADD_ID_CONSTRAINED) } - key.write(&mut self.buf); + key.write(&mut buf); if !constraints.is_empty() { for cons in constraints { match *cons { Constraint::KeyLifetime { seconds } => { - self.buf.push(msg::CONSTRAIN_LIFETIME); - self.buf.deref_mut().write_u32::(seconds)? + buf.push(msg::CONSTRAIN_LIFETIME); + buf.deref_mut().write_u32::(seconds)? } - Constraint::Confirm => self.buf.push(msg::CONSTRAIN_CONFIRM), + Constraint::Confirm => buf.push(msg::CONSTRAIN_CONFIRM), Constraint::Extensions { ref name, ref details, } => { - self.buf.push(msg::CONSTRAIN_EXTENSION); - self.buf.extend_ssh_string(name); - self.buf.extend_ssh_string(details); + buf.push(msg::CONSTRAIN_EXTENSION); + buf.extend_ssh_string(name); + buf.extend_ssh_string(details); } } } } - self.buf.write_len(); - self.stream.read_response(&mut self.buf)?; + buf.write_len(); + self.stream.request(&buf)?; Ok(()) } @@ -132,67 +129,68 @@ impl AgentClient { pin: &[u8], constraints: &[Constraint], ) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); + let mut buf = Buffer::default(); + + buf.resize(4, 0); if constraints.is_empty() { - self.buf.push(msg::ADD_SMARTCARD_KEY) + buf.push(msg::ADD_SMARTCARD_KEY) } else { - self.buf.push(msg::ADD_SMARTCARD_KEY_CONSTRAINED) + buf.push(msg::ADD_SMARTCARD_KEY_CONSTRAINED) } - self.buf.extend_ssh_string(id.as_bytes()); - self.buf.extend_ssh_string(pin); + buf.extend_ssh_string(id.as_bytes()); + buf.extend_ssh_string(pin); if !constraints.is_empty() { - self.buf - .deref_mut() + buf.deref_mut() .write_u32::(constraints.len() as u32)?; for cons in constraints { match *cons { Constraint::KeyLifetime { seconds } => { - self.buf.push(msg::CONSTRAIN_LIFETIME); - self.buf.deref_mut().write_u32::(seconds)?; + buf.push(msg::CONSTRAIN_LIFETIME); + buf.deref_mut().write_u32::(seconds)?; } - Constraint::Confirm => self.buf.push(msg::CONSTRAIN_CONFIRM), + Constraint::Confirm => buf.push(msg::CONSTRAIN_CONFIRM), Constraint::Extensions { ref name, ref details, } => { - self.buf.push(msg::CONSTRAIN_EXTENSION); - self.buf.extend_ssh_string(name); - self.buf.extend_ssh_string(details); + buf.push(msg::CONSTRAIN_EXTENSION); + buf.extend_ssh_string(name); + buf.extend_ssh_string(details); } } } } - self.buf.write_len(); - self.stream.read_response(&mut self.buf)?; + buf.write_len(); + self.stream.request(&buf)?; Ok(()) } /// Lock the agent, making it refuse to sign until unlocked. pub fn lock(&mut self, passphrase: &[u8]) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::LOCK); - self.buf.extend_ssh_string(passphrase); - self.buf.write_len(); + let mut buf = Buffer::default(); - self.stream.read_response(&mut self.buf)?; + buf.resize(4, 0); + buf.push(msg::LOCK); + buf.extend_ssh_string(passphrase); + buf.write_len(); + + self.stream.request(&buf)?; Ok(()) } /// Unlock the agent, allowing it to sign again. pub fn unlock(&mut self, passphrase: &[u8]) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::UNLOCK); - self.buf.extend_ssh_string(passphrase); - self.buf.write_len(); + let mut buf = Buffer::default(); + buf.resize(4, 0); + buf.push(msg::UNLOCK); + buf.extend_ssh_string(passphrase); + buf.write_len(); - self.stream.read_response(&mut self.buf)?; + self.stream.request(&buf)?; Ok(()) } @@ -204,16 +202,16 @@ impl AgentClient { K: Encodable, K::Error: std::error::Error + Send + Sync + 'static, { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::REQUEST_IDENTITIES); - self.buf.write_len(); - - self.stream.read_response(&mut self.buf)?; + let mut buf = Buffer::default(); + buf.resize(4, 0); + buf.push(msg::REQUEST_IDENTITIES); + buf.write_len(); let mut keys = Vec::new(); - if self.buf[0] == msg::IDENTITIES_ANSWER { - let mut r = self.buf.reader(1); + let resp = self.stream.request(&buf)?; + + if resp[0] == msg::IDENTITIES_ANSWER { + let mut r = resp.reader(1); let n = r.read_u32()?; for _ in 0..n { @@ -231,26 +229,23 @@ impl AgentClient { } /// Ask the agent to sign the supplied piece of data. - pub fn sign_request(&mut self, public: &K, data: Buffer) -> Result + pub fn sign(&mut self, public: &K, data: &[u8]) -> Result where K: Encodable + fmt::Debug, { - self.prepare_sign_request(public, &data); - self.stream.read_response(&mut self.buf)?; + let req = self.prepare_sign_request(public, data); + let resp = self.stream.request(&req)?; - if !self.buf.is_empty() && self.buf[0] == msg::SIGN_RESPONSE { - let mut signature: Signature = [0; 64]; - self.write_signature(&mut signature)?; - - Ok(signature) - } else if self.buf[0] == msg::FAILURE { + if !resp.is_empty() && resp[0] == msg::SIGN_RESPONSE { + self.read_signature(&resp) + } else if !resp.is_empty() && resp[0] == msg::FAILURE { Err(Error::AgentFailure) } else { Err(Error::AgentProtocolError) } } - fn prepare_sign_request(&mut self, public: &K, data: &[u8]) + fn prepare_sign_request(&self, public: &K, data: &[u8]) -> Buffer where K: Encodable + fmt::Debug, { @@ -264,27 +259,28 @@ impl AgentClient { let total = 1 + pk.len() + 4 + data.len() + 4; - self.buf.zeroize(); - self.buf - .write_u32::(total as u32) + let mut buf = Buffer::default(); + buf.write_u32::(total as u32) .expect("Writing to a vector never fails"); - self.buf.push(msg::SIGN_REQUEST); - self.buf.extend_from_slice(&pk); - self.buf.extend_ssh_string(data); + buf.push(msg::SIGN_REQUEST); + buf.extend_from_slice(&pk); + buf.extend_ssh_string(data); // Signature flags should be zero for ed25519. - self.buf.write_u32::(0).unwrap(); + buf.write_u32::(0).unwrap(); + buf } - fn write_signature(&self, data: &mut [u8]) -> Result<(), Error> { - let mut r = self.buf.reader(1); + fn read_signature(&self, sig: &[u8]) -> Result { + let mut r = sig.reader(1); let mut resp = r.read_string()?.reader(0); let _t = resp.read_string()?; let sig = resp.read_string()?; - data.copy_from_slice(sig); + let mut out = [0; 64]; + out.copy_from_slice(sig); - Ok(()) + Ok(out) } /// Ask the agent to remove a key from its memory. @@ -297,70 +293,71 @@ impl AgentClient { let total = 1 + pk.len(); - self.buf.zeroize(); - self.buf.write_u32::(total as u32)?; - self.buf.push(msg::REMOVE_IDENTITY); - self.buf.extend_from_slice(&pk); + let mut buf = Buffer::default(); + buf.write_u32::(total as u32)?; + buf.push(msg::REMOVE_IDENTITY); + buf.extend_from_slice(&pk); - self.stream.read_response(&mut self.buf)?; + self.stream.request(&buf)?; Ok(()) } /// Ask the agent to remove a smartcard from its memory. pub fn remove_smartcard_key(&mut self, id: &str, pin: &[u8]) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::REMOVE_SMARTCARD_KEY); - self.buf.extend_ssh_string(id.as_bytes()); - self.buf.extend_ssh_string(pin); - self.buf.write_len(); + let mut buf = Buffer::default(); + buf.resize(4, 0); + buf.push(msg::REMOVE_SMARTCARD_KEY); + buf.extend_ssh_string(id.as_bytes()); + buf.extend_ssh_string(pin); + buf.write_len(); - self.stream.read_response(&mut self.buf)?; + self.stream.request(&buf)?; Ok(()) } /// Ask the agent to forget all known keys. pub fn remove_all_identities(&mut self) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::REMOVE_ALL_IDENTITIES); - self.buf.write_len(); + let mut buf = Buffer::default(); + buf.resize(4, 0); + buf.push(msg::REMOVE_ALL_IDENTITIES); + buf.write_len(); - self.stream.read_response(&mut self.buf)?; + self.stream.request(&buf)?; Ok(()) } /// Send a custom message to the agent. pub fn extension(&mut self, typ: &[u8], ext: &[u8]) -> Result<(), Error> { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::EXTENSION); - self.buf.extend_ssh_string(typ); - self.buf.extend_ssh_string(ext); - self.buf.write_len(); + let mut buf = Buffer::default(); - self.stream.read_response(&mut self.buf)?; + buf.resize(4, 0); + buf.push(msg::EXTENSION); + buf.extend_ssh_string(typ); + buf.extend_ssh_string(ext); + buf.write_len(); + + self.stream.request(&buf)?; Ok(()) } - /// Ask the agent what extensions about supported extensions. + /// Ask the agent about supported extensions. pub fn query_extension(&mut self, typ: &[u8], mut ext: Buffer) -> Result { - self.buf.zeroize(); - self.buf.resize(4, 0); - self.buf.push(msg::EXTENSION); - self.buf.extend_ssh_string(typ); - self.buf.write_len(); + let mut req = Buffer::default(); - self.stream.read_response(&mut self.buf)?; + req.resize(4, 0); + req.push(msg::EXTENSION); + req.extend_ssh_string(typ); + req.write_len(); - let mut r = self.buf.reader(1); + let resp = self.stream.request(&req)?; + let mut r = resp.reader(1); ext.extend(r.read_string()?); - Ok(!self.buf.is_empty() && self.buf[0] == msg::SUCCESS) + Ok(!resp.is_empty() && resp[0] == msg::SUCCESS) } } @@ -384,33 +381,32 @@ impl ClientStream for TcpStream { #[cfg(unix)] impl ClientStream for UnixStream { - fn connect_socket

(path: P) -> Result, Error> + fn connect

(path: P) -> Result, Error> where P: AsRef + Send, { let stream = UnixStream::connect(path)?; - Ok(AgentClient { - stream, - buf: Vec::new().into(), - }) + + Ok(AgentClient { stream }) } - fn read_response(&mut self, buf: &mut Buffer) -> Result<(), Error> { + fn request(&mut self, msg: &[u8]) -> Result { + let mut resp = Buffer::default(); + // Write the message - self.write_all(buf)?; + self.write_all(msg)?; self.flush()?; // Read the length - buf.zeroize(); - buf.resize(4, 0); - self.read_exact(buf)?; + resp.resize(4, 0); + self.read_exact(&mut resp)?; // Read the rest of the buffer - let len = BigEndian::read_u32(buf) as usize; - buf.zeroize(); - buf.resize(len, 0); - self.read_exact(buf)?; + let len = BigEndian::read_u32(&resp) as usize; + resp.zeroize(); + resp.resize(len, 0); + self.read_exact(&mut resp)?; - Ok(()) + Ok(resp) } } diff --git a/radicle-tools/src/rad-agent.rs b/radicle-tools/src/rad-agent.rs index f93cb44d..4400863b 100644 --- a/radicle-tools/src/rad-agent.rs +++ b/radicle-tools/src/rad-agent.rs @@ -25,7 +25,7 @@ fn main() -> anyhow::Result<()> { io::stdin().read_to_end(&mut stdin)?; let mut agent = ssh::agent::connect()?; - let sig = agent.sign_request(profile.id(), stdin.into())?; + let sig = agent.sign(profile.id(), &stdin)?; let sig = crypto::Signature::from(sig); println!("{}", &sig);