feat: store patch signature on disk with patch metadata (#170)

* feat: store patch signature on disk with patch metadata

* add note about explicit lifetime

* fix tests

library/Cargo.toml

@@ -63,7 +63,7 @@ log-panics = { version = "2", features = ["with-backtrace"] }
 oslog = "0.2.0"
 
 [dev-dependencies]
-mockall = "0.11.4"
+mockall = "0.12.1"
 mockito = "1.2.0"
 # Gives #[serial] attribute for locking all of our shorebird_init
 # tests to a single thread so they don't conflict with each other.

library/src/c_api/mod.rs

@@ -480,6 +480,7 @@ mod test {
                         number: 1,
                         hash: hash.to_owned(),
                         download_url: "ignored".to_owned(),
+                        signature: None,
                     }),
                 })
             },
@@ -582,6 +583,7 @@ mod test {
                         number: 1,
                         hash: "ignored".to_owned(),
                         download_url: "ignored".to_owned(),
+                        signature: None,
                     }),
                 })
             },

library/src/cache/patch_manager.rs

@@ -28,6 +28,9 @@ struct PatchMetadata {
 
     /// The hash of the patch artifact on disk.
     hash: String,
+
+    /// The signature of `hash`.
+    signature: Option<String>,
 }
 
 /// What gets serialized to disk
@@ -53,7 +56,17 @@ struct PatchesState {
 pub trait ManagePatches {
     /// Copies the patch file at file_path to the manager's directory structure sets
     /// this patch as the next patch to boot.
-    fn add_patch(&mut self, number: usize, file_path: &Path, hash: &str) -> Result<()>;
+    ///
+    /// The explicit lifetime is required for automock to work with Options.
+    /// See https://github.com/asomers/mockall/issues/61.
+    #[allow(clippy::needless_lifetimes)]
+    fn add_patch<'a>(
+        &mut self,
+        number: usize,
+        file_path: &Path,
+        hash: &str,
+        signature: Option<&'a str>,
+    ) -> Result<()>;
 
     /// Returns the patch we most recently successfully booted from (usually the currently running patch),
     /// or None if no patch is installed.
@@ -314,7 +327,16 @@ impl PatchManager {
 }
 
 impl ManagePatches for PatchManager {
-    fn add_patch(&mut self, patch_number: usize, file_path: &Path, hash: &str) -> Result<()> {
+    // The explicit lifetime is required for automock to work with Options.
+    // See https://github.com/asomers/mockall/issues/61.
+    #[allow(clippy::needless_lifetimes)]
+    fn add_patch<'a>(
+        &mut self,
+        patch_number: usize,
+        file_path: &Path,
+        hash: &str,
+        signature: Option<&'a str>,
+    ) -> Result<()> {
         if !file_path.exists() {
             bail!("Patch file {} does not exist", file_path.display());
         }
@@ -330,6 +352,7 @@ impl ManagePatches for PatchManager {
             number: patch_number,
             size: std::fs::metadata(&patch_path)?.len(),
             hash: hash.to_owned(),
+            signature: signature.map(|s| s.to_owned()),
         };
 
         // If a patch was never booted (next_boot_patch != last_booted_patch), we should delete
@@ -456,7 +479,7 @@ impl PatchManager {
             .path()
             .join(format!("patch{}.vmcode", patch_number));
         std::fs::write(file_path, patch_number.to_string().repeat(patch_number)).unwrap();
-        self.add_patch(patch_number, file_path, "hash")
+        self.add_patch(patch_number, file_path, "hash", None)
     }
 }
 
@@ -497,7 +520,12 @@ mod add_patch_tests {
     fn errs_if_file_path_does_not_exist() {
         let mut manager = PatchManager::manager_for_test(&TempDir::new("patch_manager").unwrap());
         assert!(manager
-            .add_patch(1, Path::new("/path/to/file/that/does/not/exist"), "hash")
+            .add_patch(
+                1,
+                Path::new("/path/to/file/that/does/not/exist"),
+                "hash",
+                None,
+            )
             .is_err());
     }
 
@@ -512,7 +540,12 @@ mod add_patch_tests {
         std::fs::write(file_path, patch_file_contents).unwrap();
 
         assert!(manager
-            .add_patch(patch_number, Path::new(file_path), "hash")
+            .add_patch(
+                patch_number,
+                Path::new(file_path),
+                "hash",
+                Some("my_signature")
+            )
             .is_ok());
 
         assert_eq!(
@@ -521,6 +554,7 @@ mod add_patch_tests {
                 number: patch_number,
                 size: patch_file_contents.len() as u64,
                 hash: "hash".to_string(),
+                signature: Some("my_signature".to_owned())
             })
         );
         assert!(!file_path.exists());
@@ -538,19 +572,19 @@ mod add_patch_tests {
         // Add patch 1
         let file_path = &temp_dir.path().join("patch.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(1, file_path, "hash").is_ok());
+        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
         assert_eq!(manager.highest_seen_patch_number(), Some(1));
 
         // Add patch 4, expect 4 to be the highest patch number we've seen
         let file_path = &temp_dir.path().join("patch.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(4, file_path, "hash").is_ok());
+        assert!(manager.add_patch(4, file_path, "hash", None).is_ok());
         assert_eq!(manager.highest_seen_patch_number(), Some(4));
 
         // Add patch 3, expect 4 to still be the highest patch number we've seen
         let file_path = &temp_dir.path().join("patch.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(3, file_path, "hash").is_ok());
+        assert!(manager.add_patch(3, file_path, "hash", None).is_ok());
         assert_eq!(manager.highest_seen_patch_number(), Some(4));
 
         Ok(())
@@ -631,7 +665,7 @@ mod next_boot_patch_tests {
         let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(1, file_path, "hash").is_ok());
+        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
 
         // Write junk to the artifact, this should render the patch unbootable in the eyes
         // of the PatchManager.
@@ -659,14 +693,14 @@ mod next_boot_patch_tests {
         std::fs::write(file_path, patch_file_contents)?;
 
         // Add patch 1, pretend it booted successfully.
-        assert!(manager.add_patch(1, file_path, "hash").is_ok());
+        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
         assert!(manager.record_boot_start_for_patch(1).is_ok());
         assert!(manager.record_boot_success().is_ok());
 
         // Add patch 2, pretend it failed to boot.
         let file_path = &temp_dir.path().join("patch2.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(2, file_path, "hash").is_ok());
+        assert!(manager.add_patch(2, file_path, "hash", None).is_ok());
         assert!(manager.record_boot_start_for_patch(2).is_ok());
         assert!(manager.record_boot_failure_for_patch(2).is_ok());
 
@@ -685,14 +719,14 @@ mod next_boot_patch_tests {
         std::fs::write(file_path, patch_file_contents)?;
 
         // Add patch 1, pretend it booted successfully.
-        assert!(manager.add_patch(1, file_path, "hash").is_ok());
+        assert!(manager.add_patch(1, file_path, "hash", None).is_ok());
         assert!(manager.record_boot_start_for_patch(1).is_ok());
         assert!(manager.record_boot_success().is_ok());
 
         // Add patch 2, pretend it failed to boot.
         let file_path = &temp_dir.path().join("patch2.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(2, file_path, "hash").is_ok());
+        assert!(manager.add_patch(2, file_path, "hash", None).is_ok());
         assert!(manager.record_boot_start_for_patch(2).is_ok());
         assert!(manager.record_boot_failure_for_patch(2).is_ok());
 
@@ -809,6 +843,7 @@ mod fall_back_tests {
             number: 1,
             size: 1,
             hash: "hash".to_string(),
+            signature: Some("signature".to_owned()),
         });
         manager.try_fall_back_from_patch(1);
 
@@ -942,7 +977,9 @@ mod record_boot_success_for_patch_tests {
         let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(patch_number, file_path, "hash").is_ok());
+        assert!(manager
+            .add_patch(patch_number, file_path, "hash", None)
+            .is_ok());
         assert!(manager.record_boot_success().is_err());
 
         Ok(())
@@ -956,7 +993,9 @@ mod record_boot_success_for_patch_tests {
         let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
-        assert!(manager.add_patch(patch_number, file_path, "hash").is_ok());
+        assert!(manager
+            .add_patch(patch_number, file_path, "hash", None)
+            .is_ok());
 
         assert!(manager.record_boot_start_for_patch(1).is_ok());
         assert!(manager.record_boot_success().is_ok());
@@ -974,7 +1013,9 @@ mod record_boot_success_for_patch_tests {
         std::fs::write(file_path, patch_file_contents)?;
 
         // Add the patch, make sure it has an artifact.
-        assert!(manager.add_patch(patch_number, file_path, "hash").is_ok());
+        assert!(manager
+            .add_patch(patch_number, file_path, "hash", None)
+            .is_ok());
         let patch_artifact_path = manager.patch_artifact_path(patch_number);
         assert!(patch_artifact_path.exists());