feat: update patch manager to check signature on boot (#171)

* feat: store patch signature on disk with patch metadata

* add note about explicit lifetime

* fix tests

* feat: update patch manager to check signature on boot

* remove unused imports

* fix tests

* add log when no public key detected

* clean up log message

* pr feedback

* rename test

* rename tests null -> none

* change signature field name to hash_signature

* Add comment

* update print statements for clearer device logs

* fix test

* improve docs

* cleanup of signing error handling

Cargo.lock

@@ -107,6 +107,12 @@ version = "0.21.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "35636a1494ede3b646cc98f74f8e62c773a38a659ebc777a2cf26b9b74171df9"
 
+[[package]]
+name = "base64"
+version = "0.22.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6"
+
 [[package]]
 name = "bidiff"
 version = "1.0.0"
@@ -1220,7 +1226,7 @@ version = "0.11.22"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "046cd98826c46c2ac8ddecae268eb5c2e58628688a5fc7a2643704a73faba95b"
 dependencies = [
- "base64",
+ "base64 0.21.5",
  "bytes",
  "encoding_rs",
  "futures-core",
@@ -1256,16 +1262,17 @@ dependencies = [
 
 [[package]]
 name = "ring"
-version = "0.17.7"
+version = "0.17.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "688c63d65483050968b2a8937f7995f443e27041a0f7700aa59b0822aedebb74"
+checksum = "c17fa4cb658e3583423e915b9f3acc01cceaee1860e33d59ebae66adc3a2dc0d"
 dependencies = [
  "cc",
+ "cfg-if",
  "getrandom",
  "libc",
  "spin",
  "untrusted",
- "windows-sys 0.48.0",
+ "windows-sys 0.52.0",
 ]
 
 [[package]]
@@ -1305,7 +1312,7 @@ version = "1.0.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "1c74cae0a4cf6ccbbf5f359f08efdf8ee7e1dc532573bf0db71968cb56b1448c"
 dependencies = [
- "base64",
+ "base64 0.21.5",
 ]
 
 [[package]]
@@ -1755,6 +1762,7 @@ version = "0.1.0"
 dependencies = [
  "android_logger",
  "anyhow",
+ "base64 0.22.1",
  "bipatch",
  "cbindgen",
  "comde",
@@ -1770,6 +1778,7 @@ dependencies = [
  "oslog",
  "pipe",
  "reqwest",
+ "ring",
  "serde",
  "serde_json",
  "serde_yaml",

library/Cargo.toml

@@ -14,6 +14,7 @@ crate-type = ["lib", "cdylib", "staticlib"]
 [dependencies]
 # Used for error handling for now.
 anyhow = { version = "1.0.69", features = ["backtrace"] }
+base64 = "0.22.0"
 # For inflating compressed patch files.
 bipatch = "1.0.0"
 # Used for exposing C API
@@ -41,6 +42,7 @@ reqwest = { version = "0.11", default-features = false, features = [
     "json",
     "rustls-tls",
 ] }
+ring = "0.17.8"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0.93"
 # For reading shorebird.yaml

library/src/c_api/mod.rs

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

library/src/cache/mod.rs

@@ -1,5 +1,6 @@
 mod disk_io;
 mod patch_manager;
+mod signing;
 pub mod updater_state;
 
 pub use updater_state::UpdaterState;

library/src/cache/patch_manager.rs

@@ -1,4 +1,4 @@
-use super::{disk_io, PatchInfo};
+use super::{disk_io, signing, PatchInfo};
 use anyhow::{bail, Context, Result};
 use core::fmt::Debug;
 use serde::{Deserialize, Serialize};
@@ -124,18 +124,23 @@ pub struct PatchManager {
 
     /// Metadata about the patches we have downloaded that is persisted to disk.
     patches_state: PatchesState,
+
+    /// The key used to sign patch hashes for the current release, if any. If this is
+    /// not None, all patches must have a signature that can be verified with this key.
+    patch_public_key: Option<String>,
 }
 
 impl PatchManager {
     /// Creates a new PatchManager with the given root directory. This directory is
     /// assumed to exist. The PatchManager will use this directory to store its
     /// state and patch binaries.
-    pub fn with_root_dir(root_dir: PathBuf) -> Self {
+    pub fn new(root_dir: PathBuf, patch_public_key: Option<&str>) -> Self {
         let patches_state = Self::load_patches_state(&root_dir).unwrap_or_default();
 
         Self {
             root_dir,
             patches_state,
+            patch_public_key: patch_public_key.map(|s| s.to_owned()),
         }
     }
 
@@ -229,6 +234,20 @@ impl PatchManager {
             }
         }
 
+        if let Some(public_key) = &self.patch_public_key {
+            // If we have a public key, verify that the patch's hash has a signature.
+            let signature = patch
+                .signature
+                .clone()
+                .context("Patch signature is missing")?;
+
+            // Check that the signature is valid.
+            let patch_hash = signing::hash_file(&artifact_path)?;
+            signing::check_signature(&patch_hash, &signature, public_key)?;
+        } else {
+            info!("No public key provided, skipping signature verification");
+        }
+
         Ok(())
     }
 
@@ -471,15 +490,32 @@ impl ManagePatches for PatchManager {
 #[cfg(test)]
 impl PatchManager {
     pub fn manager_for_test(temp_dir: &TempDir) -> PatchManager {
-        PatchManager::with_root_dir(temp_dir.path().to_owned())
+        PatchManager::new(temp_dir.path().to_owned(), None)
     }
 
     pub fn add_patch_for_test(&mut self, temp_dir: &TempDir, patch_number: usize) -> Result<()> {
+        self.add_signed_patch_for_test(temp_dir, patch_number, "hash", None)
+    }
+
+    pub fn add_signed_patch_for_test(
+        &mut self,
+        temp_dir: &TempDir,
+        patch_number: usize,
+        hash: &str,
+        signature: Option<&str>,
+    ) -> Result<()> {
         let file_path = &temp_dir
             .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", None)
+        info!(
+            "Adding patch {} with contents {} hash {} at {}",
+            patch_number,
+            patch_number.to_string().repeat(patch_number),
+            hash,
+            file_path.display()
+        );
+        self.add_patch(patch_number, file_path, hash, signature)
     }
 }
 
@@ -492,18 +528,17 @@ mod debug_tests {
     #[test]
     fn manage_patches_is_debug() {
         let temp_dir = TempDir::new("patch_manager").unwrap();
-        let patch_manager: Box<dyn super::ManagePatches> = Box::new(
-            super::PatchManager::with_root_dir(temp_dir.path().to_owned()),
-        );
+        let patch_manager: Box<dyn super::ManagePatches> =
+            Box::new(PatchManager::manager_for_test(&temp_dir));
         assert_eq!(format!("{:?}", patch_manager), "ManagePatches");
     }
 
     #[test]
     fn patch_manager_is_debug() {
         let temp_dir = TempDir::new("patch_manager").unwrap();
-        let patch_manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let patch_manager = PatchManager::new(temp_dir.path().to_owned(), Some("public_key"));
         let expected_str = format!(
-            "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }} }}",
+            "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}",
             temp_dir.path().display()
         );
         assert_eq!(format!("{:?}", patch_manager), expected_str);
@@ -534,7 +569,7 @@ mod add_patch_tests {
         let patch_number = 1;
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager").unwrap();
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents).unwrap();
@@ -632,7 +667,7 @@ mod next_boot_patch_tests {
     #[test]
     fn returns_none_if_no_next_boot_patch() {
         let temp_dir = TempDir::new("patch_manager").unwrap();
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         assert!(manager.next_boot_patch().is_none());
     }
 
@@ -662,7 +697,7 @@ mod next_boot_patch_tests {
     fn clears_current_and_next_on_boot_failure_if_they_are_the_same() -> Result<()> {
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         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", None).is_ok());
@@ -688,7 +723,7 @@ mod next_boot_patch_tests {
     fn falls_back_to_last_booted_patch_if_still_bootable() -> Result<()> {
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
 
@@ -714,7 +749,7 @@ mod next_boot_patch_tests {
     fn does_not_fall_back_to_last_booted_patch_if_corrupted() -> Result<()> {
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
 
@@ -741,7 +776,7 @@ mod next_boot_patch_tests {
     }
 
     #[test]
-    fn returns_null_patch_if_first_patch_failed_to_boot() -> Result<()> {
+    fn returns_none_patch_if_first_patch_failed_to_boot() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
         let mut manager = PatchManager::manager_for_test(&temp_dir);
 
@@ -778,7 +813,7 @@ mod next_boot_patch_tests {
     }
 
     #[test]
-    fn returns_null_if_first_patch_did_not_successfully_boot() -> Result<()> {
+    fn returns_none_if_first_patch_did_not_successfully_boot() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
         let mut manager = PatchManager::manager_for_test(&temp_dir);
 
@@ -792,7 +827,7 @@ mod next_boot_patch_tests {
     }
 
     #[test]
-    fn returns_null_if_next_patch_did_not_successfully_boot() -> Result<()> {
+    fn returns_none_if_next_patch_did_not_successfully_boot() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
         let mut manager = PatchManager::manager_for_test(&temp_dir);
 
@@ -810,6 +845,97 @@ mod next_boot_patch_tests {
 
         Ok(())
     }
+
+    // The constant values below were generated by taking an arbitrary sha256 hash (INFLATED_PATCH_HASH)
+    // and using openssl to sign it with the private key corresponding to `PUBLIC_KEY`.
+
+    // The base64-encoded public key in a DER format. This is required by ring to verify signatures.
+    // See https://docs.rs/ring/latest/ring/signature/index.html#signing-and-verifying-with-rsa-pkcs1-15-padding
+    const PUBLIC_KEY: &str = "MIIBCgKCAQEA2wdpEGbuvlPsb9i0qYrfMefJnEw1BHTi8SYZTKrXOvJWmEpPE1hWfbkvYzXu5a96gV1yocF3DMwn04VmRlKhC4AhsD0NL0UNhYhotbKG91Kwi1vAXpHhCdz5gQEBw0K1uB4Jz+zK6WK+31PryYpwLwbyXNqXoY8IAAUQ4STsHYV5w+BMSi8pepWMRd7DR9RHcbNOZlJvdBQ5NxvB4JN4dRMq8cC73ez1P9d7Dfwv3TWY+he9EmuXLT2UivZSlHIrGBa7MFfqyUe2ro0F7Te/B0si12itBbWIqycvqcXjeOPNn6WEpqN7IWjb9LUh162JyYaz5Lb/VeeJX8LKtElccwIDAQAB";
+
+    // The message that was signed. In practice, this will be the sha256 hash of an inflated patch artifact.
+    const INFLATED_PATCH_HASH: &str =
+        "6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b";
+
+    // The base64-encoded signature of `INFLATED_PATCH_HASH` created using the private key corresponding
+    // to `PUBLIC_KEY`.
+    const SIGNATURE: &str = "ZGccldv01XqHQ76bXuKV/9EQnNK0Q+reQ9bJHVnGfLldF+BLRx0divgPfKP5Df9BJPA3dw1Z1VortfepmMGebP3kS593l5zoktu9MIepxvRAFWNKE5PDTIIvCL/ddTPEHt6NNCeD6HLOMLzbEX3cFZa+lq3UymGi0aqA5DlXirJBGtopojc9nOXZ22n/qHNZIHEkGcqKbSMSK9oC55whKHnlJTbCXdmSyDc65B4PcgseqJom1riVK3XGW1YMrSpuMAU+CDT7HhdESmI1UtH1bYeBITfRhQztdDTfti2vJTf2Y+lYC99CFiISgD7f1m0KUcC+VnEAMZSYtgxSk6AX2A==";
+
+    #[test]
+    fn returns_none_if_public_key_is_invalid() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some("not a valid key"));
+
+        manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?;
+
+        assert!(manager.next_boot_patch().is_none());
+
+        Ok(())
+    }
+
+    #[test]
+    fn returns_none_if_patch_is_missing_expected_signature() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY));
+
+        manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, None)?;
+
+        assert!(manager.next_boot_patch().is_none());
+
+        Ok(())
+    }
+
+    #[test]
+    fn returns_none_if_patch_has_invalid_signature() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY));
+
+        // Using MESSAGE as a signature because it is valid base64, but not a valid signature.
+        manager.add_signed_patch_for_test(
+            &temp_dir,
+            1,
+            INFLATED_PATCH_HASH,
+            Some(INFLATED_PATCH_HASH),
+        )?;
+
+        assert!(manager.next_boot_patch().is_none());
+
+        Ok(())
+    }
+
+    #[test]
+    fn returns_patch_if_patch_has_valid_signature() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY));
+
+        manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?;
+
+        assert!(manager.next_boot_patch().is_some());
+        let patch = manager.next_boot_patch().unwrap();
+        assert_eq!(patch.number, 1);
+
+        Ok(())
+    }
+
+    #[test]
+    fn returns_patch_with_arbitrary_signature_if_no_public_key() -> Result<()> {
+        let temp_dir = TempDir::new("patch_manager")?;
+        // Create a PatchManager without a public key.
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
+
+        manager.add_signed_patch_for_test(
+            &temp_dir,
+            1,
+            INFLATED_PATCH_HASH,
+            Some("not a valid signature"),
+        )?;
+
+        assert!(manager.next_boot_patch().is_some());
+        let patch = manager.next_boot_patch().unwrap();
+        assert_eq!(patch.number, 1);
+
+        Ok(())
+    }
 }
 
 #[cfg(test)]
@@ -819,7 +945,7 @@ mod fall_back_tests {
     #[test]
     fn does_nothing_if_no_patch_exists() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         assert!(manager.patches_state.last_booted_patch.is_none());
         assert!(manager.patches_state.next_boot_patch.is_none());
@@ -835,7 +961,7 @@ mod fall_back_tests {
     #[test]
     fn sets_next_patch_to_latest_patch_if_no_next_patch_exists() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         assert!(manager.patches_state.next_boot_patch.is_none());
 
@@ -858,7 +984,7 @@ mod fall_back_tests {
     #[test]
     fn sets_next_patch_to_latest_patch_if_both_are_present() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Download and successfully boot from patch 1
         manager.add_patch_for_test(&temp_dir, 1)?;
@@ -879,7 +1005,7 @@ mod fall_back_tests {
     #[test]
     fn clears_next_and_last_patches_if_both_fail_validation() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Download and successfully boot from patch 1, and then corrupt it on disk.
         manager.add_patch_for_test(&temp_dir, 1)?;
@@ -903,7 +1029,7 @@ mod fall_back_tests {
     #[test]
     fn does_not_clear_next_patch_if_changed_since_boot_start() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Simulate a situation where we download both patches 1 and 2.
         manager.add_patch_for_test(&temp_dir, 1)?;
@@ -927,7 +1053,7 @@ mod fall_back_tests {
     #[test]
     fn succeeds_if_deleting_artifacts_fails() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Download and successfully boot from patch 1, and then corrupt it on disk.
         manager.add_patch_for_test(&temp_dir, 1)?;
@@ -961,7 +1087,7 @@ mod record_boot_success_for_patch_tests {
     #[test]
     fn errs_if_no_next_boot_patch() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // This should fail because no patches have been added.
         assert!(manager.record_boot_success().is_err());
@@ -974,7 +1100,7 @@ mod record_boot_success_for_patch_tests {
         let patch_number = 1;
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
         assert!(manager
@@ -990,7 +1116,7 @@ mod record_boot_success_for_patch_tests {
         let patch_number = 1;
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
         assert!(manager
@@ -1008,7 +1134,7 @@ mod record_boot_success_for_patch_tests {
         let patch_number = 1;
         let patch_file_contents = "patch contents";
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
         let file_path = &temp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, patch_file_contents)?;
 
@@ -1044,7 +1170,7 @@ mod record_boot_success_for_patch_tests {
     #[test]
     fn deletes_other_patch_artifacts() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Download patches 1, 2, and 3 before we start booting from patch 2.
         manager.add_patch_for_test(&temp_dir, 1)?;
@@ -1076,7 +1202,7 @@ mod record_boot_success_for_patch_tests {
     #[test]
     fn deletes_unrecognized_directories_in_patches_dir() -> Result<()> {
         let temp_dir = TempDir::new("patch_manager")?;
-        let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned());
+        let mut manager = PatchManager::manager_for_test(&temp_dir);
 
         // Add a junk directory to the patches directory.
         let junk_dir = manager.patches_dir().join("junk");

library/src/cache/signing.rs

@@ -0,0 +1,134 @@
+use anyhow::{bail, Context, Result};
+use base64::Engine;
+use std::path::Path;
+// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests
+#[cfg(test)]
+use std::{println as info, println as debug}; // Workaround to use println! for logs.
+
+/// Reads the file at `path` and returns the SHA-256 hash of its contents as a String.
+pub fn hash_file<P: AsRef<Path>>(path: P) -> Result<String> {
+    use sha2::{Digest, Sha256}; // `Digest` is needed for `Sha256::new()`;
+
+    let mut file = std::fs::File::open(path)?;
+    let mut hasher = Sha256::new();
+    std::io::copy(&mut file, &mut hasher)?;
+    let hash = hasher.finalize();
+    Ok(hex::encode(hash))
+}
+
+/// `public_key` is a DER base64-encoded RSA public key.
+///
+/// Given a public_key.pem file, this can be generated with the following command:
+///   openssl rsa -pubin \
+///     -in public_key.pem \
+///     -inform PEM \
+///     -RSAPublicKey_out \
+///     -outform DER \
+///     -out public_key.der
+///
+/// See https://docs.rs/ring/latest/ring/signature/index.html#signing-and-verifying-with-rsa-pkcs1-15-padding
+/// for more information.
+pub fn check_signature(message: &str, signature: &str, public_key: &str) -> Result<()> {
+    debug!("Message is {}", message);
+    debug!("Public key is {:?}", public_key);
+    debug!("Signature is {}", signature);
+
+    let public_key_bytes = base64::prelude::BASE64_STANDARD
+        .decode(public_key)
+        .with_context(|| format!("Failed to decode public_key: {}", public_key))?;
+    let public_key = ring::signature::UnparsedPublicKey::new(
+        &ring::signature::RSA_PKCS1_2048_8192_SHA256,
+        public_key_bytes,
+    );
+    let decoded_sig = base64::prelude::BASE64_STANDARD
+        .decode(signature)
+        .map_err(|e| anyhow::Error::msg(format!("Failed to decode signature: {:?}", e)))?;
+
+    info!("Verifying patch signature...");
+    match public_key.verify(message.as_bytes(), &decoded_sig) {
+        Ok(_) => {
+            info!("Patch signature is valid");
+            Ok(())
+        }
+        Err(_) => {
+            // The error provided by `verify` is (by design) not helpful, so we ignore it.
+            // See https://docs.rs/ring/latest/ring/error/struct.Unspecified.html
+            bail!("Patch signature is invalid")
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    // The constant values below were generated by taking an arbitrary hash (`MESSAGE`) and
+    // using openssl to sign it with a private key.
+
+    // The base64-encoded public half of the key pair used to sign `MESSAGE`.
+    const PUBLIC_KEY: &str = "MIIBCgKCAQEA2wdpEGbuvlPsb9i0qYrfMefJnEw1BHTi8SYZTKrXOvJWmEpPE1hWfbkvYzXu5a96gV1yocF3DMwn04VmRlKhC4AhsD0NL0UNhYhotbKG91Kwi1vAXpHhCdz5gQEBw0K1uB4Jz+zK6WK+31PryYpwLwbyXNqXoY8IAAUQ4STsHYV5w+BMSi8pepWMRd7DR9RHcbNOZlJvdBQ5NxvB4JN4dRMq8cC73ez1P9d7Dfwv3TWY+he9EmuXLT2UivZSlHIrGBa7MFfqyUe2ro0F7Te/B0si12itBbWIqycvqcXjeOPNn6WEpqN7IWjb9LUh162JyYaz5Lb/VeeJX8LKtElccwIDAQAB";
+
+    // The message that was signed.
+    const MESSAGE: &str = "404e5caa5b906f6d03c97657e8c4d604d759f9cfba1a8bba9d5b49a5ebc174f9";
+
+    // The base64-encoded signature of `MESSAGE` using the private key corresponding to `PUBLIC_KEY`.
+    const SIGNATURE: &str = "2ixSo5LpaWUSLg2GJEV+D+uyLeLjp0c3vNXnl0yb1iJjAdpn10BFlbcwCcjaJW9PNky2HU2hKOBe62PkFHOU8DDYOfxf2LGg/ToLGPHin85WrwFAceAUYDs7JpQr43dRTbrXcT8k5tuCQOTwXecGwuWcOFFvh0GbXFnyAmi7fLfN9CtTsG2GIOle/LyYLwoviTrXn/fZTZEYrqxD/wZ4QzoWOWLWNvrPbILhqWELkBLhdZeK0+nC2CIxFRYd3bUeOi1AGtPyHKBfdwuf4VO3+HbwJVaAEiD7HU2Bj+Zp1xeSdbznmYgBV86oizrLFd23D+lBfTlmDGgdfNE9J4Z2/g==";
+
+    use std::io::Write;
+
+    use anyhow::Result;
+    use tempdir::TempDir;
+
+    #[test]
+    fn errs_if_file_does_not_exist() {
+        let path = "/tmp/does_not_exist";
+        let result = super::hash_file(path);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn hashes_file_contents() -> Result<()> {
+        // Write "hello, world!" to a file.
+        let temp_dir = TempDir::new("signing")?;
+        let file_path = temp_dir.path().join("test.txt");
+        let mut file = std::fs::File::create(&file_path)?;
+        file.write_all("hello, world!".as_bytes())?;
+
+        // Verify that the hash is correct.
+        let hashed = super::hash_file(file_path)?;
+        assert_eq!(
+            &hashed,
+            "68e656b251e67e8358bef8483ab0d51c6619f3e7a1a9f0e75838d41ff368f728"
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn errs_if_public_key_cannot_be_decoded() {
+        let result = super::check_signature(MESSAGE, SIGNATURE, "bad_public_key");
+        assert!(result.is_err());
+        let error = result.unwrap_err().to_string();
+        assert_eq!(error, "Failed to decode public_key: bad_public_key");
+    }
+
+    #[test]
+    fn errs_if_signature_cannot_be_decoded() {
+        let result = super::check_signature(MESSAGE, "signature", PUBLIC_KEY);
+        assert!(result.is_err());
+        let error = result.unwrap_err().to_string();
+        assert!(error.starts_with("Failed to decode signature"));
+    }
+
+    #[test]
+    fn errs_if_signature_is_not_valid() {
+        // Pass PUBLIC_KEY as the signature to ensure that the signature is invalid.
+        let result = super::check_signature(MESSAGE, PUBLIC_KEY, PUBLIC_KEY);
+        assert!(result.is_err());
+        let error = result.unwrap_err().to_string();
+        assert!(error.starts_with("Patch signature is invalid"));
+    }
+
+    #[test]
+    fn is_ok_if_signature_is_valid() {
+        let result = super::check_signature(MESSAGE, SIGNATURE, PUBLIC_KEY);
+        assert!(result.is_ok());
+    }
+}

library/src/cache/updater_state.rs

@@ -70,10 +70,10 @@ fn is_file_not_found(error: &anyhow::Error) -> bool {
 /// Lifecycle methods for the updater state.
 impl UpdaterState {
     /// Creates a new `UpdaterState`.
-    fn new(cache_dir: PathBuf, release_version: String) -> Self {
+    fn new(cache_dir: PathBuf, release_version: String, patch_public_key: Option<&str>) -> Self {
         Self {
             cache_dir: cache_dir.clone(),
-            patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.clone())),
+            patch_manager: Box::new(PatchManager::new(cache_dir.clone(), patch_public_key)),
             serialized_state: SerializedState {
                 release_version,
                 queued_events: Vec::new(),
@@ -82,27 +82,39 @@ impl UpdaterState {
     }
 
     /// Loads UpdaterState from disk
-    fn load(cache_dir: &Path) -> anyhow::Result<Self> {
+    fn load(cache_dir: &Path, patch_public_key: Option<&str>) -> anyhow::Result<Self> {
         let path = cache_dir.join(STATE_FILE_NAME);
         let serialized_state = disk_io::read(&path)?;
         Ok(UpdaterState {
             cache_dir: cache_dir.to_path_buf(),
-            patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.to_path_buf())),
+            patch_manager: Box::new(PatchManager::new(cache_dir.to_path_buf(), patch_public_key)),
             serialized_state,
         })
     }
 
     /// Initializes a new UpdaterState and saves it to disk.
-    fn create_new_and_save(storage_dir: &Path, release_version: &str) -> Self {
-        let state = Self::new(storage_dir.to_owned(), release_version.to_owned());
+    fn create_new_and_save(
+        storage_dir: &Path,
+        release_version: &str,
+        patch_public_key: Option<&str>,
+    ) -> Self {
+        let state = Self::new(
+            storage_dir.to_owned(),
+            release_version.to_owned(),
+            patch_public_key,
+        );
         if let Err(e) = state.save() {
             warn!("Error saving state {:?}, ignoring.", e);
         }
         state
     }
 
-    pub fn load_or_new_on_error(storage_dir: &Path, release_version: &str) -> Self {
-        let load_result = Self::load(storage_dir);
+    pub fn load_or_new_on_error(
+        storage_dir: &Path,
+        release_version: &str,
+        patch_public_key: Option<&str>,
+    ) -> Self {
+        let load_result = Self::load(storage_dir, patch_public_key);
         match load_result {
             Ok(mut loaded) => {
                 if loaded.serialized_state.release_version != release_version {
@@ -111,7 +123,11 @@ impl UpdaterState {
                         loaded.serialized_state.release_version, release_version
                     );
                     let _ = loaded.patch_manager.reset();
-                    return Self::create_new_and_save(storage_dir, release_version);
+                    return Self::create_new_and_save(
+                        storage_dir,
+                        release_version,
+                        patch_public_key,
+                    );
                 }
                 loaded
             }
@@ -119,7 +135,7 @@ impl UpdaterState {
                 if !is_file_not_found(&e) {
                     info!("No existing state file found: {:#}, creating new state.", e);
                 }
-                Self::create_new_and_save(storage_dir, release_version)
+                Self::create_new_and_save(storage_dir, release_version, patch_public_key)
             }
         }
     }
@@ -252,7 +268,7 @@ mod tests {
     #[test]
     fn release_version_changed_resets_patches() {
         let tmp_dir = TempDir::new("example").unwrap();
-        let mut patch_manager = PatchManager::with_root_dir(tmp_dir.path().to_path_buf());
+        let mut patch_manager = PatchManager::manager_for_test(&tmp_dir);
         let file_path = &tmp_dir.path().join("patch1.vmcode");
         std::fs::write(file_path, "patch file contents").unwrap();
         assert!(patch_manager.add_patch(1, file_path, "hash", None).is_ok());
@@ -261,11 +277,12 @@ mod tests {
         let release_version = state.serialized_state.release_version.clone();
         assert!(state.save().is_ok());
 
-        let mut state = UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version);
+        let mut state =
+            UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version, None);
         assert_eq!(state.next_boot_patch().unwrap().number, 1);
 
         let mut next_version_state =
-            UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2");
+            UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2", None);
         assert!(next_version_state.next_boot_patch().is_none());
         assert!(next_version_state.latest_seen_patch_number().is_none());
     }
@@ -286,9 +303,7 @@ mod tests {
         let original_tmp_dir = TempDir::new("example").unwrap();
         let original_state = UpdaterState {
             cache_dir: original_tmp_dir.path().to_path_buf(),
-            patch_manager: Box::new(PatchManager::with_root_dir(
-                original_tmp_dir.path().to_path_buf(),
-            )),
+            patch_manager: Box::new(PatchManager::manager_for_test(&original_tmp_dir)),
             serialized_state: SerializedState {
                 release_version: "1.0.0+1".to_string(),
                 queued_events: Vec::new(),
@@ -301,7 +316,7 @@ mod tests {
         let new_state_path = new_tmp_dir.path().join(STATE_FILE_NAME);
         std::fs::rename(original_state_path, new_state_path).unwrap();
 
-        let new_state = UpdaterState::load(new_tmp_dir.path()).unwrap();
+        let new_state = UpdaterState::load(new_tmp_dir.path(), None).unwrap();
         assert_eq!(new_state.cache_dir, new_tmp_dir.path());
     }
 

library/src/network.rs

@@ -148,7 +148,7 @@ pub struct Patch {
     pub download_url: String,
     /// The signature of `hash`, if this patch is signed. None otherwise.
     #[serde(default)]
-    pub signature: Option<String>,
+    pub hash_signature: Option<String>,
 }
 
 /// Any edits to this struct should be made carefully and in accordance

library/src/updater.rs

@@ -163,8 +163,11 @@ fn check_for_update_internal() -> anyhow::Result<PatchCheckResponse> {
     let (request, url, request_fn) = with_config(|config| {
         // Load UpdaterState from disk
         // If there is no state, make an empty state.
-        let state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
 
         // Get the required info to make the request.
         Ok((
@@ -258,8 +261,11 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     // We should never try to write this state as some other writer may be
     // racing with us, we should get a new state inside a lock if we want
     // to write.
-    let read_only_state =
-        UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+    let read_only_state = UpdaterState::load_or_new_on_error(
+        &config.storage_dir,
+        &config.release_version,
+        config.patch_public_key.as_deref(),
+    );
 
     // We discard any events if we have more than 3 queued to make sure
     // we don't stall the client.
@@ -272,8 +278,11 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     }
     // We're abusing the config lock as a UpdateState lock for now.
     let read_only_state = with_config(|_| {
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
         // This will clear any events which got queued between the time we
         // loaded the state now, but that's OK for now.
         let result = state.clear_events();
@@ -318,10 +327,13 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
             path: output_path,
             number: patch.number,
         };
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
         // Move/state update should be "atomic" (it isn't today).
-        state.install_patch(&patch_info, &patch.hash, patch.signature.as_deref())?;
+        state.install_patch(&patch_info, &patch.hash, patch.hash_signature.as_deref())?;
         info!("Patch {} successfully installed.", patch.number);
         // Should set some state to say the status is "update required" and that
         // we now have a different "next" version of the app from the current
@@ -389,8 +401,11 @@ where
 ///  3. `report_launch_failure()`
 pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
         Ok(state.next_boot_patch())
     })
 }
@@ -400,8 +415,11 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
 /// `next_boot_patch`.
 pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
-        let state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
         Ok(state.current_boot_patch())
     })
 }
@@ -414,8 +432,11 @@ pub fn report_launch_start() -> anyhow::Result<()> {
     info!("Reporting launch start.");
 
     with_config(|config| {
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
 
         let next_boot_patch = match state.next_boot_patch() {
             Some(patch) => patch,
@@ -432,8 +453,11 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
     info!("Reporting failed launch.");
 
     with_config(|config| {
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
 
         let patch = state
             .last_attempted_boot_patch()
@@ -464,8 +488,11 @@ pub fn report_launch_success() -> anyhow::Result<()> {
     with_config(|config| {
         // We can tell the UpdaterState that we have successfully booted from the "next" patch
         // and make that the "current" patch.
-        let mut state =
-            UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+        let mut state = UpdaterState::load_or_new_on_error(
+            &config.storage_dir,
+            &config.release_version,
+            config.patch_public_key.as_deref(),
+        );
 
         let last_attempted_boot_patch = match state.last_attempted_boot_patch() {
             Some(patch) => patch,
@@ -582,8 +609,11 @@ mod tests {
             fs::create_dir_all(&download_dir).unwrap();
             fs::write(&artifact_path, "hello").unwrap();
 
-            let mut state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let mut state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             state
                 .install_patch(
                     &PatchInfo {
@@ -703,8 +733,11 @@ mod tests {
             fs::create_dir_all(&download_dir).unwrap();
             fs::write(&artifact_path, "hello").unwrap();
 
-            let mut state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let mut state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             state
                 .install_patch(
                     &PatchInfo {
@@ -725,8 +758,11 @@ mod tests {
         super::report_launch_success().unwrap();
 
         with_config(|config| {
-            let state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             assert_eq!(state.current_boot_patch().unwrap().number, patch_number);
             Ok(())
         })
@@ -748,8 +784,11 @@ mod tests {
             fs::create_dir_all(&download_dir).unwrap();
             fs::write(&artifact_path, "hello").unwrap();
 
-            let mut state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let mut state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             state
                 .install_patch(
                     &PatchInfo {
@@ -770,8 +809,11 @@ mod tests {
         super::report_launch_failure().unwrap();
 
         with_config(|config| {
-            let mut state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let mut state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             // It's now bad.
             assert!(state.next_boot_patch().is_none());
             // And we've queued an event.
@@ -813,8 +855,11 @@ mod tests {
         init_for_testing(&tmp_dir, Some(&server.url()));
 
         with_config(|config| {
-            let mut state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let mut state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             let fail_event = PatchEvent {
                 app_id: config.app_id.clone(),
                 arch: current_arch().to_string(),
@@ -838,8 +883,11 @@ mod tests {
         event_mock.expect(3);
 
         with_config(|config| {
-            let state =
-                UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
+            let state = UpdaterState::load_or_new_on_error(
+                &config.storage_dir,
+                &config.release_version,
+                config.patch_public_key.as_deref(),
+            );
             // All 5 events should be cleared, even though only 3 were sent.
             assert_eq!(state.copy_events(10).len(), 0);
             Ok(())