diff --git a/library/README.md b/library/README.md index 3493445..31da009 100644 --- a/library/README.md +++ b/library/README.md @@ -214,14 +214,91 @@ Changes in this state can be triggered by: - Network and Disk are untrusted. - Running software (including apk service) is trusted. -- Patch contents are signed, public key is included in the APK. (not yet implemented) +- Patch contents are signed, public key is included in the APK. + +### Patch Verification Modes + +The updater supports two patch verification modes, configured via +`patch_verification` in `shorebird.yaml`. Both modes require a +`patch_public_key` to be configured for signature verification to occur. + +#### Strict Mode (default) + +```yaml +patch_verification: strict +``` + +In Strict mode, patch signature verification happens at **boot time**. This +provides the strongest security guarantee because it detects any potential +on-disk tampering to the patch file _after_ installation (e.g., if an attacker +were to modify the patch file on disk between app launches). However the +practical risk to such an attack is very low, since patches are stored within +the app's protected storage. An attacker in this case would need to have already +compromised the app itself, or the system (e.g. via a rooted device). However if +an attacker has compromised the system (rooted) they could already modify the +APK/IPA internals directly. The on-boot protection here is for cases where +developers are concerned that their app might be compromised and they wish to +ensure that such a compromise could not theoretically persist itself via editing +an installed patch file. Such a case is impractical, but we default to the +strongest-possible security stance regardless. + +Strict mode is currently default for Shorebird, however some of our large +customers requested that we add an install_only mode, since their applications +were so large (many hundreds of mb) that the hash-verification during boot +was showing up on profiles from older devices. + +**Install flow:** + +1. Download patch from server +2. Inflate patch (apply bidiff to base release) +3. `check_hash()`: Compute SHA256 of inflated file, verify it matches server-provided hash +4. Store patch file, hash, and signature to disk + +**Boot flow:** + +1. Verify patch file exists and size matches stored metadata +2. `hash_file()`: Re-compute SHA256 of patch file on disk +3. `check_signature()`: Verify the computed hash has a valid signature using the public key +4. If verification fails, fall back to last known good patch or base release + +#### Install Only Mode + +```yaml +patch_verification: install_only +``` + +In Install Only mode, patch signature verification happens at **install time** +only. This provides faster boot times but does not protect against post-install +tampering (extremely uncommon). The only case that this does not protect +against is if _your app itself_ were to accidentally (or through some other +malicious exploit of your app) modify its own data directory and modify the +patch files within such. + +**Install flow:** + +1. Download patch from server +2. Inflate patch (apply bidiff to base release) +3. `check_hash()`: Compute SHA256 of inflated file, verify it matches server-provided hash +4. `check_signature()`: Verify the server-provided hash has a valid signature using the public key +5. Store patch file, hash, and signature to disk + +**Boot flow:** + +1. Verify patch file exists and size matches stored metadata +2. (No signature verification - trusted from install time) + +#### Without a Public Key + +If no `patch_public_key` is configured, signature verification is skipped in +both modes. The `check_hash()` step still runs during install to detect +download corruption, but there is no cryptographic verification that the +patch came from a trusted source. ## TODO: - Add an async API. - Write tests for state management. - Make state management/filesystem management atomic (and tested). -- Support validating patches/slots (hashes, signatures, etc). ## Later-stage update system design docs diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index d16e834..a3bbdab 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -508,6 +508,19 @@ mod test { free_parameters(c_params); } + #[serial] + #[test] + fn init_with_invalid_patch_verification() { + testing_reset_config(); + let tmp_dir = TempDir::new("example").unwrap(); + let c_params = parameters(&tmp_dir, "/dir/lib/arm64/libapp.so"); + let c_yaml = c_string("app_id: foo\npatch_verification: bogus_mode"); + // Invalid patch_verification causes init to fail and return false + assert!(!shorebird_init(&c_params, FileCallbacks::new(), c_yaml)); + free_c_string(c_yaml); + free_parameters(c_params); + } + #[serial] #[test] fn yaml_parsing() { diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs index 19dcb38..b152cb9 100644 --- a/library/src/cache/patch_manager.rs +++ b/library/src/cache/patch_manager.rs @@ -1,4 +1,5 @@ use super::{disk_io, signing, PatchInfo}; +use crate::yaml::PatchVerificationMode; use anyhow::{bail, Context, Result}; use core::fmt::Debug; use serde::{Deserialize, Serialize}; @@ -146,19 +147,28 @@ pub struct PatchManager { /// 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, + + /// Controls when signature verification occurs: at boot time (strict) or only + /// at install time (install_only). + verification_mode: PatchVerificationMode, } 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 new(root_dir: PathBuf, patch_public_key: Option<&str>) -> Self { + pub fn new( + root_dir: PathBuf, + patch_public_key: Option<&str>, + verification_mode: PatchVerificationMode, + ) -> 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()), + verification_mode, } } @@ -208,6 +218,7 @@ impl PatchManager { /// Checks that the patch with the given number: /// - Has an artifact on disk /// - That artifact on disk is the same size it was when it was installed + /// - In Strict mode: verifies the signature against the hash /// /// Returns Ok if the patch is bootable, or an error if it is not. fn validate_patch_is_bootable(&self, patch: &PatchMetadata) -> Result<()> { @@ -230,18 +241,21 @@ 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")?; + // In Strict mode, verify the signature at boot time. + // This ensures the patch file hasn't been tampered with since installation. + if self.verification_mode == PatchVerificationMode::Strict { + if let Some(public_key) = &self.patch_public_key { + 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 { - shorebird_info!("No public key provided, skipping signature verification"); + // Compute the hash of the patch file on disk and verify it matches. + let patch_hash = signing::hash_file(&artifact_path)?; + signing::check_signature(&patch_hash, &signature, public_key)?; + } else { + shorebird_info!("No public key provided, skipping signature verification"); + } } Ok(()) @@ -367,6 +381,15 @@ impl ManagePatches for PatchManager { bail!("Patch file {} does not exist", file_path.display()); } + // In InstallOnly mode, verify signature at install time. + // In Strict mode, signature verification happens at boot time instead. + if self.verification_mode == PatchVerificationMode::InstallOnly { + if let Some(public_key) = &self.patch_public_key { + let sig = signature.context("Patch signature is missing")?; + signing::check_signature(hash, sig, public_key)?; + } + } + let patch_path = self.patch_artifact_path(patch_number); std::fs::create_dir_all(self.patch_dir(patch_number)) @@ -513,7 +536,11 @@ impl ManagePatches for PatchManager { #[cfg(test)] impl PatchManager { pub fn manager_for_test(temp_dir: &TempDir) -> PatchManager { - PatchManager::new(temp_dir.path().to_owned(), None) + PatchManager::new( + temp_dir.path().to_owned(), + None, + PatchVerificationMode::default(), + ) } pub fn add_patch_for_test(&mut self, temp_dir: &TempDir, patch_number: usize) -> Result<()> { @@ -547,6 +574,7 @@ mod debug_tests { use tempdir::TempDir; use super::PatchManager; + use crate::yaml::PatchVerificationMode; #[test] fn manage_patches_is_debug() { @@ -559,9 +587,13 @@ mod debug_tests { #[test] fn patch_manager_is_debug() { let temp_dir = TempDir::new("patch_manager").unwrap(); - let patch_manager = PatchManager::new(temp_dir.path().to_owned(), Some("public_key")); + let patch_manager = PatchManager::new( + temp_dir.path().to_owned(), + Some("public_key"), + PatchVerificationMode::default(), + ); let actual = format!("{:?}", patch_manager); - assert!(actual.contains(r#"patches_state: PatchesState { last_booted_patch: None, next_boot_patch: None, currently_booting_patch: None, known_bad_patches: {} }, patch_public_key: Some("public_key") }"#)); + assert!(actual.contains(r#"patches_state: PatchesState { last_booted_patch: None, next_boot_patch: None, currently_booting_patch: None, known_bad_patches: {} }, patch_public_key: Some("public_key")"#)); } } @@ -614,6 +646,116 @@ mod add_patch_tests { ); assert!(!file_path.exists()); } + + // InstallOnly mode signature verification tests - these verify that signature + // checking happens at install time when using PatchVerificationMode::InstallOnly. + + // 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 install_only_errs_if_public_key_is_invalid() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some("not a valid key"), + PatchVerificationMode::InstallOnly, + ); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch contents").unwrap(); + + // In InstallOnly mode, fails at install time because the public key is invalid + let result = manager.add_patch(1, file_path, INFLATED_PATCH_HASH, Some(SIGNATURE)); + assert!(result.is_err()); + assert!(manager.next_boot_patch().is_none()); + } + + #[test] + fn install_only_errs_if_signature_is_missing_when_public_key_configured() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::InstallOnly, + ); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch contents").unwrap(); + + // In InstallOnly mode, fails at install time because signature is missing + let result = manager.add_patch(1, file_path, INFLATED_PATCH_HASH, None); + assert!(result.is_err()); + assert!(manager.next_boot_patch().is_none()); + } + + #[test] + fn install_only_errs_if_signature_is_invalid() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::InstallOnly, + ); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch contents").unwrap(); + + // Using INFLATED_PATCH_HASH as a signature because it is valid base64, but not a valid signature. + // In InstallOnly mode, this fails immediately at install time. + let result = + manager.add_patch(1, file_path, INFLATED_PATCH_HASH, Some(INFLATED_PATCH_HASH)); + assert!(result.is_err()); + assert!(manager.next_boot_patch().is_none()); + } + + #[test] + fn install_only_succeeds_with_valid_signature() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::InstallOnly, + ); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch contents").unwrap(); + + // In InstallOnly mode, signature is verified at install time + let result = manager.add_patch(1, file_path, INFLATED_PATCH_HASH, Some(SIGNATURE)); + assert!(result.is_ok()); + assert!(manager.next_boot_patch().is_some()); + } + + #[test] + fn install_only_succeeds_with_any_signature_if_no_public_key() { + let temp_dir = TempDir::new("patch_manager").unwrap(); + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + None, // No public key configured + PatchVerificationMode::InstallOnly, + ); + + let file_path = &temp_dir.path().join("patch1.vmcode"); + std::fs::write(file_path, "patch contents").unwrap(); + + // Without a public key, signature verification is skipped even in InstallOnly mode + let result = manager.add_patch(1, file_path, "hash", Some("not a valid signature")); + assert!(result.is_ok()); + assert!(manager.next_boot_patch().is_some()); + } } #[cfg(test)] @@ -850,61 +992,23 @@ mod validate_next_boot_patch_tests { // 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_some()); - assert!(manager.validate_next_boot_patch().is_err()); - 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_some()); - assert!(manager.validate_next_boot_patch().is_err()); - 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_some()); - assert!(manager.validate_next_boot_patch().is_err()); - 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)); + // Strict mode boot-time signature verification tests. + // In Strict mode, signature verification happens at boot time (validate_next_boot_patch), + // not at install time. This provides protection against post-install tampering. + #[test] + fn strict_mode_succeeds_with_valid_signature_at_boot_time() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::Strict, + ); + + // In Strict mode, add_patch does NOT verify signature (that happens at boot time) manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?; + // Boot-time validation verifies the signature by computing hash and checking signature assert!(manager.next_boot_patch().is_some()); assert!(manager.validate_next_boot_patch().is_ok()); assert!(manager.next_boot_patch().is_some()); @@ -916,9 +1020,9 @@ mod validate_next_boot_patch_tests { } #[test] - fn returns_patch_with_arbitrary_signature_if_no_public_key() -> Result<()> { + fn succeeds_with_arbitrary_signature_if_no_public_key() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - // Create a PatchManager without a public key. + // Create a PatchManager without a public key - signature verification is skipped. let mut manager = PatchManager::manager_for_test(&temp_dir); manager.add_signed_patch_for_test( @@ -928,6 +1032,7 @@ mod validate_next_boot_patch_tests { Some("not a valid signature"), )?; + // Without a public key, boot-time validation only checks file existence and size assert!(manager.next_boot_patch().is_some()); assert!(manager.validate_next_boot_patch().is_ok()); assert!(manager.next_boot_patch().is_some()); @@ -936,6 +1041,95 @@ mod validate_next_boot_patch_tests { Ok(()) } + + #[test] + fn strict_mode_fails_boot_validation_if_signature_missing() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::Strict, + ); + + // In Strict mode, add_patch succeeds without signature (no install-time check) + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, None)?; + + assert!(manager.next_boot_patch().is_some()); + // But boot-time validation fails because signature is required + assert!(manager.validate_next_boot_patch().is_err()); + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn strict_mode_fails_boot_validation_if_signature_invalid() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::Strict, + ); + + // In Strict mode, add_patch succeeds with invalid signature (no install-time check) + manager.add_signed_patch_for_test( + &temp_dir, + 1, + INFLATED_PATCH_HASH, + Some(INFLATED_PATCH_HASH), // Using hash as signature, which is invalid + )?; + + assert!(manager.next_boot_patch().is_some()); + // But boot-time validation fails because signature doesn't verify + assert!(manager.validate_next_boot_patch().is_err()); + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn strict_mode_fails_boot_validation_if_public_key_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"), + PatchVerificationMode::Strict, + ); + + // In Strict mode, add_patch succeeds (no install-time check) + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?; + + assert!(manager.next_boot_patch().is_some()); + // But boot-time validation fails because public key can't be used + assert!(manager.validate_next_boot_patch().is_err()); + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn strict_mode_detects_tampered_patch_at_boot_time() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new( + temp_dir.path().to_path_buf(), + Some(PUBLIC_KEY), + PatchVerificationMode::Strict, + ); + + // Install a valid patch + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?; + + // Tamper with the patch file after installation + let patch_path = manager.patch_artifact_path(1); + std::fs::write(&patch_path, "tampered content")?; + + assert!(manager.next_boot_patch().is_some()); + // Boot-time validation detects tampering: computed hash doesn't match signature + assert!(manager.validate_next_boot_patch().is_err()); + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } } #[cfg(test)] diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 078e093..04ce631 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -11,6 +11,7 @@ use anyhow::Result; use serde::{Deserialize, Serialize}; use crate::events::PatchEvent; +use crate::yaml::PatchVerificationMode; use super::patch_manager::{ManagePatches, PatchManager}; use super::{disk_io, PatchInfo}; @@ -87,11 +88,16 @@ impl UpdaterState { cache_dir: PathBuf, release_version: String, patch_public_key: Option<&str>, + verification_mode: PatchVerificationMode, client_id: String, ) -> Self { Self { cache_dir: cache_dir.clone(), - patch_manager: Box::new(PatchManager::new(cache_dir.clone(), patch_public_key)), + patch_manager: Box::new(PatchManager::new( + cache_dir.clone(), + patch_public_key, + verification_mode, + )), serialized_state: SerializedState { client_id: client_id, release_version, @@ -101,12 +107,20 @@ impl UpdaterState { } /// Loads UpdaterState from disk - fn load(cache_dir: &Path, patch_public_key: Option<&str>) -> anyhow::Result { + fn load( + cache_dir: &Path, + patch_public_key: Option<&str>, + verification_mode: PatchVerificationMode, + ) -> anyhow::Result { 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::new(cache_dir.to_path_buf(), patch_public_key)), + patch_manager: Box::new(PatchManager::new( + cache_dir.to_path_buf(), + patch_public_key, + verification_mode, + )), serialized_state, }) } @@ -116,12 +130,14 @@ impl UpdaterState { storage_dir: &Path, release_version: &str, patch_public_key: Option<&str>, + verification_mode: PatchVerificationMode, client_id: String, ) -> Self { let mut state = Self::new( storage_dir.to_owned(), release_version.to_owned(), patch_public_key, + verification_mode, client_id, ); if let Err(e) = state.save() { @@ -136,8 +152,9 @@ impl UpdaterState { storage_dir: &Path, release_version: &str, patch_public_key: Option<&str>, + verification_mode: PatchVerificationMode, ) -> Self { - let load_result = Self::load(storage_dir, patch_public_key); + let load_result = Self::load(storage_dir, patch_public_key, verification_mode); match load_result { Ok(loaded) => { if loaded.serialized_state.release_version != release_version { @@ -150,6 +167,7 @@ impl UpdaterState { storage_dir, release_version, patch_public_key, + verification_mode, loaded.client_id(), ); } @@ -163,6 +181,7 @@ impl UpdaterState { storage_dir, release_version, patch_public_key, + verification_mode, generate_client_id(), ) } @@ -326,11 +345,11 @@ mod tests { assert!(state.save().is_ok()); let mut state = - UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version, None); + UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version, None, PatchVerificationMode::default()); 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", None); + UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2", None, PatchVerificationMode::default()); assert!(next_version_state.next_boot_patch().is_none()); } @@ -348,8 +367,8 @@ mod tests { #[test] fn creates_updater_state_with_client_id() { let tmp_dir = TempDir::new("example").unwrap(); - let state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1", None); - let saved_state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1", None); + let state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1", None, PatchVerificationMode::default()); + let saved_state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1", None, PatchVerificationMode::default()); assert_eq!( state.serialized_state.client_id, saved_state.serialized_state.client_id @@ -367,9 +386,10 @@ mod tests { &state.cache_dir, &state.serialized_state.release_version, None, + PatchVerificationMode::default(), ); - let new_loaded = UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2", None); + let new_loaded = UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2", None, PatchVerificationMode::default()); assert_eq!( original_loaded.serialized_state.client_id, @@ -396,7 +416,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(), None).unwrap(); + let new_state = UpdaterState::load(new_tmp_dir.path(), None, PatchVerificationMode::default()).unwrap(); assert_eq!(new_state.cache_dir, new_tmp_dir.path()); } @@ -537,7 +557,7 @@ mod tests { let tmp_dir = TempDir::new("example")?; // Create a new state, add a patch, and save it. - let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+1", None); + let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+1", None, PatchVerificationMode::default()); let patch = fake_patch(&tmp_dir, 1); state.install_patch(&patch, "hash", None)?; state.save()?; @@ -548,7 +568,7 @@ mod tests { std::fs::write(&state_file, "corrupt json")?; // Ensure that, by corrupting the file, we've reset the patches state. - let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+2", None); + let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+2", None, PatchVerificationMode::default()); assert!(state.next_boot_patch().is_none()); Ok(()) diff --git a/library/src/config.rs b/library/src/config.rs index 684eb7f..3a1b143 100644 --- a/library/src/config.rs +++ b/library/src/config.rs @@ -2,7 +2,7 @@ use crate::network::NetworkHooks; use crate::updater::AppConfig; -use crate::yaml::YamlConfig; +use crate::yaml::{PatchVerificationMode, YamlConfig}; use crate::{ExternalFileProvider, UpdateError}; use std::path::PathBuf; @@ -85,6 +85,7 @@ pub struct UpdateConfig { pub network_hooks: NetworkHooks, pub file_provider: Box, pub patch_public_key: Option, + pub patch_verification: PatchVerificationMode, } /// Returns Ok if the config was set successfully, Err if it was already set. @@ -127,6 +128,7 @@ pub fn set_config( network_hooks, file_provider, patch_public_key: yaml.patch_public_key.to_owned(), + patch_verification: yaml.patch_verification.unwrap_or_default(), }; shorebird_debug!("Updater configured with: {:?}", new_config); *config = Some(new_config); @@ -195,6 +197,7 @@ mod tests { auto_update: Some(true), base_url: Some("fake_base_url".to_string()), patch_public_key: None, + patch_verification: None, } } @@ -219,6 +222,7 @@ mod tests { auto_update: Some(true), base_url: Some("fake_base_url".to_string()), patch_public_key: Some("patch_public_key".to_string()), + patch_verification: None, }, NetworkHooks::default(), )?; diff --git a/library/src/test_utils.rs b/library/src/test_utils.rs index 9e79b48..ae1ebbf 100644 --- a/library/src/test_utils.rs +++ b/library/src/test_utils.rs @@ -18,6 +18,7 @@ pub fn install_fake_patch(patch_number: usize) -> anyhow::Result<()> { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); state.install_patch( &PatchInfo { diff --git a/library/src/updater.rs b/library/src/updater.rs index 48c4030..451c9ef 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -149,6 +149,7 @@ where &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); f(&state) }) @@ -163,6 +164,7 @@ where &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); f(&mut state) }) @@ -212,6 +214,7 @@ pub fn handle_prior_boot_failure_if_necessary() -> Result<(), InitError> { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); if let Some(patch) = state.currently_booting_patch() { state.record_boot_failure_for_patch(patch.number)?; @@ -600,6 +603,7 @@ pub fn report_launch_failure() -> anyhow::Result<()> { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); let patch = state.currently_booting_patch().ok_or(anyhow::Error::from( @@ -641,6 +645,7 @@ pub fn report_launch_success() -> anyhow::Result<()> { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); let booting_patch = match state.currently_booting_patch() { @@ -852,6 +857,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); assert_eq!(state.next_boot_patch().unwrap().number, 1); Ok(()) @@ -942,6 +948,39 @@ mod tests { ); } + #[serial] + #[test] + fn init_invalid_patch_verification() { + testing_reset_config(); + let tmp_dir = TempDir::new("example").unwrap(); + let cache_dir = tmp_dir.path().to_str().unwrap().to_string(); + let yaml = r#" +app_id: test_app +patch_verification: bogus_mode +"#; + let result = crate::init( + crate::AppConfig { + app_storage_dir: cache_dir.clone(), + code_cache_dir: cache_dir.clone(), + release_version: "1.0.0+1".to_string(), + original_libapp_paths: vec!["original_libapp_path".to_string()], + }, + Box::new(FakeExternalFileProvider {}), + yaml, + ); + match result { + Err(crate::InitError::InvalidArgument(field, msg)) => { + assert_eq!(field, "yaml"); + assert!( + msg.contains("unknown variant"), + "Expected 'unknown variant' in error message, got: {}", + msg + ); + } + _ => panic!("Expected InvalidArgument error, got: {:?}", result), + } + } + #[serial] #[test] fn reports_patch_download_on_update() -> anyhow::Result<()> { @@ -1115,6 +1154,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); assert_eq!( state.last_successfully_booted_patch().unwrap().number, @@ -1145,6 +1185,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); // It's now bad. assert!(state.next_boot_patch().is_none()); @@ -1188,6 +1229,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); state.record_boot_failure_for_patch(1)?; @@ -1272,6 +1314,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); let fail_event = PatchEvent { app_id: config.app_id.clone(), @@ -1303,6 +1346,7 @@ mod tests { &config.storage_dir, &config.release_version, config.patch_public_key.as_deref(), + config.patch_verification, ); // All 5 events should be cleared, even though only 3 were sent. assert_eq!(state.copy_events(10).len(), 0); diff --git a/library/src/yaml.rs b/library/src/yaml.rs index 9b748fa..227b3e4 100644 --- a/library/src/yaml.rs +++ b/library/src/yaml.rs @@ -1,5 +1,19 @@ use serde::Deserialize; +/// Controls when patch signature verification occurs. +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum PatchVerificationMode { + /// Verify patch signature at boot time (default, most secure). + /// The patch is verified each time the app boots, ensuring + /// the patch file hasn't been tampered with since installation. + #[default] + Strict, + /// Verify patch signature only at install time. + /// Faster boot times but less protection against post-install tampering. + InstallOnly, +} + /// Struct for parsing shorebird.yaml. #[derive(Deserialize)] pub struct YamlConfig { @@ -14,6 +28,8 @@ pub struct YamlConfig { pub auto_update: Option, /// Base64-encoded public key for verifying patch hash signatures. pub patch_public_key: Option, + /// When to verify patch signatures. Defaults to "strict" (verify at boot time). + pub patch_verification: Option, } impl YamlConfig { @@ -22,3 +38,56 @@ impl YamlConfig { serde_yaml::from_str(yaml) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_patch_verification_strict() { + let yaml = r#" +app_id: test_app +patch_verification: strict +"#; + let config = YamlConfig::from_yaml(yaml).unwrap(); + assert_eq!( + config.patch_verification, + Some(PatchVerificationMode::Strict) + ); + } + + #[test] + fn parses_patch_verification_install_only() { + let yaml = r#" +app_id: test_app +patch_verification: install_only +"#; + let config = YamlConfig::from_yaml(yaml).unwrap(); + assert_eq!( + config.patch_verification, + Some(PatchVerificationMode::InstallOnly) + ); + } + + #[test] + fn defaults_to_none_when_not_specified() { + let yaml = r#" +app_id: test_app +"#; + let config = YamlConfig::from_yaml(yaml).unwrap(); + assert_eq!(config.patch_verification, None); + // When unwrapped with default, should be Strict + assert_eq!( + config.patch_verification.unwrap_or_default(), + PatchVerificationMode::Strict + ); + } + + #[test] + fn default_verification_mode_is_strict() { + assert_eq!( + PatchVerificationMode::default(), + PatchVerificationMode::Strict + ); + } +}