tough: implement terminating delegations

In the pre-order depth-first search for target metadata, if a delegated
role is selected, is terminating, and does not have metadata for the
target, end the search. TUF specification v1.0.33 section 5.6.7.2.1.

Author: larvacea

tough/src/editor/mod.rs

@@ -352,6 +352,7 @@ impl RepositoryEditor {
         name: &str,
         key_source: &[Box<dyn KeySource>],
         paths: PathSet,
+        terminating: bool,
         threshold: NonZeroU64,
         expiration: DateTime<Utc>,
         version: NonZeroU64,
@@ -387,6 +388,7 @@ impl RepositoryEditor {
         self.targets_editor_mut()?.delegate_role(
             new_targets,
             paths,
+            terminating,
             key_pairs,
             keyids,
             threshold,
@@ -646,6 +648,7 @@ impl RepositoryEditor {
         name: &str,
         metadata_url: &str,
         paths: PathSet,
+        terminating: bool,
         threshold: NonZeroU64,
         keys: Option<HashMap<Decoded<Hex>, Key>>,
     ) -> Result<&mut Self> {
@@ -658,7 +661,7 @@ impl RepositoryEditor {
         self.targets_editor_mut()?.limits(limits);
         self.targets_editor_mut()?.transport(transport.clone());
         self.targets_editor_mut()?
-            .add_role(name, metadata_url, paths, threshold, keys)
+            .add_role(name, metadata_url, paths, terminating, threshold, keys)
             .await?;
 
         Ok(self)

tough/src/editor/targets.rs

@@ -336,6 +336,7 @@ impl TargetsEditor {
         &mut self,
         targets: Signed<DelegatedTargets>,
         paths: PathSet,
+        terminating: bool,
         key_pairs: HashMap<Decoded<Hex>, Key>,
         keyids: Vec<Decoded<Hex>>,
         threshold: NonZeroU64,
@@ -348,7 +349,7 @@ impl TargetsEditor {
                 paths,
                 keyids,
                 threshold,
-                terminating: false,
+                terminating,
                 targets: Some(Signed {
                     signed: targets.signed.targets,
                     signatures: targets.signatures,
@@ -388,6 +389,7 @@ impl TargetsEditor {
         name: &str,
         metadata_url: &str,
         paths: PathSet,
+        terminating: bool,
         threshold: NonZeroU64,
         keys: Option<HashMap<Decoded<Hex>, Key>>,
     ) -> Result<&mut Self> {
@@ -446,7 +448,14 @@ impl TargetsEditor {
             (key_pairs.keys().cloned().collect(), key_pairs)
         };
 
-        self.delegate_role(delegated_targets, paths, key_pairs, keyids, threshold)?;
+        self.delegate_role(
+            delegated_targets,
+            paths,
+            terminating,
+            key_pairs,
+            keyids,
+            threshold,
+        )?;
 
         Ok(self)
     }

tough/src/lib.rs

@@ -483,12 +483,20 @@ impl Repository {
         //   HASH is one of the hashes of the targets file listed in the targets metadata file
         //   found earlier in step 4. In either case, the client MUST write the file to
         //   non-volatile storage as FILENAME.EXT.
-        Ok(if let Ok(target) = self.targets.signed.find_target(name) {
-            let (sha256, file) = self.target_digest_and_filename(target, name);
-            Some(self.fetch_target(target, &sha256, file.as_str()).await?)
-        } else {
-            None
-        })
+        let mut visited_roles: BTreeSet<String> = BTreeSet::new();
+        let mut terminated = false;
+        Ok(
+            if let Ok(target) =
+                self.targets
+                    .signed
+                    .find_target(name, &mut visited_roles, &mut terminated, false)
+            {
+                let (sha256, file) = self.target_digest_and_filename(target, name);
+                Some(self.fetch_target(target, &sha256, file.as_str()).await?)
+            } else {
+                None
+            },
+        )
     }
 
     /// Fetches a target from the repository and saves it to `outdir`. Attempts to do this as safely
@@ -535,11 +543,15 @@ impl Repository {
 
         let filename = match prepend {
             Prefix::Digest => {
-                let target = self.targets.signed.find_target(name).with_context(|_| {
-                    error::CacheTargetMissingSnafu {
+                let mut visited_roles: BTreeSet<String> = BTreeSet::new();
+                let mut terminated = false;
+                let target = self
+                    .targets
+                    .signed
+                    .find_target(name, &mut visited_roles, &mut terminated, false)
+                    .with_context(|_| error::CacheTargetMissingSnafu {
                         target_name: name.clone(),
-                    }
-                })?;
+                    })?;
                 let sha256 = target.hashes.sha256.clone().into_vec();
                 format!("{}.{}", hex::encode(sha256), name.resolved())
             }
@@ -1220,6 +1232,7 @@ async fn load_targets(
 }
 
 // Follow the paths of delegations starting with the top level targets.json delegation
+#[allow(clippy::too_many_arguments)]
 #[async_recursion]
 async fn load_delegations(
     transport: &dyn Transport,

tough/src/schema/mod.rs

@@ -27,7 +27,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer};
 use serde_json::Value;
 use serde_plain::{derive_display_from_serialize, derive_fromstr_from_deserialize};
 use snafu::ResultExt;
-use std::collections::HashMap;
+use std::collections::{BTreeSet, HashMap};
 use std::num::NonZeroU64;
 use std::ops::{Deref, DerefMut};
 use std::path::Path;
@@ -516,21 +516,43 @@ impl Targets {
     ///
     /// **Caution**: does not imply that delegations in this struct or any child are valid.
     ///
-    pub fn find_target(&self, target_name: &TargetName) -> Result<&Target> {
+    pub fn find_target(
+        &self,
+        target_name: &TargetName,
+        visited: &mut BTreeSet<String>,
+        terminated: &mut bool,
+        permissive: bool,
+    ) -> Result<&Target> {
         if let Some(target) = self.targets.get(target_name) {
             return Ok(target);
         }
         if let Some(delegations) = &self.delegations {
             for role in &delegations.roles {
                 // If the target cannot match this DelegatedRole, then we do not want to recurse and
-                // check any of its child roles either.
-                if !role.paths.matches_target_name(target_name) {
+                // check any of its child roles either. If we have already visited this role, we
+                // do not need to visit it again.
+                if !role.paths.matches_target_name(target_name) || visited.contains(&role.name) {
                     continue;
                 }
+                visited.insert(role.name.clone());
                 if let Some(targets) = &role.targets {
-                    if let Ok(target) = targets.signed.find_target(target_name) {
+                    if let Ok(target) =
+                        targets
+                            .signed
+                            .find_target(target_name, visited, terminated, permissive)
+                    {
                         return Ok(target);
                     }
+                    if !permissive && *terminated {
+                        // we encountered a terminating delegation, so we stop iterating immediately
+                        break;
+                    }
+                }
+                if role.terminating && !permissive {
+                    // this role was terminating, so set terminated. This will cause all ancestors
+                    // to stop iterating and return not-found.
+                    *terminated = true;
+                    break;
                 }
             }
         }
@@ -724,7 +746,9 @@ impl Targets {
     /// that the ownership of each target is valid.
     pub(crate) fn validate(&self) -> Result<()> {
         for (target_name, _) in self.targets_iter() {
-            self.find_target(target_name)?;
+            let mut terminated = false;
+            let mut visited_roles: BTreeSet<String> = BTreeSet::new();
+            self.find_target(target_name, &mut visited_roles, &mut terminated, true)?;
         }
         Ok(())
     }

tough/tests/data/delegation-targets/a.txt

@@ -0,0 +1 @@
+This is delegated a.txt

tough/tests/data/delegation-targets/b.txt

@@ -0,0 +1 @@
+This is delegation b.txt

tough/tests/repo_editor.rs

@@ -169,6 +169,7 @@ async fn create_sign_write_reload_repo() {
             "role1",
             role1_key,
             PathSet::Paths(vec![PathPattern::new("file?.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Utc::now().checked_add_signed(days(21)).unwrap(),
             NonZeroU64::new(1).unwrap(),
@@ -189,6 +190,7 @@ async fn create_sign_write_reload_repo() {
             "role2",
             role2_key,
             PathSet::Paths(vec![PathPattern::new("file1.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Utc::now().checked_add_signed(days(21)).unwrap(),
             NonZeroU64::new(1).unwrap(),
@@ -199,6 +201,7 @@ async fn create_sign_write_reload_repo() {
             "role3",
             role1_key,
             PathSet::Paths(vec![PathPattern::new("file1.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Utc::now().checked_add_signed(days(21)).unwrap(),
             NonZeroU64::new(1).unwrap(),
@@ -219,6 +222,7 @@ async fn create_sign_write_reload_repo() {
             "role4",
             role2_key,
             PathSet::Paths(vec![PathPattern::new("file1.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Utc::now().checked_add_signed(days(21)).unwrap(),
             NonZeroU64::new(1).unwrap(),
@@ -312,6 +316,7 @@ async fn create_role_flow() {
             "A",
             metadata_base_url_out.as_str(),
             PathSet::Paths(vec![PathPattern::new("*.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Some(key_hash_map(role1_key).await),
         )
@@ -396,6 +401,7 @@ async fn create_role_flow() {
             "B",
             metadata_base_url_out.as_str(),
             PathSet::Paths(vec![PathPattern::new("file?.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Some(key_hash_map(role2_key).await),
         )
@@ -536,6 +542,7 @@ async fn update_targets_flow() {
             "A",
             metadata_base_url_out.as_str(),
             PathSet::Paths(vec![PathPattern::new("*.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Some(key_hash_map(role1_key).await),
         )
@@ -620,6 +627,7 @@ async fn update_targets_flow() {
             "B",
             metadata_base_url_out.as_str(),
             PathSet::Paths(vec![PathPattern::new("file?.txt").unwrap()]),
+            false,
             NonZeroU64::new(1).unwrap(),
             Some(key_hash_map(role2_key).await),
         )

tough/tests/target_path_safety.rs

@@ -91,6 +91,7 @@ async fn safe_target_paths() {
             "delegated",
             &keys,
             PathSet::Paths(vec![PathPattern::new("delegated/*").unwrap()]),
+            false,
             one,
             later(),
             one,

tuftool/src/add_role.rs

@@ -45,6 +45,10 @@ pub(crate) struct AddRoleArgs {
     #[arg(short, long, conflicts_with = "path_hash_prefixes")]
     paths: Option<Vec<PathPattern>>,
 
+    /// Make the delegation terminating
+    #[arg(long)]
+    terminating: bool,
+
     /// Path to root.json file for the repository
     #[arg(short, long)]
     root: PathBuf,
@@ -130,6 +134,7 @@ impl AddRoleArgs {
                 &self.delegatee,
                 self.indir.as_str(),
                 paths,
+                self.terminating,
                 self.threshold,
                 None,
             )
@@ -205,6 +210,7 @@ impl AddRoleArgs {
                 &self.delegatee,
                 self.indir.as_str(),
                 paths,
+                self.terminating,
                 self.threshold,
                 None,
             )