Repository: mozilla/cargo-vet (rust)

Issue:
implement imports.exclude
At present cargo-vet largely punts on conflict handling, we should define it.

Generally speaking, audits are additive and can't generate conflicts. However, it can happen if the range provided in a `violation` entry intersects a leaf in the audit version graph. This basically means that two parties explicitly disagree on whether the criteria are met for that version. Examples might include:
1. Somebody missed a problem when certifying an audit.
2. Somebody provided an overly-broad `violation` range.
3. Two parties fundamentally disagree on whether there's a problem.

I think (1) and (2) are the most likely cases, and so I think we want the default behavior to result in a discussion and ideally a consensus resolution. However, we can't have the tool entirely fall apart while that discussion is happening, or in the actual case of (3). So we need some mechanism to immediately resolve the conflict locally.

My spitball suggestion here is to add an `exclude` field to `imports` table entries, which lists a set of crates for which audits are dropped for that import. That strikes me as a lightweight mechanism to resolve conflicts in either direction (local violation + foreign certification, or vice versa), while also ensuring that any such conflict is surfaced for manual inspection.

@tmandry wdyt?

Pull request:
Add support for imports.exclude
This is a fairly straightforward implementation of imports.exclude. The
configuration option is already documented in the book. After changing
this configuration value, you'll need to update imports.lock by running
a successful `cargo vet` without `--locked` (or by running `cargo vet
regenerate imports`).

I also added some validation to help catch outdated `imports.lock` files
when running with `--locked`. These will never fire when unlocked, as
`imports.lock` will be automatically updated in those cases.

Fixes #80

Patch:
diff --git a/src/errors.rs b/src/errors.rs
index 5bed733..46f3844 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -245,6 +245,9 @@ pub enum StoreValidateError {
     #[diagnostic(transparent)]
     #[error(transparent)]
     InvalidCriteria(InvalidCriteriaError),
+    #[error("imports.lock is out-of-date with respect to configuration")]
+    #[diagnostic(help("run `cargo vet` without --locked to update imports"))]
+    ImportsLockOutdated,
 }
 
 #[derive(Debug, Error, Diagnostic)]
diff --git a/src/format.rs b/src/format.rs
index 02e03b0..85be6bc 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -381,10 +381,14 @@ pub static DEFAULT_POLICY_CRITERIA: CriteriaStr = SAFE_TO_DEPLOY;
 pub static DEFAULT_POLICY_DEV_CRITERIA: CriteriaStr = SAFE_TO_RUN;
 
 /// A remote audits.toml that we trust the contents of (by virtue of trusting the maintainer).
-#[derive(serde::Serialize, serde::Deserialize, Clone)]
+#[derive(serde::Serialize, serde::Deserialize, Clone, Default)]
 pub struct RemoteImport {
     /// URL of the foreign audits.toml
     pub url: String,
+    /// A list of crates for which no audits or violations should be imported.
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    #[serde(default)]
+    pub exclude: Vec<PackageName>,
     /// A list of criteria that are implied by foreign criteria
     #[serde(rename = "criteria-map")]
     pub criteria_map: Vec<CriteriaMapping>,
diff --git a/src/storage.rs b/src/storage.rs
index f51ecf9..702b9b4 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -197,10 +197,10 @@ impl Store {
         // If this command isn't locked, and the network is available, fetch the
         // live state of imported audits.
         let live_imports = if let (false, Some(network)) = (cfg.cli.locked, network) {
-            let imported_audits = tokio::runtime::Handle::current()
+            let fetched_audits = tokio::runtime::Handle::current()
                 .block_on(fetch_imported_audits(network, &config))?;
             let live_imports =
-                process_imported_audits(imported_audits, &imports, allow_criteria_changes)?;
+                process_imported_audits(fetched_audits, &config, &imports, allow_criteria_changes)?;
             Some(live_imports)
         } else {
             None
@@ -249,7 +249,7 @@ impl Store {
         allow_criteria_changes: bool,
     ) -> Result<Self, CriteriaChangeErrors> {
         let live_imports =
-            process_imported_audits(fetched_audits, &imports, allow_criteria_changes)?;
+            process_imported_audits(fetched_audits, &config, &imports, allow_criteria_changes)?;
         Ok(Self {
             lock: None,
             config,
@@ -504,9 +504,16 @@ impl Store {
             }
         }
 
+        // If we're locked, and therefore not fetching new live imports,
+        // validate that our imports.lock is in sync with config.toml.
+        let imports_lock_outdated = self
+            .imports_lock_outdated()
+            .then_some(StoreValidateError::ImportsLockOutdated);
+
         let errors = invalid_criteria_errors
             .into_iter()
             .map(StoreValidateError::InvalidCriteria)
+            .chain(imports_lock_outdated)
             .collect::<Vec<_>>();
         if !errors.is_empty() {
             return Err(StoreValidateErrors { errors });
@@ -515,6 +522,33 @@ impl Store {
         Ok(())
     }
 
+    fn imports_lock_outdated(&self) -> bool {
+        // If we have live imports, we're going to be updating imports.lock, so
+        // it's OK if it's out-of-date with regard to the config.
+        if self.live_imports.is_some() {
+            return false;
+        }
+
+        // We must have the exact same set of imports, otherwise an import has
+        // been added or removed and we're out of date.
+        if self.config.imports.keys().ne(self.imports.audits.keys()) {
+            return true;
+        }
+
+        for (import_name, config) in &self.config.imports {
+            let audits_file = self.imports.audits.get(import_name).unwrap();
+            // If we have any excluded crates in the imports.lock, it is out of
+            // date and needs to be regenerated.
+            for crate_name in &config.exclude {
+                if audits_file.audits.contains_key(crate_name) {
+                    return true;
+                }
+            }
+        }
+
+        false
+    }
+
     /// Return an updated version of the `imports.lock` file taking into account
     /// the result of running the resolver, to pick which audits need to be
     /// vendored.
@@ -622,6 +656,7 @@ impl Store {
 /// description of the live state of imported audits.
 fn process_imported_audits(
     fetched_audits: Vec<(ImportName, AuditsFile)>,
+    config_file: &ConfigFile,
     imports_lock: &ImportsFile,
     allow_criteria_changes: bool,
 ) -> Result<ImportsFile, CriteriaChangeErrors> {
@@ -630,6 +665,17 @@ fn process_imported_audits(
     };
     let mut changed_criteria = Vec::new();
     for (import_name, mut audits_file) in fetched_audits {
+        let config = config_file
+            .imports
+            .get(&import_name)
+            .expect("fetched audit without config?");
+
+        // Remove any excluded audits from the live copy. We'll effectively
+        // pretend they don't exist upstream.
+        for excluded in &config.exclude {
+            audits_file.audits.remove(excluded);
+        }
+
         // By default all audits read from the network are fresh.
         for audit_entry in audits_file.audits.values_mut().flat_map(|v| v.iter_mut()) {
             audit_entry.is_fresh_import = true;


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 6f3b8a5..1f71f68 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -69,7 +69,7 @@ fn new_peer_import() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -81,7 +81,7 @@ fn new_peer_import() {
         OTHER_FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: OTHER_FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -141,7 +141,7 @@ fn existing_peer_skip_import() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -195,7 +195,7 @@ fn existing_peer_remove_unused() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -283,7 +283,7 @@ fn existing_peer_import_delta_audit() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -295,7 +295,7 @@ fn existing_peer_import_delta_audit() {
         OTHER_FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: OTHER_FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -366,7 +366,7 @@ fn existing_peer_import_custom_criteria() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -428,7 +428,7 @@ fn new_audit_for_unused_criteria_basic() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -494,7 +494,7 @@ fn new_audit_for_unused_criteria_transitive() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -546,7 +546,7 @@ fn existing_peer_revoked_audit() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -607,7 +607,7 @@ fn existing_peer_add_violation() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -658,7 +658,7 @@ fn peer_audits_exemption_no_minimize() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -709,7 +709,7 @@ fn peer_audits_exemption_minimize() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -734,6 +734,86 @@ fn peer_audits_exemption_minimize() {
     insta::assert_snapshot!(output);
 }
 
+#[test]
+fn peer_audits_import_exclusion() {
+    // (Pass) Exclusions in the config should make a crate's audits and
+    // violations appear to be revoked upstream, but audits for other crates
+    // shouldn't be impacted.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, mut imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("transitive-third-party1");
+
+    let old_foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [
+            (
+                "third-party2".to_owned(),
+                vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+            ),
+            (
+                "third-party1".to_owned(),
+                vec![violation("*".parse().unwrap(), SAFE_TO_DEPLOY)],
+            ),
+            (
+                "transitive-third-party1".to_owned(),
+                vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+            ),
+        ]
+        .into_iter()
+        .collect(),
+    };
+
+    let new_foreign_audits = old_foreign_audits.clone();
+
+    imports
+        .audits
+        .insert(FOREIGN.to_owned(), old_foreign_audits);
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: FOREIGN_URL.to_owned(),
+            exclude: vec!["third-party1".to_owned(), "third-party2".to_owned()],
+            ..Default::default()
+        },
+    );
+
+    let store = Store::mock_online(
+        config,
+        audits,
+        imports,
+        vec![(FOREIGN.to_owned(), new_foreign_audits)],
+        true,
+    )
+    .unwrap();
+
+    let imported = store
+        .imported_audits()
+        .get(FOREIGN)
+        .expect("The remote should be present in `imported_audits`");
+
+    assert!(
+        !imported.audits.contains_key("third-party1"),
+        "The `third-party1` crate should be completely missing from `imported_audits`"
+    );
+    assert!(
+        !imported.audits.contains_key("third-party2"),
+        "The `third-party2` crate should be completely missing from `imported_audits`"
+    );
+    assert!(
+        imported.audits.contains_key("transitive-third-party1"),
+        "The `transitive-third-party1` crate should still be present in `imported_audits`"
+    );
+
+    let output = get_imports_file_changes(&metadata, &store, false);
+    insta::assert_snapshot!(output);
+}
+
 // Other tests worth adding:
 //
 // - used edges with dependency-criteria should cause criteria to be imported
diff --git a/src/tests/snapshots/cargo_vet__tests__import__peer_audits_import_exclusion.snap b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_import_exclusion.snap
new file mode 100644
index 0000000..0d8ddc3
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_import_exclusion.snap
@@ -0,0 +1,17 @@
+---
+source: src/tests/import.rs
+expression: output
+---
+ 
+-[[audits.peer-company.audits.third-party1]]
+-criteria = "safe-to-deploy"
+-violation = "*"
+-
+-[[audits.peer-company.audits.third-party2]]
+-criteria = "safe-to-deploy"
+-version = "10.0.0"
+-
+ [[audits.peer-company.audits.transitive-third-party1]]
+ criteria = "safe-to-deploy"
+ version = "10.0.0"
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_excluded_crate.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_excluded_crate.snap
new file mode 100644
index 0000000..24259b1
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_excluded_crate.snap
@@ -0,0 +1,11 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Your cargo-vet store (supply-chain) has consistency errors
+
+Error: 
+  × imports.lock is out-of-date with respect to configuration
+  help: run `cargo vet` without --locked to update imports
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_extra_peer.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_extra_peer.snap
new file mode 100644
index 0000000..24259b1
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_extra_peer.snap
@@ -0,0 +1,11 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Your cargo-vet store (supply-chain) has consistency errors
+
+Error: 
+  × imports.lock is out-of-date with respect to configuration
+  help: run `cargo vet` without --locked to update imports
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_missing_peer.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_missing_peer.snap
new file mode 100644
index 0000000..24259b1
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_missing_peer.snap
@@ -0,0 +1,11 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Your cargo-vet store (supply-chain) has consistency errors
+
+Error: 
+  × imports.lock is out-of-date with respect to configuration
+  help: run `cargo vet` without --locked to update imports
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_ok.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_ok.snap
new file mode 100644
index 0000000..802d949
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__outdated_imports_lock_ok.snap
@@ -0,0 +1,5 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
diff --git a/src/tests/store_parsing.rs b/src/tests/store_parsing.rs
index 89bdb8b..18a4c34 100644
--- a/src/tests/store_parsing.rs
+++ b/src/tests/store_parsing.rs
@@ -108,3 +108,106 @@ implies = ["safe-to-deploy"]
     let acquire_errors = get_valid_store(config, audits, EMPTY_IMPORTS);
     insta::assert_snapshot!(acquire_errors);
 }
+
+#[test]
+fn test_outdated_imports_lock_extra_peer() {
+    let config = r##"
+[imports.peer1]
+url = "https://peer1.com"
+criteria-map = []
+
+"##;
+
+    let imports = r##"
+[[audits.peer1.audits.third-party1]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+[[audits.peer2.audits.third-party2]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
+
+#[test]
+fn test_outdated_imports_lock_missing_peer() {
+    let config = r##"
+[imports.peer1]
+url = "https://peer1.com"
+criteria-map = []
+
+[imports.peer2]
+url = "https://peer2.com"
+criteria-map = []
+
+"##;
+
+    let imports = r##"
+[[audits.peer1.audits.third-party1]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
+
+#[test]
+fn test_outdated_imports_lock_excluded_crate() {
+    let config = r##"
+[imports.peer1]
+url = "https://peer1.com"
+exclude = ["third-party1"]
+criteria-map = []
+
+"##;
+
+    let imports = r##"
+[[audits.peer1.audits.third-party1]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+[[audits.peer1.audits.third-party2]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
+
+#[test]
+fn test_outdated_imports_lock_ok() {
+    let config = r##"
+[imports.peer1]
+url = "https://peer1.com"
+exclude = ["third-party2"]
+criteria-map = []
+
+[imports.peer2]
+url = "https://peer1.com"
+criteria-map = []
+
+"##;
+
+    let imports = r##"
+[[audits.peer1.audits.third-party1]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+[[audits.peer2.audits.third-party2]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
diff --git a/src/tests/vet.rs b/src/tests/vet.rs
index 8d560d7..e874a5c 100644
--- a/src/tests/vet.rs
+++ b/src/tests/vet.rs
@@ -2478,7 +2478,7 @@ fn builtin_simple_foreign_audited() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
     audits.audits.clear();
@@ -2504,7 +2504,7 @@ fn mock_simple_foreign_audited_pun_no_mapping() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
     audits.audits.clear();
@@ -2537,6 +2537,7 @@ fn mock_simple_foreign_audited_pun_mapped() {
                 ours: DEFAULT_CRIT.to_owned(),
                 theirs: vec![DEFAULT_CRIT.to_owned().into()],
             }],
+            ..Default::default()
         },
     );
     audits.audits.clear();
@@ -2570,13 +2571,14 @@ fn mock_simple_foreign_audited_pun_wrong_mapped() {
                 ours: DEFAULT_CRIT.to_owned(),
                 theirs: vec![DEFAULT_CRIT.to_owned().into()],
             }],
+            ..Default::default()
         },
     );
     config.imports.insert(
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: vec![],
+            ..Default::default()
         },
     );
 
@@ -2619,7 +2621,7 @@ fn builtin_simple_foreign_tag_team() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -2658,7 +2660,7 @@ fn builtin_simple_mega_foreign_tag_team() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
     imports.audits.insert(
@@ -2677,7 +2679,7 @@ fn builtin_simple_mega_foreign_tag_team() {
         OTHER_FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: OTHER_FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -2727,7 +2729,7 @@ fn builtin_simple_foreign_dep_criteria_fail() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );
 
@@ -2787,7 +2789,7 @@ fn builtin_simple_foreign_dep_criteria_pass() {
         FOREIGN.to_owned(),
         crate::format::RemoteImport {
             url: FOREIGN_URL.to_owned(),
-            criteria_map: Vec::new(),
+            ..Default::default()
         },
     );