From e584bd6c1cbb5c8bb9b74a856f6fdd7c22392efe Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Fri, 17 Feb 2023 10:35:33 +0100 Subject: [PATCH] crypto: Use `ssh-key` crate for signatures --- Cargo.lock | 1 - radicle-cob/src/backend/git/change.rs | 20 ++- radicle-cob/src/change.rs | 4 +- radicle-cob/src/change/store.rs | 2 +- radicle-cob/src/change_graph.rs | 4 +- radicle-cob/src/lib.rs | 4 +- radicle-cob/src/signatures.rs | 50 ++---- radicle-crypto/Cargo.toml | 8 +- radicle-crypto/src/ssh.rs | 232 +++++++------------------- 9 files changed, 91 insertions(+), 234 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6745c554..f49cd505 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1919,7 +1919,6 @@ name = "radicle-crypto" version = "0.1.0" dependencies = [ "amplify", - "base64", "cyphernet", "ec25519", "fastrand", diff --git a/radicle-cob/src/backend/git/change.rs b/radicle-cob/src/backend/git/change.rs index df0c652d..15e384c4 100644 --- a/radicle-cob/src/backend/git/change.rs +++ b/radicle-cob/src/backend/git/change.rs @@ -12,10 +12,11 @@ use git_trailers::OwnedTrailer; use nonempty::NonEmpty; use crate::history::entry::Timestamp; +use crate::signatures; use crate::{ change::{self, store, Change}, history::entry, - signatures::{Signature, Signatures}, + signatures::{ExtendedSignature, Signatures}, trailers, }; @@ -40,6 +41,8 @@ pub mod error { #[error(transparent)] Signer(#[from] Box), #[error(transparent)] + Signatures(#[from] Signatures), + #[error(transparent)] Utf8(#[from] Utf8Error), } @@ -84,7 +87,7 @@ impl change::Storage for git2::Repository { type ObjectId = Oid; type Resource = Oid; - type Signatures = Signature; + type Signatures = ExtendedSignature; fn store( &self, @@ -113,7 +116,7 @@ impl change::Storage for git2::Repository { let signature = { let sig = signer.sign(revision.as_bytes()); let key = signer.public_key(); - Signature::from((*key, sig)) + ExtendedSignature::new(*key, sig) }; let (id, timestamp) = write_commit(self, resource, tips, message, signature.clone(), tree)?; @@ -135,7 +138,7 @@ impl change::Storage for git2::Repository { let mut signatures = Signatures::try_from(&commit)? .into_iter() .collect::>(); - let Some(signature) = signatures.pop() else { + let Some((key, sig)) = signatures.pop() else { return Err(error::Load::ChangeNotSigned(id)); }; if !signatures.is_empty() { @@ -149,7 +152,7 @@ impl change::Storage for git2::Repository { Ok(Change { id, revision: tree.id().into(), - signature: signature.into(), + signature: ExtendedSignature::new(key, sig), resource, manifest, contents, @@ -220,7 +223,7 @@ fn write_commit( resource: O, tips: Vec, message: String, - signature: Signature, + signature: ExtendedSignature, tree: git2::Tree, ) -> Result<(Oid, Timestamp), error::Create> where @@ -240,7 +243,10 @@ where let mut headers = commit::Headers::new(); headers.push( "gpgsig", - &String::from_utf8(crypto::ssh::ExtendedSignature::from(signature).to_armored())?, + signature + .to_pem() + .map_err(signatures::error::Signatures::from)? + .as_str(), ); let author = commit::Author::try_from(&author)?; diff --git a/radicle-cob/src/change.rs b/radicle-cob/src/change.rs index aa64059b..e3e59234 100644 --- a/radicle-cob/src/change.rs +++ b/radicle-cob/src/change.rs @@ -8,9 +8,9 @@ use git_ext::Oid; pub mod store; pub use store::{Storage, Template}; -use crate::signatures::Signature; +use crate::signatures::ExtendedSignature; /// A single change in the change graph. The layout of changes in the repository /// is specified in the RFC (docs/rfc/0662-collaborative-objects.adoc) /// under "Change Commits". -pub type Change = store::Change; +pub type Change = store::Change; diff --git a/radicle-cob/src/change/store.rs b/radicle-cob/src/change/store.rs index 750ccc60..08248659 100644 --- a/radicle-cob/src/change/store.rs +++ b/radicle-cob/src/change/store.rs @@ -108,7 +108,7 @@ where } } -impl Change +impl Change where Id: AsRef<[u8]>, { diff --git a/radicle-cob/src/change_graph.rs b/radicle-cob/src/change_graph.rs index 23833bb3..a523cb25 100644 --- a/radicle-cob/src/change_graph.rs +++ b/radicle-cob/src/change_graph.rs @@ -9,7 +9,7 @@ use git_ext::Oid; use radicle_dag::{Dag, Node}; use crate::{ - change, object, signatures::Signature, Change, CollaborativeObject, ObjectId, TypeName, + change, object, signatures::ExtendedSignature, Change, CollaborativeObject, ObjectId, TypeName, }; mod evaluation; @@ -31,7 +31,7 @@ impl ChangeGraph { oid: &ObjectId, ) -> Option where - S: change::Storage, + S: change::Storage, { log::info!("loading object '{}' '{}'", typename, oid); let mut builder = GraphBuilder::default(); diff --git a/radicle-cob/src/lib.rs b/radicle-cob/src/lib.rs index f18465bf..b486a779 100644 --- a/radicle-cob/src/lib.rs +++ b/radicle-cob/src/lib.rs @@ -97,7 +97,7 @@ pub use history::{Contents, Entry, History}; mod pruning_fold; pub mod signatures; -use signatures::Signature; +use signatures::ExtendedSignature; pub mod type_name; pub use type_name::TypeName; @@ -135,7 +135,7 @@ where LoadError = git::change::error::Load, ObjectId = git_ext::Oid, Resource = git_ext::Oid, - Signatures = Signature, + Signatures = ExtendedSignature, >, { } diff --git a/radicle-cob/src/signatures.rs b/radicle-cob/src/signatures.rs index 746c28d6..4d04a89d 100644 --- a/radicle-cob/src/signatures.rs +++ b/radicle-cob/src/signatures.rs @@ -10,45 +10,15 @@ use std::{ ops::{Deref, DerefMut}, }; -use crypto::{ssh::ExtendedSignature, PublicKey}; +use crypto::{ssh, PublicKey}; use git_commit::{ Commit, Signature::{Pgp, Ssh}, }; +pub use ssh::ExtendedSignature; pub mod error; -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct Signature { - pub(super) key: PublicKey, - pub(super) sig: crypto::Signature, -} - -impl Signature { - pub fn verify(&self, payload: &[u8]) -> bool { - self.key.verify(payload, &self.sig).is_ok() - } -} - -impl From for ExtendedSignature { - fn from(sig: Signature) -> Self { - Self::new(sig.key, sig.sig) - } -} - -impl From for Signature { - fn from(ex: ExtendedSignature) -> Self { - let (key, sig) = ex.into(); - Self { key, sig } - } -} - -impl From<(PublicKey, crypto::Signature)> for Signature { - fn from((key, sig): (PublicKey, crypto::Signature)) -> Self { - Self { key, sig } - } -} - // FIXME(kim): This should really be a HashMap with a no-op Hasher -- PublicKey // collisions are catastrophic #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] @@ -68,8 +38,8 @@ impl DerefMut for Signatures { } } -impl From for Signatures { - fn from(Signature { key, sig }: Signature) -> Self { +impl From for Signatures { + fn from(ExtendedSignature { key, sig }: ExtendedSignature) -> Self { let mut map = BTreeMap::new(); map.insert(key, sig); map.into() @@ -98,13 +68,13 @@ impl TryFrom<&Commit> for Signatures { match signature { // Skip PGP signatures Pgp(_) => None, - Ssh(armored) => Some( - ExtendedSignature::from_armored(armored.as_bytes()) + Ssh(pem) => Some( + ExtendedSignature::from_pem(pem.as_bytes()) .map_err(error::Signatures::from), ), } }) - .map(|ex| ex.map(|ex| ex.into())) + .map(|r| r.map(|es| (es.key, es.sig))) .collect::>() } } @@ -127,12 +97,12 @@ impl IntoIterator for Signatures { } } -impl Extend for Signatures { +impl Extend for Signatures { fn extend(&mut self, iter: T) where - T: IntoIterator, + T: IntoIterator, { - for Signature { key, sig } in iter { + for ExtendedSignature { key, sig } in iter { self.insert(key, sig); } } diff --git a/radicle-crypto/Cargo.toml b/radicle-crypto/Cargo.toml index 34c8207b..97b84c31 100644 --- a/radicle-crypto/Cargo.toml +++ b/radicle-crypto/Cargo.toml @@ -10,7 +10,7 @@ edition = "2021" [features] test = ["fastrand", "qcheck"] -ssh = ["base64", "radicle-ssh", "ssh-key"] +ssh = ["radicle-ssh", "ssh-key"] [dependencies] amplify = { version = "4.0.0-beta.4" } @@ -34,7 +34,7 @@ optional = true [dependencies.ssh-key] version = "0.5.1" default-features = false -features = ["std", "encryption", "rand_core", "getrandom"] +features = ["std", "encryption", "getrandom"] optional = true [dependencies.qcheck] @@ -48,10 +48,6 @@ version = "0" default-features = false optional = true -[dependencies.base64] -version = "0.13" -optional = true - [dev-dependencies] fastrand = { version = "1.8.0", default-features = false } qcheck-macros = { version = "1", default-features = false } diff --git a/radicle-crypto/src/ssh.rs b/radicle-crypto/src/ssh.rs index 0109b1f1..1f7e7e54 100644 --- a/radicle-crypto/src/ssh.rs +++ b/radicle-crypto/src/ssh.rs @@ -15,6 +15,65 @@ use crate::PublicKey; pub use keystore::{Keystore, Passphrase}; +#[derive(Debug, Error)] +pub enum ExtendedSignatureError { + #[error(transparent)] + Ssh(#[from] ssh_key::Error), + #[error(transparent)] + Crypto(#[from] crypto::Error), + #[error("unsupported signature algorithm")] + UnsupportedAlgorithm, +} + +/// Signature with public key, used for SSH signing. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ExtendedSignature { + pub key: crypto::PublicKey, + pub sig: crypto::Signature, +} + +impl ExtendedSignature { + /// Create a new extended signature. + pub fn new(public_key: crypto::PublicKey, signature: crypto::Signature) -> Self { + Self { + key: public_key, + sig: signature, + } + } + + /// Convert to OpenSSH standard PEM format. + pub fn to_pem(&self) -> Result { + ssh_key::SshSig::new( + ssh_key::public::KeyData::from(ssh_key::public::Ed25519PublicKey(**self.key)), + String::from("radicle"), + ssh_key::HashAlg::Sha256, + ssh_key::Signature::new(ssh_key::Algorithm::Ed25519, **self.sig)?, + )? + .to_pem(ssh_key::LineEnding::default()) + .map_err(ExtendedSignatureError::from) + } + + /// Create from OpenSSH PEM format. + pub fn from_pem(pem: impl AsRef<[u8]>) -> Result { + let sig = ssh_key::SshSig::from_pem(pem)?; + + Ok(Self { + key: crypto::PublicKey::from( + sig.public_key() + .ed25519() + .ok_or(ExtendedSignatureError::UnsupportedAlgorithm)? + .0, + ), + sig: crypto::Signature::try_from(sig.signature().as_bytes())?, + }) + } + + /// Verify the signature for a given payload. + pub fn verify(&self, payload: &[u8]) -> bool { + self.key.verify(payload, &self.sig).is_ok() + } +} + pub mod fmt { use crate::PublicKey; @@ -179,139 +238,12 @@ impl Encodable for crypto::SecretKey { } } -#[derive(Debug, Error)] -pub enum ExtendedSignatureError { - #[error(transparent)] - Base64Encoding(#[from] base64::DecodeError), - #[error("wrong preamble")] - MagicPreamble([u8; 6]), - #[error("missing armored footer")] - MissingFooter, - #[error("missing armored header")] - MissingHeader, - #[error(transparent)] - Encoding(#[from] encoding::Error), - #[error(transparent)] - PublicKey(#[from] PublicKeyError), - #[error(transparent)] - SignatureError(#[from] SignatureError), - #[error("unsupported version '{0}'")] - UnsupportedVersion(u32), -} - -/// An SSH signature's decoded format. -/// -/// See -#[derive(Clone, Debug)] -pub struct ExtendedSignature { - version: u32, - public_key: crypto::PublicKey, - /// Unambigious interpretation domain to prevent cross-protocol attacks. - namespace: Vec, - reserved: Vec, - /// Hash used for signature. For example 'sha256'. - hash_algorithm: Vec, - signature: crypto::Signature, -} - -impl From for (crypto::PublicKey, crypto::Signature) { - fn from(ex: ExtendedSignature) -> Self { - (ex.public_key, ex.signature) - } -} - -impl Encodable for ExtendedSignature { - type Error = ExtendedSignatureError; - - 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 { - 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(r)?, - }) - } - - 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(buf); - } -} - -impl ExtendedSignature { - const ARMORED_HEADER: &[u8] = b"-----BEGIN SSH SIGNATURE-----"; - const ARMORED_FOOTER: &[u8] = b"-----END SSH SIGNATURE-----"; - const ARMORED_WIDTH: usize = 70; - const MAGIC_PREAMBLE: &[u8] = b"SSHSIG"; - - pub fn new(public_key: crypto::PublicKey, signature: crypto::Signature) -> Self { - Self { - version: 1, - public_key, - namespace: b"radicle".to_vec(), - reserved: b"".to_vec(), - hash_algorithm: b"sha256".to_vec(), - signature, - } - } - - pub fn from_armored(s: &[u8]) -> Result { - let s = s - .strip_prefix(Self::ARMORED_HEADER) - .ok_or(ExtendedSignatureError::MissingHeader)?; - let s = s - .strip_suffix(Self::ARMORED_FOOTER) - .ok_or(ExtendedSignatureError::MissingFooter)?; - let s: Vec = s.iter().filter(|b| *b != &b'\n').copied().collect(); - - let buf = base64::decode(s)?; - let mut reader = buf.reader(0); - - let preamble: [u8; 6] = reader.read_bytes()?; - if preamble != Self::MAGIC_PREAMBLE { - return Err(ExtendedSignatureError::MagicPreamble(preamble)); - } - - ExtendedSignature::read(&mut reader) - } - - pub fn to_armored(&self) -> Vec { - 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 = 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 - } -} - #[cfg(test)] mod test { use std::sync::{Arc, Mutex}; use qcheck_macros::quickcheck; - use super::{fmt, ExtendedSignature}; use crate as crypto; use crate::{PublicKey, SecretKey}; use radicle_ssh::agent::client::{AgentClient, ClientStream, Error}; @@ -406,50 +338,4 @@ mod test { expected.as_slice() ); } - - #[test] - fn test_signature_encode_decode() { - let armored: &[u8] = b"-----BEGIN SSH SIGNATURE----- -U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgvjrQogRxxLjzzWns8+mKJAGzEX -4fm2ALoN7pyvD2ttQAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5 -AAAAQI84aPZsXxlQigpy1/Y/iJSmHSS//CIgvqvUMQIb/TM2vhCKruduH0cK02k9G8wOI+ -EUMf2bSDyxbJyZThOEiAs= ------END SSH SIGNATURE-----"; - - let public_key = - "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIL460KIEccS4881p7PPpiiQBsxF+H5tgC6De6crw9rbU"; - let signature = ExtendedSignature::from_armored(armored).unwrap(); - - 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(); - } }