Radish alpha
h
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
radicle/sigrefs: Implement feature detection
Lorenz Leutgeb committed 1 month ago
commit 372a78754a025d493906c9357d7a906b1f92dba5
parent 9a4539f
17 files changed +505 -149
modified 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

modified 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;
        }
modified 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(())
    }
modified 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"));
modified 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<Option<refs::SignedRefsAt>, refs::sigrefs::read::error::MissingIdentity>,
+
    pub refs: Option<refs::SignedRefsAt>,
    /// Sync time of the repository.
    pub synced_at: Option<SyncedAt>,
}
modified 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);
    }
}
modified 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<R>(remote: RemoteId, repo: &R) -> Result<Self, sigrefs::read::error::Read>
    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<R>(
@@ -296,16 +339,21 @@ impl SignedRefs {
        R: HasRepoId,
        R: sigrefs::git::object::Reader + sigrefs::git::reference::Reader,
    {
+
        Self::load_internal(remote, repo, sigrefs::read::Tip::Commit(oid))
+
    }
+

+
    fn load_internal<R>(
+
        remote: RemoteId,
+
        repo: &R,
+
        tip: Tip,
+
    ) -> Result<Self, sigrefs::read::error::Read>
+
    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)
    }
}

modified 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()
+
    }
+
}
modified crates/radicle/src/storage/refs/sigrefs/property.rs
@@ -53,7 +53,7 @@ fn roundtrip(BoundedVec(all_refs): BoundedVec<Refs>) -> 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();
        }
    }
modified 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<FeatureLevel, Oid>);
+

+
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<Item = (FeatureLevel, Oid)>) -> 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::<Vec<_>>()
+
                .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::<crypto::Signature, NonEmpty<Oid>>::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::<crypto::Signature, NonEmpty<Oid>>::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 verified.level < level {
+
                // To avoid downgrade attacks, we skip `commit`.
+
                continue;
+
            }
+

+
            let commit = verified.commit();
+

+
            let commits = seen.get(&commit.signature).expect(SIGNATURES_COLLECTED);

            if commits.len_nonzero() == ONE {
-
                return Ok(commit);
+
                return Ok(verified);
            }

-
            let id = commit.id();
+
            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<Oid>,
    refs: Refs,
@@ -245,6 +325,11 @@ struct Commit {
}

impl Commit {
+
    #[cfg(test)]
+
    pub(super) fn refs(&self) -> &Refs {
+
        &self.refs
+
    }
+

    fn verify<V>(mut self, expected: RepoId, verifier: &V) -> Result<VerifiedCommit, error::Verify>
    where
        V: signature::Verifier<crypto::Signature>,
@@ -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,
        })
    }
}
modified 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)]
modified 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);
}
modified 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);
}
modified 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,12 +397,38 @@ 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;
    const SIGNATURE_2: u8 = 2;
@@ -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]
modified 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);
}
modified 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,
            },
        }
    }
modified 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())