feat: add verification_mode config option (#308)
* feat: move patch verification from boot time to install time * feat: make it switchable * chore: update comments * fix: test invalid yaml * chore: update readme * feat: add more comments to readme * doc: more readme updates * chore: rename to patch_verification
This commit is contained in:
+79
-2
@@ -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
|
||||
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Vendored
+263
-69
@@ -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<String>,
|
||||
|
||||
/// 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)]
|
||||
|
||||
Vendored
+32
-12
@@ -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<Self> {
|
||||
fn load(
|
||||
cache_dir: &Path,
|
||||
patch_public_key: Option<&str>,
|
||||
verification_mode: PatchVerificationMode,
|
||||
) -> 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::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(())
|
||||
|
||||
@@ -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<dyn ExternalFileProvider>,
|
||||
pub patch_public_key: Option<String>,
|
||||
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(),
|
||||
)?;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<bool>,
|
||||
/// Base64-encoded public key for verifying patch hash signatures.
|
||||
pub patch_public_key: Option<String>,
|
||||
/// When to verify patch signatures. Defaults to "strict" (verify at boot time).
|
||||
pub patch_verification: Option<PatchVerificationMode>,
|
||||
}
|
||||
|
||||
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
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user