From 0e7ffcc9167b2b549c86ef0da30f388a94811589 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Fri, 14 Apr 2023 16:41:21 +0200 Subject: [PATCH] node: Properly validate signed refs on fetch With this change, we not only verify the signed refs before the repository "transfer", but also validate that all refs in the repo are signed and there is no discrepancy with the signed refs file. --- radicle-node/src/test/environment.rs | 8 ++- radicle-node/src/test/simulator.rs | 2 +- radicle-node/src/tests/e2e.rs | 90 +++++++++++++++++++++++++- radicle-node/src/worker/fetch.rs | 58 +++++++++++------ radicle-node/src/worker/fetch/error.rs | 2 + radicle/src/node.rs | 1 + radicle/src/storage.rs | 16 +++-- radicle/src/storage/git.rs | 85 ++++++++---------------- radicle/src/test/fixtures.rs | 16 +++-- radicle/src/test/storage.rs | 2 +- 10 files changed, 187 insertions(+), 93 deletions(-) diff --git a/radicle-node/src/test/environment.rs b/radicle-node/src/test/environment.rs index 93814f8b..8fdeae21 100644 --- a/radicle-node/src/test/environment.rs +++ b/radicle-node/src/test/environment.rs @@ -22,7 +22,7 @@ use radicle::node::Handle as _; use radicle::profile::Home; use radicle::profile::Profile; use radicle::rad; -use radicle::storage::ReadStorage as _; +use radicle::storage::{ReadStorage as _, WriteRepository}; use radicle::test::fixtures; use radicle::Storage; @@ -366,6 +366,12 @@ impl + Signer + Clone> Node { } git::push(repo, "rad", refs.iter().map(|(a, b)| (a, b))).unwrap(); + self.storage + .repository(id) + .unwrap() + .sign_refs(&self.signer) + .unwrap(); + id } diff --git a/radicle-node/src/test/simulator.rs b/radicle-node/src/test/simulator.rs index 60b4166a..3b283ad6 100644 --- a/radicle-node/src/test/simulator.rs +++ b/radicle-node/src/test/simulator.rs @@ -699,7 +699,7 @@ fn fetch( remote.fetch(&[refspec], Some(&mut opts), None)?; drop(opts); - repo.verify()?; + repo.validate()?; repo.set_head()?; Ok(updates) diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index 2d142904..a1a2b965 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/radicle-node/src/tests/e2e.rs @@ -1,8 +1,9 @@ use std::{collections::HashSet, thread, time}; use radicle::crypto::{test::signer::MockSigner, Signer}; +use radicle::git; use radicle::node::{FetchResult, Handle as _}; -use radicle::storage::{ReadRepository, ReadStorage}; +use radicle::storage::{ReadRepository, ReadStorage, WriteRepository, WriteStorage}; use radicle::test::fixtures; use radicle::{assert_matches, rad}; @@ -194,7 +195,90 @@ fn test_replication() { assert_eq!(inventory.first(), Some(&acme)); assert_eq!(alice_refs, bob_refs); - assert_matches!(alice.storage.repository(acme).unwrap().verify(), Ok(())); + assert_matches!(alice.storage.repository(acme).unwrap().validate(), Ok(())); +} + +#[test] +fn test_replication_no_delegates() { + logger::init(log::Level::Debug); + + let tmp = tempfile::tempdir().unwrap(); + let alice = Node::init(tmp.path()); + let mut bob = Node::init(tmp.path()); + let (repo, _) = fixtures::repository(tmp.path().join("acme")); + + let acme = bob.project_from("acme", "", &repo); + + // Populate repo, but don't sign the refs. + let branches = fixtures::populate(&repo, 1); + git::push(&repo, "rad", branches.iter().map(|b| (b, b))).unwrap(); + + let mut alice = alice.spawn(service::Config::default()); + let bob = bob.spawn(service::Config::default()); + + alice.connect(&bob); + converge([&alice, &bob]); + + alice.handle.track_repo(acme, Scope::All).unwrap(); + let result = alice.handle.fetch(acme, bob.id).unwrap(); + + assert_matches!( + result, + FetchResult::Failed { + reason + } if reason == "no delegates in transfer" + ); +} + +#[test] +fn test_replication_invalid() { + let tmp = tempfile::tempdir().unwrap(); + let alice = Node::init(tmp.path()); + let mut bob = Node::init(tmp.path()); + let carol = MockSigner::default(); + let acme = bob.project("acme", ""); + let repo = bob.storage.repository_mut(acme).unwrap(); + let (_, head) = repo.head().unwrap(); + let id = repo.identity_head().unwrap(); + + // Create some unsigned refs for Carol in Bob's storage. + repo.raw() + .reference( + &git::qualified!("refs/heads/carol").with_namespace(carol.public_key().into()), + *head, + true, + &String::default(), + ) + .unwrap(); + repo.raw() + .reference( + &git::refs::storage::id(carol.public_key()), + id.into(), + true, + &String::default(), + ) + .unwrap(); + + let mut alice = alice.spawn(service::Config::default()); + let bob = bob.spawn(service::Config::default()); + + alice.connect(&bob); + converge([&alice, &bob]); + + alice.handle.track_node(*carol.public_key(), None).unwrap(); + alice.handle.track_repo(acme, Scope::Trusted).unwrap(); + let result = alice.handle.fetch(acme, bob.id).unwrap(); + + // Fetch is successful despite not fetching Carol's refs, since she isn't a delegate. + assert!(result.is_success()); + + let repo = alice.storage.repository(acme).unwrap(); + let mut remotes = repo.remote_ids().unwrap(); + + assert_eq!(remotes.next().unwrap().unwrap(), bob.id); + assert!(remotes.next().is_none()); + + repo.validate().unwrap(); } #[test] @@ -244,7 +328,7 @@ fn test_migrated_clone() { .unwrap(); assert_eq!(alice_refs, bob_refs); - assert_matches!(alice.storage.repository(acme).unwrap().verify(), Ok(())); + assert_matches!(alice.storage.repository(acme).unwrap().validate(), Ok(())); } #[test] diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index 613eb620..132c8548 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/radicle-node/src/worker/fetch.rs @@ -258,6 +258,8 @@ impl<'a> StagingPhaseFinal<'a> { .into_iter() .flat_map(|(remote, verified)| match verified { VerifiedRemote::Failed { reason } => { + // TODO: We should include the skipped remotes in the fetch result, + // with the reason why they're skipped. log::warn!( target: "worker", "{remote} failed to verify, will not fetch any further refs: {reason}", @@ -276,21 +278,23 @@ impl<'a> StagingPhaseFinal<'a> { for refname in [heads, cobs, tags, notes] { let pattern = refname.with_pattern(git::refspec::STAR); - refspecs.push( + refspecs.push(( + remote.id, Refspec { src: pattern.clone(), dst: pattern, force: true, } .to_string(), - ); + )); } // Then add the special refs. let id = ns.join(&*radicle::git::refs::storage::IDENTITY_BRANCH); let sigrefs = ns.join(&*radicle::git::refs::storage::SIGREFS_BRANCH); - refspecs.push( + refspecs.push(( + remote.id, Refspec { src: id.clone(), dst: id, @@ -298,8 +302,9 @@ impl<'a> StagingPhaseFinal<'a> { force: true, } .to_string(), - ); - refspecs.push( + )); + refspecs.push(( + remote.id, Refspec { src: sigrefs.clone(), dst: sigrefs, @@ -307,11 +312,22 @@ impl<'a> StagingPhaseFinal<'a> { force: false, } .to_string(), - ); + )); refspecs } }) .collect::>(); + + let (fetching, specs): (HashSet<_>, Vec<_>) = specs.into_iter().unzip(); + + if !self + .repo + .delegates()? + .iter() + .all(|d| fetching.contains(d.as_key())) + { + return Err(error::Transfer::NoDelegates); + } log::debug!(target: "worker", "Transferring staging to production {url}"); let mut opts = git::raw::FetchOptions::default(); @@ -349,21 +365,21 @@ impl<'a> StagingPhaseFinal<'a> { self.trusted .iter() .map(|remote| { - let verification = match ( - self.repo.identity_doc_of(remote), - SignedRefs::load(remote, self.repo.deref()), - ) { - (Ok(doc), Ok(refs)) => VerifiedRemote::Success { - _doc: doc, - remote: Remote::new(*remote, refs), - }, - (Err(e), _) => VerifiedRemote::Failed { - reason: e.to_string(), - }, - (_, Err(e)) => VerifiedRemote::Failed { - reason: e.to_string(), - }, - }; + let verification = + match (self.repo.identity_doc_of(remote), self.repo.remote(remote)) { + (Ok(doc), Ok(remote)) => match self.repo.validate_remote(&remote) { + Ok(()) => VerifiedRemote::Success { _doc: doc, remote }, + Err(e) => VerifiedRemote::Failed { + reason: e.to_string(), + }, + }, + (Err(e), _) => VerifiedRemote::Failed { + reason: e.to_string(), + }, + (_, Err(e)) => VerifiedRemote::Failed { + reason: e.to_string(), + }, + }; (*remote, verification) }) .collect() diff --git a/radicle-node/src/worker/fetch/error.rs b/radicle-node/src/worker/fetch/error.rs index 066063b8..abf8eff9 100644 --- a/radicle-node/src/worker/fetch/error.rs +++ b/radicle-node/src/worker/fetch/error.rs @@ -30,6 +30,8 @@ pub enum Transfer { Identity(#[from] identity::IdentityError), #[error(transparent)] Storage(#[from] storage::Error), + #[error("no delegates in transfer")] + NoDelegates, } #[derive(Debug, Error)] diff --git a/radicle/src/node.rs b/radicle/src/node.rs index 39c78e3d..dc06eed1 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -252,6 +252,7 @@ impl Seeds { #[serde(tag = "status", rename_all = "kebab-case")] pub enum FetchResult { Success { updated: Vec }, + // TODO: Create enum for reason. Failed { reason: String }, } diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index 12223eff..91dacc94 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -296,10 +296,18 @@ pub trait ReadRepository { fn blob_at<'a>(&'a self, commit: Oid, path: &'a Path) -> Result, git_ext::Error>; - /// Verify all references in the repository, checking that they are signed - /// as part of 'sigrefs'. Also verify that no signed reference is missing - /// from the repository. - fn verify(&self) -> Result<(), VerifyError>; + /// Validate all remotes with [`ReadStorage::validate`]. + fn validate(&self) -> Result<(), VerifyError> { + for (_, remote) in self.remotes()? { + self.validate_remote(&remote)?; + } + Ok(()) + } + + /// Validate all the remote's references in the repository, checking that they are signed as + /// part of the remote's signed refs. Also verify that no signed reference is missing from the + /// repository. + fn validate_remote(&self, remote: &Remote) -> Result<(), VerifyError>; /// Get the head of this repository. /// diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 77902d33..2ffd02e2 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -208,6 +208,8 @@ pub enum VerifyError { MissingRef(RemoteId, git::RefString), #[error("git: {0}")] Git(#[from] git2::Error), + #[error(transparent)] + Storage(#[from] Error), } impl Repository { @@ -346,33 +348,6 @@ impl Repository { }); Ok(remotes) } - - /// Return all references that are namespaced, ie. that are signed by a node and verified. - fn namespaced_references( - &self, - ) -> Result>, git2::Error> - { - let refs = self.backend.references_glob("refs/namespaces/*")?; - let refs = refs - .map(|reference| { - let r = reference?; - let name = r.name().ok_or(refs::Error::InvalidRef)?; - let (namespace, refname) = git::parse_ref_namespaced::(name)?; - let Some(oid) = r.target() else { - // Ignore symbolic refs, eg. `HEAD`. - return Ok(None); - }; - - if refname == *refs::SIGREFS_BRANCH { - // Ignore the signed-refs reference, as this is what we're verifying. - return Ok(None); - } - Ok(Some((namespace, refname.to_owned(), oid.into()))) - }) - .filter_map(Result::transpose); - - Ok(refs) - } } impl ReadRepository for Repository { @@ -396,39 +371,32 @@ impl ReadRepository for Repository { .get(&self.backend) } - fn verify(&self) -> Result<(), VerifyError> { - let mut remotes: HashMap = self - .remotes()? - .map(|remote| { - let (id, remote) = remote?; - Ok((id, remote.refs.into())) - }) - .collect::>()?; + fn validate_remote(&self, remote: &Remote) -> Result<(), VerifyError> { + // Contains a copy of the signed refs of this remote. + let mut refs = BTreeMap::from((*remote.refs).clone()); - for entry in self.namespaced_references()? { - let (remote_id, refname, oid) = entry?; - let remote = remotes - .get_mut(&remote_id) - .ok_or(VerifyError::InvalidRemote(remote_id))?; - let refname = RefString::from(refname); - let signed_oid = remote + // Check all repository references, making sure they are present in the signed refs map. + for (refname, oid) in self.references_of(&remote.id)? { + // Skip validation of the signed refs branch, as it is not part of `Remote`. + if refname == refs::SIGREFS_BRANCH.to_ref_string() { + continue; + } + let signed_oid = refs .remove(&refname) - .ok_or_else(|| VerifyError::UnknownRef(remote_id, refname.clone()))?; + .ok_or_else(|| VerifyError::UnknownRef(remote.id, refname.clone()))?; if oid != signed_oid { - return Err(VerifyError::InvalidRefTarget(remote_id, refname, *oid)); + return Err(VerifyError::InvalidRefTarget(remote.id, refname, *oid)); } } - for (remote, refs) in remotes.into_iter() { - // The refs that are left in the map, are ones that were signed, but are not - // in the repository. - if let Some((name, _)) = refs.into_iter().next() { - return Err(VerifyError::MissingRef(remote, name)); - } - // Verify identity history of remote. - self.identity_of(&remote)?.verified(self.id)?; + // The refs that are left in the map, are ones that were signed, but are not + // in the repository. If any are left, bail. + if let Some((name, _)) = refs.into_iter().next() { + return Err(VerifyError::MissingRef(remote.id, name)); } + // Finally, verify the identity history of remote. + self.identity_of(&remote.id)?.verified(self.id)?; Ok(()) } @@ -727,7 +695,7 @@ mod tests { } #[test] - fn test_namespaced_references() { + fn test_references_of() { let tmp = tempfile::tempdir().unwrap(); let signer = MockSigner::default(); let storage = Storage::open(tmp.path().join("storage")).unwrap(); @@ -739,14 +707,17 @@ mod tests { let proj = storage.repository(id).unwrap(); let mut refs = proj - .namespaced_references() + .references_of(signer.public_key()) .unwrap() - .map(|r| r.unwrap()) - .map(|(_, r, _)| r.to_string()) + .iter() + .map(|(r, _)| r.to_string()) .collect::>(); refs.sort(); - assert_eq!(refs, vec!["refs/heads/master", "refs/rad/id"]); + assert_eq!( + refs, + vec!["refs/heads/master", "refs/rad/id", "refs/rad/sigrefs"] + ); } #[test] diff --git a/radicle/src/test/fixtures.rs b/radicle/src/test/fixtures.rs index c20cc4b2..ef411c5a 100644 --- a/radicle/src/test/fixtures.rs +++ b/radicle/src/test/fixtures.rs @@ -86,24 +86,26 @@ pub fn repository>(path: P) -> (git2::Repository, git2::Oid) { } /// Populate a repository with commits, branches and blobs. -pub fn populate(repo: &git2::Repository, scale: usize) { +pub fn populate(repo: &git2::Repository, scale: usize) -> Vec { assert!( scale <= 8, "Scale parameter must be less than or equal to 8" ); if scale == 0 { - return; + return vec![]; } let head = repo.head().unwrap().peel_to_commit().unwrap(); let rng = fastrand::Rng::with_seed(42); let mut buffer = vec![0; 1024 * 1024 * scale]; + let mut refs = Vec::new(); for _ in 0..scale { let random = std::iter::repeat_with(|| rng.alphanumeric()) .take(7) .collect::() .to_lowercase(); - let name = format!("feature/{random}"); + let name = + git::refname!("feature").join(git::RefString::try_from(random.as_str()).unwrap()); let signature = git2::Signature::now("Radicle", "radicle@radicle.xyz").unwrap(); rng.fill(&mut buffer); @@ -113,17 +115,21 @@ pub fn populate(repo: &git2::Repository, scale: usize) { builder.insert("random.txt", blob, 0o100_644).unwrap(); let tree_oid = builder.write().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); + let refstr = git::refs::workdir::branch(&name); repo.commit( - Some(&format!("refs/heads/{name}")), + Some(&refstr), &signature, &signature, - &format!("Initialize new branch feature/{random}"), + &format!("Initialize new branch {name}"), &tree, &[&head], ) .unwrap(); + + refs.push(git::Qualified::from_refstr(refstr).unwrap()); } + refs } /// Generate random fixtures. diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index b1469845..5f54d2ef 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -140,7 +140,7 @@ impl ReadRepository for MockRepository { todo!() } - fn verify(&self) -> Result<(), VerifyError> { + fn validate_remote(&self, _remote: &Remote) -> Result<(), VerifyError> { Ok(()) }