From a79ef67b2a6d391349064a6ba4b6fa6f8d875646 Mon Sep 17 00:00:00 2001
From: Alexis Sellier <alexis@radicle.xyz>
Date: Thu, 3 Nov 2022 10:12:22 +0100
Subject: [PATCH] Add `Signer::try_sign` method and cleanup usage

To support ssh-agent, add a signing method that can fail.
Also cleanup usage so that we always accept signer references.

Signed-off-by: Alexis Sellier <alexis@radicle.xyz>
---
 radicle-crypto/src/lib.rs           | 46 ++++++++++++-----------------
 radicle-crypto/src/test/signer.rs   |  6 +++-
 radicle-node/src/service/message.rs |  6 ++--
 radicle-node/src/test/gossip.rs     |  2 +-
 radicle-node/src/test/tests.rs      |  2 +-
 radicle/src/identity/project.rs     |  2 +-
 radicle/src/profile.rs              |  6 +++-
 radicle/src/rad.rs                  |  8 ++---
 radicle/src/storage.rs              |  2 +-
 radicle/src/storage/git.rs          |  8 ++---
 radicle/src/storage/refs.rs         | 10 ++++---
 radicle/src/test/fixtures.rs        | 13 ++------
 radicle/src/test/storage.rs         |  2 +-
 13 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/radicle-crypto/src/lib.rs b/radicle-crypto/src/lib.rs
index 85ae0fcc..116a8094 100644
--- a/radicle-crypto/src/lib.rs
+++ b/radicle-crypto/src/lib.rs
@@ -1,4 +1,3 @@
-use std::sync::Arc;
 use std::{fmt, ops::Deref, str::FromStr};
 
 use ed25519_compact as ed25519;
@@ -20,37 +19,30 @@ pub struct Verified;
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct Unverified;
 
+/// Error returned if signing fails, eg. due to an HSM or KMS.
+#[derive(Debug, Error)]
+#[error(transparent)]
+pub struct SignerError {
+    #[from]
+    source: Box<dyn std::error::Error + Send + Sync>,
+}
+
+impl SignerError {
+    pub fn new(source: impl std::error::Error + Send + Sync + 'static) -> Self {
+        Self {
+            source: Box::new(source),
+        }
+    }
+}
+
 pub trait Signer: Send + Sync {
     /// Return this signer's public/verification key.
     fn public_key(&self) -> &PublicKey;
     /// Sign a message and return the signature.
     fn sign(&self, msg: &[u8]) -> Signature;
-}
-
-impl<T> Signer for Arc<T>
-where
-    T: Signer + ?Sized,
-{
-    fn sign(&self, msg: &[u8]) -> Signature {
-        self.deref().sign(msg)
-    }
-
-    fn public_key(&self) -> &PublicKey {
-        self.deref().public_key()
-    }
-}
-
-impl<T> Signer for &T
-where
-    T: Signer + ?Sized,
-{
-    fn sign(&self, msg: &[u8]) -> Signature {
-        self.deref().sign(msg)
-    }
-
-    fn public_key(&self) -> &PublicKey {
-        self.deref().public_key()
-    }
+    /// Sign a message and return the signature, or fail if the signer was unable
+    /// to produce a signature.
+    fn try_sign(&self, msg: &[u8]) -> Result<Signature, SignerError>;
 }
 
 /// Cryptographic signature.
diff --git a/radicle-crypto/src/test/signer.rs b/radicle-crypto/src/test/signer.rs
index db518fb4..cc8fa277 100644
--- a/radicle-crypto/src/test/signer.rs
+++ b/radicle-crypto/src/test/signer.rs
@@ -1,4 +1,4 @@
-use crate::{KeyPair, PublicKey, SecretKey, Seed, Signature, Signer};
+use crate::{KeyPair, PublicKey, SecretKey, Seed, Signature, Signer, SignerError};
 
 #[derive(Debug, Clone)]
 pub struct MockSigner {
@@ -62,4 +62,8 @@ impl Signer for MockSigner {
     fn sign(&self, msg: &[u8]) -> Signature {
         self.sk.sign(msg, None).into()
     }
+
+    fn try_sign(&self, msg: &[u8]) -> Result<Signature, SignerError> {
+        Ok(self.sign(msg))
+    }
 }
diff --git a/radicle-node/src/service/message.rs b/radicle-node/src/service/message.rs
index af7e888c..dda34d9a 100644
--- a/radicle-node/src/service/message.rs
+++ b/radicle-node/src/service/message.rs
@@ -247,7 +247,7 @@ pub enum AnnouncementMessage {
 
 impl AnnouncementMessage {
     /// Sign this announcement message.
-    pub fn signed<S: crypto::Signer>(self, signer: S) -> Announcement {
+    pub fn signed<G: crypto::Signer>(self, signer: &G) -> Announcement {
         let msg = wire::serialize(&self);
         let signature = signer.sign(&msg);
 
@@ -407,11 +407,11 @@ impl Message {
         .into()
     }
 
-    pub fn node<S: crypto::Signer>(message: NodeAnnouncement, signer: S) -> Self {
+    pub fn node<G: crypto::Signer>(message: NodeAnnouncement, signer: &G) -> Self {
         AnnouncementMessage::from(message).signed(signer).into()
     }
 
-    pub fn inventory<S: crypto::Signer>(message: InventoryAnnouncement, signer: S) -> Self {
+    pub fn inventory<G: crypto::Signer>(message: InventoryAnnouncement, signer: &G) -> Self {
         AnnouncementMessage::from(message).signed(signer).into()
     }
 
diff --git a/radicle-node/src/test/gossip.rs b/radicle-node/src/test/gossip.rs
index b7908792..2360ff00 100644
--- a/radicle-node/src/test/gossip.rs
+++ b/radicle-node/src/test/gossip.rs
@@ -29,7 +29,7 @@ pub fn messages(count: usize, now: LocalTime, delta: LocalDuration) -> Vec<Messa
                 inventory: arbitrary::gen(3),
                 timestamp: time.as_secs(),
             },
-            signer,
+            &signer,
         ))
     }
     msgs
diff --git a/radicle-node/src/test/tests.rs b/radicle-node/src/test/tests.rs
index 6d8b25f8..1be06133 100644
--- a/radicle-node/src/test/tests.rs
+++ b/radicle-node/src/test/tests.rs
@@ -202,7 +202,7 @@ fn test_inventory_sync() {
         Storage::open(tmp.path().join("alice")).unwrap(),
     );
     let bob_signer = MockSigner::default();
-    let bob_storage = fixtures::storage(tmp.path().join("bob"), bob_signer).unwrap();
+    let bob_storage = fixtures::storage(tmp.path().join("bob"), &bob_signer).unwrap();
     let bob = Peer::new("bob", [8, 8, 8, 8], bob_storage);
     let now = LocalTime::now().as_secs();
     let projs = bob.storage().inventory().unwrap();
diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs
index a2337b8d..2d807ed6 100644
--- a/radicle/src/identity/project.rs
+++ b/radicle/src/identity/project.rs
@@ -137,7 +137,7 @@ impl Doc<Verified> {
         false
     }
 
-    pub fn sign<G: crypto::Signer>(&self, signer: G) -> Result<(git::Oid, Signature), DocError> {
+    pub fn sign<G: crypto::Signer>(&self, signer: &G) -> Result<(git::Oid, Signature), DocError> {
         let (oid, bytes) = self.encode()?;
         let sig = signer.sign(&bytes);
 
diff --git a/radicle/src/profile.rs b/radicle/src/profile.rs
index 3d9fe3f3..9deb79d7 100644
--- a/radicle/src/profile.rs
+++ b/radicle/src/profile.rs
@@ -15,7 +15,7 @@ use std::{env, io};
 
 use thiserror::Error;
 
-use crate::crypto::{KeyPair, PublicKey, SecretKey, Signature, Signer};
+use crate::crypto::{KeyPair, PublicKey, SecretKey, Signature, Signer, SignerError};
 use crate::keystore::UnsafeKeystore;
 use crate::node;
 use crate::storage::git::transport;
@@ -45,6 +45,10 @@ impl Signer for UnsafeSigner {
     fn sign(&self, msg: &[u8]) -> Signature {
         Signature(self.secret.sign(msg, None))
     }
+
+    fn try_sign(&self, msg: &[u8]) -> Result<Signature, SignerError> {
+        Ok(self.sign(msg))
+    }
 }
 
 #[derive(Debug)]
diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs
index 0049217a..a7fc93ed 100644
--- a/radicle/src/rad.rs
+++ b/radicle/src/rad.rs
@@ -47,7 +47,7 @@ pub fn init<G: Signer>(
     name: &str,
     description: &str,
     default_branch: BranchName,
-    signer: G,
+    signer: &G,
     storage: &Storage,
 ) -> Result<(Id, SignedRefs<Verified>), InitError> {
     // TODO: Better error when project id already exists in storage, but remote doesn't.
@@ -114,7 +114,7 @@ pub enum ForkError {
 pub fn fork_remote<G: Signer, S: storage::WriteStorage>(
     proj: Id,
     remote: &RemoteId,
-    signer: G,
+    signer: &G,
     storage: S,
 ) -> Result<(), ForkError> {
     // TODO: Copy tags over?
@@ -150,7 +150,7 @@ pub fn fork_remote<G: Signer, S: storage::WriteStorage>(
         &format!("creating identity branch for {me}"),
     )?;
 
-    repository.sign_refs(&signer)?;
+    repository.sign_refs(signer)?;
 
     Ok(())
 }
@@ -179,7 +179,7 @@ pub fn fork<G: Signer, S: storage::WriteStorage>(
         false,
         &format!("creating identity branch for {me}"),
     )?;
-    repository.sign_refs(&signer)?;
+    repository.sign_refs(signer)?;
 
     Ok(())
 }
diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs
index 6dad0c6d..b90d58af 100644
--- a/radicle/src/storage.rs
+++ b/radicle/src/storage.rs
@@ -304,7 +304,7 @@ pub trait WriteRepository: ReadRepository {
         namespaces: impl Into<Namespaces>,
     ) -> Result<Vec<RefUpdate>, FetchError>;
     fn set_head(&self) -> Result<Oid, ProjectError>;
-    fn sign_refs<G: Signer>(&self, signer: G) -> Result<SignedRefs<Verified>, Error>;
+    fn sign_refs<G: Signer>(&self, signer: &G) -> Result<SignedRefs<Verified>, Error>;
     fn raw(&self) -> &git2::Repository;
 }
 
diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs
index 6440d2aa..50d6b5f0 100644
--- a/radicle/src/storage/git.rs
+++ b/radicle/src/storage/git.rs
@@ -638,10 +638,10 @@ impl WriteRepository for Repository {
         Ok(head)
     }
 
-    fn sign_refs<G: Signer>(&self, signer: G) -> Result<SignedRefs<Verified>, Error> {
+    fn sign_refs<G: Signer>(&self, signer: &G) -> Result<SignedRefs<Verified>, Error> {
         let remote = signer.public_key();
         let refs = self.references(remote)?;
-        let signed = refs.signed(&signer)?;
+        let signed = refs.signed(signer)?;
 
         signed.save(remote, self)?;
 
@@ -751,7 +751,7 @@ mod tests {
         let tmp = tempfile::tempdir().unwrap();
         let alice_signer = MockSigner::default();
         let alice_pk = *alice_signer.public_key();
-        let alice = fixtures::storage(tmp.path().join("alice"), alice_signer).unwrap();
+        let alice = fixtures::storage(tmp.path().join("alice"), &alice_signer).unwrap();
         let bob = Storage::open(tmp.path().join("bob")).unwrap();
         let inventory = alice.inventory().unwrap();
         let proj = *inventory.first().unwrap();
@@ -893,7 +893,7 @@ mod tests {
             "radicle",
             "radicle",
             git::refname!("master"),
-            signer,
+            &signer,
             &storage,
         )
         .unwrap();
diff --git a/radicle/src/storage/refs.rs b/radicle/src/storage/refs.rs
index a498e146..a2676a47 100644
--- a/radicle/src/storage/refs.rs
+++ b/radicle/src/storage/refs.rs
@@ -7,7 +7,7 @@ use std::ops::{Deref, DerefMut};
 use std::path::Path;
 use std::str::FromStr;
 
-use crypto::{PublicKey, Signature, Signer, Unverified, Verified};
+use crypto::{PublicKey, Signature, Signer, SignerError, Unverified, Verified};
 use thiserror::Error;
 
 use crate::git;
@@ -36,6 +36,8 @@ pub enum Updated {
 pub enum Error {
     #[error("invalid signature: {0}")]
     InvalidSignature(#[from] crypto::Error),
+    #[error("signer error: {0}")]
+    Signer(#[from] SignerError),
     #[error("canonical refs: {0}")]
     Canonical(#[from] canonical::Error),
     #[error("invalid reference")]
@@ -84,13 +86,13 @@ impl Refs {
     }
 
     /// Sign these refs with the given signer and return [`SignedRefs`].
-    pub fn signed<S>(self, signer: S) -> Result<SignedRefs<Verified>, Error>
+    pub fn signed<G>(self, signer: &G) -> Result<SignedRefs<Verified>, Error>
     where
-        S: Signer,
+        G: Signer,
     {
         let refs = self;
         let msg = refs.canonical();
-        let signature = signer.sign(&msg);
+        let signature = signer.try_sign(&msg)?;
 
         Ok(SignedRefs {
             refs,
diff --git a/radicle/src/test/fixtures.rs b/radicle/src/test/fixtures.rs
index 691c54e6..4d708540 100644
--- a/radicle/src/test/fixtures.rs
+++ b/radicle/src/test/fixtures.rs
@@ -9,7 +9,7 @@ use crate::storage::git::Storage;
 use crate::storage::refs::SignedRefs;
 
 /// Create a new storage with a project.
-pub fn storage<P: AsRef<Path>, G: Signer>(path: P, signer: G) -> Result<Storage, rad::InitError> {
+pub fn storage<P: AsRef<Path>, G: Signer>(path: P, signer: &G) -> Result<Storage, rad::InitError> {
     let path = path.as_ref();
     let storage = Storage::open(path.join("storage"))?;
 
@@ -22,14 +22,7 @@ pub fn storage<P: AsRef<Path>, G: Signer>(path: P, signer: G) -> Result<Storage,
         ("rx", "A pixel editor"),
     ] {
         let (repo, _) = repository(path.join("workdir").join(name));
-        rad::init(
-            &repo,
-            name,
-            desc,
-            git::refname!("master"),
-            &signer,
-            &storage,
-        )?;
+        rad::init(&repo, name, desc, git::refname!("master"), signer, &storage)?;
     }
 
     Ok(storage)
@@ -39,7 +32,7 @@ pub fn storage<P: AsRef<Path>, G: Signer>(path: P, signer: G) -> Result<Storage,
 pub fn project<P: AsRef<Path>, G: Signer>(
     path: P,
     storage: &Storage,
-    signer: G,
+    signer: &G,
 ) -> Result<(Id, SignedRefs<Verified>, git2::Repository, git2::Oid), rad::InitError> {
     transport::local::register(storage.clone());
 
diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs
index 1796c8f9..58d973b2 100644
--- a/radicle/src/test/storage.rs
+++ b/radicle/src/test/storage.rs
@@ -151,7 +151,7 @@ impl WriteRepository for MockRepository {
 
     fn sign_refs<G: Signer>(
         &self,
-        _signer: G,
+        _signer: &G,
     ) -> Result<crate::storage::refs::SignedRefs<Verified>, Error> {
         todo!()
     }