diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index b2eb9afe..f4f4655b 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -868,9 +868,9 @@ where } // Process a peer inventory update announcement by (maybe) fetching. AnnouncementMessage::Refs(message) => { - for (remote_id, theirs) in message.refs.iter() { - if theirs.verify(remote_id).is_err() { - warn!(target: "service", "Peer {relayer} relayed refs announcement with invalid signature for {remote_id}"); + for theirs in message.refs.iter() { + if theirs.verify(&theirs.id).is_err() { + warn!(target: "service", "Peer {relayer} relayed refs announcement with invalid signature for {}", theirs.id); return Err(session::Error::Misbehavior); } } @@ -1033,10 +1033,7 @@ where trusted.remove(&self.node_id()); // Check if there is at least one trusted ref. - Ok(message - .refs - .iter() - .any(|(pub_key, _refs)| trusted.contains(pub_key))) + Ok(message.refs.iter().any(|refs| trusted.contains(&refs.id))) } Err(NamespacesError::NoTrusted { rid }) => { debug!(target: "service", "No trusted nodes to fetch {}", &rid); @@ -1212,7 +1209,7 @@ where for remote_id in remotes.into_iter() { if refs - .push((remote_id, repo.remote(&remote_id)?.refs.unverified())) + .push(repo.remote(&remote_id)?.refs.unverified()) .is_err() { warn!( diff --git a/radicle-node/src/service/message.rs b/radicle-node/src/service/message.rs index 306f6a49..85cae4ca 100644 --- a/radicle-node/src/service/message.rs +++ b/radicle-node/src/service/message.rs @@ -147,7 +147,7 @@ pub struct RefsAnnouncement { /// Repository identifier. pub rid: Id, /// Updated refs. - pub refs: BoundedVec<(NodeId, SignedRefs), REF_REMOTE_LIMIT>, + pub refs: BoundedVec, REF_REMOTE_LIMIT>, /// Time of announcement. pub timestamp: Timestamp, } @@ -163,8 +163,8 @@ impl RefsAnnouncement { Ok(r) => r, }; - for (remote_id, theirs) in self.refs.iter() { - if let Ok(ours) = repo.remote(remote_id) { + for theirs in self.refs.iter() { + if let Ok(ours) = repo.remote(&theirs.id) { if *ours.refs != theirs.refs { return Ok(true); } @@ -188,7 +188,7 @@ impl RefsAnnouncement { Ok(r) => r, }; - if let Some((_, refs)) = self.refs.iter().find(|(nid, _)| nid == remote) { + if let Some(refs) = self.refs.iter().find(|refs| &refs.id == remote) { let local_refs = repo.remote(remote)?.refs.unverified(); return Ok(&local_refs == refs); } @@ -534,8 +534,7 @@ mod tests { assert_eq!(refs.capacity(), REF_REMOTE_LIMIT); for _ in 0..refs.capacity() { - refs.push((*signer.public_key(), signed_refs.clone())) - .unwrap(); + refs.push(signed_refs.clone()).unwrap(); } let msg: Message = AnnouncementMessage::from(RefsAnnouncement { @@ -588,9 +587,7 @@ mod tests { let signer = MockSigner::new(&mut fastrand::Rng::new()); let timestamp = 0; let signed_refs = refs.signed(&signer).unwrap(); - let refs = BoundedVec::collect_from( - &mut [(*signer.public_key(), signed_refs.unverified())].into_iter(), - ); + let refs = BoundedVec::collect_from(&mut [signed_refs.unverified()].into_iter()); let message = AnnouncementMessage::Refs(RefsAnnouncement { rid, refs, diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs index 35a22f63..b2bb7b01 100644 --- a/radicle-node/src/test/peer.rs +++ b/radicle-node/src/test/peer.rs @@ -279,7 +279,7 @@ where if let Ok(false) = repo.is_empty() { if let Ok(remotes) = repo.remotes() { for (remote_id, remote) in remotes.into_iter() { - if let Err(e) = refs.push((remote_id, remote.refs.unverified())) { + if let Err(e) = refs.push(remote.refs.unverified()) { debug!(target: "test", "Failed to push {remote_id} to refs: {e}"); break; } diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index 5d3de70d..ad4ad29a 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -1150,7 +1150,7 @@ fn test_refs_synced_event() { .unverified(); let ann = AnnouncementMessage::from(RefsAnnouncement { rid: acme, - refs: vec![(alice.id, refs)].try_into().unwrap(), + refs: vec![refs].try_into().unwrap(), timestamp: bob.timestamp(), }); let msg = ann.signed(bob.signer()); diff --git a/radicle-node/src/wire.rs b/radicle-node/src/wire.rs index 3c907beb..81c04ce3 100644 --- a/radicle-node/src/wire.rs +++ b/radicle-node/src/wire.rs @@ -429,6 +429,7 @@ impl Encode for SignedRefs { fn encode(&self, writer: &mut W) -> Result { let mut n = 0; + n += self.id.encode(writer)?; n += self.refs.encode(writer)?; n += self.signature.encode(writer)?; @@ -438,10 +439,11 @@ impl Encode for SignedRefs { impl Decode for SignedRefs { fn decode(reader: &mut R) -> Result { + let id = NodeId::decode(reader)?; let refs = Refs::decode(reader)?; let signature = Signature::decode(reader)?; - Ok(Self::new(refs, signature)) + Ok(Self::new(refs, id, signature)) } } diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index ea33f4de..4bcd0068 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -205,8 +205,6 @@ impl From> for HashMap { /// A project remote. #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct Remote { - /// ID of remote. - pub id: PublicKey, /// Git references published under this remote, and their hashes. #[serde(flatten)] pub refs: SignedRefs, @@ -214,13 +212,10 @@ pub struct Remote { pub delegate: bool, } -impl Remote { - // TODO(finto): This function seems out of place in the API for a couple of reasons: - // * The SignedRefs aren't guaranteed to be by the `id` - // * I could write `Remote::::new(id, refs) and because of the above, it's a LIE - pub fn new(id: PublicKey, refs: impl Into>) -> Self { +impl Remote { + /// Create a new unverified remotes object. + pub fn new(refs: impl Into>) -> Self { Self { - id, refs: refs.into(), delegate: false, } @@ -229,10 +224,9 @@ impl Remote { impl Remote { pub fn verified(self) -> Result, crypto::Error> { - let refs = self.refs.verified(&self.id)?; + let refs = self.refs.verified()?; Ok(Remote { - id: self.id, refs, delegate: self.delegate, }) @@ -240,15 +234,30 @@ impl Remote { } impl Remote { + /// Create a new unverified remotes object. + pub fn new(refs: impl Into>) -> Self { + Self { + refs: refs.into(), + delegate: false, + } + } + pub fn unverified(self) -> Remote { Remote { - id: self.id, refs: self.refs.unverified(), delegate: self.delegate, } } } +impl Deref for Remote { + type Target = SignedRefs; + + fn deref(&self) -> &Self::Target { + &self.refs + } +} + /// Read-only operations on a storage instance. pub trait ReadStorage { type Repository: ReadRepository; diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 95d1ca19..736668bb 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -436,8 +436,8 @@ impl ReadRepository for Repository { } fn remote(&self, remote: &RemoteId) -> Result, refs::Error> { - let refs = SignedRefs::load(remote, self)?; - Ok(Remote::new(*remote, refs)) + let refs = SignedRefs::load(*remote, self)?; + Ok(Remote::::new(refs)) } fn references_of(&self, remote: &RemoteId) -> Result { diff --git a/radicle/src/storage/refs.rs b/radicle/src/storage/refs.rs index 8af765d1..330569ac 100644 --- a/radicle/src/storage/refs.rs +++ b/radicle/src/storage/refs.rs @@ -70,7 +70,7 @@ impl Refs { /// Verify the given signature on these refs, and return [`SignedRefs`] on success. pub fn verified( self, - signer: &PublicKey, + signer: PublicKey, signature: Signature, ) -> Result, Error> { let refs = self; @@ -80,6 +80,7 @@ impl Refs { Ok(()) => Ok(SignedRefs { refs, signature, + id: signer, _verified: PhantomData, }), Err(e) => Err(e.into()), @@ -98,6 +99,7 @@ impl Refs { Ok(SignedRefs { refs, signature, + id: *signer.public_key(), _verified: PhantomData, }) } @@ -200,27 +202,34 @@ impl DerefMut for Refs { /// [`Unverified`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct SignedRefs { + /// The signed refs. pub refs: Refs, + /// The signature of the signer over the refs. #[serde(skip)] pub signature: Signature, + /// This is the remote under which these refs exist, and the public key of the signer. + pub id: PublicKey, + #[serde(skip)] _verified: PhantomData, } impl SignedRefs { - pub fn new(refs: Refs, signature: Signature) -> Self { + pub fn new(refs: Refs, id: PublicKey, signature: Signature) -> Self { Self { refs, signature, + id, _verified: PhantomData, } } - pub fn verified(self, signer: &PublicKey) -> Result, crypto::Error> { - match self.verify(signer) { + pub fn verified(self) -> Result, crypto::Error> { + match self.verify(&self.id) { Ok(()) => Ok(SignedRefs { refs: self.refs, signature: self.signature, + id: self.id, _verified: PhantomData, }), Err(e) => Err(e), @@ -238,16 +247,16 @@ impl SignedRefs { } impl SignedRefs { - pub fn load(remote: &RemoteId, repo: &S) -> Result + pub fn load(remote: RemoteId, repo: &S) -> Result where S: ReadRepository, { - let oid = repo.reference_oid(remote, &SIGREFS_BRANCH)?; + let oid = repo.reference_oid(&remote, &SIGREFS_BRANCH)?; SignedRefs::load_at(oid, remote, repo) } - pub fn load_at(oid: Oid, remote: &RemoteId, repo: &S) -> Result + pub fn load_at(oid: Oid, remote: RemoteId, repo: &S) -> Result where S: storage::ReadRepository, { @@ -262,6 +271,7 @@ impl SignedRefs { Ok(Self { refs, signature, + id: remote, _verified: PhantomData, }) } @@ -335,6 +345,7 @@ impl SignedRefs { SignedRefs { refs: self.refs, signature: self.signature, + id: self.id, _verified: PhantomData, } } diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 631021e4..0749f001 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -4,7 +4,7 @@ use std::ops::RangeBounds; use std::{iter, net}; use crypto::test::signer::MockSigner; -use crypto::{PublicKey, Signer, Unverified, Verified}; +use crypto::{PublicKey, Unverified, Verified}; use nonempty::NonEmpty; use qcheck::Arbitrary; @@ -137,9 +137,10 @@ impl Arbitrary for SignedRefs { fn arbitrary(g: &mut qcheck::Gen) -> Self { let bytes: [u8; 64] = Arbitrary::arbitrary(g); let signature = crypto::Signature::from(bytes); + let id = PublicKey::arbitrary(g); let refs = Refs::arbitrary(g); - Self::new(refs, signature) + Self::new(refs, id, signature) } } @@ -195,7 +196,7 @@ impl Arbitrary for storage::Remote { let signer = MockSigner::arbitrary(g); let signed = refs.signed(&signer).unwrap(); - storage::Remote::new(*signer.public_key(), signed) + storage::Remote::::new(signed) } } diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index b09eff65..cd4e51d9 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -151,10 +151,13 @@ impl ReadRepository for MockRepository { todo!() } - fn remote(&self, remote: &RemoteId) -> Result, refs::Error> { + fn remote(&self, id: &RemoteId) -> Result, refs::Error> { self.remotes - .get(remote) - .map(|refs| Remote::new(*remote, refs.clone())) + .get(id) + .map(|refs| Remote { + refs: refs.clone(), + delegate: false, + }) .ok_or(refs::Error::InvalidRef) } @@ -162,7 +165,15 @@ impl ReadRepository for MockRepository { Ok(self .remotes .iter() - .map(|(id, refs)| (*id, Remote::new(*id, refs.clone()))) + .map(|(id, refs)| { + ( + *id, + Remote { + refs: refs.clone(), + delegate: false, + }, + ) + }) .collect()) }