FXCM-2281: Integrate EncryptorDecryptor into the autofill database

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 [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

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"

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,7 +260,7 @@ 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
     ///

components/autofill/src/db/credit_cards.rs

@@ -9,10 +9,11 @@ use crate::db::{
         Metadata,
     },
     schema::{CREDIT_CARD_COMMON_COLS, CREDIT_CARD_COMMON_VALS},
+    AutofillDb,
 };
+use crate::encryption::decrypt_str;
 use crate::error::*;
 
-use jwcrypto::EncryptorDecryptor;
 use rusqlite::{Connection, Transaction};
 use sync_guid::Guid;
 use types::Timestamp;
@@ -226,16 +227,15 @@ 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<CreditCardsDeletionMetrics> {
+    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| decrypt_str(db.encdec.as_ref(), &credit_card.cc_number_enc).is_err())
         .map(|credit_card| credit_card.guid)
         .collect::<Vec<_>>();
 
@@ -290,7 +290,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 +588,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 +610,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 +620,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 +657,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,27 +683,24 @@ 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,
                 cc_type: "visa".to_string(),
             },
         )?;
 
-        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 +713,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)?;

components/autofill/src/db/mod.rs

@@ -9,6 +9,7 @@ pub mod passports;
 pub mod schema;
 pub mod store;
 
+use crate::encryption::EncryptorDecryptor;
 use crate::error::*;
 
 use error_support::error;
@@ -24,21 +25,22 @@ use url::Url;
 
 pub struct AutofillDb {
     pub writer: Connection,
+    pub encdec: Arc<dyn EncryptorDecryptor>,
     interrupt_handle: Arc<SqlInterruptHandle>,
 }
 
 impl AutofillDb {
-    pub fn new(db_path: impl AsRef<Path>) -> Result<Self> {
+    pub fn new(db_path: impl AsRef<Path>, encdec: Arc<dyn EncryptorDecryptor>) -> Result<Self> {
         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<Self> {
+    pub fn new_memory(db_path: &str, encdec: Arc<dyn EncryptorDecryptor>) -> Result<Self> {
         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<Self> {
+    fn new_named(db_path: PathBuf, encdec: Arc<dyn EncryptorDecryptor>) -> Result<Self> {
         // 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 +57,7 @@ impl AutofillDb {
         Ok(Self {
             interrupt_handle: Arc::new(SqlInterruptHandle::new(&conn)),
             writer: conn,
+            encdec,
         })
     }
 
@@ -152,10 +155,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<dyn EncryptorDecryptor> {
+        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<dyn EncryptorDecryptor>) -> 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")
     }
 }

components/autofill/src/db/schema.rs

@@ -306,7 +306,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))

components/autofill/src/db/store.rs

@@ -11,6 +11,7 @@ use crate::db::models::passport::{Passport, UpdatablePassportFields};
 use crate::db::{
     addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, passports, AutofillDb,
 };
+use crate::encryption::EncryptorDecryptor;
 use crate::error::*;
 use error_support::handle_error;
 use parking_lot::{MappedMutexGuard, Mutex, MutexGuard};
@@ -58,9 +59,9 @@ pub struct Store {
 
 impl Store {
     #[handle_error(Error)]
-    pub fn new(db_path: impl AsRef<Path>) -> ApiResult<Self> {
+    pub fn new(db_path: impl AsRef<Path>, encdec: Arc<dyn EncryptorDecryptor>) -> ApiResult<Self> {
         Ok(Self {
-            db: Mutex::new(Some(AutofillDb::new(db_path)?)),
+            db: Mutex::new(Some(AutofillDb::new(db_path, encdec)?)),
         })
     }
 
@@ -74,9 +75,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<Self> {
+    pub fn new_shared_memory(
+        db_name: &str,
+        encdec: Arc<dyn EncryptorDecryptor>,
+    ) -> ApiResult<Self> {
         Ok(Self {
-            db: Mutex::new(Some(AutofillDb::new_memory(db_name)?)),
+            db: Mutex::new(Some(AutofillDb::new_memory(db_name, encdec)?)),
         })
     }
 
@@ -312,14 +316,10 @@ impl Store {
     #[handle_error(Error)]
     pub fn scrub_undecryptable_credit_card_data_for_remote_replacement(
         self: Arc<Self>,
-        local_encryption_key: String,
     ) -> ApiResult<CreditCardsDeletionMetrics> {
         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
@@ -393,8 +393,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 +434,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 +457,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 +474,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 +492,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");
     }
 }