mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

radicle: Fix unverified `Remote::new` constructor 

We were allowing remotes to be constructed via a function that didn't
check its inputs.

This is now fixed by moving the public key into the `SignedRefs` struct.
This commit is contained in:
Alexis Sellier 2023-04-26 10:57:26 +02:00
parent ccbfe5b6fa
commit df25e9a2a5
No known key found for this signature in database
10 changed files with 75 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
radicle-node/src/service.rs
View File

@ -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!(

15
radicle-node/src/service/message.rs
View File

@ -147,7 +147,7 @@ pub struct RefsAnnouncement {
/// Repository identifier.
pub rid: Id,
/// Updated refs.
pub refs: BoundedVec<(NodeId, SignedRefs<Unverified>), REF_REMOTE_LIMIT>,
pub refs: BoundedVec<SignedRefs<Unverified>, 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,

2
radicle-node/src/test/peer.rs
View File

@ -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;
}

2
radicle-node/src/tests.rs
View File

@ -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());

4
radicle-node/src/wire.rs
View File

@ -429,6 +429,7 @@ impl<V> Encode for SignedRefs<V> {
fn encode<W: io::Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
let mut n = 0;
n += self.id.encode(writer)?;
n += self.refs.encode(writer)?;
n += self.signature.encode(writer)?;
@ -438,10 +439,11 @@ impl<V> Encode for SignedRefs<V> {
impl Decode for SignedRefs<Unverified> {
fn decode<R: io::Read + ?Sized>(reader: &mut R) -> Result<Self, Error> {
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))
}
}

31
radicle/src/storage.rs
View File

@ -205,8 +205,6 @@ impl<V> From<Remotes<V>> for HashMap<RemoteId, Refs> {
/// A project remote.
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
pub struct Remote<V = Verified> {
/// ID of remote.
pub id: PublicKey,
/// Git references published under this remote, and their hashes.
#[serde(flatten)]
pub refs: SignedRefs<V>,
@ -214,13 +212,10 @@ pub struct Remote<V = Verified> {
pub delegate: bool,
}
impl<V> Remote<V> {
// 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::<Verified>::new(id, refs) and because of the above, it's a LIE
pub fn new(id: PublicKey, refs: impl Into<SignedRefs<V>>) -> Self {
impl Remote<Unverified> {
/// Create a new unverified remotes object.
pub fn new(refs: impl Into<SignedRefs<Unverified>>) -> Self {
Self {
id,
refs: refs.into(),
delegate: false,
}
@ -229,10 +224,9 @@ impl<V> Remote<V> {
impl Remote<Unverified> {
pub fn verified(self) -> Result<Remote<Verified>, 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<Unverified> {
}
impl Remote<Verified> {
/// Create a new unverified remotes object.
pub fn new(refs: impl Into<SignedRefs<Verified>>) -> Self {
Self {
refs: refs.into(),
delegate: false,
}
}
pub fn unverified(self) -> Remote<Unverified> {
Remote {
id: self.id,
refs: self.refs.unverified(),
delegate: self.delegate,
}
}
}
impl<V> Deref for Remote<V> {
type Target = SignedRefs<V>;
fn deref(&self) -> &Self::Target {
&self.refs
}
}
/// Read-only operations on a storage instance.
pub trait ReadStorage {
type Repository: ReadRepository;

4
radicle/src/storage/git.rs
View File

@ -436,8 +436,8 @@ impl ReadRepository for Repository {
}
fn remote(&self, remote: &RemoteId) -> Result<Remote<Verified>, refs::Error> {
let refs = SignedRefs::load(remote, self)?;
Ok(Remote::new(*remote, refs))
let refs = SignedRefs::load(*remote, self)?;
Ok(Remote::<Verified>::new(refs))
}
fn references_of(&self, remote: &RemoteId) -> Result<Refs, Error> {

25
radicle/src/storage/refs.rs
View File

@ -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<SignedRefs<Verified>, 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<V> {
/// 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<V>,
}
impl SignedRefs<Unverified> {
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<SignedRefs<Verified>, crypto::Error> {
match self.verify(signer) {
pub fn verified(self) -> Result<SignedRefs<Verified>, 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<Unverified> {
}
impl SignedRefs<Verified> {
pub fn load<S>(remote: &RemoteId, repo: &S) -> Result<Self, Error>
pub fn load<S>(remote: RemoteId, repo: &S) -> Result<Self, Error>
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<S>(oid: Oid, remote: &RemoteId, repo: &S) -> Result<Self, Error>
pub fn load_at<S>(oid: Oid, remote: RemoteId, repo: &S) -> Result<Self, Error>
where
S: storage::ReadRepository,
{
@ -262,6 +271,7 @@ impl SignedRefs<Verified> {
Ok(Self {
refs,
signature,
id: remote,
_verified: PhantomData,
})
}
@ -335,6 +345,7 @@ impl SignedRefs<Verified> {
SignedRefs {
refs: self.refs,
signature: self.signature,
id: self.id,
_verified: PhantomData,
}
}

7
radicle/src/test/arbitrary.rs
View File

@ -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<Unverified> {
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<crypto::Verified> {
let signer = MockSigner::arbitrary(g);
let signed = refs.signed(&signer).unwrap();
storage::Remote::new(*signer.public_key(), signed)
storage::Remote::<crypto::Verified>::new(signed)
}
}

19
radicle/src/test/storage.rs
View File

@ -151,10 +151,13 @@ impl ReadRepository for MockRepository {
todo!()
}
fn remote(&self, remote: &RemoteId) -> Result<Remote<Verified>, refs::Error> {
fn remote(&self, id: &RemoteId) -> Result<Remote<Verified>, 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())
}