From c5829f6012394a9da9a4bd73111aaff2194050b8 Mon Sep 17 00:00:00 2001 From: Frederik Rothenberger Date: Wed, 21 Dec 2022 14:16:53 +0100 Subject: [PATCH] Encapsulate anchor in its own module (#1100) * Encapsulate anchor in its own module This makes the anchor and device constraints unit testable and separates the logic of validating data from the other tasks such as authentication or housekeeping. * Implement review input --- .../src/anchor_management.rs | 215 +++------- .../src/anchor_management/registration.rs | 23 +- src/internet_identity/src/archive.rs | 2 +- src/internet_identity/src/main.rs | 6 +- src/internet_identity/src/state.rs | 69 +--- src/internet_identity/src/storage.rs | 19 +- src/internet_identity/src/storage/anchor.rs | 391 ++++++++++++++++++ .../src/storage/anchor/tests.rs | 361 ++++++++++++++++ src/internet_identity/src/storage/tests.rs | 212 +++++----- src/internet_identity/tests/tests.rs | 6 +- 10 files changed, 926 insertions(+), 378 deletions(-) create mode 100644 src/internet_identity/src/storage/anchor.rs create mode 100644 src/internet_identity/src/storage/anchor/tests.rs diff --git a/src/internet_identity/src/anchor_management.rs b/src/internet_identity/src/anchor_management.rs index 31d2692f8d..4b62632be0 100644 --- a/src/internet_identity/src/anchor_management.rs +++ b/src/internet_identity/src/anchor_management.rs @@ -1,8 +1,8 @@ use crate::archive::{archive_operation, device_diff}; use crate::state::RegistrationState::DeviceTentativelyAdded; -use crate::state::{Anchor, Device, TentativeDeviceRegistration}; +use crate::state::TentativeDeviceRegistration; +use crate::storage::anchor::{Anchor, Device}; use crate::{delegation, state, trap_if_not_authenticated}; -use candid::Principal; use ic_cdk::api::time; use ic_cdk::{caller, trap}; use internet_identity_interface::archive::{DeviceDataWithoutAlias, Operation}; @@ -15,7 +15,11 @@ pub fn get_anchor_info(anchor_number: AnchorNumber) -> IdentityAnchorInfo { let anchor = state::anchor(anchor_number); trap_if_not_authenticated(&anchor); - let devices = anchor.devices.into_iter().map(DeviceData::from).collect(); + let devices = anchor + .into_devices() + .into_iter() + .map(DeviceData::from) + .collect(); let now = time(); state::tentative_device_registrations(|tentative_device_registrations| { @@ -51,36 +55,18 @@ pub fn get_anchor_info(anchor_number: AnchorNumber) -> IdentityAnchorInfo { } pub fn add(anchor_number: AnchorNumber, device_data: DeviceData) { - const MAX_ENTRIES_PER_ANCHOR: usize = 10; - let mut anchor = state::anchor(anchor_number); // must be called before the first await because it requires caller() trap_if_not_authenticated(&anchor); let new_device = Device::from(device_data); - check_device(&new_device, &anchor.devices); - - if anchor - .devices - .iter() - .find(|e| e.pubkey == new_device.pubkey) - .is_some() - { - trap("Device already added."); - } - - if anchor.devices.len() >= MAX_ENTRIES_PER_ANCHOR { + anchor.add_device(new_device.clone()).unwrap_or_else(|err| { trap(&format!( - "at most {} authentication information entries are allowed per anchor", - MAX_ENTRIES_PER_ANCHOR, - )); - } - - anchor.devices.push(new_device.clone()); + "failed to add device to anchor {}: {}", + anchor_number, err + )) + }); write_anchor(anchor_number, anchor); - - delegation::prune_expired_signatures(); - archive_operation( anchor_number, caller(), @@ -88,69 +74,39 @@ pub fn add(anchor_number: AnchorNumber, device_data: DeviceData) { device: DeviceDataWithoutAlias::from(new_device), }, ); + delegation::prune_expired_signatures(); } -/// Replace or remove an existing device. -/// -/// NOTE: all mutable operations should call this function because it handles device protection -fn mutate_device_or_trap( - anchor: &mut Anchor, - device_key: DeviceKey, - new_value: Option, -) -> Operation { - let index = match anchor.devices.iter().position(|e| e.pubkey == device_key) { - None => trap("Could not find device to mutate, check device key"), - Some(index) => index, - }; - - let existing_device = anchor.devices.get_mut(index).unwrap(); +pub fn update(user_number: AnchorNumber, device_key: DeviceKey, device_data: DeviceData) { + let mut anchor = state::anchor(user_number); + trap_if_not_authenticated(&anchor); - // Run appropriate checks for protected devices - match existing_device.protection { - DeviceProtection::Unprotected => (), - DeviceProtection::Protected => { - // If the call is not authenticated with the device to mutate, abort - if caller() != Principal::self_authenticating(&existing_device.pubkey) { - trap("Device is protected. Must be authenticated with this device to mutate"); - } - } + let Some(existing_device) = anchor.device(&device_key) else { + trap("Could not find device to update, check device key") }; - match new_value { - Some(new_device) => { - let diff = device_diff(existing_device, &new_device); - *existing_device = new_device; - Operation::UpdateDevice { - device: device_key, - new_values: diff, - } - } - None => { - // NOTE: we void the more efficient remove_swap to ensure device ordering - // is not changed - anchor.devices.remove(index); - Operation::RemoveDevice { device: device_key } - } - } -} - -pub fn update(anchor_number: AnchorNumber, device_key: DeviceKey, device_data: DeviceData) { - if device_key != device_data.pubkey { - trap("device key may not be updated"); - } - let mut anchor = state::anchor(anchor_number); - - trap_if_not_authenticated(&anchor); let new_device = Device::from(device_data); - check_device(&new_device, &anchor.devices); - - let operation = mutate_device_or_trap(&mut anchor, device_key, Some(new_device)); + let diff = device_diff(existing_device, &new_device); - write_anchor(anchor_number, anchor); + anchor + .modify_device(&device_key, new_device.clone()) + .unwrap_or_else(|err| { + trap(&format!( + "failed to modify device of anchor {}: {}", + user_number, err + )) + }); + write_anchor(user_number, anchor); delegation::prune_expired_signatures(); - - archive_operation(anchor_number, caller(), operation); + archive_operation( + user_number, + caller(), + Operation::UpdateDevice { + device: device_key, + new_values: diff, + }, + ); } pub fn remove(anchor_number: AnchorNumber, device_key: DeviceKey) { @@ -158,11 +114,20 @@ pub fn remove(anchor_number: AnchorNumber, device_key: DeviceKey) { // must be called before the first await because it requires caller() trap_if_not_authenticated(&anchor); - let operation = mutate_device_or_trap(&mut anchor, device_key, None); + anchor.remove_device(&device_key).unwrap_or_else(|err| { + trap(&format!( + "failed to remove device of anchor {}: {}", + anchor_number, err + )) + }); write_anchor(anchor_number, anchor); + archive_operation( + anchor_number, + caller(), + Operation::RemoveDevice { device: device_key }, + ); delegation::prune_expired_signatures(); - archive_operation(anchor_number, caller(), operation); } /// Writes the supplied entries to stable memory and updates the anchor operation metric. @@ -180,89 +145,3 @@ fn write_anchor(anchor_number: AnchorNumber, anchor: Anchor) { metrics.anchor_operation_counter += 1; }); } - -/// This checks some device invariants, in particular: -/// * Sizes of various fields do not exceed limits -/// * Sum of sizes of all variable length fields does not exceed limit -/// * Only recovery phrases can be protected -/// * There can only be one recovery phrase -/// -/// Otherwise, trap. -/// -/// NOTE: while in the future we may lift this restriction, for now we do ensure that -/// protected devices are limited to recovery phrases, which the webapp expects. -fn check_device(device: &Device, existing_devices: &[Device]) { - /// Single devices can use up to 564 bytes for the variable length fields alone. - /// In order to not give away all the anchor space to the device vector, we limit the sum of the - /// size of all variable fields of all devices. This ensures that we have the flexibility to expand - /// or change anchors in the future. - /// The value 2048 was chosen because it is the max anchor size before the stable memory migration. - /// This means that all pre-existing anchors are below this limit. And after the migration, the - /// candid encoded `vec devices` will stay far below 4KB in size (testing showed anchors of up to - /// 2259 bytes). - const VARIABLE_FIELDS_LIMIT: usize = 2048; - - check_entry_limits(device); - - if device.protection == DeviceProtection::Protected && device.key_type != KeyType::SeedPhrase { - trap(&format!( - "Only recovery phrases can be protected but key type is {:?}", - device.key_type - )); - } - - // if the device is a recovery phrase, check if a different recovery phrase already exists - if device.key_type == KeyType::SeedPhrase - && existing_devices.iter().any(|existing_device| { - existing_device.pubkey != device.pubkey - && existing_device.key_type == KeyType::SeedPhrase - }) - { - trap("There is already a recovery phrase and only one is allowed."); - } - - let existing_variable_size: usize = existing_devices - .iter() - // filter out the device being checked to not count it twice in case of update operations - .filter(|elem| elem.pubkey != device.pubkey) - .map(|device| device.variable_fields_len()) - .sum(); - - if existing_variable_size + device.variable_fields_len() > VARIABLE_FIELDS_LIMIT { - trap("Devices exceed allowed storage limit. Either use shorter aliases or remove an existing device.") - } -} - -fn check_entry_limits(device: &Device) { - const ALIAS_LEN_LIMIT: usize = 64; - const PK_LEN_LIMIT: usize = 300; - const CREDENTIAL_ID_LEN_LIMIT: usize = 200; - - let n = device.alias.len(); - if n > ALIAS_LEN_LIMIT { - trap(&format!( - "alias length {} exceeds the limit of {} bytes", - n, ALIAS_LEN_LIMIT, - )); - } - - let n = device.pubkey.len(); - if n > PK_LEN_LIMIT { - trap(&format!( - "public key length {} exceeds the limit of {} bytes", - n, PK_LEN_LIMIT, - )); - } - - let n = device - .credential_id - .as_ref() - .map(|bytes| bytes.len()) - .unwrap_or_default(); - if n > CREDENTIAL_ID_LEN_LIMIT { - trap(&format!( - "credential id length {} exceeds the limit of {} bytes", - n, CREDENTIAL_ID_LEN_LIMIT, - )); - } -} diff --git a/src/internet_identity/src/anchor_management/registration.rs b/src/internet_identity/src/anchor_management/registration.rs index 490f1b7eb2..be6943b1ad 100644 --- a/src/internet_identity/src/anchor_management/registration.rs +++ b/src/internet_identity/src/anchor_management/registration.rs @@ -1,6 +1,7 @@ -use crate::anchor_management::{check_device, write_anchor}; +use crate::anchor_management::write_anchor; use crate::archive::archive_operation; -use crate::state::{Anchor, ChallengeInfo, Device}; +use crate::state::ChallengeInfo; +use crate::storage::anchor::Device; use crate::storage::Salt; use crate::{delegation, secs_to_nanos, state}; use candid::Principal; @@ -161,7 +162,6 @@ pub fn register(device_data: DeviceData, challenge_result: ChallengeAttempt) -> } let device = Device::from(device_data); - check_device(&device, &vec![]); if caller() != Principal::self_authenticating(&device.pubkey) { trap(&format!( @@ -171,15 +171,16 @@ pub fn register(device_data: DeviceData, challenge_result: ChallengeAttempt) -> )); } - let allocation = state::storage_mut(|storage| storage.allocate_anchor_number()); + let allocation = state::storage_mut(|storage| storage.allocate_anchor()); match allocation { - Some(anchor_number) => { - write_anchor( - anchor_number, - Anchor { - devices: vec![device.clone()], - }, - ); + Some((anchor_number, mut anchor)) => { + anchor.add_device(device.clone()).unwrap_or_else(|err| { + trap(&format!( + "failed to register anchor {}: {}", + anchor_number, err + )) + }); + write_anchor(anchor_number, anchor); archive_operation( anchor_number, caller(), diff --git a/src/internet_identity/src/archive.rs b/src/internet_identity/src/archive.rs index 0ea52461c0..a85896b72b 100644 --- a/src/internet_identity/src/archive.rs +++ b/src/internet_identity/src/archive.rs @@ -1,5 +1,5 @@ use crate::state; -use crate::state::Device; +use crate::storage::anchor::Device; use candid::{CandidType, Deserialize, Principal}; use ic_cdk::api::call::{call_with_payment, CallResult}; use ic_cdk::api::management_canister::main::{ diff --git a/src/internet_identity/src/main.rs b/src/internet_identity/src/main.rs index 606a43b6a5..831001066e 100644 --- a/src/internet_identity/src/main.rs +++ b/src/internet_identity/src/main.rs @@ -1,7 +1,7 @@ use crate::anchor_management::tentative_device_registration; use crate::archive::ArchiveState; use crate::assets::init_assets; -use crate::state::Anchor; +use crate::storage::anchor::Anchor; use candid::Principal; use ic_cdk::api::{caller, set_certified_data, trap}; use ic_cdk_macros::{init, post_upgrade, pre_upgrade, query, update}; @@ -90,7 +90,7 @@ fn lookup(anchor_number: AnchorNumber) -> Vec { storage .read(anchor_number) .unwrap_or_default() - .devices + .into_devices() .into_iter() .map(DeviceData::from) .collect() @@ -259,7 +259,7 @@ fn update_root_hash() { /// Checks if the caller is authenticated against the anchor provided and traps if not. fn trap_if_not_authenticated(anchor: &Anchor) { - for device in &anchor.devices { + for device in anchor.devices() { if caller() == Principal::self_authenticating(&device.pubkey) { return; } diff --git a/src/internet_identity/src/state.rs b/src/internet_identity/src/state.rs index 485ead246e..278c354f29 100644 --- a/src/internet_identity/src/state.rs +++ b/src/internet_identity/src/state.rs @@ -1,4 +1,5 @@ use crate::archive::{ArchiveData, ArchiveInfo, ArchiveState, ArchiveStatusCache}; +use crate::storage::anchor::Anchor; use crate::storage::DEFAULT_RANGE_SIZE; use crate::{Salt, Storage}; use candid::{CandidType, Deserialize, Principal}; @@ -8,7 +9,6 @@ use ic_cdk::{call, trap}; use ic_certified_map::{Hash, RbTree}; use ic_stable_structures::DefaultMemoryImpl; use internet_identity::signature_map::SignatureMap; -use internet_identity_interface::archive::*; use internet_identity_interface::*; use std::cell::{Cell, RefCell}; use std::collections::HashMap; @@ -22,73 +22,6 @@ thread_local! { static ASSETS: RefCell = RefCell::new(HashMap::default()); } -/// Internal representation of the anchor. -/// After the upcoming stable memory migration, new fields can be added to this struct. -#[derive(Clone, Debug, Default, CandidType, Deserialize, Eq, PartialEq)] -pub struct Anchor { - pub devices: Vec, -} - -/// This is an internal version of `DeviceData` useful to provide a -/// backwards compatible level between device data stored in stable memory. -/// It is similar to `DeviceDataInternal` but with redundant options removed -/// (which is possible due to the stable memory candid schema migration). -#[derive(Clone, Debug, CandidType, Deserialize, Eq, PartialEq)] -pub struct Device { - pub pubkey: DeviceKey, - pub alias: String, - pub credential_id: Option, - pub purpose: Purpose, - pub key_type: KeyType, - pub protection: DeviceProtection, -} - -impl Device { - pub fn variable_fields_len(&self) -> usize { - self.alias.len() - + self.pubkey.len() - + self.credential_id.as_ref().map(|id| id.len()).unwrap_or(0) - } -} - -impl From for Device { - fn from(device_data: DeviceData) -> Self { - Self { - pubkey: device_data.pubkey, - alias: device_data.alias, - credential_id: device_data.credential_id, - purpose: device_data.purpose, - key_type: device_data.key_type, - protection: device_data.protection, - } - } -} - -impl From for DeviceData { - fn from(device: Device) -> Self { - Self { - pubkey: device.pubkey, - alias: device.alias, - credential_id: device.credential_id, - purpose: device.purpose, - key_type: device.key_type, - protection: device.protection, - } - } -} - -impl From for DeviceDataWithoutAlias { - fn from(device_data: Device) -> Self { - Self { - pubkey: device_data.pubkey, - credential_id: device_data.credential_id, - purpose: device_data.purpose, - key_type: device_data.key_type, - protection: device_data.protection, - } - } -} - pub struct TentativeDeviceRegistration { pub expiration: Timestamp, pub state: RegistrationState, diff --git a/src/internet_identity/src/storage.rs b/src/internet_identity/src/storage.rs index f3b53ff6fa..63c41bd495 100644 --- a/src/internet_identity/src/storage.rs +++ b/src/internet_identity/src/storage.rs @@ -64,7 +64,8 @@ //! without the risk of running out of space (which might easily happen if the RESERVED_HEADER_BYTES //! were used instead). -use crate::state::{Anchor, Device, PersistentState}; +use crate::state::PersistentState; +use crate::storage::anchor::{Anchor, Device}; use candid; use candid::{CandidType, Deserialize}; use ic_cdk::api::trap; @@ -78,6 +79,8 @@ use std::fmt; use std::io::{Read, Write}; use std::ops::RangeInclusive; +pub mod anchor; + #[cfg(test)] mod tests; @@ -269,14 +272,14 @@ impl Storage { /// /// Returns None if the range of Identity Anchor assigned to this /// storage is exhausted. - pub fn allocate_anchor_number(&mut self) -> Option { + pub fn allocate_anchor(&mut self) -> Option<(AnchorNumber, Anchor)> { let anchor_number = self.header.id_range_lo + self.header.num_anchors as u64; if anchor_number >= self.header.id_range_hi { return None; } self.header.num_anchors += 1; self.flush(); - Some(anchor_number) + Some((anchor_number, Anchor::new())) } /// Writes the data of the specified anchor to stable memory. @@ -289,7 +292,7 @@ impl Storage { } else { // use the old candid layout candid::encode_one( - data.devices + data.into_devices() .into_iter() .map(|d| DeviceDataInternal::from(d)) .collect::>(), @@ -334,9 +337,7 @@ impl Storage { // use the old candid layout let devices: Vec = candid::decode_one(&data_buf).map_err(StorageError::DeserializationError)?; - Anchor { - devices: devices.into_iter().map(|d| Device::from(d)).collect(), - } + Anchor::from_devices(devices.into_iter().map(|d| Device::from(d)).collect()) }; Ok(anchor) @@ -599,9 +600,7 @@ impl Storage { let devices: Vec = candid::decode_one(&old_schema_bytes).map_err(StorageError::DeserializationError)?; - let anchor = Anchor { - devices: devices.into_iter().map(|d| Device::from(d)).collect(), - }; + let anchor = Anchor::from_devices(devices.into_iter().map(|d| Device::from(d)).collect()); let new_schema_bytes = candid::encode_one(anchor).map_err(StorageError::SerializationError)?; diff --git a/src/internet_identity/src/storage/anchor.rs b/src/internet_identity/src/storage/anchor.rs new file mode 100644 index 0000000000..9787f1ff0d --- /dev/null +++ b/src/internet_identity/src/storage/anchor.rs @@ -0,0 +1,391 @@ +use candid::{CandidType, Deserialize, Principal}; +use internet_identity_interface::archive::DeviceDataWithoutAlias; +use internet_identity_interface::*; +use std::{fmt, iter}; + +#[cfg(test)] +mod tests; + +/// Internal representation of the anchor. +/// The anchor has limited visibility for the constructor to make sure it is loaded from storage. +/// The devices can only be modified by the exposed functions which keeps invariant checking local +/// to this module. +/// +/// After the upcoming stable memory migration, new fields can be added to this struct. +#[derive(Clone, Debug, Default, CandidType, Deserialize, Eq, PartialEq)] +pub struct Anchor { + devices: Vec, +} + +impl From for Device { + fn from(device_data: DeviceData) -> Self { + Self { + pubkey: device_data.pubkey, + alias: device_data.alias, + credential_id: device_data.credential_id, + purpose: device_data.purpose, + key_type: device_data.key_type, + protection: device_data.protection, + } + } +} + +impl From for DeviceData { + fn from(device: Device) -> Self { + Self { + pubkey: device.pubkey, + alias: device.alias, + credential_id: device.credential_id, + purpose: device.purpose, + key_type: device.key_type, + protection: device.protection, + } + } +} + +impl From for DeviceDataWithoutAlias { + fn from(device_data: Device) -> Self { + Self { + pubkey: device_data.pubkey, + credential_id: device_data.credential_id, + purpose: device_data.purpose, + key_type: device_data.key_type, + protection: device_data.protection, + } + } +} + +impl Anchor { + /// Creation of new anchors is restricted in order to make sure that the device checks are + /// not accidentally bypassed. + pub(super) fn new() -> Anchor { + Self { devices: vec![] } + } + + /// Creation of new anchors is restricted in order to make sure that the device checks are + /// not accidentally bypassed. + pub(super) fn from_devices(devices: Vec) -> Anchor { + // We do _not_ check invariants here, because there might be anchors that do not fulfill + // the invariants still stored in stable memory (e.g. anchors with multiple recovery phrases). + Self { devices } + } + + pub fn add_device(&mut self, device: Device) -> Result<(), AnchorError> { + if self + .devices + .iter() + .find(|e| e.pubkey == device.pubkey) + .is_some() + { + return Err(AnchorError::DuplicateDevice { + device_key: device.pubkey, + }); + } + check_device_invariants(&device)?; + check_anchor_invariants(&self.devices.iter().chain(iter::once(&device)).collect())?; + self.devices.push(device); + Ok(()) + } + + /// Removes a device from this anchor. + /// **Note:** Does not check invariants, based on the assumption that no invariant can be + /// violated by removing a device. See also the documentation on + /// [check_invariants](Anchor::check_invariants). + pub fn remove_device(&mut self, device_key: &DeviceKey) -> Result<(), AnchorError> { + let index = self.device_index(device_key)?; + check_mutation_allowed(&self.devices[index])?; + + self.devices.remove(index); + + // We do _not_ check invariants here, because there might be anchors that do not fulfill + // the invariants still stored in stable memory (e.g. anchors with multiple recovery phrases). + // By allowing the removal of devices on such anchors, they can be made conforming to the invariants + // by removing devices. + Ok(()) + } + + pub fn modify_device( + &mut self, + device_key: &DeviceKey, + modified_device: Device, + ) -> Result<(), AnchorError> { + if device_key != &modified_device.pubkey { + return Err(AnchorError::CannotModifyDeviceKey); + } + check_device_invariants(&modified_device)?; + let index = self.device_index(device_key)?; + check_mutation_allowed(&self.devices[index])?; + check_anchor_invariants( + &self + .devices + .iter() + // filter out the device before modification + .filter(|e| e.pubkey != device_key) + // append the device with modification + .chain(iter::once(&modified_device)) + .collect(), + )?; + + self.devices[index] = modified_device; + Ok(()) + } + + fn device_index(&self, device_key: &DeviceKey) -> Result { + let Some(index) = self.devices.iter().position(|e| e.pubkey == device_key) else { + return Err(AnchorError::NotFound {device_key: device_key.clone()}); + }; + Ok(index) + } + + /// Returns a reference to the device given the key. + pub fn device(&self, device_key: &DeviceKey) -> Option<&Device> { + self.devices.iter().find(|e| e.pubkey == device_key) + } + + /// Returns a reference to the list of devices. + pub fn devices(&self) -> &Vec { + &self.devices + } + + /// Consumes self and exposes the devices. + pub fn into_devices(self) -> Vec { + self.devices + } +} + +/// This is an internal version of `DeviceData` useful to provide a +/// backwards compatible level between device data stored in stable memory. +/// It is similar to `DeviceDataInternal` but with redundant options removed +/// (which is possible due to the stable memory candid schema migration). +#[derive(Clone, Debug, CandidType, Deserialize, Eq, PartialEq)] +pub struct Device { + pub pubkey: DeviceKey, + pub alias: String, + pub credential_id: Option, + pub purpose: Purpose, + pub key_type: KeyType, + pub protection: DeviceProtection, +} + +impl Device { + pub fn variable_fields_len(&self) -> usize { + self.alias.len() + + self.pubkey.len() + + self.credential_id.as_ref().map(|id| id.len()).unwrap_or(0) + } +} + +fn check_mutation_allowed(device: &Device) -> Result<(), AnchorError> { + match device.protection { + DeviceProtection::Unprotected => (), + DeviceProtection::Protected => { + if caller() != Principal::self_authenticating(&device.pubkey) { + return Err(AnchorError::MutationNotAllowed { + actual_principal: caller(), + authorized_principal: Principal::self_authenticating(&device.pubkey), + }); + } + } + }; + Ok(()) +} + +#[cfg(not(test))] +fn caller() -> Principal { + ic_cdk::caller() +} + +/// This is required because [ic_cdk::caller()] traps when executed in a non-canister environment. +#[cfg(test)] +fn caller() -> Principal { + tests::test_caller() +} + +/// This checks anchor invariants, in particular: +/// * Max number of devices +/// * Sum of sizes of all variable length fields does not exceed limit +/// * There can only be one recovery phrase +/// +/// **Important:** +/// Do **not** introduce new invariants that can be violated by _removing_ devices. The reason +/// is that there might still be devices in stable memory that do not fulfill the invariants. +/// In order to not break those anchors, they need to have a path back to satisfying the invariants. +/// To allow that transition, [remove_device](Anchor::remove_device) does _not_ check the invariants based on the assumption +/// that the state of an anchor cannot get worse by removing a device. +fn check_anchor_invariants(devices: &Vec<&Device>) -> Result<(), AnchorError> { + /// The number of devices is limited. The front-end limits the devices further + /// by only allowing 8 devices with purpose `authentication` to make sure there is always + /// a slot for the recovery devices. + /// Note however, that a free device slot does not guarantee that it will fit the the anchor + /// due to the `VARIABLE_FIELDS_LIMIT`. + const MAX_DEVICES_PER_ANCHOR: usize = 10; + + /// Single devices can use up to 564 bytes for the variable length fields alone. + /// In order to not give away all the anchor space to the device vector, we limit the sum of the + /// size of all variable fields of all devices. This ensures that we have the flexibility to expand + /// or change anchors in the future. + /// The value 2048 was chosen because it is the max anchor size before the stable memory migration. + /// This means that all pre-existing anchors are below this limit. And after the migration, the + /// candid encoded `vec devices` will stay far below 4KB in size (testing showed anchors of up to + /// 2259 bytes). + const VARIABLE_FIELDS_LIMIT: usize = 2048; + + if devices.len() > MAX_DEVICES_PER_ANCHOR { + return Err(AnchorError::TooManyDevices { + num_devices: devices.len(), + limit: MAX_DEVICES_PER_ANCHOR, + }); + } + + let existing_variable_size: usize = devices + .iter() + // filter out the device being checked to not count it twice in case of update operations + .map(|device| device.variable_fields_len()) + .sum(); + + if existing_variable_size > VARIABLE_FIELDS_LIMIT { + return Err(AnchorError::CumulativeDataLimitExceeded { + length: existing_variable_size, + limit: VARIABLE_FIELDS_LIMIT, + }); + } + + // check that there is only a single recovery phrase + if devices + .iter() + .filter(|device| device.key_type == KeyType::SeedPhrase) + .count() + > 1 + { + return Err(AnchorError::MultipleRecoveryPhrases); + } + + Ok(()) +} + +/// This checks device invariants, in particular: +/// * Sizes of various fields do not exceed limits +/// * Only recovery phrases can be protected +/// +/// NOTE: while in the future we may lift this restriction, for now we do ensure that +/// protected devices are limited to recovery phrases, which the webapp expects. +fn check_device_invariants(device: &Device) -> Result<(), AnchorError> { + check_device_limits(device)?; + + if device.protection == DeviceProtection::Protected && device.key_type != KeyType::SeedPhrase { + return Err(AnchorError::InvalidDeviceProtection { + key_type: device.key_type.clone(), + }); + } + Ok(()) +} + +fn check_device_limits(device: &Device) -> Result<(), AnchorError> { + const ALIAS_LEN_LIMIT: usize = 64; + const PK_LEN_LIMIT: usize = 300; + const CREDENTIAL_ID_LEN_LIMIT: usize = 200; + + let n = device.alias.len(); + if n > ALIAS_LEN_LIMIT { + return Err(AnchorError::DeviceLimitExceeded { + field: "alias".to_string(), + length: n, + limit: ALIAS_LEN_LIMIT, + }); + } + + let n = device.pubkey.len(); + if n > PK_LEN_LIMIT { + return Err(AnchorError::DeviceLimitExceeded { + field: "pubkey".to_string(), + length: n, + limit: PK_LEN_LIMIT, + }); + } + + let n = device + .credential_id + .as_ref() + .map(|bytes| bytes.len()) + .unwrap_or_default(); + if n > CREDENTIAL_ID_LEN_LIMIT { + return Err(AnchorError::DeviceLimitExceeded { + field: "credential_id".to_string(), + length: n, + limit: CREDENTIAL_ID_LEN_LIMIT, + }); + } + Ok(()) +} + +#[derive(Debug, Eq, PartialEq)] +pub enum AnchorError { + TooManyDevices { + limit: usize, + num_devices: usize, + }, + DeviceLimitExceeded { + field: String, + length: usize, + limit: usize, + }, + CumulativeDataLimitExceeded { + length: usize, + limit: usize, + }, + InvalidDeviceProtection { + key_type: KeyType, + }, + MutationNotAllowed { + authorized_principal: Principal, + actual_principal: Principal, + }, + MultipleRecoveryPhrases, + CannotModifyDeviceKey, + NotFound { + device_key: DeviceKey, + }, + DuplicateDevice { + device_key: DeviceKey, + }, +} + +impl fmt::Display for AnchorError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + AnchorError::TooManyDevices { num_devices, limit } => write!( + f, + "Anchor device limit exceeded: num devices {}, limit {}", + num_devices, limit + ), + AnchorError::DeviceLimitExceeded { + field, + length, + limit, + } => write!( + f, + "{} limit exceeded: length {}, limit {}", + field, length, limit + ), + AnchorError::CumulativeDataLimitExceeded { length, limit } => write!( + f, + "Cumulative size of variable sized fields exceeds limit: length {}, limit {}. Either use shorter aliases or remove an existing device.", + length, limit + ), + AnchorError::InvalidDeviceProtection { key_type } => write!( + f, + "Only recovery phrases can be protected but key type is {:?}", + key_type + ), + AnchorError::MutationNotAllowed { actual_principal, authorized_principal } => write!( + f, + "Device is protected. Must be authenticated with this device to mutate: authorized principal {}, actual principal {}", + authorized_principal, actual_principal + ), + AnchorError::MultipleRecoveryPhrases => write!(f, "There is already a recovery phrase and only one is allowed."), + AnchorError::CannotModifyDeviceKey => write!(f, "Device key cannot be updated."), + AnchorError::NotFound { device_key } => write!(f, "Device with key {} not found.", hex::encode(device_key)), + AnchorError::DuplicateDevice { device_key } => write!(f, "Device with key {} already exists on this anchor.", hex::encode(device_key)), + } + } +} diff --git a/src/internet_identity/src/storage/anchor/tests.rs b/src/internet_identity/src/storage/anchor/tests.rs new file mode 100644 index 0000000000..c68ed82cdb --- /dev/null +++ b/src/internet_identity/src/storage/anchor/tests.rs @@ -0,0 +1,361 @@ +use crate::storage::anchor::{Anchor, AnchorError, Device}; +use candid::Principal; +use internet_identity_interface::{DeviceProtection, KeyType, Purpose}; +use serde_bytes::ByteBuf; + +const TEST_CALLER_PUBKEY: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; + +#[test] +fn should_add_device() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + + assert_eq!(anchor.devices, vec![sample_device()]) +} + +#[test] +fn should_remove_device() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + assert_eq!(anchor.devices, vec![sample_device()]); + + anchor.remove_device(&sample_device().pubkey).unwrap(); + + assert_eq!(anchor.devices, vec![]); +} + +#[test] +fn should_modify_device() { + let mut anchor = Anchor::new(); + let mut device = sample_device(); + anchor.add_device(device.clone()).unwrap(); + device.alias = "modified alias".to_string(); + + anchor + .modify_device(&device.pubkey, device.clone()) + .unwrap(); + + assert_eq!(anchor.devices, vec![device]); +} + +#[test] +fn should_enforce_max_number_of_devices() { + let mut anchor = Anchor::new(); + for i in 0..10 { + anchor.add_device(device(i)).unwrap(); + } + + let result = anchor.add_device(device(10)); + + assert!(matches!(result, Err(AnchorError::TooManyDevices { .. }))); + assert_eq!(anchor.devices().len(), 10); +} + +#[test] +fn should_enforce_pubkey_limit() { + let mut anchor = Anchor::new(); + let mut device = sample_device(); + device.pubkey = ByteBuf::from([0; 301]); + + let result = anchor.add_device(device); + + assert!(matches!( + result, + Err(AnchorError::DeviceLimitExceeded { .. }) + )); + assert!(anchor.devices().is_empty()); +} + +#[test] +fn should_enforce_credential_id_limit() { + let mut anchor = Anchor::new(); + let mut device = sample_device(); + device.credential_id = Some(ByteBuf::from([0; 201])); + + let result = anchor.add_device(device); + + assert!(matches!( + result, + Err(AnchorError::DeviceLimitExceeded { .. }) + )); + assert!(anchor.devices().is_empty()); +} + +#[test] +fn should_enforce_alias_limit() { + let mut anchor = Anchor::new(); + let mut device = sample_device(); + device.alias = "a".repeat(65); + + let result = anchor.add_device(device); + + assert!(matches!( + result, + Err(AnchorError::DeviceLimitExceeded { .. }) + )); + assert!(anchor.devices().is_empty()); +} + +#[test] +fn should_enforce_unique_device_keys() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + + let result = anchor.add_device(sample_device()); + + assert!(matches!(result, Err(AnchorError::DuplicateDevice { .. }))); + assert_eq!(anchor.devices().len(), 1); +} + +#[test] +fn should_enforce_cumulative_device_limit() { + let mut anchor = Anchor::new(); + + for i in 0..4 { + anchor.add_device(large_device(i)).unwrap(); + } + let minimal_device = Device { + pubkey: Default::default(), + alias: "a".to_string(), + credential_id: None, + purpose: Purpose::Recovery, + key_type: KeyType::Unknown, + protection: DeviceProtection::Unprotected, + }; + + let result = anchor.add_device(minimal_device); + + assert!(matches!( + result, + Err(AnchorError::CumulativeDataLimitExceeded { .. }) + )); + assert_eq!(anchor.devices().len(), 4); +} + +#[test] +fn should_enforce_single_recovery_phrase() { + let mut anchor = Anchor::new(); + + anchor + .add_device(recovery_phrase(0, DeviceProtection::Unprotected)) + .unwrap(); + let result = anchor.add_device(recovery_phrase(1, DeviceProtection::Unprotected)); + + assert!(matches!( + result, + Err(AnchorError::MultipleRecoveryPhrases { .. }) + )); + assert_eq!(anchor.devices().len(), 1); +} + +#[test] +fn should_allow_protection_only_on_recovery_phrases() { + let mut anchor = Anchor::new(); + + let result = anchor.add_device(Device { + pubkey: Default::default(), + alias: "".to_string(), + credential_id: None, + purpose: Purpose::Recovery, + key_type: KeyType::Unknown, + protection: DeviceProtection::Protected, + }); + + assert!(matches!( + result, + Err(AnchorError::InvalidDeviceProtection { .. }) + )); + assert!(anchor.devices().is_empty()); +} + +#[test] +fn should_prevent_mutation_when_invariants_are_violated() { + let mut device1 = recovery_phrase(1, DeviceProtection::Unprotected); + let mut anchor = Anchor { + devices: vec![ + device1.clone(), + recovery_phrase(2, DeviceProtection::Unprotected), + ], + }; + + device1.alias = "new alias".to_string(); + let result = anchor.modify_device(&device1.pubkey.clone(), device1); + assert!(matches!(result, Err(AnchorError::MultipleRecoveryPhrases))); + assert_eq!(anchor.devices().len(), 2); +} + +#[test] +fn should_prevent_addition_when_invariants_are_violated() { + let mut anchor = Anchor { + devices: vec![ + recovery_phrase(1, DeviceProtection::Unprotected), + recovery_phrase(2, DeviceProtection::Unprotected), + ], + }; + + let result = anchor.add_device(sample_device()); + assert!(matches!(result, Err(AnchorError::MultipleRecoveryPhrases))); + assert_eq!(anchor.devices().len(), 2); +} + +#[test] +fn should_allow_removal_when_invariants_are_violated() { + let device1 = recovery_phrase(1, DeviceProtection::Unprotected); + let mut anchor = Anchor { + devices: vec![ + device1.clone(), + recovery_phrase(2, DeviceProtection::Unprotected), + ], + }; + + anchor.remove_device(&device1.pubkey).unwrap(); + + assert_eq!(anchor.devices().len(), 1); +} + +#[test] +fn should_enforce_caller_on_removal_of_protected_devices() { + let device1 = recovery_phrase(1, DeviceProtection::Protected); + let mut anchor = Anchor::new(); + anchor.add_device(device1.clone()).unwrap(); + + let result = anchor.remove_device(&device1.pubkey); + + assert!(matches!( + result, + Err(AnchorError::MutationNotAllowed { .. }) + )); + assert_eq!(anchor.devices().len(), 1); +} + +#[test] +fn should_enforce_caller_on_modification_of_protected_devices() { + let mut device1 = recovery_phrase(1, DeviceProtection::Protected); + let mut anchor = Anchor::new(); + anchor.add_device(device1.clone()).unwrap(); + + device1.alias = "new alias".to_string(); + + let result = anchor.modify_device(&device1.pubkey.clone(), device1); + + assert!(matches!( + result, + Err(AnchorError::MutationNotAllowed { .. }) + )); + assert_eq!(anchor.devices()[0].alias, "recovery phrase 1"); +} + +#[test] +fn should_allow_removal_of_protected_device_with_matching_caller() { + let mut device1 = recovery_phrase(1, DeviceProtection::Protected); + device1.pubkey = ByteBuf::from(TEST_CALLER_PUBKEY.clone()); + + let mut anchor = Anchor::new(); + anchor.add_device(device1.clone()).unwrap(); + + anchor.remove_device(&device1.pubkey).unwrap(); + + assert!(anchor.devices.is_empty()); +} + +#[test] +fn should_allow_modification_of_protected_device_with_matching_caller() { + let mut device1 = recovery_phrase(1, DeviceProtection::Protected); + device1.pubkey = ByteBuf::from(TEST_CALLER_PUBKEY.clone()); + + let mut anchor = Anchor::new(); + anchor.add_device(device1.clone()).unwrap(); + + device1.alias = "new alias".to_string(); + + anchor + .modify_device(&device1.pubkey.clone(), device1) + .unwrap(); + + assert_eq!(anchor.devices()[0].alias, "new alias"); +} + +#[test] +fn should_not_remove_unknown_device() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + + let result = anchor.remove_device(&device(1).pubkey); + + assert!(matches!(result, Err(AnchorError::NotFound { .. }))); + assert_eq!(anchor.devices().len(), 1); +} + +#[test] +fn should_not_modify_unknown_device() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + + let result = anchor.modify_device(&device(1).pubkey, device(1)); + + assert!(matches!(result, Err(AnchorError::NotFound { .. }))); + assert_eq!(anchor.devices()[0], sample_device()); +} + +#[test] +fn should_not_allow_modification_of_device_key() { + let mut anchor = Anchor::new(); + anchor.add_device(sample_device()).unwrap(); + + let result = anchor.modify_device(&sample_device().pubkey, device(1)); + + assert!(matches!( + result, + Err(AnchorError::CannotModifyDeviceKey { .. }) + )); + assert_eq!(anchor.devices()[0], sample_device()); +} + +fn sample_device() -> Device { + Device { + pubkey: ByteBuf::from("public key of some sample device"), + alias: "Test alias".to_string(), + credential_id: Some(ByteBuf::from("credential id of some sample device")), + purpose: Purpose::Authentication, + key_type: KeyType::Platform, + protection: DeviceProtection::Unprotected, + } +} + +fn device(n: u8) -> Device { + Device { + pubkey: ByteBuf::from([n; 100]), + alias: format!("test alias {}", n), + credential_id: Some(ByteBuf::from([n; 64])), + purpose: Purpose::Authentication, + key_type: KeyType::Platform, + protection: DeviceProtection::Unprotected, + } +} + +/// Device with variable fields size of 512 bytes. +fn large_device(n: u8) -> Device { + Device { + pubkey: ByteBuf::from(vec![n; 300]), + alias: "alias12chars".to_string(), + credential_id: Some(ByteBuf::from(vec![n; 200])), + purpose: Purpose::Authentication, + key_type: KeyType::Unknown, + protection: DeviceProtection::Unprotected, + } +} + +fn recovery_phrase(n: u8, protection: DeviceProtection) -> Device { + Device { + pubkey: ByteBuf::from(vec![n; 96]), + alias: format!("recovery phrase {}", n), + credential_id: Some(ByteBuf::from(vec![n; 64])), + purpose: Purpose::Recovery, + key_type: KeyType::SeedPhrase, + protection, + } +} + +pub fn test_caller() -> Principal { + Principal::self_authenticating(&TEST_CALLER_PUBKEY) +} diff --git a/src/internet_identity/src/storage/tests.rs b/src/internet_identity/src/storage/tests.rs index 76176860c1..12c8f63e07 100644 --- a/src/internet_identity/src/storage/tests.rs +++ b/src/internet_identity/src/storage/tests.rs @@ -1,5 +1,6 @@ use crate::archive::{ArchiveData, ArchiveInfo, ArchiveState}; -use crate::state::{Anchor, Device, PersistentState}; +use crate::state::PersistentState; +use crate::storage::anchor::{Anchor, Device}; use crate::storage::{DeviceDataInternal, Header, PersistentStateError, StorageError}; use crate::Storage; use candid::Principal; @@ -64,38 +65,16 @@ fn should_recover_header_from_memory() { assert_eq!(storage.version(), 3); } -#[test] -fn should_enforce_size_limit_for_devices() { - let mut storage = Storage::new((123456, 654321), VectorMemory::default()); - let user_number1 = storage.allocate_anchor_number().unwrap(); - - // anchor with size ~2200 bytes now fits v3 storage (it didn't in v1) - let mut anchor1 = sample_anchor_record(); - anchor1.devices.get_mut(0).unwrap().pubkey = ByteBuf::from(vec![1u8; 2048]); - let result = storage.write(user_number1, anchor1); - assert!(matches!(result, Ok(()))); - - // anchor with size ~4200 bytes is still rejected - let user_number2 = storage.allocate_anchor_number().unwrap(); - let mut anchor2 = sample_anchor_record(); - anchor2.devices.get_mut(0).unwrap().pubkey = ByteBuf::from(vec![1u8; 4096]); - let result = storage.write(user_number2, anchor2); - assert!(matches!( - result, - Err(StorageError::EntrySizeLimitExceeded(_)) - )); -} - #[test] fn should_read_previous_write_v3() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((12345, 678910), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); - let read_anchor = storage.read(user_number).unwrap(); + let read_anchor = storage.read(anchor_number).unwrap(); assert_eq!(anchor, read_anchor); } @@ -103,12 +82,12 @@ fn should_read_previous_write_v3() { fn should_read_previous_write_v5() { let memory = VectorMemory::default(); let mut storage = Storage::new((12345, 678910), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); - let read_anchor = storage.read(user_number).unwrap(); + let read_anchor = storage.read(anchor_number).unwrap(); assert_eq!(anchor, read_anchor); } @@ -117,11 +96,11 @@ fn should_serialize_first_record_v3() { const EXPECTED_LENGTH: usize = 192; let memory = VectorMemory::default(); let mut storage = layout_v3_storage((123, 456), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 123u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 123u64); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); let mut buf = [0u8; EXPECTED_LENGTH]; memory.read(RESERVED_HEADER_BYTES, &mut buf); @@ -130,7 +109,7 @@ fn should_serialize_first_record_v3() { .into_iter() .map(|d| Device::from(d)) .collect(); - assert_eq!(decoded_from_memory, anchor.devices); + assert_eq!(&decoded_from_memory, anchor.devices()); } #[test] @@ -138,11 +117,11 @@ fn should_serialize_first_record_v5() { const EXPECTED_LENGTH: usize = 191; let memory = VectorMemory::default(); let mut storage = Storage::new((123, 456), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 123u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 123u64); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); let mut buf = [0u8; EXPECTED_LENGTH]; memory.read(RESERVED_HEADER_BYTES, &mut buf); @@ -157,13 +136,13 @@ fn should_serialize_subsequent_record_to_expected_memory_location_v3() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((123, 456), memory.clone()); for _ in 0..100 { - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); } - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 223u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 223u64); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); let mut buf = [0u8; EXPECTED_LENGTH]; memory.read(RESERVED_HEADER_BYTES + EXPECTED_RECORD_OFFSET, &mut buf); @@ -172,7 +151,7 @@ fn should_serialize_subsequent_record_to_expected_memory_location_v3() { .into_iter() .map(|d| Device::from(d)) .collect(); - assert_eq!(decoded_from_memory, anchor.devices); + assert_eq!(&decoded_from_memory, anchor.devices()); } #[test] @@ -182,13 +161,13 @@ fn should_serialize_subsequent_record_to_expected_memory_location_v5() { let memory = VectorMemory::default(); let mut storage = Storage::new((123, 456), memory.clone()); for _ in 0..100 { - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); } - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 223u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 223u64); - let anchor = sample_anchor_record(); - storage.write(user_number, anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + storage.write(anchor_number, anchor.clone()).unwrap(); let mut buf = [0u8; EXPECTED_LENGTH]; memory.read(RESERVED_HEADER_BYTES + EXPECTED_RECORD_OFFSET, &mut buf); @@ -200,9 +179,9 @@ fn should_serialize_subsequent_record_to_expected_memory_location_v5() { fn should_not_write_using_anchor_number_outside_allocated_range() { let memory = VectorMemory::default(); let mut storage = Storage::new((123, 456), memory.clone()); - storage.allocate_anchor_number().unwrap(); + let (_, anchor) = storage.allocate_anchor().unwrap(); - let result = storage.write(222, sample_anchor_record().clone()); + let result = storage.write(222, anchor); assert!(matches!(result, Err(StorageError::BadAnchorNumber(_)))) } @@ -211,11 +190,11 @@ fn should_deserialize_first_record_v3() { let memory = VectorMemory::default(); memory.grow(3); let mut storage = layout_v3_storage((123, 456), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 123u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 123u64); - let anchor = sample_anchor_record(); - let buf = candid::encode_one(&anchor.devices).unwrap(); + anchor.add_device(sample_device()).unwrap(); + let buf = candid::encode_one(&anchor.devices()).unwrap(); memory.write(RESERVED_HEADER_BYTES, &(buf.len() as u16).to_le_bytes()); memory.write(RESERVED_HEADER_BYTES + 2, &buf); @@ -228,10 +207,10 @@ fn should_deserialize_first_record_v5() { let memory = VectorMemory::default(); memory.grow(3); let mut storage = Storage::new((123, 456), memory.clone()); - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 123u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 123u64); - let anchor = sample_anchor_record(); + anchor.add_device(sample_device()).unwrap(); let buf = candid::encode_one(&anchor).unwrap(); memory.write(RESERVED_HEADER_BYTES, &(buf.len() as u16).to_le_bytes()); memory.write(RESERVED_HEADER_BYTES + 2, &buf); @@ -247,13 +226,13 @@ fn should_deserialize_subsequent_record_at_expected_memory_location_v3() { memory.grow(9); // grow memory to accommodate a write to record 100 let mut storage = layout_v3_storage((123, 456), memory.clone()); for _ in 0..100 { - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); } - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 223u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 223u64); - let anchor = sample_anchor_record(); - let buf = candid::encode_one(&anchor.devices).unwrap(); + anchor.add_device(sample_device()).unwrap(); + let buf = candid::encode_one(&anchor.devices()).unwrap(); memory.write( RESERVED_HEADER_BYTES + EXPECTED_RECORD_OFFSET, &(buf.len() as u16).to_le_bytes(), @@ -271,12 +250,12 @@ fn should_deserialize_subsequent_record_at_expected_memory_location_v5() { memory.grow(9); // grow memory to accommodate a write to record 100 let mut storage = Storage::new((123, 456), memory.clone()); for _ in 0..100 { - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); } - let user_number = storage.allocate_anchor_number().unwrap(); - assert_eq!(user_number, 223u64); + let (anchor_number, mut anchor) = storage.allocate_anchor().unwrap(); + assert_eq!(anchor_number, 223u64); - let anchor = sample_anchor_record(); + anchor.add_device(sample_device()).unwrap(); let buf = candid::encode_one(&anchor).unwrap(); memory.write( RESERVED_HEADER_BYTES + EXPECTED_RECORD_OFFSET, @@ -292,7 +271,7 @@ fn should_deserialize_subsequent_record_at_expected_memory_location_v5() { fn should_not_read_using_anchor_number_outside_allocated_range() { let memory = VectorMemory::default(); let mut storage = Storage::new((123, 456), memory.clone()); - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); let result = storage.read(222); assert!(matches!(result, Err(StorageError::BadAnchorNumber(_)))) @@ -303,7 +282,7 @@ fn should_save_and_restore_persistent_state() { let memory = VectorMemory::default(); let mut storage = Storage::new((123, 456), memory.clone()); storage.flush(); - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); let persistent_state = sample_persistent_state(); @@ -357,7 +336,7 @@ fn should_save_persistent_state_at_expected_memory_address_with_anchors() { storage.flush(); for _ in 0..100 { - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); } storage.write_persistent_state(&sample_persistent_state()); @@ -393,7 +372,7 @@ fn should_not_panic_on_unallocated_persistent_state_mem_address() { let mut storage = Storage::new((10_000, 3_784_873), memory.clone()); storage.flush(); for _ in 0..32 { - storage.allocate_anchor_number(); + storage.allocate_anchor(); } assert!(matches!( @@ -410,15 +389,15 @@ fn should_overwrite_persistent_state_with_next_anchor() { let mut storage = Storage::new((10_000, 3_784_873), memory.clone()); storage.flush(); - storage.allocate_anchor_number().unwrap(); + storage.allocate_anchor().unwrap(); storage.write_persistent_state(&sample_persistent_state()); let mut buf = vec![0u8; 4]; memory.read(EXPECTED_ADDRESS, &mut buf); assert_eq!(buf, PERSISTENT_STATE_MAGIC); - let anchor = storage.allocate_anchor_number().unwrap(); - storage.write(anchor, sample_anchor_record()).unwrap(); + let (anchor_number, anchor) = storage.allocate_anchor().unwrap(); + storage.write(anchor_number, anchor).unwrap(); let mut buf = vec![0u8; 4]; memory.read(EXPECTED_ADDRESS, &mut buf); @@ -460,7 +439,7 @@ fn should_stay_in_v3_if_no_migration_configured() { let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); for _ in 0..32 { - storage.allocate_anchor_number(); + storage.allocate_anchor(); } assert!(matches!( @@ -475,7 +454,7 @@ fn should_start_migration_when_configuring() { let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); for _ in 0..32 { - storage.allocate_anchor_number(); + storage.allocate_anchor(); } storage.configure_migration(100); @@ -494,12 +473,13 @@ fn should_migrate_anchors() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); + let (anchor_number, anchor) = storage.allocate_anchor().unwrap(); for _ in 0..32 { - storage.allocate_anchor_number(); + storage.allocate_anchor(); } storage.configure_migration(100); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(anchor_number, anchor).unwrap(); assert_eq!(storage.migration_state(), MigrationState::Finished); } @@ -509,31 +489,34 @@ fn should_load_anchors_from_memory_after_migration() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); + let (_, mut anchor) = storage.allocate_anchor().unwrap(); for _ in 0..32 { - storage.allocate_anchor_number(); + storage.allocate_anchor(); } // write some data to check for migration - let mut test_anchor = sample_anchor_record(); - test_anchor.devices.push(Device { - pubkey: ByteBuf::from("recovery pub key"), - alias: "my protected recovery phrase".to_string(), - credential_id: None, - purpose: Purpose::Recovery, - key_type: KeyType::SeedPhrase, - protection: DeviceProtection::Protected, - }); - storage.write(10_001, test_anchor.clone()).unwrap(); + anchor.add_device(sample_device()).unwrap(); + anchor + .add_device(Device { + pubkey: ByteBuf::from("recovery pub key"), + alias: "my protected recovery phrase".to_string(), + credential_id: None, + purpose: Purpose::Recovery, + key_type: KeyType::SeedPhrase, + protection: DeviceProtection::Protected, + }) + .unwrap(); + storage.write(10_001, anchor.clone()).unwrap(); storage.configure_migration(100); // write data to trigger migration - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor.clone()).unwrap(); assert_eq!(storage.migration_state(), MigrationState::Finished); let storage = Storage::from_memory(memory.clone()).unwrap(); assert_eq!(storage.version(), 5); - assert_eq!(storage.read(10_001).unwrap(), test_anchor); + assert_eq!(storage.read(10_001).unwrap(), anchor); } #[test] @@ -541,8 +524,9 @@ fn should_pause_migration_with_batch_size_0() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); - for _ in 0..32 { - storage.allocate_anchor_number(); + let (_, anchor) = storage.allocate_anchor().unwrap(); + for _ in 0..31 { + storage.allocate_anchor(); } storage.configure_migration(5); @@ -555,7 +539,7 @@ fn should_pause_migration_with_batch_size_0() { } ); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor.clone()).unwrap(); assert_eq!( storage.migration_state(), @@ -575,7 +559,7 @@ fn should_pause_migration_with_batch_size_0() { } ); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor).unwrap(); assert_eq!( storage.migration_state(), @@ -591,11 +575,12 @@ fn should_recover_migration_state_from_memory() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); - for _ in 0..32 { - storage.allocate_anchor_number(); + let (_, anchor) = storage.allocate_anchor().unwrap(); + for _ in 0..31 { + storage.allocate_anchor(); } storage.configure_migration(5); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor).unwrap(); assert_eq!( storage.migration_state(), @@ -620,12 +605,13 @@ fn should_restart_paused_migration() { let memory = VectorMemory::default(); let mut storage = layout_v3_storage((10_000, 3_784_873), memory.clone()); storage.flush(); - for _ in 0..32 { - storage.allocate_anchor_number(); + let (_, anchor) = storage.allocate_anchor().unwrap(); + for _ in 0..31 { + storage.allocate_anchor(); } storage.configure_migration(5); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor.clone()).unwrap(); storage.configure_migration(0); assert_eq!( @@ -637,20 +623,18 @@ fn should_restart_paused_migration() { ); storage.configure_migration(100); - storage.write(10_000, sample_anchor_record()).unwrap(); + storage.write(10_000, anchor).unwrap(); assert_eq!(storage.migration_state(), MigrationState::Finished); } -fn sample_anchor_record() -> Anchor { - Anchor { - devices: vec![Device { - pubkey: ByteBuf::from("hello world, I am a public key"), - alias: "my test device".to_string(), - credential_id: Some(ByteBuf::from("this is the credential id")), - purpose: Purpose::Authentication, - key_type: KeyType::Unknown, - protection: DeviceProtection::Protected, - }], +fn sample_device() -> Device { + Device { + pubkey: ByteBuf::from("hello world, I am a public key"), + alias: "my test device".to_string(), + credential_id: Some(ByteBuf::from("this is the credential id")), + purpose: Purpose::Authentication, + key_type: KeyType::Unknown, + protection: DeviceProtection::Unprotected, } } diff --git a/src/internet_identity/tests/tests.rs b/src/internet_identity/tests/tests.rs index 7ad775141a..3fca957ecc 100644 --- a/src/internet_identity/tests/tests.rs +++ b/src/internet_identity/tests/tests.rs @@ -851,7 +851,7 @@ mod device_management_tests { expect_user_error_with_message( result, CanisterCalledTrap, - Regex::new("Device already added\\.").unwrap(), + Regex::new("Device with key \\w+ already exists on this anchor\\.").unwrap(), ); Ok(()) } @@ -956,7 +956,7 @@ mod device_management_tests { expect_user_error_with_message( result, CanisterCalledTrap, - Regex::new("Devices exceed allowed storage limit\\. Either use shorter aliases or remove an existing device\\.").unwrap(), + Regex::new("Cumulative size of variable sized fields exceeds limit: length \\d+, limit \\d+\\. Either use shorter aliases or remove an existing device\\.").unwrap(), ); Ok(()) } @@ -1054,7 +1054,7 @@ mod device_management_tests { expect_user_error_with_message( result, CanisterCalledTrap, - Regex::new("device key may not be updated").unwrap(), + Regex::new("Device key cannot be updated\\.").unwrap(), ); }