diff --git a/CHANGELOG.md b/CHANGELOG.md index ffce80ff4d3..d5b37905415 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,18 @@ [Full Changelog](In progress) +## ⚠️ Breaking Changes ⚠️ + +### Autofill + +- **BREAKING**: `Store::new()` now takes an `EncryptorDecryptor`, which the store hands to the database and which is used for every encrypted column, and `scrub_undecryptable_credit_card_data_for_remote_replacement()` no longer takes an encryption key. Credit-card encryption moved to the shared `db-crypto` crate. `encrypt_string()` and `decrypt_string()` are unchanged. + ## ✨ What's Changed ✨ ### Autofill - `update_address()` now sets `time_last_modified` to the time of the update, matching `update_credit_card()` and `update_passport()`. +- Credit-card numbers are now stored as a versioned JSON blob rather than the bare number, so a second encrypted field can be added later without rewriting every row again. `run_maintenance()` rewrites existing rows once, in place, without touching sync metadata. The schema version is bumped to 6, so opening the database with an older build now fails rather than reading a blob as a card number. ### Ads-Client diff --git a/Cargo.lock b/Cargo.lock index 625905a7e70..1845cbe5235 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -208,6 +208,7 @@ name = "autofill" version = "0.1.0" dependencies = [ "anyhow", + "db-crypto", "error-support", "interrupt-support", "jwcrypto", diff --git a/components/autofill/Cargo.toml b/components/autofill/Cargo.toml index ed0f3199966..6d52686307b 100644 --- a/components/autofill/Cargo.toml +++ b/components/autofill/Cargo.toml @@ -12,6 +12,7 @@ exclude = ["/android", "/ios"] anyhow = "1.0" error-support = { path = "../support/error" } interrupt-support = { path = "../support/interrupt" } +db-crypto = { path = "../support/db-crypto" } jwcrypto = { path = "../support/jwcrypto" } lazy_static = "1.4" parking_lot = ">=0.11,<=0.12" diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index 7fc40bace05..c2ba882d1d6 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -164,9 +164,12 @@ interface AutofillApiError { UnexpectedAutofillApiError(string reason); }; +[External = "db_crypto"] +typedef trait_with_foreign EncryptorDecryptor; + interface Store { [Throws=AutofillApiError] - constructor(string dbpath); + constructor(string dbpath, EncryptorDecryptor encdec); [Throws=AutofillApiError] CreditCard add_credit_card(UpdatableCreditCardFields cc); @@ -257,10 +260,13 @@ interface Store { /// NB: This function was created to unblock iOS credit card users who are unable to sync records and should not be used /// outside of this use case. [Throws=AutofillApiError, Self=ByArc] - CreditCardsDeletionMetrics scrub_undecryptable_credit_card_data_for_remote_replacement(string local_encryption_key); + CreditCardsDeletionMetrics scrub_undecryptable_credit_card_data_for_remote_replacement(); /// Run maintenance on the DB /// + /// Also completes the one-off rewrite of credit-card rows that predate the + /// versioned secure-fields encoding. Cheap once that has finished. + /// /// This is intended to be run during idle time and will take steps / to clean up / shrink the /// database. [Throws=AutofillApiError] diff --git a/components/autofill/src/db/credit_cards.rs b/components/autofill/src/db/credit_cards.rs index 3499b646621..47def3cb5f4 100644 --- a/components/autofill/src/db/credit_cards.rs +++ b/components/autofill/src/db/credit_cards.rs @@ -5,14 +5,14 @@ use crate::db::{ models::{ - credit_card::{InternalCreditCard, UpdatableCreditCardFields}, + credit_card::{InternalCreditCard, SecureCreditCardFields, UpdatableCreditCardFields}, Metadata, }, schema::{CREDIT_CARD_COMMON_COLS, CREDIT_CARD_COMMON_VALS}, + AutofillDb, }; use crate::error::*; -use jwcrypto::EncryptorDecryptor; use rusqlite::{Connection, Transaction}; use sync_guid::Guid; use types::Timestamp; @@ -226,16 +226,22 @@ pub fn scrub_encrypted_credit_card_data(conn: &Connection) -> Result<()> { } pub fn scrub_undecryptable_credit_card_data_for_remote_replacement( - conn: &Connection, - local_encryption_key: String, + db: &AutofillDb, ) -> Result { + let conn = &db.writer; let tx = conn.unchecked_transaction()?; let mut scrubbed_records = 0; - let encdec = EncryptorDecryptor::new(local_encryption_key.as_str()).unwrap(); let undecryptable_record_ids = get_all_credit_cards(conn)? .into_iter() - .filter(|credit_card| encdec.decrypt(&credit_card.cc_number_enc).is_err()) + .filter(|credit_card| { + SecureCreditCardFields::decrypt( + &credit_card.cc_number_enc, + db.encdec.as_ref(), + credit_card.guid.as_str(), + ) + .is_err() + }) .map(|credit_card| credit_card.guid) .collect::>(); @@ -290,7 +296,7 @@ pub fn touch(conn: &Connection, guid: &Guid) -> Result<()> { pub(crate) mod tests { use super::*; use crate::db::test::new_mem_db; - use crate::encryption::EncryptorDecryptor; + use crate::encryption::{encrypt_str, random_key_encryptor}; use nss_as::ensure_initialized; use sync15::bso::IncomingBso; @@ -588,13 +594,13 @@ pub(crate) mod tests { fn test_credit_card_delete() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = db.encdec.clone(); let saved_credit_card = add_credit_card( &db, UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec.encrypt("1234567812345678")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234567812345678")?, cc_number_last_4: "5678".to_string(), cc_exp_month: 10, cc_exp_year: 2025, @@ -610,7 +616,7 @@ pub(crate) mod tests { &db, UpdatableCreditCardFields { cc_name: "john doe".to_string(), - cc_number_enc: encdec.encrypt("1234123412341234")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234123412341234")?, cc_number_last_4: "1234".to_string(), cc_exp_month: 5, cc_exp_year: 2024, @@ -620,7 +626,8 @@ pub(crate) mod tests { // create a mirror record to check that a tombstone record is created upon deletion let cc2_guid = saved_credit_card2.guid.clone(); - let payload = saved_credit_card2.into_test_incoming_bso(&encdec, Default::default()); + let payload = + saved_credit_card2.into_test_incoming_bso(encdec.as_ref(), Default::default()); test_insert_mirror_record(&db, payload); @@ -656,14 +663,14 @@ pub(crate) mod tests { fn test_scrub_encrypted_credit_card_data() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = db.encdec.clone(); let mut saved_credit_cards = Vec::with_capacity(10); for _ in 0..5 { saved_credit_cards.push(add_credit_card( &db, UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec.encrypt("1234567812345678")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234567812345678")?, cc_number_last_4: "5678".to_string(), cc_exp_month: 10, cc_exp_year: 2025, @@ -682,19 +689,16 @@ pub(crate) mod tests { } #[test] - fn test_scrub_undecryptable_credit_card_date_for_remote_replacement() -> Result<()> { + fn test_scrub_undecryptable_credit_card_data_for_remote_replacement() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let old_key = EncryptorDecryptor::create_key()?; - let old_encdec = EncryptorDecryptor::new(&old_key)?; - let key = EncryptorDecryptor::create_key()?; - let encdec = EncryptorDecryptor::new(&key)?; + let foreign_encdec = random_key_encryptor()?; let undecryptable_credit_card = add_credit_card( &db, UpdatableCreditCardFields { cc_name: "jane doe".to_string(), - cc_number_enc: old_encdec.encrypt("2345678923456789")?, + cc_number_enc: encrypt_str(&foreign_encdec, "2345678923456789")?, cc_number_last_4: "6789".to_string(), cc_exp_month: 9, cc_exp_year: 2027, @@ -702,7 +706,7 @@ pub(crate) mod tests { }, )?; - let encrypted_cc_number = encdec.encrypt("567812345678123456781")?; + let encrypted_cc_number = encrypt_str(db.encdec.as_ref(), "567812345678123456781")?; let credit_card = add_credit_card( &db, UpdatableCreditCardFields { @@ -715,7 +719,7 @@ pub(crate) mod tests { }, )?; - let metrics = scrub_undecryptable_credit_card_data_for_remote_replacement(&db.writer, key)?; + let metrics = scrub_undecryptable_credit_card_data_for_remote_replacement(&db)?; assert_eq!(metrics.total_scrubbed_records, 1); let credit_cards = get_all_credit_cards(&db)?; diff --git a/components/autofill/src/db/migrate_cc_secure_fields.rs b/components/autofill/src/db/migrate_cc_secure_fields.rs new file mode 100644 index 00000000000..fe2055c198e --- /dev/null +++ b/components/autofill/src/db/migrate_cc_secure_fields.rs @@ -0,0 +1,389 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. +*/ + +//! Rewrites credit-card rows that predate the versioned secure-fields blob. + +use crate::db::store::{get_meta, put_meta}; +use crate::db::AutofillDb; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; +use crate::error::Result; +use error_support::info; +use rusqlite::{named_params, Transaction}; +use serde::{Deserialize, Serialize}; + +const MIGRATION_DONE_META_KEY: &str = "cc_secure_fields_migrated"; + +/// Frozen on purpose: a later change to `SecureCreditCardFields` must not alter +/// what this migration already wrote for rows it has rewritten. +#[derive(Serialize)] +struct FrozenV1<'a> { + v: u8, + n: &'a str, +} + +/// Only the presence of `v` matters, so the payload is irrelevant. +#[derive(Deserialize)] +struct VersionProbe { + #[allow(dead_code)] + v: u8, +} + +#[derive(Debug, Default, PartialEq, Eq)] +pub struct CreditCardMigrationMetrics { + pub migrated: u64, + pub already_migrated: u64, + pub undecryptable: u64, + pub conflicted: u64, +} + +/// The flag is only set when nothing was skipped: a row the key could not read +/// is left for a later run, because the key can be wrong now and right later. +pub(crate) fn migrate_cc_secure_fields_if_needed( + db: &AutofillDb, +) -> Result { + if get_meta::(&db.writer, MIGRATION_DONE_META_KEY)?.unwrap_or(false) { + return Ok(CreditCardMigrationMetrics::default()); + } + + let tx = db.writer.unchecked_transaction()?; + let metrics = migrate_cc_secure_fields(&tx, db.encdec.as_ref())?; + if metrics.undecryptable == 0 && metrics.conflicted == 0 { + put_meta(&tx, MIGRATION_DONE_META_KEY, &true)?; + } + tx.commit()?; + + if metrics != CreditCardMigrationMetrics::default() { + // No guids and no card data - just counts, so this is safe to log. + info!( + "cc secure-fields migration: {} migrated, {} already migrated, \ + {} unreadable, {} conflicted", + metrics.migrated, metrics.already_migrated, metrics.undecryptable, metrics.conflicted + ); + } + Ok(metrics) +} + +pub(crate) fn migrate_cc_secure_fields( + tx: &Transaction<'_>, + encdec: &dyn EncryptorDecryptor, +) -> Result { + let mut metrics = CreditCardMigrationMetrics::default(); + + // Empty ciphertext marks scrubbed data waiting to be replaced from Sync, + // not an encrypted number, so it has to stay empty. + let rows = tx + .prepare("SELECT guid, cc_number_enc FROM credit_cards_data WHERE cc_number_enc != ''")? + .query_map([], |row| { + Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) + })? + .collect::>>()?; + + for (guid, old_ciphertext) in rows { + // Giving up on a card is what + // `scrub_undecryptable_credit_card_data_for_remote_replacement` is for. + let Ok(cleartext) = decrypt_str(encdec, &old_ciphertext) else { + metrics.undecryptable += 1; + continue; + }; + + if serde_json::from_str::(&cleartext).is_ok() { + metrics.already_migrated += 1; + continue; + } + + let blob = serde_json::to_string(&FrozenV1 { + v: 1, + n: &cleartext, + })?; + let new_ciphertext = encrypt_str(encdec, &blob)?; + + // Matching the old ciphertext makes this a compare-and-swap, so a row + // written between the SELECT above and here is not clobbered. No other + // column is named, which is the point of doing this in raw SQL. + let updated = tx.execute( + "UPDATE credit_cards_data + SET cc_number_enc = :new + WHERE guid = :guid AND cc_number_enc = :old", + named_params! { + ":new": &new_ciphertext, + ":guid": &guid, + ":old": &old_ciphertext, + }, + )?; + + if updated == 1 { + metrics.migrated += 1; + } else { + metrics.conflicted += 1; + } + } + + Ok(metrics) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::credit_cards::{add_credit_card, get_credit_card}; + use crate::db::models::credit_card::{ + InternalCreditCard, SecureCreditCardFields, UpdatableCreditCardFields, + }; + use crate::db::test::{new_mem_db, new_mem_db_with_encdec}; + use crate::encryption::{static_key_encryptor, ManagedEncryptorDecryptor}; + use nss_as::ensure_initialized; + use std::sync::Arc; + use sync_guid::Guid; + + const NUMBER: &str = "4111111111117629"; + + fn add_legacy_card( + db: &AutofillDb, + encdec: &dyn EncryptorDecryptor, + number: &str, + ) -> InternalCreditCard { + add_credit_card( + db, + UpdatableCreditCardFields { + cc_name: "jane doe".to_string(), + cc_number_enc: encrypt_str(encdec, number).unwrap(), + cc_number_last_4: number[number.len() - 4..].to_string(), + cc_exp_month: 9, + cc_exp_year: 2027, + cc_type: "visa".to_string(), + }, + ) + .unwrap() + } + + fn run(db: &AutofillDb) -> CreditCardMigrationMetrics { + let tx = db.writer.unchecked_transaction().unwrap(); + let metrics = migrate_cc_secure_fields(&tx, db.encdec.as_ref()).unwrap(); + tx.commit().unwrap(); + metrics + } + + fn run_if_needed(db: &AutofillDb) -> CreditCardMigrationMetrics { + migrate_cc_secure_fields_if_needed(db).unwrap() + } + + fn new_encdec() -> ManagedEncryptorDecryptor { + ensure_initialized(); + static_key_encryptor(&db_crypto::create_key().unwrap()).unwrap() + } + + #[test] + fn test_legacy_row_is_rewritten_as_a_blob() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + let metrics = run(&db); + assert_eq!(metrics.migrated, 1); + assert_eq!(metrics.already_migrated, 0); + + let stored = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + assert_ne!( + stored, card.cc_number_enc, + "the ciphertext must have been replaced" + ); + assert_eq!( + decrypt_str(db.encdec.as_ref(), &stored).unwrap(), + format!(r#"{{"v":1,"n":"{NUMBER}"}}"#), + "the on-disk format is frozen - a change here breaks v1 readers" + ); + } + + #[test] + fn test_the_number_survives_the_rewrite() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + run(&db); + + let stored = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + assert_eq!( + SecureCreditCardFields::decrypt(&stored, db.encdec.as_ref(), card.guid.as_str()) + .unwrap() + .cc_number, + NUMBER + ); + } + + #[test] + fn test_sync_metadata_is_not_touched() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let before = get_credit_card(&db, &card.guid).unwrap().metadata; + + run(&db); + + let after = get_credit_card(&db, &card.guid).unwrap().metadata; + assert_eq!( + after.sync_change_counter, before.sync_change_counter, + "a re-encryption is not a user edit - bumping this uploads every card" + ); + assert_eq!(after.time_last_modified, before.time_last_modified); + assert_eq!(after.time_created, before.time_created); + assert_eq!(after.time_last_used, before.time_last_used); + assert_eq!(after.times_used, before.times_used); + } + + #[test] + fn test_second_run_is_a_noop() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + assert_eq!(run(&db).migrated, 1); + let after_first = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + + let metrics = run(&db); + assert_eq!(metrics.migrated, 0); + assert_eq!(metrics.already_migrated, 1); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + after_first, + "a row already in the new format must not be re-encrypted" + ); + } + + #[test] + fn test_undecryptable_row_is_left_alone() { + let db = new_mem_db_with_encdec(Arc::new(new_encdec())); + let old_encdec = new_encdec(); + let card = add_legacy_card(&db, &old_encdec, "2345678923456789"); + + let metrics = run(&db); + assert_eq!(metrics.undecryptable, 1); + assert_eq!(metrics.migrated, 0); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + card.cc_number_enc, + "an unreadable row must survive untouched, not be scrubbed" + ); + } + + #[test] + fn test_scrubbed_row_stays_scrubbed() { + let db = new_mem_db(); + let card = add_credit_card( + &db, + UpdatableCreditCardFields { + cc_name: "jane doe".to_string(), + cc_number_enc: String::new(), + cc_number_last_4: "7629".to_string(), + cc_exp_month: 9, + cc_exp_year: 2027, + cc_type: "visa".to_string(), + }, + ) + .unwrap(); + + let metrics = run(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + assert!( + get_credit_card(&db, &card.guid) + .unwrap() + .cc_number_enc + .is_empty(), + "empty ciphertext means 'replace from Sync', not 'encrypted'" + ); + } + + #[test] + fn test_mixed_formats_in_one_pass() { + let db = new_mem_db(); + let legacy = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let already = add_credit_card( + &db, + UpdatableCreditCardFields { + cc_name: "john doe".to_string(), + cc_number_enc: SecureCreditCardFields { + cc_number: "5500005555555559".to_string(), + } + .encrypt(db.encdec.as_ref(), "new-row") + .unwrap(), + cc_number_last_4: "5559".to_string(), + cc_exp_month: 1, + cc_exp_year: 2030, + cc_type: "mastercard".to_string(), + }, + ) + .unwrap(); + + let metrics = run(&db); + assert_eq!(metrics.migrated, 1); + assert_eq!(metrics.already_migrated, 1); + + for (guid, expected) in [(&legacy.guid, NUMBER), (&already.guid, "5500005555555559")] { + let stored = get_credit_card(&db, guid).unwrap().cc_number_enc; + assert_eq!( + SecureCreditCardFields::decrypt(&stored, db.encdec.as_ref(), guid.as_str()) + .unwrap() + .cc_number, + expected + ); + } + } + + #[test] + fn test_empty_table_is_fine() { + let db = new_mem_db(); + let metrics = run(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + let _ = Guid::new("unused"); + } + + #[test] + fn test_the_flag_stops_the_second_pass() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + assert_eq!(run_if_needed(&db).migrated, 1); + let after_first = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + + let metrics = run_if_needed(&db); + assert_eq!( + metrics, + CreditCardMigrationMetrics::default(), + "a finished migration must not look at the rows again - not even to \ + count them as already migrated" + ); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + after_first + ); + } + + #[test] + fn test_the_flag_is_withheld_while_a_row_is_unreadable() { + let db = new_mem_db_with_encdec(Arc::new(new_encdec())); + let old_encdec = new_encdec(); + add_legacy_card(&db, &old_encdec, "2345678923456789"); + add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + let first = run_if_needed(&db); + assert_eq!(first.migrated, 1); + assert_eq!(first.undecryptable, 1); + + let second = run_if_needed(&db); + assert_eq!(second.undecryptable, 1, "the pass was wrongly marked done"); + assert_eq!(second.already_migrated, 1); + assert_eq!(second.migrated, 0); + } + + #[test] + fn test_an_empty_table_still_marks_the_migration_done() { + let db = new_mem_db(); + assert_eq!(run_if_needed(&db), CreditCardMigrationMetrics::default()); + + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let metrics = run_if_needed(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + card.cc_number_enc, + "the flag is set, so this legacy row is not picked up - which is why \ + the format switch has to ship with the migration, not after it" + ); + } +} diff --git a/components/autofill/src/db/mod.rs b/components/autofill/src/db/mod.rs index cfb5e35500f..3ca5c26efe7 100644 --- a/components/autofill/src/db/mod.rs +++ b/components/autofill/src/db/mod.rs @@ -4,11 +4,13 @@ pub mod addresses; pub mod credit_cards; +pub(crate) mod migrate_cc_secure_fields; pub mod models; pub mod passports; pub mod schema; pub mod store; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use error_support::error; @@ -24,21 +26,22 @@ use url::Url; pub struct AutofillDb { pub writer: Connection, + pub encdec: Arc, interrupt_handle: Arc, } impl AutofillDb { - pub fn new(db_path: impl AsRef) -> Result { + pub fn new(db_path: impl AsRef, encdec: Arc) -> Result { let db_path = normalize_path(db_path)?; - Self::new_named(db_path) + Self::new_named(db_path, encdec) } - pub fn new_memory(db_path: &str) -> Result { + pub fn new_memory(db_path: &str, encdec: Arc) -> Result { let name = PathBuf::from(format!("file:{}?mode=memory&cache=shared", db_path)); - Self::new_named(name) + Self::new_named(name, encdec) } - fn new_named(db_path: PathBuf) -> Result { + fn new_named(db_path: PathBuf, encdec: Arc) -> Result { // We always create the read-write connection for an initial open so // we can create the schema and/or do version upgrades. let flags = OpenFlags::SQLITE_OPEN_NO_MUTEX @@ -55,6 +58,7 @@ impl AutofillDb { Ok(Self { interrupt_handle: Arc::new(SqlInterruptHandle::new(&conn)), writer: conn, + encdec, }) } @@ -152,10 +156,19 @@ pub mod test { // A helper for our tests to get their own memory Api. static ATOMIC_COUNTER: AtomicUsize = AtomicUsize::new(0); + pub fn test_encdec() -> Arc { + nss_as::ensure_initialized(); + Arc::new(crate::encryption::random_key_encryptor().expect("should get a key")) + } + pub fn new_mem_db() -> AutofillDb { + new_mem_db_with_encdec(test_encdec()) + } + + pub fn new_mem_db_with_encdec(encdec: Arc) -> AutofillDb { error_support::init_for_tests(); let counter = ATOMIC_COUNTER.fetch_add(1, Ordering::Relaxed); - AutofillDb::new_memory(&format!("test_autofill-api-{}", counter)) + AutofillDb::new_memory(&format!("test_autofill-api-{}", counter), encdec) .expect("should get an API") } } diff --git a/components/autofill/src/db/models/credit_card.rs b/components/autofill/src/db/models/credit_card.rs index a4149c6e52f..8e733625bb4 100644 --- a/components/autofill/src/db/models/credit_card.rs +++ b/components/autofill/src/db/models/credit_card.rs @@ -3,8 +3,19 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +//! Credit-card models and the cleartext fields that are stored encrypted. +//! +//! Only the number is encrypted today, but the encrypted value is a versioned +//! JSON blob rather than the bare number, so a CVV can be added later. +//! +//! Rows written before that change still decrypt to a bare number. `decrypt` +//! accepts both, and `db::migrate_cc_secure_fields` rewrites the old ones. + use super::Metadata; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; +use crate::error::Error; use rusqlite::Row; +use serde::{Deserialize, Serialize}; use sync_guid::Guid; #[derive(Debug, Clone, Default)] @@ -104,3 +115,163 @@ impl InternalCreditCard { self.cc_number_enc.is_empty() } } + +/// The version written today. A reader that meets a higher version fails rather +/// than guessing, so a future format cannot be misread as this one. +const SECURE_FIELDS_VERSION: u8 = 1; + +/// `db::migrate_cc_secure_fields` keeps its own frozen copy of the v1 shape, so +/// changing this struct does not change what it already wrote. +#[derive(Serialize, Deserialize)] +struct StoredSecureFields { + v: u8, + n: String, +} + +/// Cleartext credit-card fields that are encrypted for local storage. +#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] +pub struct SecureCreditCardFields { + pub cc_number: String, +} + +impl SecureCreditCardFields { + /// `guid` only identifies the record in error messages. + pub fn encrypt( + &self, + encdec: &dyn EncryptorDecryptor, + guid: &str, + ) -> crate::error::Result { + let stored = StoredSecureFields { + v: SECURE_FIELDS_VERSION, + n: self.cc_number.clone(), + }; + let cleartext = serde_json::to_string(&stored) + .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting {guid})")))?; + encrypt_str(encdec, &cleartext) + .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting {guid})"))) + } + + pub fn decrypt( + ciphertext: &str, + encdec: &dyn EncryptorDecryptor, + guid: &str, + ) -> crate::error::Result { + let cleartext = decrypt_str(encdec, ciphertext).map_err(|e| { + Error::DecryptionFailed(format!( + "{e} (decrypting {guid}, ciphertext length: {})", + ciphertext.len() + )) + })?; + + match serde_json::from_str::(&cleartext) { + Ok(stored) if stored.v == SECURE_FIELDS_VERSION => Ok(Self { + cc_number: stored.n, + }), + Ok(stored) => Err(Error::DecryptionFailed(format!( + "unsupported secure-fields version {} (decrypting {guid})", + stored.v + ))), + // A bare number is not valid JSON for the blob, so a parse failure + // is how a row written before the migration identifies itself. + Err(_) => Ok(Self { + cc_number: cleartext, + }), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::encryption::{random_key_encryptor, ManagedEncryptorDecryptor}; + use nss_as::ensure_initialized; + + fn encdec() -> ManagedEncryptorDecryptor { + ensure_initialized(); + random_key_encryptor().unwrap() + } + + #[test] + fn test_roundtrip() { + let encdec = encdec(); + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec, "test-guid") + .unwrap(); + + assert!(!stored.is_empty()); + assert_ne!( + stored, "4111111111117629", + "the stored value must not be the cleartext" + ); + assert_eq!( + SecureCreditCardFields::decrypt(&stored, &encdec, "test-guid") + .unwrap() + .cc_number, + "4111111111117629" + ); + } + + #[test] + fn test_decrypt_with_the_wrong_key_fails() { + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec(), "test-guid") + .unwrap(); + assert!(SecureCreditCardFields::decrypt(&stored, &encdec(), "test-guid").is_err()); + } + + #[test] + fn test_scrubbed_is_the_default() { + // Empty ciphertext marks data to be replaced from Sync. + assert!(InternalCreditCard::default().has_scrubbed_data()); + } + + #[test] + fn test_decrypt_accepts_a_pre_migration_row() { + let encdec = encdec(); + let legacy = crate::encryption::encrypt_str(&encdec, "4111111111117629").unwrap(); + assert_eq!( + SecureCreditCardFields::decrypt(&legacy, &encdec, "test-guid") + .unwrap() + .cc_number, + "4111111111117629" + ); + } + + #[test] + fn test_encrypt_writes_a_versioned_blob() { + let encdec = encdec(); + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec, "test-guid") + .unwrap(); + assert_eq!( + crate::encryption::decrypt_str(&encdec, &stored).unwrap(), + r#"{"v":1,"n":"4111111111117629"}"# + ); + } + + #[test] + fn test_decrypt_refuses_an_unknown_version() { + let encdec = encdec(); + let future = + crate::encryption::encrypt_str(&encdec, r#"{"v":2,"n":"4111111111117629"}"#).unwrap(); + assert!(SecureCreditCardFields::decrypt(&future, &encdec, "test-guid").is_err()); + } + + #[test] + fn test_encrypting_twice_gives_different_ciphertext() { + let encdec = encdec(); + let fields = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + }; + assert_ne!( + fields.encrypt(&encdec, "test-guid").unwrap(), + fields.encrypt(&encdec, "test-guid").unwrap() + ); + } +} diff --git a/components/autofill/src/db/schema.rs b/components/autofill/src/db/schema.rs index cf2e0f2d9ce..d286d4d680d 100644 --- a/components/autofill/src/db/schema.rs +++ b/components/autofill/src/db/schema.rs @@ -108,7 +108,7 @@ pub struct AutofillConnectionInitializer; impl ConnectionInitializer for AutofillConnectionInitializer { const NAME: &'static str = "autofill db"; - const END_VERSION: u32 = 5; + const END_VERSION: u32 = 6; fn prepare(&self, conn: &Connection, _db_empty: bool) -> Result<()> { define_functions(conn)?; @@ -139,6 +139,7 @@ impl ConnectionInitializer for AutofillConnectionInitializer { 2 => upgrade_from_v2(db), 3 => upgrade_from_v3(db), 4 => upgrade_from_v4(db), + 5 => upgrade_from_v5(db), _ => Err(Error::IncompatibleVersion(version)), } } @@ -279,6 +280,14 @@ fn upgrade_from_v4(db: &Connection) -> Result<()> { Ok(()) } +fn upgrade_from_v5(_db: &Connection) -> Result<()> { + // v5 -> v6 changes no tables. The bump exists so that a downgrade to a + // build without the versioned secure-fields blob fails in `open_database` + // with `IncompatibleVersion`, instead of reading a blob as a card number + // and uploading it to the server. + Ok(()) +} + pub fn create_empty_sync_temp_tables(db: &Connection) -> Result<()> { debug!("Initializing sync temp tables"); db.execute_batch(CREATE_SYNC_TEMP_TABLES_SQL)?; @@ -306,7 +315,8 @@ mod tests { fn test_wal_size_is_bounded() { // A memory database has no -wal file, so open a real one. let db_file = MigratedDatabaseFile::new(AutofillConnectionInitializer, ""); - let db = AutofillDb::new(&db_file.path).expect("should open the database"); + let db = AutofillDb::new(&db_file.path, crate::db::test::test_encdec()) + .expect("should open the database"); let journal_mode: String = db .query_row("PRAGMA journal_mode", [], |row| row.get(0)) diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index 4bb7d1cd257..07ce5598808 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -9,8 +9,10 @@ use crate::db::models::address::{ use crate::db::models::credit_card::{CreditCard, UpdatableCreditCardFields}; use crate::db::models::passport::{Passport, UpdatablePassportFields}; use crate::db::{ - addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, passports, AutofillDb, + addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, + migrate_cc_secure_fields::migrate_cc_secure_fields_if_needed, passports, AutofillDb, }; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use error_support::handle_error; use parking_lot::{MappedMutexGuard, Mutex, MutexGuard}; @@ -58,9 +60,9 @@ pub struct Store { impl Store { #[handle_error(Error)] - pub fn new(db_path: impl AsRef) -> ApiResult { + pub fn new(db_path: impl AsRef, encdec: Arc) -> ApiResult { Ok(Self { - db: Mutex::new(Some(AutofillDb::new(db_path)?)), + db: Mutex::new(Some(AutofillDb::new(db_path, encdec)?)), }) } @@ -74,9 +76,12 @@ impl Store { /// Creates a store backed by an in-memory database that shares its memory API (required for autofill sync tests). #[handle_error(Error)] - pub fn new_shared_memory(db_name: &str) -> ApiResult { + pub fn new_shared_memory( + db_name: &str, + encdec: Arc, + ) -> ApiResult { Ok(Self { - db: Mutex::new(Some(AutofillDb::new_memory(db_name)?)), + db: Mutex::new(Some(AutofillDb::new_memory(db_name, encdec)?)), }) } @@ -312,14 +317,10 @@ impl Store { #[handle_error(Error)] pub fn scrub_undecryptable_credit_card_data_for_remote_replacement( self: Arc, - local_encryption_key: String, ) -> ApiResult { let db = self.lock_db()?; let deletion_stats = - credit_cards::scrub_undecryptable_credit_card_data_for_remote_replacement( - &db.writer, - local_encryption_key, - )?; + credit_cards::scrub_undecryptable_credit_card_data_for_remote_replacement(&db)?; // Here we reset the local sync data so that the credit card engine syncs as if // it were the first sync. This will potentially allow a previous sync of the @@ -333,6 +334,9 @@ impl Store { #[handle_error(Error)] pub fn run_maintenance(&self) -> ApiResult<()> { let conn = self.lock_db()?; + // Before the vacuum below, so the old ciphertext does not stay behind + // in freed pages. + migrate_cc_secure_fields_if_needed(&conn)?; run_maintenance(&conn)?; Ok(()) } @@ -393,8 +397,8 @@ pub(crate) fn delete_meta(conn: &Connection, key: &str) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::db::test::new_mem_db; - use crate::encryption::EncryptorDecryptor; + use crate::db::test::{new_mem_db, test_encdec}; + use crate::encryption::encrypt_str; use nss_as::ensure_initialized; #[test] @@ -434,7 +438,7 @@ mod tests { #[test] fn test_sync_manager_registration() { - let store = Arc::new(Store::new_shared_memory("sync-mgr-test").unwrap()); + let store = Arc::new(Store::new_shared_memory("sync-mgr-test", test_encdec()).unwrap()); assert_eq!(Arc::strong_count(&store), 1); assert_eq!(Arc::weak_count(&store), 0); Arc::clone(&store).register_with_sync_manager(); @@ -457,7 +461,7 @@ mod tests { #[test] fn test_shutdown_closes_the_store() { - let store = Store::new_shared_memory("shutdown-test").expect("create store"); + let store = Store::new_shared_memory("shutdown-test", test_encdec()).expect("create store"); // Operations succeed before shutdown. assert_eq!(store.count_all_passports().expect("count"), 0); @@ -474,15 +478,15 @@ mod tests { #[test] fn test_scrub_undecryptable_credit_card_data_for_remote_replacement() { ensure_initialized(); - let store = Arc::new(Store::new_shared_memory("sync-mgr-test").expect("create store")); - let key = EncryptorDecryptor::create_key().expect("create key"); - let encdec = EncryptorDecryptor::new(&key).expect("create EncryptorDecryptor"); + let store = + Arc::new(Store::new_shared_memory("scrub-test", test_encdec()).expect("create store")); + // The guard has to go out of scope before we touch the store again. + let encdec = store.lock_db().expect("db").encdec.clone(); store .add_credit_card(UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec - .encrypt("567812345678123456781") + cc_number_enc: encrypt_str(encdec.as_ref(), "567812345678123456781") .expect("encrypt cc number"), cc_number_last_4: "6781".to_string(), cc_exp_month: 10, @@ -492,7 +496,7 @@ mod tests { .expect("add credit card to database"); store - .scrub_undecryptable_credit_card_data_for_remote_replacement(key) + .scrub_undecryptable_credit_card_data_for_remote_replacement() .expect("scrub credit card record"); } } diff --git a/components/autofill/src/encryption.rs b/components/autofill/src/encryption.rs index b9fa9b1cfac..2fba15662f0 100644 --- a/components/autofill/src/encryption.rs +++ b/components/autofill/src/encryption.rs @@ -10,9 +10,10 @@ // * We use regular sqlite, but want to ensure the credit-card numbers are // encrypted in the DB - so we store the number encrypted, and the key // is managed by the app. -// * The credit-card API always just accepts and returns the encrypted string, -// so we also expose encryption and decryption public functions that take -// the key and text. The core storage API never knows the unencrypted number. +// * The app hands us an `EncryptorDecryptor` when it builds the store, and the +// db holds on to it. Everything that reads or writes an encrypted column +// takes it from the db. The core storage API never knows the unencrypted +// number. // // This makes life tricky for Sync - sync has its own encryption and its own // management of sync keys. The entire records are encrypted on the server - @@ -23,14 +24,40 @@ // * When transforming a record from Sync into a DB record, we need to *encrypt* // the field. // -// So Sync needs to know the key etc, and that needs to get passed down -// multiple layers, from the app saying "sync now" all the way down to the -// low level sync code. -// To make life a little easier, we do that via a struct. +// The sync code takes the encryptor from the store it already holds. +use crate::db::models::credit_card::SecureCreditCardFields; use crate::error::*; use error_support::handle_error; -pub use jwcrypto::EncryptorDecryptor; +use std::sync::Arc; + +pub use db_crypto::{EncryptorDecryptor, KeyManager, ManagedEncryptorDecryptor, StaticKeyManager}; + +// TODO(FXCM-2282): only `encrypt_string` and `decrypt_string` still build an +// encryptor from a key. When those go, so does this. +pub(crate) fn static_key_encryptor(key: &str) -> Result { + // Validate eagerly so an invalid key isn't treated as undecryptable card data. + jwcrypto::EncryptorDecryptor::new(key)?; + + Ok(ManagedEncryptorDecryptor::new(Arc::new( + StaticKeyManager::new(key.to_string()), + ))) +} + +#[cfg(test)] +pub(crate) fn random_key_encryptor() -> Result { + static_key_encryptor(&db_crypto::create_key()?) +} + +pub(crate) fn encrypt_str(encdec: &dyn EncryptorDecryptor, cleartext: &str) -> Result { + let ciphertext = encdec.encrypt(cleartext.as_bytes().to_vec())?; + String::from_utf8(ciphertext).map_err(|e| Error::CryptoNotUtf8(format!("encrypting: {e}"))) +} + +pub(crate) fn decrypt_str(encdec: &dyn EncryptorDecryptor, ciphertext: &str) -> Result { + let cleartext = encdec.decrypt(ciphertext.as_bytes().to_vec())?; + String::from_utf8(cleartext).map_err(|e| Error::CryptoNotUtf8(format!("decrypting: {e}"))) +} // public functions we expose over the FFI (which is why they take `String` // rather than the `&str` you'd otherwise expect) @@ -38,19 +65,25 @@ pub use jwcrypto::EncryptorDecryptor; pub fn encrypt_string(key: String, cleartext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - Ok(EncryptorDecryptor::new(&key)?.encrypt(&cleartext)?) + SecureCreditCardFields { + cc_number: cleartext, + } + .encrypt(&static_key_encryptor(&key)?, "") } #[handle_error(Error)] pub fn decrypt_string(key: String, ciphertext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - Ok(EncryptorDecryptor::new(&key)?.decrypt(&ciphertext)?) + Ok( + SecureCreditCardFields::decrypt(&ciphertext, &static_key_encryptor(&key)?, "")? + .cc_number, + ) } #[handle_error(Error)] pub fn create_autofill_key() -> ApiResult { - Ok(EncryptorDecryptor::create_key()?) + Ok(db_crypto::create_key()?) } #[cfg(test)] @@ -61,28 +94,41 @@ mod test { #[test] fn test_encrypt() { ensure_initialized(); - let ed = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ed = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); let cleartext = "secret"; - let ciphertext = ed.encrypt(cleartext).unwrap(); - assert_eq!(ed.decrypt(&ciphertext).unwrap(), cleartext); - let ed2 = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ciphertext = encrypt_str(&ed, cleartext).unwrap(); + assert_eq!(decrypt_str(&ed, &ciphertext).unwrap(), cleartext); + let ed2 = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); assert!(matches!( - ed2.decrypt(&ciphertext).map_err(Error::from), - Err(Error::CryptoError(_)) + decrypt_str(&ed2, &ciphertext), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )) )); } #[test] fn test_decryption_errors() { + // The shared crate maps all jwcrypto decryption failures to DecryptionFailed. ensure_initialized(); - let ed = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ed = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); assert!(matches!( - ed.decrypt("invalid-ciphertext").map_err(Error::from), - Err(Error::CryptoError(_)), + decrypt_str(&ed, "invalid-ciphertext"), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )), )); assert!(matches!( - ed.decrypt("").unwrap_err(), - jwcrypto::JwCryptoError::EmptyCyphertext, + decrypt_str(&ed, ""), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )), )); } + + #[test] + fn test_an_invalid_key_is_rejected_up_front() { + ensure_initialized(); + assert!(static_key_encryptor("not-a-key").is_err()); + } } diff --git a/components/autofill/src/error.rs b/components/autofill/src/error.rs index 5eab83daba6..a9804a331d5 100644 --- a/components/autofill/src/error.rs +++ b/components/autofill/src/error.rs @@ -71,8 +71,21 @@ pub enum Error { #[error("Crypto Error: {0}")] CryptoError(#[from] jwcrypto::JwCryptoError), - #[error("Missing local encryption key")] - MissingEncryptionKey, + #[error("Encryption Error: {0}")] + EncryptionError(#[from] db_crypto::DbCryptoApiError), + + #[error("Crypto data is not valid UTF-8: {0}")] + CryptoNotUtf8(String), + + // Encrypting or decrypting a record's secure fields failed. The string + // carries the underlying error plus the record guid, never the data itself + // - see the PII warning below. Mirrors logins, which reports the login id + // the same way (components/logins/src/error.rs:81). + #[error("Encryption failed: {0}")] + EncryptionFailed(String), + + #[error("Decryption failed: {0}")] + DecryptionFailed(String), #[error("No record with guid exists: {0}")] NoSuchRecord(String), @@ -131,10 +144,36 @@ impl GetErrorHandling for Error { }) .report_error("autofill-crypto-error"), - Self::MissingEncryptionKey => ErrorHandling::convert(AutofillApiError::CryptoError { - reason: "Missing encryption key".to_string(), + Self::EncryptionError(e) => ErrorHandling::convert(AutofillApiError::CryptoError { + reason: e.to_string(), }) - .report_error("autofill-missing-encryption-key"), + .report_error("autofill-encryption-error"), + + Self::CryptoNotUtf8(reason) => ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), + }) + .report_error("autofill-crypto-not-utf8"), + + // Logged locally, deliberately NOT reported: the message carries the + // record guid so an operator can tell which record failed, and a + // guid is stable and also exists on the sync server, so shipping it + // off-device would make a report linkable to one user's record. + // This matches how autofill already treats `NoSuchRecord`. logins + // does report the equivalent, via its catch-all arm - we are being + // stricter on purpose, because these are card numbers. + Self::EncryptionFailed(reason) => { + ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), + }) + .log_warning() + } + + Self::DecryptionFailed(reason) => { + ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), + }) + .log_warning() + } Self::NoSuchRecord(guid) => { ErrorHandling::convert(AutofillApiError::NoSuchRecord { guid: guid.clone() }) diff --git a/components/autofill/src/lib.rs b/components/autofill/src/lib.rs index 5eae09c76e6..c4fb3dff5cd 100644 --- a/components/autofill/src/lib.rs +++ b/components/autofill/src/lib.rs @@ -22,6 +22,7 @@ use crate::db::models::passport::*; use crate::db::store::Store; use crate::encryption::{create_autofill_key, decrypt_string, encrypt_string}; pub use crate::sync::AddressesBridgedEngine; +use db_crypto::EncryptorDecryptor; pub use error::{ApiResult, AutofillApiError, Error, Result}; uniffi::include_scaffolding!("autofill"); diff --git a/components/autofill/src/sync/address/mod.rs b/components/autofill/src/sync/address/mod.rs index 61ef0e6d701..566a4f062e2 100644 --- a/components/autofill/src/sync/address/mod.rs +++ b/components/autofill/src/sync/address/mod.rs @@ -13,6 +13,7 @@ use super::{ UnknownFields, }; use crate::db::models::address::InternalAddress; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::sync_merge_field_check; use incoming::IncomingAddressesImpl; @@ -41,9 +42,8 @@ pub(super) struct AddressesEngineStorageImpl {} impl SyncEngineStorageImpl for AddressesEngineStorageImpl { fn get_incoming_impl( &self, - enc_key: &Option, + _encdec: &Arc, ) -> Result>> { - assert!(enc_key.is_none()); Ok(Box::new(IncomingAddressesImpl {})) } @@ -57,9 +57,8 @@ impl SyncEngineStorageImpl for AddressesEngineStorageImpl { fn get_outgoing_impl( &self, - enc_key: &Option, + _encdec: &Arc, ) -> Result>> { - assert!(enc_key.is_none()); Ok(Box::new(OutgoingAddressesImpl {})) } } diff --git a/components/autofill/src/sync/bridge.rs b/components/autofill/src/sync/bridge.rs index 58715aa696a..631ffb39000 100644 --- a/components/autofill/src/sync/bridge.rs +++ b/components/autofill/src/sync/bridge.rs @@ -30,7 +30,9 @@ mod tests { fn test_sync_meta() { error_support::init_for_tests(); - let store = Arc::new(Store::new_shared_memory("addresses-bridge").unwrap()); + let store = Arc::new( + Store::new_shared_memory("addresses-bridge", crate::db::test::test_encdec()).unwrap(), + ); let bridge = store.addresses_bridged_engine(); bridge.sync_started().unwrap(); @@ -67,7 +69,10 @@ mod tests { fn test_sync_via_bridge() { error_support::init_for_tests(); - let store = Arc::new(Store::new_shared_memory("addresses-bridge-roundtrip").unwrap()); + let store = Arc::new( + Store::new_shared_memory("addresses-bridge-roundtrip", crate::db::test::test_encdec()) + .unwrap(), + ); // A local-only address: nothing on the server knows about it yet, so it // should be uploaded. diff --git a/components/autofill/src/sync/credit_card/incoming.rs b/components/autofill/src/sync/credit_card/incoming.rs index f73feb3298c..d777d55d915 100644 --- a/components/autofill/src/sync/credit_card/incoming.rs +++ b/components/autofill/src/sync/credit_card/incoming.rs @@ -5,9 +5,9 @@ use super::CreditCardPayload; use crate::db::credit_cards::{add_internal_credit_card, update_internal_credit_card}; -use crate::db::models::credit_card::InternalCreditCard; +use crate::db::models::credit_card::{InternalCreditCard, SecureCreditCardFields}; use crate::db::schema::CREDIT_CARD_COMMON_COLS; -use crate::encryption::EncryptorDecryptor; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; use crate::sync::common::*; use crate::sync::{ @@ -17,6 +17,7 @@ use crate::sync::{ use interrupt_support::Interruptee; use rusqlite::{named_params, Transaction}; use sql_support::ConnExt; +use std::sync::Arc; use sync_guid::Guid as SyncGuid; // Takes a raw payload, as stored in our database, and returns an InternalCreditCard @@ -25,9 +26,9 @@ use sync_guid::Guid as SyncGuid; fn raw_payload_to_incoming( id: SyncGuid, raw: String, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result> { - let payload = encdec.decrypt(&raw)?; + let payload = decrypt_str(encdec, &raw)?; // Turn it into a BSO let bso = IncomingBso { envelope: IncomingEnvelope { @@ -58,7 +59,7 @@ fn raw_payload_to_incoming( } pub(super) struct IncomingCreditCardsImpl { - pub(super) encdec: EncryptorDecryptor, + pub(super) encdec: Arc, } impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { @@ -76,7 +77,7 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { .into_iter() .map(|bso| { // consider turning this into malformed? - let encrypted = self.encdec.encrypt(&bso.payload)?; + let encrypted = encrypt_str(self.encdec.as_ref(), &bso.payload)?; Ok((bso.envelope.id, encrypted, bso.envelope.modified)) }) .collect::>()?; @@ -121,7 +122,7 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { // the 'guid' and 's_payload' rows must be non-null. let guid: SyncGuid = row.get("guid")?; let incoming = - raw_payload_to_incoming(guid.clone(), row.get("s_payload")?, &self.encdec)?; + raw_payload_to_incoming(guid.clone(), row.get("s_payload")?, self.encdec.as_ref())?; Ok(IncomingState { incoming, local: match row.get_unwrap::<_, Option>("l_guid") { @@ -155,7 +156,8 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { match row.get::<_, Option>("m_payload")? { Some(m_payload) => { // a tombstone in the mirror can be treated as though it's missing. - raw_payload_to_incoming(guid, m_payload, &self.encdec)?.content() + raw_payload_to_incoming(guid, m_payload, self.encdec.as_ref())? + .content() } None => None, } @@ -208,9 +210,21 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { Ok(Self::Record::from_row(row)?) })?; - let incoming_cc_number = self.encdec.decrypt(&incoming.cc_number_enc)?; + let incoming_cc_number = SecureCreditCardFields::decrypt( + &incoming.cc_number_enc, + self.encdec.as_ref(), + incoming.guid.as_str(), + )? + .cc_number; for record in records { - if self.encdec.decrypt(&record.cc_number_enc)? == incoming_cc_number { + if SecureCreditCardFields::decrypt( + &record.cc_number_enc, + self.encdec.as_ref(), + record.guid.as_str(), + )? + .cc_number + == incoming_cc_number + { return Ok(Some(record)); } } @@ -264,6 +278,7 @@ mod tests { use super::super::super::test::new_syncable_mem_db; use super::*; use crate::db::credit_cards::get_credit_card; + use crate::encryption::random_key_encryptor; use crate::sync::common::tests::*; use error_support::{info, trace}; @@ -332,7 +347,7 @@ mod tests { .clone() } - fn test_record(guid_prefix: char, encdec: &EncryptorDecryptor) -> InternalCreditCard { + fn test_record(guid_prefix: char, encdec: &dyn EncryptorDecryptor) -> InternalCreditCard { let json = test_json_record(guid_prefix); let payload = serde_json::from_value(json).unwrap(); InternalCreditCard::from_payload(payload, encdec).expect("should be valid") @@ -385,7 +400,7 @@ mod tests { for tc in test_cases { info!("starting new testcase"); let tx = db.transaction().unwrap(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec: Arc = Arc::new(random_key_encryptor().unwrap()); // Add required items to the mirrors. let mirror_sql = "INSERT OR REPLACE INTO credit_cards_mirror (guid, payload) @@ -395,7 +410,7 @@ mod tests { mirror_sql, rusqlite::named_params! { ":guid": payload["id"].as_str().unwrap(), - ":payload": encdec.encrypt(&payload.to_string())?, + ":payload": encrypt_str(encdec.as_ref(), &payload.to_string())?, }, ) .expect("should insert mirror record"); @@ -414,7 +429,7 @@ mod tests { |row| -> Result> { let guid: SyncGuid = row.get_unwrap("guid"); let enc_payload: String = row.get_unwrap("payload"); - raw_payload_to_incoming(guid, enc_payload, &ri.encdec) + raw_payload_to_incoming(guid, enc_payload, ri.encdec.as_ref()) }, )?; @@ -441,10 +456,10 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction()?; let ri = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - ri.insert_local_record(&tx, test_record('C', &ri.encdec))?; + ri.insert_local_record(&tx, test_record('C', ri.encdec.as_ref()))?; ri.change_record_guid( &tx, @@ -463,12 +478,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let record = test_record('C', &ci.encdec); + let record = test_record('C', ci.encdec.as_ref()); let bso = record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); do_test_incoming_same(&ci, &tx, record, bso); } @@ -478,9 +493,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - do_test_incoming_tombstone(&ci, &tx, test_record('C', &ci.encdec)); + do_test_incoming_tombstone(&ci, &tx, test_record('C', ci.encdec.as_ref())); } #[test] @@ -489,12 +504,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let mut scrubbed_record = test_record('A', &ci.encdec); + let mut scrubbed_record = test_record('A', ci.encdec.as_ref()); let bso = scrubbed_record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); scrubbed_record.cc_number_enc = "".to_string(); do_test_scrubbed_local_data(&ci, &tx, scrubbed_record, bso); } @@ -505,12 +520,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let record = test_record('C', &ci.encdec); + let record = test_record('C', ci.encdec.as_ref()); let bso = record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); do_test_staged_to_mirror(&ci, &tx, record, bso, "credit_cards_mirror"); } @@ -519,15 +534,16 @@ mod tests { ensure_initialized(); let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); - let ci = IncomingCreditCardsImpl { encdec }; - let local_record = test_record('C', &ci.encdec); + let ci = IncomingCreditCardsImpl { + encdec: Arc::new(random_key_encryptor().unwrap()), + }; + let local_record = test_record('C', ci.encdec.as_ref()); let local_guid = local_record.guid.clone(); ci.insert_local_record(&tx, local_record.clone()).unwrap(); // Now the same record incoming - it should find the one we just added // above as a dupe. - let mut incoming_record = test_record('C', &ci.encdec); + let mut incoming_record = test_record('C', ci.encdec.as_ref()); // sanity check that the encrypted numbers are different even though // the decrypted numbers are identical. assert_ne!(local_record.cc_number_enc, incoming_record.cc_number_enc); @@ -555,9 +571,10 @@ mod tests { ensure_initialized(); let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); - let ci = IncomingCreditCardsImpl { encdec }; - let local_record = test_record('C', &ci.encdec); + let ci = IncomingCreditCardsImpl { + encdec: Arc::new(random_key_encryptor().unwrap()), + }; + let local_record = test_record('C', ci.encdec.as_ref()); let local_guid = local_record.guid.clone(); ci.insert_local_record(&tx, local_record.clone()).unwrap(); diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 213f8bf158c..a38149ce9d8 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -11,7 +11,9 @@ use super::{ MergeResult, Metadata, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord, UnknownFields, }; -use crate::db::models::credit_card::InternalCreditCard; +use crate::db::models::credit_card::{InternalCreditCard, SecureCreditCardFields}; +#[cfg(test)] +use crate::encryption::static_key_encryptor; use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::sync_merge_field_check; @@ -40,14 +42,11 @@ pub(super) struct CreditCardsEngineStorageImpl {} impl SyncEngineStorageImpl for CreditCardsEngineStorageImpl { fn get_incoming_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>> { - let enc_key = match enc_key { - None => return Err(Error::MissingEncryptionKey), - Some(enc_key) => enc_key, - }; - let encdec = EncryptorDecryptor::new(enc_key)?; - Ok(Box::new(IncomingCreditCardsImpl { encdec })) + Ok(Box::new(IncomingCreditCardsImpl { + encdec: Arc::clone(encdec), + })) } fn reset_storage(&self, tx: &Transaction<'_>) -> Result<()> { @@ -60,14 +59,11 @@ impl SyncEngineStorageImpl for CreditCardsEngineStorageImpl fn get_outgoing_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>> { - let enc_key = match enc_key { - None => return Err(Error::MissingEncryptionKey), - Some(enc_key) => enc_key, - }; - let encdec = EncryptorDecryptor::new(enc_key)?; - Ok(Box::new(OutgoingCreditCardsImpl { encdec })) + Ok(Box::new(OutgoingCreditCardsImpl { + encdec: Arc::clone(encdec), + })) } } @@ -113,7 +109,7 @@ pub(super) struct PayloadEntry { } impl InternalCreditCard { - fn from_payload(p: CreditCardPayload, encdec: &EncryptorDecryptor) -> Result { + fn from_payload(p: CreditCardPayload, encdec: &dyn EncryptorDecryptor) -> Result { if p.entry.version != 3 { // when new versions are introduced we will start accepting and // converting old ones - but 3 is the lowest we support. @@ -123,8 +119,11 @@ impl InternalCreditCard { ))); } // need to encrypt the cleartext in the sync record. - let cc_number_enc = encdec.encrypt(&p.entry.cc_number)?; let cc_number_last_4 = get_last_4(&p.entry.cc_number); + let cc_number_enc = SecureCreditCardFields { + cc_number: p.entry.cc_number, + } + .encrypt(encdec, p.id.as_str())?; Ok(InternalCreditCard { guid: p.id, @@ -144,8 +143,10 @@ impl InternalCreditCard { }) } - pub(crate) fn into_payload(self, encdec: &EncryptorDecryptor) -> Result { - let cc_number = encdec.decrypt(&self.cc_number_enc)?; + pub(crate) fn into_payload(self, encdec: &dyn EncryptorDecryptor) -> Result { + let cc_number = + SecureCreditCardFields::decrypt(&self.cc_number_enc, encdec, self.guid.as_str())? + .cc_number; Ok(CreditCardPayload { id: self.guid, entry: PayloadEntry { @@ -269,7 +270,7 @@ fn test_to_from_payload() { cc_type: "foo".to_string(), ..Default::default() }; - let encdec = EncryptorDecryptor::new(&key).unwrap(); + let encdec = static_key_encryptor(&key).unwrap(); let payload: CreditCardPayload = cc.clone().into_payload(&encdec).unwrap(); assert_eq!(payload.id, cc.guid); diff --git a/components/autofill/src/sync/credit_card/outgoing.rs b/components/autofill/src/sync/credit_card/outgoing.rs index cb315a729ba..9bda3a4f63c 100644 --- a/components/autofill/src/sync/credit_card/outgoing.rs +++ b/components/autofill/src/sync/credit_card/outgoing.rs @@ -5,11 +5,12 @@ use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::CREDIT_CARD_COMMON_COLS; -use crate::encryption::EncryptorDecryptor; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; use crate::sync::common::*; use crate::sync::{credit_card::CreditCardPayload, OutgoingBso, ProcessOutgoingRecordImpl}; use rusqlite::{Row, Transaction}; +use std::sync::Arc; use sync_guid::Guid as SyncGuid; const DATA_TABLE_NAME: &str = "credit_cards_data"; @@ -17,7 +18,7 @@ const MIRROR_TABLE_NAME: &str = "credit_cards_mirror"; const STAGING_TABLE_NAME: &str = "credit_cards_sync_outgoing_staging"; pub(super) struct OutgoingCreditCardsImpl { - pub(super) encdec: EncryptorDecryptor, + pub(super) encdec: Arc, } impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { @@ -47,12 +48,13 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { common_cols = CREDIT_CARD_COMMON_COLS, ); let record_from_data_row: &dyn Fn(&Row<'_>) -> Result<(OutgoingBso, i64)> = &|row| { - let mut record = InternalCreditCard::from_row(row)?.into_payload(&self.encdec)?; + let mut record = + InternalCreditCard::from_row(row)?.into_payload(self.encdec.as_ref())?; // If the server had unknown fields we fetch it and add it to the record if let Some(enc_s) = row.get::<_, Option>("payload")? { // The full payload in the credit cards mirror is encrypted let mirror_payload: CreditCardPayload = - serde_json::from_str(&self.encdec.decrypt(&enc_s)?)?; + serde_json::from_str(&decrypt_str(self.encdec.as_ref(), &enc_s)?)?; record.entry.unknown_fields = mirror_payload.entry.unknown_fields; }; @@ -74,7 +76,7 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { .into_iter() .map(|(bso, change_counter)| { // Turn the record into an encrypted repr to save in the mirror. - let encrypted = self.encdec.encrypt(&bso.payload)?; + let encrypted = encrypt_str(self.encdec.as_ref(), &bso.payload)?; Ok((bso.envelope.id, encrypted, change_counter)) }) .collect::>()?; @@ -110,6 +112,7 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { mod tests { use super::*; use crate::db::credit_cards::{add_internal_credit_card, tests::test_insert_mirror_record}; + use crate::encryption::random_key_encryptor; use crate::sync::{common::tests::*, test::new_syncable_mem_db, UnknownFields}; use serde_json::{json, Map, Value}; use types::Timestamp; @@ -164,7 +167,7 @@ mod tests { .clone() } - fn test_record(guid_prefix: char, encdec: &EncryptorDecryptor) -> InternalCreditCard { + fn test_record(guid_prefix: char, encdec: &dyn EncryptorDecryptor) -> InternalCreditCard { let json = test_json_record(guid_prefix); let payload = serde_json::from_value(json).unwrap(); InternalCreditCard::from_payload(payload, encdec).expect("should be valid") @@ -175,9 +178,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); // create date record assert!(add_internal_credit_card(&tx, &test_record).is_ok()); @@ -196,9 +199,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); // create tombstone record assert!(tx @@ -231,19 +234,19 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with non-zero sync_change_counter - let mut test_record = test_record('C', &co.encdec); + let mut test_record = test_record('C', co.encdec.as_ref()); let initial_change_counter_val = 2; test_record.metadata.sync_change_counter = initial_change_counter_val; assert!(add_internal_credit_card(&tx, &test_record).is_ok()); let guid = test_record.guid.clone(); //test_insert_mirror_record doesn't encrypt the mirror payload, but in reality we do // so we encrypt here so our fetch_outgoing_records doesn't break - let mut bso = test_record.into_test_incoming_bso(&co.encdec, Default::default()); - bso.payload = co.encdec.encrypt(&bso.payload).unwrap(); + let mut bso = test_record.into_test_incoming_bso(co.encdec.as_ref(), Default::default()); + bso.payload = encrypt_str(co.encdec.as_ref(), &bso.payload).unwrap(); test_insert_mirror_record(&tx, bso); exists_with_counter_value_in_table(&tx, DATA_TABLE_NAME, &guid, initial_change_counter_val); @@ -262,16 +265,16 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with no changes (sync_change_counter = 0) - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); let guid = test_record.guid.clone(); assert!(add_internal_credit_card(&tx, &test_record).is_ok()); test_insert_mirror_record( &tx, - test_record.into_test_incoming_bso(&co.encdec, Default::default()), + test_record.into_test_incoming_bso(co.encdec.as_ref(), Default::default()), ); do_test_outgoing_synced_with_no_change( @@ -288,11 +291,11 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with non-zero sync_change_counter - let mut test_record = test_record('D', &co.encdec); + let mut test_record = test_record('D', co.encdec.as_ref()); let initial_change_counter_val = 2; test_record.metadata.sync_change_counter = initial_change_counter_val; assert!(add_internal_credit_card(&tx, &test_record).is_ok()); @@ -304,8 +307,8 @@ mod tests { // so we encrypt here so our fetch_outgoing_records doesn't break let mut bso = test_record .clone() - .into_test_incoming_bso(&co.encdec, unknown_fields); - bso.payload = co.encdec.encrypt(&bso.payload).unwrap(); + .into_test_incoming_bso(co.encdec.as_ref(), unknown_fields); + bso.payload = encrypt_str(co.encdec.as_ref(), &bso.payload).unwrap(); test_insert_mirror_record(&tx, bso); exists_with_counter_value_in_table( &tx, diff --git a/components/autofill/src/sync/engine.rs b/components/autofill/src/sync/engine.rs index 204d0e7eedd..47cfa645165 100644 --- a/components/autofill/src/sync/engine.rs +++ b/components/autofill/src/sync/engine.rs @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use super::{plan_incoming, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord}; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::Store; use error_support::warn; @@ -33,12 +34,12 @@ pub const COLLECTION_SYNCID_META_KEY: &str = "sync_id"; pub trait SyncEngineStorageImpl: Send + Sync { fn get_incoming_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>>; fn reset_storage(&self, conn: &Transaction<'_>) -> Result<()>; fn get_outgoing_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>>; } @@ -47,7 +48,6 @@ pub struct ConfigSyncEngine { pub(crate) config: EngineConfig, pub(crate) store: Arc, pub(crate) storage_impl: Box>, - local_enc_key: Option, } impl ConfigSyncEngine { @@ -60,7 +60,6 @@ impl ConfigSyncEngine { config, store, storage_impl, - local_enc_key: None, } } fn put_meta(&self, conn: &Connection, tail: &str, value: &dyn ToSql) -> Result<()> { @@ -102,11 +101,6 @@ impl SyncEngine for ConfigSyncEngine { self.config.collection.clone() } - fn set_local_encryption_key(&mut self, key: &str) -> anyhow::Result<()> { - self.local_enc_key = Some(key.to_string()); - Ok(()) - } - fn sync_started(&self) -> anyhow::Result<()> { let db = self.store.lock_db()?; let signal = db.begin_interrupt_scope()?; @@ -128,7 +122,7 @@ impl SyncEngine for ConfigSyncEngine { incoming_telemetry.applied(inbound.len() as u32); telem.incoming(incoming_telemetry); let tx = db.writer.unchecked_transaction()?; - let incoming_impl = self.storage_impl.get_incoming_impl(&self.local_enc_key)?; + let incoming_impl = self.storage_impl.get_incoming_impl(&db.encdec)?; incoming_impl.stage_incoming(&tx, inbound, &signal)?; tx.commit()?; @@ -143,8 +137,8 @@ impl SyncEngine for ConfigSyncEngine { let db = self.store.lock_db()?; let signal = db.begin_interrupt_scope()?; let tx = db.writer.unchecked_transaction()?; - let incoming_impl = self.storage_impl.get_incoming_impl(&self.local_enc_key)?; - let outgoing_impl = self.storage_impl.get_outgoing_impl(&self.local_enc_key)?; + let incoming_impl = self.storage_impl.get_incoming_impl(&db.encdec)?; + let outgoing_impl = self.storage_impl.get_outgoing_impl(&db.encdec)?; // Get "states" for each record... for state in incoming_impl.fetch_incoming_states(&tx)? { @@ -178,7 +172,7 @@ impl SyncEngine for ConfigSyncEngine { let db = self.store.lock_db()?; self.put_meta(&db.writer, LAST_SYNC_META_KEY, &new_timestamp.as_millis())?; let tx = db.writer.unchecked_transaction()?; - let outgoing_impl = self.storage_impl.get_outgoing_impl(&self.local_enc_key)?; + let outgoing_impl = self.storage_impl.get_outgoing_impl(&db.encdec)?; outgoing_impl.finish_synced_items(&tx, ids)?; tx.commit()?; Ok(()) @@ -268,7 +262,7 @@ mod tests { }; use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::create_empty_sync_temp_tables; - use crate::encryption::EncryptorDecryptor; + use crate::encryption::{encrypt_str, random_key_encryptor, EncryptorDecryptor}; use crate::sync::{IncomingBso, UnknownFields}; use nss_as::ensure_initialized; use sql_support::ConnExt; @@ -276,7 +270,7 @@ mod tests { impl InternalCreditCard { pub fn into_test_incoming_bso( self, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, unknown_fields: UnknownFields, ) -> IncomingBso { let mut payload = self.into_payload(encdec).expect("is json"); @@ -303,11 +297,7 @@ mod tests { #[test] fn test_credit_card_engine_apply_timestamp() -> Result<()> { ensure_initialized(); - let mut credit_card_engine = create_engine(); - let test_key = crate::encryption::create_autofill_key().unwrap(); - credit_card_engine - .set_local_encryption_key(&test_key) - .unwrap(); + let credit_card_engine = create_engine(); { let db = credit_card_engine.store.lock_db()?; create_empty_sync_temp_tables(&db.writer)?; @@ -367,12 +357,12 @@ mod tests { fn test_engine_sync_reset() -> Result<()> { ensure_initialized(); let engine = create_engine(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = random_key_encryptor().unwrap(); let cc = InternalCreditCard { guid: Guid::random(), cc_name: "Ms Jane Doe".to_string(), - cc_number_enc: encdec.encrypt("12341232412341234")?, + cc_number_enc: encrypt_str(&encdec, "12341232412341234")?, cc_number_last_4: "1234".to_string(), cc_exp_month: 12, cc_exp_year: 2021,