diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b982ad7..0d7d539f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## Fixed Bugs +## New Features - Fix the signed references reading process by correctly choosing the first, non-replayed commit. This only occurs if duplicate signatures are found and the process needs to find the first legitimate commit of the namespace. +- Signed references now implement feature detection. The features are marked as + `none`, `root`, and `parent`. If the signed references are marked as `none` + this implies that they only contain `/refs` and `/signature` with no + `refs/rad/root` entry nor `refs/rad/sigrefs-parent`. The next feature is + `root`, which implies `none`, but includes `refs/rad/root`. Finally, `parent` + implies `root`, but includes `refs/rad/sigrefs-parent`. + This means that feature levels are monotonically increasing. This allows the + signed references processes to detect downgrades, preventing downgrade + attacks. + Note that this means that a node which upgrades, and subsequently downgrades + will appear as a downgrade attacker. ## 1.7.1 diff --git a/crates/radicle-cli/src/commands/ls.rs b/crates/radicle-cli/src/commands/ls.rs index f48ef295..387f4c81 100644 --- a/crates/radicle-cli/src/commands/ls.rs +++ b/crates/radicle-cli/src/commands/ls.rs @@ -28,14 +28,6 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { .. } in repos { - let refs = match refs { - Ok(refs) => refs, - Err(_) => { - term::warning(format!("Repository {rid} must be migrated by starting your node (e.g. via `rad node start`).")); - continue; - } - }; - if doc.is_public() && args.private { continue; } diff --git a/crates/radicle-protocol/src/service.rs b/crates/radicle-protocol/src/service.rs index 20a4bfb6..492adf36 100644 --- a/crates/radicle-protocol/src/service.rs +++ b/crates/radicle-protocol/src/service.rs @@ -37,7 +37,7 @@ use radicle::node::routing::Store as _; use radicle::node::seed; use radicle::node::seed::Store as _; use radicle::node::{Penalty, Severity}; -use radicle::storage::refs::SIGREFS_BRANCH; +use radicle::storage::refs::{FeatureLevel, SIGREFS_BRANCH}; use radicle::storage::RepositoryError; use radicle_fetch::policy::SeedingPolicy; @@ -523,14 +523,24 @@ where } fn upgrade_sigrefs(&mut self, info: &radicle::storage::RepositoryInfo) -> Result<(), Error> { - match &info.refs { - Ok(_) => return Ok(()), - Err(err) => { - log::info!("Migrating `rad/sigrefs` due to: {err}"); - } + let Some(ref refs) = info.refs else { + // No refs, nothing to upgrade. + return Ok(()); + }; + + if refs.feature_level() >= FeatureLevel::LATEST { + // Refs are at target level or above, nothing to upgrade. + return Ok(()); } + log::info!( + "Migrating `rad/sigrefs` from level {} which is lower than target level {}.", + refs.feature_level(), + FeatureLevel::LATEST + ); + let repo = self.storage.repository_mut(info.rid)?; + // NOTE: We assume to reach `FeatureLevel::MAX` by signing refs. repo.sign_refs(&self.signer)?; Ok(()) } diff --git a/crates/radicle/src/rad.rs b/crates/radicle/src/rad.rs index 9e49c22d..ca2994fd 100644 --- a/crates/radicle/src/rad.rs +++ b/crates/radicle/src/rad.rs @@ -563,7 +563,7 @@ mod tests { .unwrap(), ); - assert_eq!(remotes[&public_key].refs, refs); + assert_eq!(remotes[&public_key].refs.refs(), refs.refs()); assert_eq!(project.name(), "acme"); assert_eq!(project.description(), "Acme's repo"); assert_eq!(project.default_branch(), &git::fmt::refname!("master")); diff --git a/crates/radicle/src/storage.rs b/crates/radicle/src/storage.rs index 5164d0f7..8c7c50d5 100644 --- a/crates/radicle/src/storage.rs +++ b/crates/radicle/src/storage.rs @@ -42,7 +42,7 @@ pub struct RepositoryInfo { pub doc: Doc, /// Local signed refs, if any. /// Repositories with this set to `None` are ones that are seeded but not forked. - pub refs: Result, refs::sigrefs::read::error::MissingIdentity>, + pub refs: Option, /// Sync time of the repository. pub synced_at: Option, } diff --git a/crates/radicle/src/storage/git.rs b/crates/radicle/src/storage/git.rs index 28a1c171..039c6dd1 100644 --- a/crates/radicle/src/storage/git.rs +++ b/crates/radicle/src/storage/git.rs @@ -167,18 +167,12 @@ impl ReadStorage for Storage { } }; // Nb. This will be `None` if they were not found. - let refs = match refs::SignedRefsAt::load(self.info.key, &repo) { - Err(sigrefs::read::error::Read::Verify( - sigrefs::read::error::Verify::MissingIdentity(err), - )) => Err(err), - Err(err) => return Err(Error::Refs(refs::Error::Read(err))), - Ok(refs) => Ok(refs), - }; - - let synced_at = match &refs { - Ok(Some(refs)) => Some(node::SyncedAt::new(refs.at, &repo)?), - _ => None, - }; + let refs = refs::SignedRefsAt::load(self.info.key, &repo) + .map_err(|err| Error::Refs(refs::Error::Read(err)))?; + let synced_at = refs + .as_ref() + .map(|r| node::SyncedAt::new(r.at, &repo)) + .transpose()?; repos.push(RepositoryInfo { rid, @@ -264,22 +258,12 @@ impl Storage { let repo = self.repository(*rid)?; let (_, head) = repo.head()?; - let refs = match refs::SignedRefsAt::load(self.info.key, &repo) { - Err(refs::sigrefs::read::error::Read::Verify( - refs::sigrefs::read::error::Verify::MissingIdentity(err), - )) => Err(err), - Err(err) => { - return Err(RepositoryError::Storage(Error::Refs(refs::Error::Read( - err, - )))) - } - Ok(refs) => Ok(refs), - }; - - let synced_at = match &refs { - Ok(Some(refs)) => Some(SyncedAt::new(refs.at, &repo)?), - _ => None, - }; + let refs = refs::SignedRefsAt::load(self.info.key, &repo) + .map_err(|err| RepositoryError::Refs(refs::Error::Read(err)))?; + let synced_at = refs + .as_ref() + .map(|r| SyncedAt::new(r.at, &repo)) + .transpose()?; Ok(RepositoryInfo { rid: *rid, @@ -1209,7 +1193,7 @@ mod tests { // The signed refs doesn't contain the signature ref itself. unsigned.remove_sigrefs().unwrap(); - assert_eq!(remote.refs, signed); + assert_eq!(remote.refs.refs(), signed.refs()); assert_eq!(*remote.refs, unsigned); } } diff --git a/crates/radicle/src/storage/refs.rs b/crates/radicle/src/storage/refs.rs index 4507d215..c9c18750 100644 --- a/crates/radicle/src/storage/refs.rs +++ b/crates/radicle/src/storage/refs.rs @@ -20,6 +20,7 @@ use crate::git; use crate::git::raw::ErrorExt as _; use crate::git::Oid; use crate::storage; +use crate::storage::refs::sigrefs::read::Tip; use crate::storage::{ReadRepository, RemoteId}; pub use crate::git::refs::storage::*; @@ -81,6 +82,10 @@ impl Refs { R: sigrefs::git::object::Reader + sigrefs::git::object::Writer, R: sigrefs::git::reference::Reader + sigrefs::git::reference::Writer, { + // FIXME(lorenz): We promise this feature level to the caller, but + // have not actually verified it. + const LEVEL_PROMISED: FeatureLevel = FeatureLevel::Parent; + let msg = "Update signed refs\n"; let reflog = format!("Save {} signed references", self.len()); let update = sigrefs::write::SignedRefsWriter::new(self, namespace, repo, signer).write( @@ -89,7 +94,9 @@ impl Refs { reflog, )?; match update { - sigrefs::write::Update::Changed { entry } => Ok(entry.into_sigrefs_at(namespace)), + sigrefs::write::Update::Changed { entry } => { + Ok(entry.into_sigrefs_at(namespace, LEVEL_PROMISED)) + } sigrefs::write::Update::Unchanged { commit, refs, @@ -99,6 +106,7 @@ impl Refs { refs, signature, id: namespace, + level: LEVEL_PROMISED, }; Ok(SignedRefsAt { sigrefs, @@ -245,6 +253,42 @@ where } } +/// The Signed References feature has evolved over time. +/// This enum captures the corresponding "feature level". +/// +/// Feature levels are monotonic, in the sense that a greater feature level +/// encompasses all the features of smaller ones. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Default)] +#[non_exhaustive] +pub enum FeatureLevel { + /// References are stored without additional metadata. + #[default] + None, + /// Introduced in Radicle 1.1.0, in commit + /// `989edacd564fa658358f5ccfd08c243c5ebd8cda`, + /// this requires [`IDENTITY_ROOT`]. + Root, + /// Introduced in Radicle 1.7.0, in commit + /// `d3bc868e84c334f113806df1737f52cc57c5453d`, + /// this requires [`SIGREFS_PARENT`]. + Parent, +} + +impl FeatureLevel { + pub const LATEST: Self = FeatureLevel::Parent; +} + +impl std::fmt::Display for FeatureLevel { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let s = match &self { + Self::None => "none", + Self::Root => "root", + Self::Parent => "parent", + }; + f.write_str(s) + } +} + /// Combination of [`Refs`] and a [`Signature`]. The signature is a cryptographic /// signature over the refs. This allows us to easily verify if a set of refs /// came from a particular key. @@ -257,6 +301,9 @@ pub struct SignedRefs { signature: Signature, /// This is the remote under which these refs exist, and the public key of the signer. id: PublicKey, + + #[serde(skip)] + level: FeatureLevel, } impl SignedRefs { @@ -270,21 +317,17 @@ impl SignedRefs { &self.refs } + /// Returns the [`FeatureLevel`] computed for the signed references. + pub fn feature_level(&self) -> FeatureLevel { + self.level + } + pub fn load(remote: RemoteId, repo: &R) -> Result where R: HasRepoId, R: sigrefs::git::object::Reader + sigrefs::git::reference::Reader, { - let root = repo.rid(); - let tip = sigrefs::read::Tip::Reference(remote); - let latest = sigrefs::SignedRefsReader::new(root, tip, repo, &remote).read()?; - let signature = *latest.signature(); - let refs = latest.into_refs(); - Ok(SignedRefs { - refs, - signature, - id: remote, - }) + Self::load_internal(remote, repo, sigrefs::read::Tip::Reference(remote)) } pub fn load_at( @@ -292,20 +335,25 @@ impl SignedRefs { remote: RemoteId, repo: &R, ) -> Result + where + R: HasRepoId, + R: sigrefs::git::object::Reader + sigrefs::git::reference::Reader, + { + Self::load_internal(remote, repo, sigrefs::read::Tip::Commit(oid)) + } + + fn load_internal( + remote: RemoteId, + repo: &R, + tip: Tip, + ) -> Result where R: HasRepoId, R: sigrefs::git::object::Reader + sigrefs::git::reference::Reader, { let root = repo.rid(); - let tip = sigrefs::read::Tip::Commit(oid); let latest = sigrefs::SignedRefsReader::new(root, tip, repo, &remote).read()?; - let signature = *latest.signature(); - let refs = latest.into_refs(); - Ok(SignedRefs { - refs, - signature, - id: remote, - }) + Ok(latest.into_sigrefs_at(remote).sigrefs) } } diff --git a/crates/radicle/src/storage/refs/arbitrary.rs b/crates/radicle/src/storage/refs/arbitrary.rs index c5307ccb..9fbd30e3 100644 --- a/crates/radicle/src/storage/refs/arbitrary.rs +++ b/crates/radicle/src/storage/refs/arbitrary.rs @@ -13,7 +13,10 @@ where let mut refs = Refs::arbitrary(g); refs.insert(IDENTITY_ROOT.to_ref_string(), root); + let mut level = None; + if bool::arbitrary(g) { + level = Some(FeatureLevel::Parent); let parent = Oid::from_sha1(Arbitrary::arbitrary(g)); refs.insert(SIGREFS_PARENT.to_ref_string(), parent); } @@ -23,6 +26,7 @@ where refs, signature, id: *signer.node_id(), + level: level.unwrap_or_else(|| FeatureLevel::arbitrary(g)), }; SignedRefsAt { sigrefs, @@ -68,3 +72,10 @@ impl Arbitrary for RefsAt { } } } + +impl Arbitrary for FeatureLevel { + fn arbitrary(g: &mut qcheck::Gen) -> Self { + *g.choose(&[FeatureLevel::None, FeatureLevel::Root, FeatureLevel::Parent]) + .unwrap() + } +} diff --git a/crates/radicle/src/storage/refs/sigrefs/property.rs b/crates/radicle/src/storage/refs/sigrefs/property.rs index 715580b3..c33c0c82 100644 --- a/crates/radicle/src/storage/refs/sigrefs/property.rs +++ b/crates/radicle/src/storage/refs/sigrefs/property.rs @@ -53,7 +53,7 @@ fn roundtrip(BoundedVec(all_refs): BoundedVec) -> TestResult { Err(e) => return TestResult::error(format!("read error: {e}")), }; - if written_refs != verified.into_refs() { + if written_refs != *verified.commit().refs() { return TestResult::failed(); } } diff --git a/crates/radicle/src/storage/refs/sigrefs/read.rs b/crates/radicle/src/storage/refs/sigrefs/read.rs index e7ca2ab7..04de113d 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read.rs @@ -5,11 +5,11 @@ mod iter; #[cfg(test)] mod test; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::num::NonZeroUsize; use std::path::Path; -use crypto::signature; +use crypto::{signature, PublicKey}; use nonempty::NonEmpty; use radicle_core::{NodeId, RepoId}; use radicle_git_metadata::commit::CommitData; @@ -19,7 +19,8 @@ use crate::git; use crate::identity::doc; use crate::storage::refs::sigrefs::git::{object, reference}; use crate::storage::refs::{ - Refs, IDENTITY_ROOT, REFS_BLOB_PATH, SIGNATURE_BLOB_PATH, SIGREFS_BRANCH, + FeatureLevel, Refs, SignedRefs, SignedRefsAt, IDENTITY_ROOT, REFS_BLOB_PATH, + SIGNATURE_BLOB_PATH, SIGREFS_BRANCH, }; /// A `rad/sigrefs` that has passed the following verification checks: @@ -33,30 +34,31 @@ use crate::storage::refs::{ pub struct VerifiedCommit { /// The commit that was verified. commit: Commit, - /// Whether verification successfully found the correct - /// value for [`SIGREFS_PARENT`] in the refs of [`Self::commit`]. - parent: bool, + /// The feature level that was recognized for the commit that was verified. + level: FeatureLevel, } impl VerifiedCommit { - /// The [`Oid`] of the commit. - pub fn id(&self) -> &Oid { - &self.commit.oid + /// Borrow the [`Commit`] that was verified. + pub(super) fn commit(&self) -> &Commit { + &self.commit } - /// The [`crypto::Signature`] found in the tree of the commit. - pub fn signature(&self) -> &crypto::Signature { - &self.commit.signature + // The [`FeatureLevel`] of the refs in this commit. + pub fn level(&self) -> FeatureLevel { + self.level } - /// The [`Refs`] found in the tree of the commit. - pub fn into_refs(self) -> Refs { - self.commit.refs - } - - /// The parent [`Oid`] of the commit, unless it is the root commit. - pub fn parent(&self) -> Option<&Oid> { - self.commit.parent.as_ref() + pub(crate) fn into_sigrefs_at(self, id: PublicKey) -> SignedRefsAt { + SignedRefsAt { + sigrefs: SignedRefs { + refs: self.commit.refs, + signature: self.commit.signature, + id, + level: self.level, + }, + at: self.commit.oid, + } } } @@ -86,6 +88,44 @@ pub enum Tip { Commit(Oid), } +/// Describes the feature levels of a history of commits. +#[derive(Debug, PartialEq)] +pub struct FeatureLevels(BTreeMap); + +impl FeatureLevels { + fn new() -> Self { + Self(BTreeMap::new()) + } + + fn max(&self) -> FeatureLevel { + self.0.last_key_value().map(|(k, _)| *k).unwrap_or_default() + } + + fn insert(&mut self, verified: &VerifiedCommit) { + if verified.level != FeatureLevel::None { + self.0.entry(verified.level).or_insert(verified.commit.oid); + } + } + + #[cfg(any(test, feature = "test"))] + pub fn test(from: impl IntoIterator) -> Self { + Self(BTreeMap::from_iter(from)) + } +} + +impl std::fmt::Display for FeatureLevels { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str( + &self + .0 + .iter() + .map(|(level, at)| format!("{level}@{at}")) + .collect::>() + .join(", "), + ) + } +} + impl<'a, R, V> SignedRefsReader<'a, R, V> where R: object::Reader + reference::Reader, @@ -129,14 +169,19 @@ where const ONE: NonZeroUsize = NonZeroUsize::new(1).expect("one is non-zero"); const SIGNATURES_COLLECTED: &str = "all signatures were collected"; - let head = CommitReader::new(self.resolve_tip()?, self.repository) + let mut head = CommitReader::new(self.resolve_tip()?, self.repository) .read() .map_err(error::Read::Commit)? .verify(self.rid, self.verifier) .map_err(error::Read::Verify)?; - #[cfg(not(debug_assertions))] - if head.parent { + if head.commit.parent.is_none() && head.level == FeatureLevel::Root { + head.level = FeatureLevel::Parent; + } + + let head = head; + + if head.level >= FeatureLevel::Parent { // `head` is verified, thus we know that if the parent reference // exists, its target actually matches the parent OID. // The fact that the parent OID is a hash over all previous history @@ -146,23 +191,58 @@ where return Ok(head); } - // Note that for all sets of commits which share the same signature, - // the `NonEmpty` in `seen` will be in reverse order of the walk. - // That is, the latest commit in the set will be at the first position, - // and the earliest commit will be at the last position. - let seen = iter::Walk::new(*head.id(), self.repository).try_fold( - HashMap::>::new(), - |mut seen, commit| { - let current = commit.map_err(error::Read::Commit)?; - seen.entry(current.signature) - .and_modify(|value| value.push(current.oid)) - .or_insert_with(|| NonEmpty::new(current.oid)); - Ok(seen) + // `seen` maps from signatures to the `NonEmpty` of commits they were + // seen in. Note that for all sets of commits which share the same + // signature, the `NonEmpty` in `seen` will be in reverse order of the + // walk. That is, the latest commit in the set will be at the first + // position, and the earliest commit will be at the last position. + // + // `level` is the feature level of the history, which is + // the maximum feature level over all commits in the history. + let (seen, levels) = iter::Walk::new(head.commit.oid, self.repository).try_fold( + ( + HashMap::>::new(), + FeatureLevels::new(), + ), + |(mut seen, mut levels), commit| { + let commit = commit.map_err(error::Read::Commit)?; + + seen.entry(commit.signature) + .and_modify(|value| value.push(commit.oid)) + .or_insert_with(|| NonEmpty::new(commit.oid)); + + // Before `commit` can be interpreted for feature detection, + // it must be verified. In particular, we do not want to + // detect features on commits that have an invalid signature. + // However, if we have already reached the maximum level, + // this is not required anymore, since it cannot increase any + // further. + if levels.max() < FeatureLevel::LATEST { + let commit = commit + .verify(self.rid, self.verifier) + .map_err(error::Read::Verify)?; + + if commit.level > FeatureLevel::None { + levels.insert(&commit); + } + } + + Ok((seen, levels)) }, )?; - let parent = if seen - .get(head.signature()) + let level = levels.max(); + + if head.level < level { + return Err(error::Read::Downgrade { + levels, + actual: head.level, + commit: head.commit.oid, + }); + } + + if seen + .get(&head.commit.signature) .expect(SIGNATURES_COLLECTED) .len_nonzero() == ONE @@ -171,47 +251,47 @@ where // include the parent reference in the `/refs` blob. Maintains // backwards-compatibility. return Ok(head); - } else { - #[cfg(debug_assertions)] - { - if head.parent { - panic!("duplicate signature found even though parent ref did verify") - } - } + } - // If the signature in head was seen twice, then - // head must have a parent. - *head.parent().expect("parent must exist") - }; + // If the signature in `head` was seen twice, then + // `head` must have a parent. + let parent = head.commit.parent.expect("parent must exist"); - // The second walk can start from the parent of head. We do not need to - // verify head twice, and we already know that the parent exists. + // The second walk can start from the parent of `head`. We do not need to + // verify `head` twice, and we already know that the parent exists. for commit in iter::Walk::new(parent, self.repository) { - let commit = commit + let verified = commit .map_err(error::Read::Commit)? .verify(self.rid, self.verifier) .map_err(error::Read::Verify)?; - let commits = seen.get(commit.signature()).expect(SIGNATURES_COLLECTED); - - if commits.len_nonzero() == ONE { - return Ok(commit); + if verified.level < level { + // To avoid downgrade attacks, we skip `commit`. + continue; } - let id = commit.id(); + let commit = verified.commit(); + + let commits = seen.get(&commit.signature).expect(SIGNATURES_COLLECTED); + + if commits.len_nonzero() == ONE { + return Ok(verified); + } + + let id = &commit.oid; if id == commits.last() { // If this commit is the last element of `commits`, // then this means it is the earliest of all that share // its signature. It thus cannot have been replayed. - return Ok(commit); + return Ok(verified); } if id == commits.first() { // We only log one warning per set of duplicates, and that is // when we reach the first element of `commits`, which is the // latest in the history. - log::warn!("Duplicate sigrefs found in commits {commits:?}"); + log::warn!("Duplicate signature found in commits {commits:?}"); } } @@ -236,7 +316,7 @@ where } #[derive(Clone, Debug, PartialEq, Eq)] -struct Commit { +pub(super) struct Commit { oid: Oid, parent: Option, refs: Refs, @@ -245,6 +325,11 @@ struct Commit { } impl Commit { + #[cfg(test)] + pub(super) fn refs(&self) -> &Refs { + &self.refs + } + fn verify(mut self, expected: RepoId, verifier: &V) -> Result where V: signature::Verifier, @@ -253,7 +338,7 @@ impl Commit { .verify(&self.refs.canonical(), &self.signature) .map_err(error::Verify::Signature)?; - if let Some(IdentityRoot { + let level = if let Some(IdentityRoot { commit: identity_commit, rid, }) = self.identity_root @@ -266,7 +351,7 @@ impl Commit { found: rid, }); } else { - // Identity verification succeeds. + FeatureLevel::Root } } else { let err = error::Verify::MissingIdentity(error::MissingIdentity { @@ -279,21 +364,50 @@ impl Commit { // TODO: Make this return the error // and enable test `test::commit_reader::missing_identity`. - // Identity verification succeeds. - } + FeatureLevel::None + }; self.refs.remove_sigrefs(); - let parent = match (self.parent, self.refs.remove_parent()) { - (None, None) => true, - (Some(_), None) => false, + let level = match (self.parent, self.refs.remove_parent()) { + (None, None) | (Some(_), None) => { + // Pattern 1: + // We are looking at a root commit. + // This is a special case, as there is no good value + // for `rad/refs/sigrefs-parent` to target. The zero OID would + // be a candidate, but it is filtered out in [`Refs`]. + // Upgrading to `FeatureLevel::Parent` is not a good idea + // either, otherwise any history containing this commit + // would be at that level from the root onwards. + + // Pattern 2: + // The ref `refs/rad/sigrefs-parent` is simply absent, + // we remain on the same feature level. + + level + } (None, Some(actual)) => { + // We are looking at a root commit. + // Any target OID is treated as dangling. return Err(error::Verify::DanglingParent { sigrefs_commit: self.oid, actual, - }) + }); + } + (Some(expected), Some(actual)) if expected == actual => { + // We have a good value for `refs/rad/sigrefs-parent`, however, + // as feature levels are monotonic, we also make sure that the + // earlier check of `refs/rad/root` was positive. + // In case the prior feature level was not `FeatureLevel::Root`, + // we can even error early. + if level == FeatureLevel::Root { + FeatureLevel::Parent + } else { + return Err(error::Verify::IdentityRootDowngrade { + sigrefs_commit: self.oid, + }); + } } - (Some(expected), Some(actual)) if expected == actual => true, (Some(expected), Some(actual)) => { return Err(error::Verify::MismatchedParent { sigrefs_commit: self.oid, @@ -305,7 +419,7 @@ impl Commit { Ok(VerifiedCommit { commit: self, - parent, + level, }) } } diff --git a/crates/radicle/src/storage/refs/sigrefs/read/error.rs b/crates/radicle/src/storage/refs/sigrefs/read/error.rs index ef0e0bd0..c28eca9f 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/error.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/error.rs @@ -6,8 +6,9 @@ use radicle_git_metadata::commit; use radicle_oid::Oid; use thiserror::Error; -use crate::storage::refs::canonical; use crate::storage::refs::sigrefs::git::{object, reference}; +use crate::storage::refs::sigrefs::read::FeatureLevels; +use crate::storage::refs::{canonical, FeatureLevel}; #[derive(Debug, Error)] #[non_exhaustive] @@ -22,6 +23,12 @@ pub enum Read { Verify(Verify), #[error("failed to find a valid set of signed references starting from {head}")] NoValidCommit { head: Oid }, + #[error("refusing to downgrade from history '{levels}' to '{actual}' at commit {commit}")] + Downgrade { + levels: FeatureLevels, + actual: FeatureLevel, + commit: Oid, + }, } #[derive(Debug, Error)] @@ -128,6 +135,8 @@ pub enum Verify { expected: Oid, actual: Oid, }, + #[error("expected identity root in refs commit '{sigrefs_commit}' but found none")] + IdentityRootDowngrade { sigrefs_commit: Oid }, } #[derive(Debug, Error, Clone, PartialEq, Eq)] diff --git a/crates/radicle/src/storage/refs/sigrefs/read/test/commit_reader.rs b/crates/radicle/src/storage/refs/sigrefs/read/test/commit_reader.rs index 44ef80c9..5fac6fba 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/test/commit_reader.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/test/commit_reader.rs @@ -105,5 +105,5 @@ fn read_ok() { .with_signature(head, 1); let vc = read_at(head, repo).unwrap(); - assert_eq!(vc.id(), &head); + assert_eq!(vc.commit.oid, head); } diff --git a/crates/radicle/src/storage/refs/sigrefs/read/test/resolve_tip.rs b/crates/radicle/src/storage/refs/sigrefs/read/test/resolve_tip.rs index df2569cb..65b84615 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/test/resolve_tip.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/test/resolve_tip.rs @@ -63,5 +63,5 @@ fn resolve_tip_ok() { ) .read() .unwrap(); - assert_eq!(*vc.id(), root); + assert_eq!(vc.commit.oid, root); } diff --git a/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs b/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs index 78b7a12a..4c437f2e 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/test/signed_refs_reader.rs @@ -1,9 +1,9 @@ use radicle_oid::Oid; use crate::storage::refs::sigrefs::read::error::{Read, Verify}; -use crate::storage::refs::sigrefs::read::{error, Commit, SignedRefsReader, Tip}; +use crate::storage::refs::sigrefs::read::{error, Commit, FeatureLevels, SignedRefsReader, Tip}; use crate::storage::refs::sigrefs::VerifiedCommit; -use crate::storage::refs::{IDENTITY_ROOT, SIGREFS_PARENT}; +use crate::storage::refs::{FeatureLevel, IDENTITY_ROOT, SIGREFS_PARENT}; use crate::{assert_matches, git}; use super::mock; @@ -19,6 +19,17 @@ fn refs_without_parent(head_oid: Oid) -> Vec<(git::fmt::RefString, Oid)> { ] } +fn refs_without_root(head: Oid, parent: Oid) -> Vec<(git::fmt::RefString, Oid)> { + vec![ + (mock::refs_heads_main(), head), + (SIGREFS_PARENT.to_ref_string(), parent), + ] +} + +fn refs_without_root_and_parent(head: Oid) -> Vec<(git::fmt::RefString, Oid)> { + vec![(mock::refs_heads_main(), head)] +} + fn refs(head_oid: Oid, parent_oid: Oid) -> Vec<(git::fmt::RefString, Oid)> { vec![ (mock::refs_heads_main(), head_oid), @@ -149,7 +160,7 @@ fn single_commit() { let repo = mock::setup_chain([(head, 1, refs_without_parent(head))]); let vc = read(head, repo).unwrap(); - assert_eq!(*vc.id(), head); + assert_eq!(vc.commit.oid, head); } #[test] @@ -162,7 +173,7 @@ fn two_commits() { ]); let vc = read(head, repo).unwrap(); - assert_eq!(*vc.id(), head); + assert_eq!(vc.commit.oid, head); } /// We test a handful scenarios with replayed commits (or rather, references @@ -198,7 +209,10 @@ mod replay { } assert_eq!( - *read(mock::oid((chain.len() - 1) as u8), repo).unwrap().id(), + read(mock::oid((chain.len() - 1) as u8), repo) + .unwrap() + .commit + .oid, mock::oid(expected) ) } @@ -224,6 +238,141 @@ mod replay { } } +mod downgrade { + use super::*; + + #[test] + fn parent() { + const SIGNATURE_1: u8 = 1; + const SIGNATURE_2: u8 = 2; + const SIGNATURE_3: u8 = 3; + + let c1 = mock::oid(1); + let c2 = mock::oid(2); + let c3 = mock::oid(3); + + let r3 = refs_without_parent(mock::oid(10)); + let r2 = refs(mock::oid(10), c3); + let r1 = refs_without_parent(mock::oid(10)); + + let repo = mock::setup_chain([ + (c3, SIGNATURE_3, r3), + (c2, SIGNATURE_2, r2), + (c1, SIGNATURE_1, r1), + ]); + + let expected_levels = + FeatureLevels::test([(FeatureLevel::Parent, c2), (FeatureLevel::Root, c1)]); + + assert_matches!( + read(c1, repo), + Err(error::Read::Downgrade { + levels, + actual: FeatureLevel::Root, + .. + }) + if levels == expected_levels + ); + } + + #[test] + fn root() { + const SIGNATURE_2: u8 = 2; + const SIGNATURE_3: u8 = 3; + + let c2 = mock::oid(2); + let c3 = mock::oid(3); + + let r3 = refs_without_parent(mock::oid(10)); + let r2 = refs_without_root_and_parent(mock::oid(10)); + + let repo = mock::setup_chain([(c3, SIGNATURE_3, r3), (c2, SIGNATURE_2, r2)]); + + let expected_levels = FeatureLevels::test([(FeatureLevel::Root, c3)]); + + assert_matches!( + read(c2, repo), + Err(error::Read::Downgrade { + levels, + actual: FeatureLevel::None, + .. + }) + if levels == expected_levels + ); + } + + #[test] + fn root_with_parent() { + const SIGNATURE_2: u8 = 2; + const SIGNATURE_3: u8 = 3; + + let c2 = mock::oid(2); + let c3 = mock::oid(3); + + let r3 = refs_without_root_and_parent(mock::oid(10)); + let r2 = refs_without_root(mock::oid(10), c3); + + let repo = mock::setup_chain([(c3, SIGNATURE_3, r3), (c2, SIGNATURE_2, r2)]); + + assert_matches!( + read(c2, repo), + Err(error::Read::Verify(Verify::IdentityRootDowngrade { + sigrefs_commit + })) + if sigrefs_commit == c2 + ); + } + + #[test] + fn restore() { + const SIGNATURE_1: u8 = 1; + const SIGNATURE_2: u8 = 2; + const SIGNATURE_3: u8 = 3; + const SIGNATURE_4: u8 = 4; + + let c1 = mock::oid(1); + let c2 = mock::oid(2); + let c3 = mock::oid(3); + let c4 = mock::oid(4); + + let r1 = refs_without_parent(mock::oid(10)); + let r2 = refs(mock::oid(10), c1); + let r3 = refs_without_parent(mock::oid(10)); + let r4 = refs(mock::oid(10), c3); + + let repo = mock::setup_chain([ + (c1, SIGNATURE_1, r1), + (c2, SIGNATURE_2, r2), + (c3, SIGNATURE_3, r3), + (c4, SIGNATURE_4, r4), + ]); + + assert_eq!(read(c4, repo).unwrap().commit.oid, c4); + } +} + +mod detect_parent { + use super::*; + + #[test] + fn root_without_parent() { + const SIG: u8 = 3; + let commit = mock::oid(3); + let refs = refs_without_parent(mock::oid(10)); + let repo = mock::setup_chain([(commit, SIG, refs)]); + assert_matches!(read(commit, repo).unwrap().level, FeatureLevel::Parent); + } + + #[test] + fn root_without_root() { + const SIG: u8 = 3; + let commit = mock::oid(3); + let refs = refs_without_root_and_parent(mock::oid(10)); + let repo = mock::setup_chain([(commit, SIG, refs)]); + assert_matches!(read(commit, repo).unwrap().level, FeatureLevel::None); + } +} + #[test] fn read_ok_no_parent() { const SIGNATURE_1: u8 = 1; @@ -236,7 +385,7 @@ fn read_ok_no_parent() { let repo = mock::setup_chain([(c2, SIGNATURE_2, r.clone()), (c1, SIGNATURE_1, r)]); let vc = read(c1, repo).unwrap(); - assert_eq!(*vc.id(), c1); + assert_eq!(vc.commit.oid, c1); assert_matches!( vc, @@ -248,11 +397,37 @@ fn read_ok_no_parent() { signature: _, identity_root: Some(_) }, - parent: false + level: FeatureLevel::Root, } ); } +#[test] +fn read_ok_root() { + const SIGNATURE_1: u8 = 1; + const SIGNATURE_2: u8 = 2; + + let c1 = mock::oid(1); + let c2 = mock::oid(2); + + let repo = mock::setup_chain([ + (c2, SIGNATURE_2, refs_without_root_and_parent(mock::oid(10))), + (c1, SIGNATURE_1, refs_without_parent(mock::oid(20))), + ]); + + let vc = read(c1, repo).unwrap(); + assert_eq!(vc.commit.oid, c1); + assert_eq!(vc.commit.parent, Some(c2)); + + assert_matches!(vc, VerifiedCommit { commit: Commit { + oid, + parent: Some(parent), + refs: _, + signature: _, + identity_root: Some(_) + }, level: FeatureLevel::Root } if parent == c2 && oid == c1); +} + #[test] fn read_ok_parent() { const SIGNATURE_1: u8 = 1; @@ -267,7 +442,7 @@ fn read_ok_parent() { ]); let vc = read(c1, repo).unwrap(); - assert_eq!(*vc.id(), c1); + assert_eq!(vc.commit.oid, c1); assert_matches!(vc, VerifiedCommit { commit: Commit { oid, @@ -275,7 +450,7 @@ fn read_ok_parent() { refs: _, signature: _, identity_root: Some(_) - }, parent: true } if parent == c2 && oid == c1); + }, level: FeatureLevel::Parent } if parent == c2 && oid == c1); } #[test] diff --git a/crates/radicle/src/storage/refs/sigrefs/read/test/tree_reader.rs b/crates/radicle/src/storage/refs/sigrefs/read/test/tree_reader.rs index 44880303..a9ced3b3 100644 --- a/crates/radicle/src/storage/refs/sigrefs/read/test/tree_reader.rs +++ b/crates/radicle/src/storage/refs/sigrefs/read/test/tree_reader.rs @@ -144,5 +144,5 @@ fn read_ok() { .with_signature(head, 1); let vc = read_at(head, repo).unwrap(); - assert_eq!(*vc.id(), head); + assert_eq!(vc.commit.oid, head); } diff --git a/crates/radicle/src/storage/refs/sigrefs/write.rs b/crates/radicle/src/storage/refs/sigrefs/write.rs index 7ee27dc8..09a573e8 100644 --- a/crates/radicle/src/storage/refs/sigrefs/write.rs +++ b/crates/radicle/src/storage/refs/sigrefs/write.rs @@ -15,7 +15,8 @@ use radicle_oid::Oid; use crate::git; use crate::storage::refs::sigrefs::git::{object, reference, Committer}; use crate::storage::refs::{ - Refs, IDENTITY_ROOT, REFS_BLOB_PATH, SIGNATURE_BLOB_PATH, SIGREFS_BRANCH, SIGREFS_PARENT, + FeatureLevel, Refs, IDENTITY_ROOT, REFS_BLOB_PATH, SIGNATURE_BLOB_PATH, SIGREFS_BRANCH, + SIGREFS_PARENT, }; use crate::storage::refs::{SignedRefs, SignedRefsAt}; @@ -168,13 +169,14 @@ impl Commit { self.refs } - pub(crate) fn into_sigrefs_at(self, id: PublicKey) -> SignedRefsAt { + pub(crate) fn into_sigrefs_at(self, id: PublicKey, level: FeatureLevel) -> SignedRefsAt { SignedRefsAt { at: self.oid, sigrefs: SignedRefs { id, signature: self.signature, refs: self.refs, + level, }, } } diff --git a/crates/radicle/src/test/storage.rs b/crates/radicle/src/test/storage.rs index c6e0b549..3364c434 100644 --- a/crates/radicle/src/test/storage.rs +++ b/crates/radicle/src/test/storage.rs @@ -102,7 +102,7 @@ impl ReadStorage for MockStorage { rid: *rid, head: r.head().unwrap().1, doc: r.doc.clone().into(), - refs: Ok(None), + refs: None, synced_at: None, }) .collect())