diff --git a/.gitignore b/.gitignore index d8085cba53..a09b2ac41b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ website/build target credentials.json +logins.jwk *-engine.json *.db .*.swp diff --git a/components/logins/README.md b/components/logins/README.md index ea138948da..8c2b05df03 100644 --- a/components/logins/README.md +++ b/components/logins/README.md @@ -89,7 +89,9 @@ To effectively work on the Logins component, you will need to be familiar with: ### Implementation Overview -Logins implements encrypted storage for login records on top of NSS. The storage schema is based on the one +Logins implements encrypted storage for login records on top of a consumer +implemented EncryptorDecryptor, or via ManagedEncryptorDecryptor, using NSS +based crypto algorithms (AES256-GCM). The storage schema is based on the one originally used in [Firefox for iOS](https://github.com/mozilla-mobile/firefox-ios/blob/faa6a2839abf4da2c54ff1b3291174b50b31ab2c/Storage/SQL/SQLiteLogins.swift), but with the following notable differences: diff --git a/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt b/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt index 8e7acaf1b7..0ad07f84fd 100644 --- a/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt +++ b/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt @@ -21,11 +21,12 @@ import org.mozilla.appservices.logins.GleanMetrics.LoginsStore as LoginsStoreMet * LoginStore. */ -class DatabaseLoginsStorage(dbPath: String) : AutoCloseable { +class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoCloseable { private var store: LoginStore init { - this.store = LoginStore(dbPath) + val encdec = createManagedEncdec(keyManager) + this.store = LoginStore(dbPath, encdec) } @Throws(LoginsApiException::class) @@ -46,7 +47,7 @@ class DatabaseLoginsStorage(dbPath: String) : AutoCloseable { } @Throws(LoginsApiException::class) - fun get(id: String): EncryptedLogin? { + fun get(id: String): Login? { return readQueryCounters.measure { store.get(id) } @@ -60,44 +61,44 @@ class DatabaseLoginsStorage(dbPath: String) : AutoCloseable { } @Throws(LoginsApiException::class) - fun list(): List { + fun list(): List { return readQueryCounters.measure { store.list() } } @Throws(LoginsApiException::class) - fun getByBaseDomain(baseDomain: String): List { + fun getByBaseDomain(baseDomain: String): List { return readQueryCounters.measure { store.getByBaseDomain(baseDomain) } } @Throws(LoginsApiException::class) - fun findLoginToUpdate(look: LoginEntry, encryptionKey: String): Login? { + fun findLoginToUpdate(look: LoginEntry): Login? { return readQueryCounters.measure { - store.findLoginToUpdate(look, encryptionKey) + store.findLoginToUpdate(look) } } @Throws(LoginsApiException::class) - fun add(entry: LoginEntry, encryptionKey: String): EncryptedLogin { + fun add(entry: LoginEntry): Login { return writeQueryCounters.measure { - store.add(entry, encryptionKey) + store.add(entry) } } @Throws(LoginsApiException::class) - fun update(id: String, entry: LoginEntry, encryptionKey: String): EncryptedLogin { + fun update(id: String, entry: LoginEntry): Login { return writeQueryCounters.measure { - store.update(id, entry, encryptionKey) + store.update(id, entry) } } @Throws(LoginsApiException::class) - fun addOrUpdate(entry: LoginEntry, encryptionKey: String): EncryptedLogin { + fun addOrUpdate(entry: LoginEntry): Login { return writeQueryCounters.measure { - store.addOrUpdate(entry, encryptionKey) + store.addOrUpdate(entry) } } diff --git a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt index 9088d912f9..be917a833c 100644 --- a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt +++ b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt @@ -32,14 +32,15 @@ class DatabaseLoginsStorageTest { @get:Rule val gleanRule = GleanTestRule(ApplicationProvider.getApplicationContext()) + protected val encryptionKey = createKey() + fun createTestStore(): DatabaseLoginsStorage { Megazord.init() val dbPath = dbFolder.newFile() - return DatabaseLoginsStorage(dbPath = dbPath.absolutePath) + val keyManager = createStaticKeyManager(key = encryptionKey) + return DatabaseLoginsStorage(dbPath = dbPath.absolutePath, keyManager = keyManager) } - protected val encryptionKey = createKey() - protected fun getTestStore(): DatabaseLoginsStorage { val store = createTestStore() @@ -57,7 +58,6 @@ class DatabaseLoginsStorageTest { password = "hunter2", ), ), - encryptionKey, ) store.add( @@ -74,7 +74,6 @@ class DatabaseLoginsStorageTest { username = "Foobar2000", ), ), - encryptionKey, ) return store @@ -106,7 +105,6 @@ class DatabaseLoginsStorageTest { password = "hunter2", ), ), - encryptionKey, ) assertEquals(LoginsStoreMetrics.writeQueryCount.testGetValue(), 1) @@ -128,7 +126,7 @@ class DatabaseLoginsStorageTest { ) try { - store.add(invalid, encryptionKey) + store.add(invalid) fail("Should have thrown") } catch (e: LoginsApiException.InvalidRecord) { // All good. diff --git a/components/logins/ios/Logins/LoginsStorage.swift b/components/logins/ios/Logins/LoginsStorage.swift index 13421e7ee8..35882a059a 100644 --- a/components/logins/ios/Logins/LoginsStorage.swift +++ b/components/logins/ios/Logins/LoginsStorage.swift @@ -17,8 +17,8 @@ open class LoginsStorage { private var store: LoginStore private let queue = DispatchQueue(label: "com.mozilla.logins-storage") - public init(databasePath: String) throws { - store = try LoginStore(path: databasePath) + public init(databasePath: String, keyManager: KeyManager) throws { + store = try LoginStore(path: databasePath, encdec: createManagedEncdec(keyManager: keyManager)) } open func wipeLocal() throws { @@ -47,36 +47,36 @@ open class LoginsStorage { /// then this throws `LoginStoreError.DuplicateGuid` if there is a collision /// /// Returns the `id` of the newly inserted record. - open func add(login: LoginEntry, encryptionKey: String) throws -> EncryptedLogin { + open func add(login: LoginEntry) throws -> Login { return try queue.sync { - try self.store.add(login: login, encryptionKey: encryptionKey) + try self.store.add(login: login) } } /// Update `login` in the database. If `login.id` does not refer to a known /// login, then this throws `LoginStoreError.NoSuchRecord`. - open func update(id: String, login: LoginEntry, encryptionKey: String) throws -> EncryptedLogin { + open func update(id: String, login: LoginEntry) throws -> Login { return try queue.sync { - try self.store.update(id: id, login: login, encryptionKey: encryptionKey) + try self.store.update(id: id, login: login) } } /// Get the record with the given id. Returns nil if there is no such record. - open func get(id: String) throws -> EncryptedLogin? { + open func get(id: String) throws -> Login? { return try queue.sync { try self.store.get(id: id) } } /// Get the entire list of records. - open func list() throws -> [EncryptedLogin] { + open func list() throws -> [Login] { return try queue.sync { try self.store.list() } } /// Get the list of records for some base domain. - open func getByBaseDomain(baseDomain: String) throws -> [EncryptedLogin] { + open func getByBaseDomain(baseDomain: String) throws -> [Login] { return try queue.sync { try self.store.getByBaseDomain(baseDomain: baseDomain) } diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 55275d23d1..2bf1ffcfdc 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -212,7 +212,7 @@ impl LoginDb { pub fn find_login_to_update( &self, look: LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result> { let look = look.fixup()?; let logins = self @@ -355,7 +355,11 @@ impl LoginDb { Ok(()) } - pub fn add(&self, entry: LoginEntry, encdec: &EncryptorDecryptor) -> Result { + pub fn add( + &self, + entry: LoginEntry, + encdec: &dyn EncryptorDecryptor, + ) -> Result { let guid = Guid::random(); let now_ms = util::system_time_ms_i64(SystemTime::now()); @@ -381,7 +385,7 @@ impl LoginDb { &self, sguid: &str, entry: LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { let guid = Guid::new(sguid); let now_ms = util::system_time_ms_i64(SystemTime::now()); @@ -442,7 +446,7 @@ impl LoginDb { pub fn add_or_update( &self, entry: LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { // Make sure to fixup the entry first, in case that changes the username let entry = entry.fixup()?; @@ -456,7 +460,7 @@ impl LoginDb { &self, guid: &Guid, entry: LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { let entry = entry.fixup()?; self.check_for_dupes(guid, &entry, encdec)?; @@ -467,7 +471,7 @@ impl LoginDb { &self, guid: &Guid, entry: &LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result<()> { if self.dupe_exists(guid, entry, encdec)? { return Err(InvalidLogin::DuplicateLogin.into()); @@ -479,7 +483,7 @@ impl LoginDb { &self, guid: &Guid, entry: &LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { Ok(self.find_dupe(guid, entry, encdec)?.is_some()) } @@ -488,7 +492,7 @@ impl LoginDb { &self, guid: &Guid, entry: &LoginEntry, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result> { for possible in self.get_by_entry_target(entry)? { if possible.guid() != *guid { @@ -879,7 +883,7 @@ pub mod test_utils { #[cfg(test)] mod tests { use super::*; - use crate::encryption::test_utils::TEST_ENCRYPTOR; + use crate::encryption::test_utils::TEST_ENCDEC; use crate::sync::merge::LocalLogin; use crate::SecureLoginFields; use std::{thread, time}; @@ -899,13 +903,13 @@ mod tests { }; let db = LoginDb::open_in_memory().unwrap(); - db.add(login.clone(), &TEST_ENCRYPTOR) + db.add(login.clone(), &*TEST_ENCDEC) .expect("should be able to add first login"); // We will reject new logins with the same username value... let exp_err = "Invalid login: Login already exists"; assert_eq!( - db.add(login.clone(), &TEST_ENCRYPTOR) + db.add(login.clone(), &*TEST_ENCDEC) .unwrap_err() .to_string(), exp_err @@ -913,11 +917,11 @@ mod tests { // Add one with an empty username - not a dupe. login.sec_fields.username = "".to_string(); - db.add(login.clone(), &TEST_ENCRYPTOR) + db.add(login.clone(), &*TEST_ENCDEC) .expect("empty login isn't a dupe"); assert_eq!( - db.add(login, &TEST_ENCRYPTOR).unwrap_err().to_string(), + db.add(login, &*TEST_ENCDEC).unwrap_err().to_string(), exp_err ); @@ -943,7 +947,7 @@ mod tests { password: "😍".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); let fetched = db @@ -958,7 +962,7 @@ mod tests { ); assert_eq!(fetched.fields.username_field, "😍"); assert_eq!(fetched.fields.password_field, "😍"); - let sec_fields = fetched.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + let sec_fields = fetched.decrypt_fields(&*TEST_ENCDEC).unwrap(); assert_eq!(sec_fields.username, "😍"); assert_eq!(sec_fields.password, "😍"); } @@ -980,7 +984,7 @@ mod tests { password: "😍".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); let fetched = db @@ -1025,7 +1029,7 @@ mod tests { ..Default::default() }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); } @@ -1114,7 +1118,7 @@ mod tests { password: "test_password".into(), }, }; - let login = db.add(to_add, &TEST_ENCRYPTOR).unwrap(); + let login = db.add(to_add, &*TEST_ENCDEC).unwrap(); let login2 = db.get_by_id(&login.record.id).unwrap().unwrap(); assert_eq!(login.fields.origin, login2.fields.origin); @@ -1138,7 +1142,7 @@ mod tests { password: "password1".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); db.update( @@ -1154,7 +1158,7 @@ mod tests { password: "password2".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); @@ -1165,7 +1169,7 @@ mod tests { login2.fields.http_realm, Some("https://www.example2.com".into()) ); - let sec_fields = login2.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + let sec_fields = login2.decrypt_fields(&*TEST_ENCDEC).unwrap(); assert_eq!(sec_fields.username, "user2"); assert_eq!(sec_fields.password, "password2"); } @@ -1186,7 +1190,7 @@ mod tests { password: "password1".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); // Simulate touch happening at another "time" @@ -1213,7 +1217,7 @@ mod tests { password: "test_password".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); @@ -1250,9 +1254,9 @@ mod tests { } fn make_saved_login(db: &LoginDb, username: &str, password: &str) -> Login { - db.add(make_entry(username, password), &TEST_ENCRYPTOR) + db.add(make_entry(username, password), &*TEST_ENCDEC) .unwrap() - .decrypt(&TEST_ENCRYPTOR) + .decrypt(&*TEST_ENCDEC) .unwrap() } @@ -1262,7 +1266,7 @@ mod tests { let login = make_saved_login(&db, "user", "pass"); assert_eq!( Some(login), - db.find_login_to_update(make_entry("user", "pass"), &TEST_ENCRYPTOR) + db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) .unwrap(), ); } @@ -1285,7 +1289,7 @@ mod tests { password: "pass".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); // Non-match because it uses form_action_origin instead of http_realm @@ -1301,12 +1305,12 @@ mod tests { password: "pass".into(), }, }, - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); assert_eq!( None, - db.find_login_to_update(make_entry("user", "pass"), &TEST_ENCRYPTOR) + db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) .unwrap(), ); } @@ -1317,7 +1321,7 @@ mod tests { let login = make_saved_login(&db, "", "pass"); assert_eq!( Some(login), - db.find_login_to_update(make_entry("user", "pass"), &TEST_ENCRYPTOR) + db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) .unwrap(), ); } @@ -1329,7 +1333,7 @@ mod tests { let username_match = make_saved_login(&db, "user", "pass"); assert_eq!( Some(username_match), - db.find_login_to_update(make_entry("user", "pass"), &TEST_ENCRYPTOR) + db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) .unwrap(), ); } @@ -1347,7 +1351,7 @@ mod tests { }, ..LoginEntry::default() }, - &TEST_ENCRYPTOR + &*TEST_ENCDEC ) .is_err()); } @@ -1358,17 +1362,17 @@ mod tests { // without triggering a DuplicateLogin error let db = LoginDb::open_in_memory().unwrap(); let login = make_saved_login(&db, "user", "pass"); - let mut dupe = login.clone().encrypt(&TEST_ENCRYPTOR).unwrap(); + let mut dupe = login.clone().encrypt(&*TEST_ENCDEC).unwrap(); dupe.record.id = "different-guid".to_string(); db.insert_new_login(&dupe).unwrap(); let mut entry = login.entry(); entry.sec_fields.password = "pass2".to_string(); - db.update(&login.record.id, entry, &TEST_ENCRYPTOR).unwrap(); + db.update(&login.record.id, entry, &*TEST_ENCDEC).unwrap(); let mut entry = login.entry(); entry.sec_fields.password = "pass3".to_string(); - db.add_or_update(entry, &TEST_ENCRYPTOR).unwrap(); + db.add_or_update(entry, &*TEST_ENCDEC).unwrap(); } } } diff --git a/components/logins/src/encryption.rs b/components/logins/src/encryption.rs index 8344a037f2..ee18d223bf 100644 --- a/components/logins/src/encryption.rs +++ b/components/logins/src/encryption.rs @@ -26,24 +26,154 @@ // 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. +// +// Consumers of the Login component have 3 options for setting up encryption: +// 1. Implement EncryptorDecryptor directly +// eg `LoginStore::new(MyEncryptorDecryptor)` +// 2. Implement KeyManager and use ManagedEncryptorDecryptor +// eg `LoginStore::new(ManagedEncryptorDecryptor::new(MyKeyManager))` +// 3. Generate a single key and create a StaticKeyManager and use it together with +// ManagedEncryptorDecryptor +// eg `LoginStore::new(ManagedEncryptorDecryptor::new(StaticKeyManager { key: myKey }))` +// +// You can implement EncryptorDecryptor directly to keep full control over the encryption +// algorithm. For example, on the desktop, this could make use of NSS's SecretDecoderRing to +// achieve transparent key management. +// +// If the application wants to keep the current encryption, like Android and iOS, for example, but +// control the key management itself, the KeyManager can be implemented and the encryption can be +// done on the Rust side with the ManagedEncryptorDecryptor. +// +// In tests or some command line tools, it can be practical to use a static key that does not +// change at runtime and is already present when the LoginsStore is initialized. In this case, it +// makes sense to use the provided StaticKeyManager. use crate::error::*; +use std::sync::Arc; + +/// This is the generic EncryptorDecryptor trait, as handed over to the Store during initialization. +/// Consumers can implement either this generic trait and bring in their own crypto, or leverage the +/// ManagedEncryptorDecryptor below, which provides encryption algorithms out of the box. +/// +/// Note that EncryptorDecryptor must not call any LoginStore methods. The login store can call out +/// to the EncryptorDecryptor when it's internal mutex is held so calling back in to the LoginStore +/// may deadlock. +pub trait EncryptorDecryptor: Send + Sync { + fn encrypt(&self, cleartext: Vec) -> ApiResult>; + fn decrypt(&self, ciphertext: Vec) -> ApiResult>; +} + +impl EncryptorDecryptor for Arc { + fn encrypt(&self, clearbytes: Vec) -> ApiResult> { + (**self).encrypt(clearbytes) + } + + fn decrypt(&self, cipherbytes: Vec) -> ApiResult> { + (**self).decrypt(cipherbytes) + } +} + +/// The ManagedEncryptorDecryptor makes use of the NSS provided cryptographic algorithms. The +/// ManagedEncryptorDecryptor uses a KeyManager for encryption key retrieval. +pub struct ManagedEncryptorDecryptor { + key_manager: Arc, +} + +impl ManagedEncryptorDecryptor { + pub fn new(key_manager: Arc) -> Self { + Self { key_manager } + } +} + +impl EncryptorDecryptor for ManagedEncryptorDecryptor { + fn encrypt(&self, clearbytes: Vec) -> ApiResult> { + let keybytes = self + .key_manager + .get_key() + .map_err(|_| LoginsApiError::MissingKey)?; + let key = std::str::from_utf8(&keybytes).map_err(|_| LoginsApiError::InvalidKey)?; + + let encdec = jwcrypto::EncryptorDecryptor::new(key) + .map_err(|_: jwcrypto::EncryptorDecryptorError| LoginsApiError::InvalidKey)?; + + let cleartext = + std::str::from_utf8(&clearbytes).map_err(|e| LoginsApiError::EncryptionFailed { + reason: e.to_string(), + })?; + encdec + .encrypt(cleartext, "encrypt SecureLoginFields") + .map_err( + |e: jwcrypto::EncryptorDecryptorError| LoginsApiError::EncryptionFailed { + reason: e.to_string(), + }, + ) + .map(|text| text.into()) + } + + fn decrypt(&self, cipherbytes: Vec) -> ApiResult> { + let keybytes = self + .key_manager + .get_key() + .map_err(|_| LoginsApiError::MissingKey)?; + let key = std::str::from_utf8(&keybytes).map_err(|_| LoginsApiError::InvalidKey)?; + + let encdec = jwcrypto::EncryptorDecryptor::new(key) + .map_err(|_: jwcrypto::EncryptorDecryptorError| LoginsApiError::InvalidKey)?; + + let ciphertext = + std::str::from_utf8(&cipherbytes).map_err(|e| LoginsApiError::DecryptionFailed { + reason: e.to_string(), + })?; + encdec + .decrypt(ciphertext, "decrypt SecureLoginFields") + .map_err( + |e: jwcrypto::EncryptorDecryptorError| LoginsApiError::DecryptionFailed { + reason: e.to_string(), + }, + ) + .map(|text| text.into()) + } +} + +/// Consumers can implement the KeyManager in combination with the ManagedEncryptorDecryptor to hand +/// over the encryption key whenever encryption or decryption happens. +pub trait KeyManager: Send + Sync { + fn get_key(&self) -> ApiResult>; +} -pub type EncryptorDecryptor = jwcrypto::EncryptorDecryptor; +/// Last but not least we provide a StaticKeyManager, which can be +/// used in cases where there is a single key during runtime, for example in tests. +pub struct StaticKeyManager { + key: String, +} + +impl StaticKeyManager { + pub fn new(key: String) -> Self { + Self { key } + } +} + +impl KeyManager for StaticKeyManager { + #[handle_error(Error)] + fn get_key(&self) -> ApiResult> { + Ok(self.key.as_bytes().into()) + } +} #[handle_error(Error)] pub fn create_canary(text: &str, key: &str) -> ApiResult { - EncryptorDecryptor::new(key)?.create_canary(text) + jwcrypto::EncryptorDecryptor::new(key)?.create_canary(text) } -#[handle_error(Error)] pub fn check_canary(canary: &str, text: &str, key: &str) -> ApiResult { - EncryptorDecryptor::new(key)?.check_canary(canary, text) + let encdec = jwcrypto::EncryptorDecryptor::new(key) + .map_err(|_: jwcrypto::EncryptorDecryptorError| LoginsApiError::InvalidKey)?; + Ok(encdec.check_canary(canary, text).unwrap_or(false)) } #[handle_error(Error)] pub fn create_key() -> ApiResult { - EncryptorDecryptor::create_key() + jwcrypto::EncryptorDecryptor::create_key() } #[cfg(test)] @@ -53,23 +183,17 @@ pub mod test_utils { lazy_static::lazy_static! { pub static ref TEST_ENCRYPTION_KEY: String = serde_json::to_string(&jwcrypto::Jwk::new_direct_key(Some("test-key".to_string())).unwrap()).unwrap(); - pub static ref TEST_ENCRYPTOR: EncryptorDecryptor = EncryptorDecryptor::new(&TEST_ENCRYPTION_KEY).unwrap(); - } - pub fn encrypt(value: &str) -> String { - TEST_ENCRYPTOR.encrypt(value, "test encrypt").unwrap() - } - pub fn decrypt(value: &str) -> String { - TEST_ENCRYPTOR.decrypt(value, "test decrypt").unwrap() + pub static ref TEST_ENCDEC: Arc = Arc::new(ManagedEncryptorDecryptor::new(Arc::new(StaticKeyManager { key: TEST_ENCRYPTION_KEY.clone() }))); } + pub fn encrypt_struct(fields: &T) -> String { - TEST_ENCRYPTOR - .encrypt_struct(fields, "test encrypt struct") - .unwrap() + let string = serde_json::to_string(fields).unwrap(); + let cipherbytes = TEST_ENCDEC.encrypt(string.as_bytes().into()).unwrap(); + std::str::from_utf8(&cipherbytes).unwrap().to_owned() } pub fn decrypt_struct(ciphertext: String) -> T { - TEST_ENCRYPTOR - .decrypt_struct(&ciphertext, "test decrypt struct") - .unwrap() + let jsonbytes = TEST_ENCDEC.decrypt(ciphertext.as_bytes().into()).unwrap(); + serde_json::from_str(std::str::from_utf8(&jsonbytes).unwrap()).unwrap() } } @@ -78,22 +202,65 @@ mod test { use super::*; #[test] - fn test_encrypt() { - let ed = EncryptorDecryptor::new(&create_key().unwrap()).unwrap(); + fn test_static_key_manager() { + let key = create_key().unwrap(); + let key_manager = StaticKeyManager { key: key.clone() }; + assert_eq!(key.as_bytes(), key_manager.get_key().unwrap()); + } + + #[test] + fn test_managed_encdec_with_invalid_key() { + let key_manager = Arc::new(StaticKeyManager { + key: "bad_key".to_owned(), + }); + let encdec = ManagedEncryptorDecryptor { key_manager }; + assert!(matches!( + encdec.encrypt("secret".as_bytes().into()).err().unwrap(), + LoginsApiError::InvalidKey + )); + } + + #[test] + fn test_managed_encdec_with_missing_key() { + struct MyKeyManager {} + impl KeyManager for MyKeyManager { + fn get_key(&self) -> ApiResult> { + Err(LoginsApiError::MissingKey) + } + } + let key_manager = Arc::new(MyKeyManager {}); + let encdec = ManagedEncryptorDecryptor { key_manager }; + assert!(matches!( + encdec.encrypt("secret".as_bytes().into()).err().unwrap(), + LoginsApiError::MissingKey + )); + } + + #[test] + fn test_managed_encdec() { + let key = create_key().unwrap(); + let key_manager = Arc::new(StaticKeyManager { key }); + let encdec = ManagedEncryptorDecryptor { key_manager }; let cleartext = "secret"; - let ciphertext = ed.encrypt(cleartext, "test encrypt").unwrap(); - assert_eq!(ed.decrypt(&ciphertext, "test decrypt").unwrap(), cleartext); - let ed2 = EncryptorDecryptor::new(&create_key().unwrap()).unwrap(); + let ciphertext = encdec.encrypt(cleartext.as_bytes().into()).unwrap(); + assert_eq!( + encdec.decrypt(ciphertext.clone()).unwrap(), + cleartext.as_bytes() + ); + let other_encdec = ManagedEncryptorDecryptor { + key_manager: Arc::new(StaticKeyManager { + key: create_key().unwrap(), + }), + }; assert!(matches!( - ed2.decrypt(&ciphertext, "test decrypt").err().unwrap(), - Error::CryptoError(jwcrypto::EncryptorDecryptorError { description, .. }) - if description == "test decrypt" + other_encdec.decrypt(ciphertext).err().unwrap(), + LoginsApiError::DecryptionFailed { reason: _ } )); } #[test] fn test_key_error() { - let storage_err = EncryptorDecryptor::new("bad-key").err().unwrap(); + let storage_err = jwcrypto::EncryptorDecryptor::new("bad-key").err().unwrap(); assert!(matches!( storage_err, Error::CryptoError(jwcrypto::EncryptorDecryptorError { @@ -111,11 +278,12 @@ mod test { assert!(check_canary(&canary, CANARY_TEXT, &key).unwrap()); let different_key = create_key().unwrap(); + assert!(!check_canary(&canary, CANARY_TEXT, &different_key).unwrap()); + + let bad_key = "bad_key".to_owned(); assert!(matches!( - check_canary(&canary, CANARY_TEXT, &different_key) - .err() - .unwrap(), - LoginsApiError::IncorrectKey + check_canary(&canary, CANARY_TEXT, &bad_key).err().unwrap(), + LoginsApiError::InvalidKey )); } } diff --git a/components/logins/src/error.rs b/components/logins/src/error.rs index 5d5525e77d..548acb6205 100644 --- a/components/logins/src/error.rs +++ b/components/logins/src/error.rs @@ -21,8 +21,17 @@ pub enum LoginsApiError { #[error("No record with guid exists (when one was required): {reason:?}")] NoSuchRecord { reason: String }, - #[error("Encryption key is in the correct format, but is not the correct key.")] - IncorrectKey, + #[error("Encryption key is missing.")] + MissingKey, + + #[error("Encryption key is not valid.")] + InvalidKey, + + #[error("encryption failed: {reason}")] + EncryptionFailed { reason: String }, + + #[error("decryption failed: {reason}")] + DecryptionFailed { reason: String }, #[error("{reason}")] Interrupted { reason: String }, @@ -55,8 +64,11 @@ pub enum Error { #[error("The logins tables are not empty")] NonEmptyTable, - #[error("local encryption key not set")] - EncryptionKeyMissing, + #[error("encryption failed: {0:?}")] + EncryptionFailed(String), + + #[error("decryption failed: {0:?}")] + DecryptionFailed(String), #[error("Error synchronizing: {0}")] SyncAdapterError(#[from] sync15::Error), @@ -136,8 +148,6 @@ impl GetErrorHandling for Error { }) .report_error("logins-migration") } - Self::CryptoError { .. } => ErrorHandling::convert(LoginsApiError::IncorrectKey) - .report_error("logins-crypto-error"), Self::Interrupted(_) => ErrorHandling::convert(LoginsApiError::Interrupted { reason: self.to_string(), }), diff --git a/components/logins/src/lib.rs b/components/logins/src/lib.rs index 09b0eca031..593e606f12 100644 --- a/components/logins/src/lib.rs +++ b/components/logins/src/lib.rs @@ -16,6 +16,9 @@ mod store; mod sync; mod util; +use crate::encryption::{ + EncryptorDecryptor, KeyManager, ManagedEncryptorDecryptor, StaticKeyManager, +}; uniffi::include_scaffolding!("logins"); pub use crate::db::LoginDb; @@ -24,29 +27,18 @@ pub use crate::error::*; pub use crate::login::*; pub use crate::store::*; pub use crate::sync::LoginsSyncEngine; - -// Public encryption functions. We publish these as top-level functions to expose them across -// UniFFI -#[handle_error(Error)] -fn encrypt_login(login: Login, enc_key: &str) -> ApiResult { - let encdec = encryption::EncryptorDecryptor::new(enc_key)?; - login.encrypt(&encdec) -} - -#[handle_error(Error)] -fn decrypt_login(login: EncryptedLogin, enc_key: &str) -> ApiResult { - let encdec = encryption::EncryptorDecryptor::new(enc_key)?; - login.decrypt(&encdec) -} - -#[handle_error(Error)] -fn encrypt_fields(sec_fields: SecureLoginFields, enc_key: &str) -> ApiResult { - let encdec = encryption::EncryptorDecryptor::new(enc_key)?; - sec_fields.encrypt(&encdec) +use std::sync::Arc; + +// Utility function to create a StaticKeyManager to be used for the time being until support lands +// for [trait implementation of an UniFFI +// interface](https://mozilla.github.io/uniffi-rs/next/proc_macro/index.html#structs-implementing-traits) +// in UniFFI. +pub fn create_static_key_manager(key: String) -> Arc { + Arc::new(StaticKeyManager::new(key)) } -#[handle_error(Error)] -fn decrypt_fields(sec_fields: String, enc_key: &str) -> ApiResult { - let encdec = encryption::EncryptorDecryptor::new(enc_key)?; - SecureLoginFields::decrypt(&sec_fields, &encdec) +// Similar to create_static_key_manager above, create a +// ManagedEncryptorDecryptor by passing in a KeyManager +pub fn create_managed_encdec(key_manager: Arc) -> Arc { + Arc::new(ManagedEncryptorDecryptor::new(key_manager)) } diff --git a/components/logins/src/login.rs b/components/logins/src/login.rs index e68ca402c0..a1057bda2c 100644 --- a/components/logins/src/login.rs +++ b/components/logins/src/login.rs @@ -366,12 +366,23 @@ pub struct SecureLoginFields { } impl SecureLoginFields { - pub fn encrypt(&self, encdec: &EncryptorDecryptor) -> Result { - encdec.encrypt_struct(&self, "encrypt SecureLoginFields") + pub fn encrypt(&self, encdec: &dyn EncryptorDecryptor) -> Result { + let string = serde_json::to_string(&self)?; + let cipherbytes = encdec + .encrypt(string.as_bytes().into()) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + let ciphertext = std::str::from_utf8(&cipherbytes) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + Ok(ciphertext.to_owned()) } - pub fn decrypt(ciphertext: &str, encdec: &EncryptorDecryptor) -> Result { - encdec.decrypt_struct(ciphertext, "decrypt SecureLoginFields") + pub fn decrypt(ciphertext: &str, encdec: &dyn EncryptorDecryptor) -> Result { + let jsonbytes = encdec + .decrypt(ciphertext.as_bytes().into()) + .map_err(|e| Error::DecryptionFailed(e.to_string()))?; + let json = + std::str::from_utf8(&jsonbytes).map_err(|e| Error::DecryptionFailed(e.to_string()))?; + Ok(serde_json::from_str(json)?) } } @@ -413,7 +424,7 @@ impl Login { } } - pub fn encrypt(self, encdec: &EncryptorDecryptor) -> Result { + pub fn encrypt(self, encdec: &dyn EncryptorDecryptor) -> Result { Ok(EncryptedLogin { record: self.record, fields: self.fields, @@ -442,7 +453,7 @@ impl EncryptedLogin { &self.record.id } - pub fn decrypt(self, encdec: &EncryptorDecryptor) -> Result { + pub fn decrypt(self, encdec: &dyn EncryptorDecryptor) -> Result { Ok(Login { record: self.record, fields: self.fields, @@ -450,7 +461,7 @@ impl EncryptedLogin { }) } - pub fn decrypt_fields(&self, encdec: &EncryptorDecryptor) -> Result { + pub fn decrypt_fields(&self, encdec: &dyn EncryptorDecryptor) -> Result { SecureLoginFields::decrypt(&self.sec_fields, encdec) } @@ -720,6 +731,7 @@ pub mod test_utils { origin: format!("https://{}.example.com", id), ..Default::default() }, + // TODO: fixme sec_fields: encrypt_struct(&sec_fields), } } diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index 06016c40cd..22b0674e09 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -9,29 +9,25 @@ namespace logins { [Throws=LoginsApiError] string create_key(); - /// Decrypt an `EncryptedLogin` to a `Login` - [Throws=LoginsApiError] - Login decrypt_login(EncryptedLogin login, [ByRef]string encryption_key); - - /// Encrypt a `Login` to an `EncryptedLogin` - [Throws=LoginsApiError] - EncryptedLogin encrypt_login(Login login, [ByRef]string encryption_key); - - /// Decrypt an encrypted `string` to `SecureLoginFields` - [Throws=LoginsApiError] - SecureLoginFields decrypt_fields(string sec_fields, [ByRef]string encryption_key); - - /// Encrypt `SecureLoginFields` to an encrypted `string` - [Throws=LoginsApiError] - string encrypt_fields(SecureLoginFields sec_fields, [ByRef]string encryption_key); - - /// Create a "canary" string, which can be used to test if the encryption key is still valid for the logins data + /// Create a "canary" string, which can be used to test if the encryption + //key is still valid for the logins data [Throws=LoginsApiError] string create_canary([ByRef]string text, [ByRef]string encryption_key); - /// Check that key is still valid using the output of `create_canary`. `text` much match the text you initially passed to `create_canary()` + /// Check that key is still valid using the output of `create_canary`. + //`text` much match the text you initially passed to `create_canary()` [Throws=LoginsApiError] boolean check_canary([ByRef]string canary, [ByRef]string text, [ByRef]string encryption_key); + + /// Utility function to create a StaticKeyManager to be used for the time + /// being until support lands for [trait implementation of an UniFFI + /// interface](https://mozilla.github.io/uniffi-rs/next/proc_macro/index.html#structs-implementing-traits) + /// in UniFFI. + KeyManager create_static_key_manager(string key); + + /// Similar to create_static_key_manager above, create a + /// ManagedEncryptorDecryptor by passing in a KeyManager + EncryptorDecryptor create_managed_encdec(KeyManager key_manager); }; /// The fields you can add or update. @@ -43,7 +39,7 @@ dictionary LoginFields { string password_field; }; -/// Fields available only while the encryption key is known. +/// Fields which are encrypted at rest. dictionary SecureLoginFields { string password; string username; @@ -59,7 +55,7 @@ dictionary RecordFields { }; /// A login entry from the user, not linked to any database record. -/// The add/update APIs input these, alongside an encryption key. +/// The add/update APIs input these. dictionary LoginEntry { LoginFields fields; SecureLoginFields sec_fields; @@ -72,15 +68,6 @@ dictionary Login { SecureLoginFields sec_fields; }; -/// An encrypted version of [Login]. This is what we return for all the "read" -/// APIs - we never return the cleartext of encrypted fields. -dictionary EncryptedLogin { - RecordFields record; - LoginFields fields; - /// ciphertext of a SecureLoginFields - string sec_fields; -}; - /// These are the errors returned by our public API. [Error] interface LoginsApiError { @@ -90,8 +77,17 @@ interface LoginsApiError { /// Asking to do something with a guid which doesn't exist. NoSuchRecord(string reason); - /// The encryption key supplied of the correct format, but not the correct key. - IncorrectKey(); + /// Encryption key is missing. + MissingKey(); + + /// Encryption key is not valid. + InvalidKey(); + + /// encryption failed + EncryptionFailed(string reason); + + /// decryption failed + DecryptionFailed(string reason); /// An operation was interrupted at the request of the consuming app. Interrupted(string reason); @@ -107,19 +103,41 @@ interface LoginsApiError { UnexpectedLoginsApiError(string reason); }; +[Trait, WithForeign] +interface EncryptorDecryptor { + [Throws=LoginsApiError] + bytes encrypt(bytes cleartext); + + [Throws=LoginsApiError] + bytes decrypt(bytes ciphertext); +}; + +[Trait, WithForeign] +interface KeyManager { + [Throws=LoginsApiError] + bytes get_key(); +}; + +interface StaticKeyManager { + constructor(string key); +}; + +interface ManagedEncryptorDecryptor { + constructor(KeyManager key_manager); +}; interface LoginStore { [Throws=LoginsApiError] - constructor(string path); + constructor(string path, EncryptorDecryptor encdec); [Throws=LoginsApiError] - EncryptedLogin add(LoginEntry login, [ByRef]string encryption_key); + Login add(LoginEntry login); [Throws=LoginsApiError] - EncryptedLogin update([ByRef] string id, LoginEntry login, [ByRef]string encryption_key); + Login update([ByRef] string id, LoginEntry login); [Throws=LoginsApiError] - EncryptedLogin add_or_update(LoginEntry login, [ByRef]string encryption_key); + Login add_or_update(LoginEntry login); [Throws=LoginsApiError] boolean delete([ByRef] string id); @@ -134,16 +152,19 @@ interface LoginStore { void touch([ByRef] string id); [Throws=LoginsApiError] - sequence list(); + sequence list(); + + [Throws=LoginsApiError] + sequence get_by_base_domain([ByRef] string base_domain); [Throws=LoginsApiError] - sequence get_by_base_domain([ByRef] string base_domain); + boolean has_logins_by_base_domain([ByRef] string base_domain); [Throws=LoginsApiError] - Login? find_login_to_update(LoginEntry look, [ByRef]string encryption_key); + Login? find_login_to_update(LoginEntry look); [Throws=LoginsApiError] - EncryptedLogin? get([ByRef] string id); + Login? get([ByRef] string id); [Self=ByArc] void register_with_sync_manager(); diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index afc5126b77..bd55f72bb6 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -4,7 +4,7 @@ use crate::db::LoginDb; use crate::encryption::EncryptorDecryptor; use crate::error::*; -use crate::login::{EncryptedLogin, Login, LoginEntry}; +use crate::login::{Login, LoginEntry}; use crate::LoginsSyncEngine; use parking_lot::Mutex; use std::path::Path; @@ -48,48 +48,76 @@ fn create_sync_engine( pub struct LoginStore { pub db: Mutex, + pub encdec: Arc, } impl LoginStore { #[handle_error(Error)] - pub fn new(path: impl AsRef) -> ApiResult { + pub fn new(path: impl AsRef, encdec: Arc) -> ApiResult { let db = Mutex::new(LoginDb::open(path)?); - Ok(Self { db }) + Ok(Self { db, encdec }) } - pub fn new_from_db(db: LoginDb) -> Self { - Self { db: Mutex::new(db) } + pub fn new_from_db(db: LoginDb, encdec: Arc) -> Self { + Self { + db: Mutex::new(db), + encdec, + } } #[handle_error(Error)] - pub fn new_in_memory() -> ApiResult { + pub fn new_in_memory(encdec: Arc) -> ApiResult { let db = Mutex::new(LoginDb::open_in_memory()?); - Ok(Self { db }) + Ok(Self { db, encdec }) } #[handle_error(Error)] - pub fn list(&self) -> ApiResult> { - self.db.lock().get_all() + pub fn list(&self) -> ApiResult> { + self.db.lock().get_all().and_then(|logins| { + logins + .into_iter() + .map(|login| login.decrypt(self.encdec.as_ref())) + .collect() + }) } #[handle_error(Error)] - pub fn get(&self, id: &str) -> ApiResult> { - self.db.lock().get_by_id(id) + pub fn get(&self, id: &str) -> ApiResult> { + match self.db.lock().get_by_id(id) { + Ok(result) => match result { + Some(enc_login) => enc_login.decrypt(self.encdec.as_ref()).map(Some), + None => Ok(None), + }, + Err(err) => Err(err), + } + } + + #[handle_error(Error)] + pub fn get_by_base_domain(&self, base_domain: &str) -> ApiResult> { + self.db + .lock() + .get_by_base_domain(base_domain) + .and_then(|logins| { + logins + .into_iter() + .map(|login| login.decrypt(self.encdec.as_ref())) + .collect() + }) } #[handle_error(Error)] - pub fn get_by_base_domain(&self, base_domain: &str) -> ApiResult> { - self.db.lock().get_by_base_domain(base_domain) + pub fn has_logins_by_base_domain(&self, base_domain: &str) -> ApiResult { + self.db + .lock() + .get_by_base_domain(base_domain) + .map(|logins| !logins.is_empty()) } #[handle_error(Error)] - pub fn find_login_to_update( - &self, - entry: LoginEntry, - enc_key: &str, - ) -> ApiResult> { - let encdec = EncryptorDecryptor::new(enc_key)?; - self.db.lock().find_login_to_update(entry, &encdec) + pub fn find_login_to_update(&self, entry: LoginEntry) -> ApiResult> { + self.db + .lock() + .find_login_to_update(entry, self.encdec.as_ref()) } #[handle_error(Error)] @@ -119,21 +147,27 @@ impl LoginStore { } #[handle_error(Error)] - pub fn update(&self, id: &str, entry: LoginEntry, enc_key: &str) -> ApiResult { - let encdec = EncryptorDecryptor::new(enc_key)?; - self.db.lock().update(id, entry, &encdec) + pub fn update(&self, id: &str, entry: LoginEntry) -> ApiResult { + self.db + .lock() + .update(id, entry, self.encdec.as_ref()) + .and_then(|enc_login| enc_login.decrypt(self.encdec.as_ref())) } #[handle_error(Error)] - pub fn add(&self, entry: LoginEntry, enc_key: &str) -> ApiResult { - let encdec = EncryptorDecryptor::new(enc_key)?; - self.db.lock().add(entry, &encdec) + pub fn add(&self, entry: LoginEntry) -> ApiResult { + self.db + .lock() + .add(entry, self.encdec.as_ref()) + .and_then(|enc_login| enc_login.decrypt(self.encdec.as_ref())) } #[handle_error(Error)] - pub fn add_or_update(&self, entry: LoginEntry, enc_key: &str) -> ApiResult { - let encdec = EncryptorDecryptor::new(enc_key)?; - self.db.lock().add_or_update(entry, &encdec) + pub fn add_or_update(&self, entry: LoginEntry) -> ApiResult { + self.db + .lock() + .add_or_update(entry, self.encdec.as_ref()) + .and_then(|enc_login| enc_login.decrypt(self.encdec.as_ref())) } // This allows the embedding app to say "make this instance available to @@ -161,23 +195,22 @@ impl LoginStore { #[cfg(test)] mod test { use super::*; - use crate::encryption::test_utils::{TEST_ENCRYPTION_KEY, TEST_ENCRYPTOR}; + use crate::encryption::test_utils::TEST_ENCDEC; use crate::util; use crate::{LoginFields, SecureLoginFields}; use more_asserts::*; use std::cmp::Reverse; use std::time::SystemTime; - fn assert_logins_equiv(a: &LoginEntry, b: &EncryptedLogin) { - let b_e = b.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + fn assert_logins_equiv(a: &LoginEntry, b: &Login) { assert_eq!(a.fields, b.fields); - assert_eq!(b_e.username, a.sec_fields.username); - assert_eq!(b_e.password, a.sec_fields.password); + assert_eq!(b.sec_fields.username, a.sec_fields.username); + assert_eq!(b.sec_fields.password, a.sec_fields.password); } #[test] fn test_general() { - let store = LoginStore::new_in_memory().unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); let list = store.list().expect("Grabbing Empty list to work"); assert_eq!(list.len(), 0); let start_us = util::system_time_ms_i64(SystemTime::now()); @@ -207,16 +240,8 @@ mod test { password: "fdsa".into(), }, }; - let a_id = store - .add(a.clone(), &TEST_ENCRYPTION_KEY) - .expect("added a") - .record - .id; - let b_id = store - .add(b.clone(), &TEST_ENCRYPTION_KEY) - .expect("added b") - .record - .id; + let a_id = store.add(a.clone()).expect("added a").record.id; + let b_id = store.add(b.clone()).expect("added b").record.id; let a_from_db = store .get(&a_id) @@ -259,12 +284,22 @@ mod test { assert_eq!(list.len(), 1); assert_eq!(list[0], b_from_db); + let has_logins = store + .has_logins_by_base_domain("example2.com") + .expect("Expect a result for this origin"); + assert!(has_logins); + let list = store .get_by_base_domain("example2.com") .expect("Expect a list for this origin"); assert_eq!(list.len(), 1); assert_eq!(list[0], b_from_db); + let has_logins = store + .has_logins_by_base_domain("www.example.com") + .expect("Expect a result for this origin"); + assert!(!has_logins); + let list = store .get_by_base_domain("www.example.com") .expect("Expect an empty list"); @@ -280,7 +315,7 @@ mod test { }; store - .update(&b_id, b2.clone(), &TEST_ENCRYPTION_KEY) + .update(&b_id, b2.clone()) .expect("update b should work"); let b_after_update = store @@ -299,7 +334,7 @@ mod test { #[test] fn test_sync_manager_registration() { - let store = Arc::new(LoginStore::new_in_memory().unwrap()); + let store = Arc::new(LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap()); assert_eq!(Arc::strong_count(&store), 1); assert_eq!(Arc::weak_count(&store), 0); Arc::clone(&store).register_with_sync_manager(); diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 613a6504ce..3e1fbe9b93 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -6,7 +6,6 @@ use super::merge::{LocalLogin, MirrorLogin, SyncLoginData}; use super::update_plan::UpdatePlan; use super::SyncStatus; use crate::db::CLONE_ENTIRE_MIRROR_SQL; -use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::login::EncryptedLogin; use crate::schema; @@ -30,26 +29,15 @@ pub struct LoginsSyncEngine { pub store: Arc, pub scope: SqlInterruptScope, pub staged: RefCell>, - // It's unfortunate this is an Option<>, but tricky to change because sometimes we construct - // an engine for, say, a `reset()` where this isn't needed or known. - encdec: Option, } impl LoginsSyncEngine { - fn encdec(&self) -> Result<&EncryptorDecryptor> { - match &self.encdec { - Some(encdec) => Ok(encdec), - None => Err(Error::EncryptionKeyMissing), - } - } - pub fn new(store: Arc) -> Result { let scope = store.db.lock().begin_interrupt_scope()?; Ok(Self { store, scope, staged: RefCell::new(vec![]), - encdec: None, }) } @@ -60,7 +48,6 @@ impl LoginsSyncEngine { telem: &mut telemetry::EngineIncoming, ) -> Result { let mut plan = UpdatePlan::default(); - let encdec = self.encdec()?; for mut record in records { self.scope.err_if_interrupted()?; @@ -82,7 +69,7 @@ impl LoginsSyncEngine { upstream, upstream_time, server_now, - encdec, + self.store.encdec.as_ref(), )?; telem.reconciled(1); } @@ -145,7 +132,7 @@ impl LoginsSyncEngine { let mut seen_ids: HashSet = HashSet::with_capacity(records.len()); for incoming in records.into_iter() { let id = incoming.envelope.id.clone(); - match SyncLoginData::from_bso(incoming, self.encdec()?) { + match SyncLoginData::from_bso(incoming, self.store.encdec.as_ref()) { Ok(v) => sync_data.push(v), Err(e) => { match e { @@ -266,7 +253,8 @@ impl LoginsSyncEngine { OutgoingBso::new_tombstone(envelope) } else { let unknown = row.get::<_, Option>("enc_unknown_fields")?; - let mut bso = EncryptedLogin::from_row(row)?.into_bso(self.encdec()?, unknown)?; + let mut bso = + EncryptedLogin::from_row(row)?.into_bso(self.store.encdec.as_ref(), unknown)?; bso.envelope.sortindex = Some(DEFAULT_SORTINDEX); bso }) @@ -398,8 +386,7 @@ impl LoginsSyncEngine { .form_action_origin .as_ref() .and_then(|s| util::url_host_port(s)); - let encdec = self.encdec()?; - let enc_fields = l.decrypt_fields(encdec)?; + let enc_fields = l.decrypt_fields(self.store.encdec.as_ref())?; let args = named_params! { ":origin": l.fields.origin, ":http_realm": l.fields.http_realm, @@ -424,7 +411,7 @@ impl LoginsSyncEngine { .query_and_then(args, EncryptedLogin::from_row)? .collect::>>()? { - let this_enc_fields = login.decrypt_fields(encdec)?; + let this_enc_fields = login.decrypt_fields(self.store.encdec.as_ref())?; if enc_fields.username == this_enc_fields.username { return Ok(Some(login)); } @@ -438,11 +425,6 @@ impl SyncEngine for LoginsSyncEngine { "passwords".into() } - fn set_local_encryption_key(&mut self, key: &str) -> anyhow::Result<()> { - self.encdec = Some(EncryptorDecryptor::new(key)?); - Ok(()) - } - fn stage_incoming( &self, mut inbound: Vec, @@ -508,7 +490,7 @@ impl SyncEngine for LoginsSyncEngine { mod tests { use super::*; use crate::db::test_utils::insert_login; - use crate::encryption::test_utils::{TEST_ENCRYPTION_KEY, TEST_ENCRYPTOR}; + use crate::encryption::test_utils::TEST_ENCDEC; use crate::login::test_utils::enc_login; use crate::{LoginEntry, LoginFields, RecordFields, SecureLoginFields}; use std::collections::HashMap; @@ -519,25 +501,19 @@ mod tests { engine: &mut LoginsSyncEngine, records: Vec, ) -> (Vec, telemetry::EngineIncoming) { - engine - .set_local_encryption_key(&TEST_ENCRYPTION_KEY) - .unwrap(); let mut telem = sync15::telemetry::EngineIncoming::new(); (engine.fetch_login_data(records, &mut telem).unwrap(), telem) } fn run_fetch_outgoing(store: LoginStore) -> Vec { - let mut engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); - engine - .set_local_encryption_key(&TEST_ENCRYPTION_KEY) - .unwrap(); + let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); engine.fetch_outgoing().unwrap() } #[test] fn test_fetch_login_data() { // Test some common cases with fetch_login data - let store = LoginStore::new_in_memory().unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); insert_login(&store.db.lock(), "updated_remotely", None, Some("password")); insert_login(&store.db.lock(), "deleted_remotely", None, Some("password")); insert_login( @@ -554,15 +530,15 @@ mod tests { vec![ IncomingBso::new_test_tombstone(Guid::new("deleted_remotely")), enc_login("added_remotely", "password") - .into_bso(&TEST_ENCRYPTOR, None) + .into_bso(&*TEST_ENCDEC, None) .unwrap() .to_test_incoming(), enc_login("updated_remotely", "new-password") - .into_bso(&TEST_ENCRYPTOR, None) + .into_bso(&*TEST_ENCDEC, None) .unwrap() .to_test_incoming(), enc_login("three_way_merge", "new-remote-password") - .into_bso(&TEST_ENCRYPTOR, None) + .into_bso(&*TEST_ENCDEC, None) .unwrap() .to_test_incoming(), ], @@ -584,13 +560,13 @@ mod tests { let LocalLogin::Alive { login, .. } = local_login else { unreachable!("this test is not expecting a tombstone"); }; - login.decrypt_fields(&TEST_ENCRYPTOR).unwrap().password + login.decrypt_fields(&*TEST_ENCDEC).unwrap().password }), mirror: sync_login_data.mirror.map(|mirror_login| { guids_seen.insert(mirror_login.login.record.id.clone()); mirror_login .login - .decrypt_fields(&TEST_ENCRYPTOR) + .decrypt_fields(&*TEST_ENCDEC) .unwrap() .password }), @@ -598,7 +574,7 @@ mod tests { guids_seen.insert(incoming.login.record.id.clone()); incoming .login - .decrypt_fields(&TEST_ENCRYPTOR) + .decrypt_fields(&*TEST_ENCDEC) .unwrap() .password }), @@ -644,7 +620,7 @@ mod tests { #[test] fn test_fetch_outgoing() { - let store = LoginStore::new_in_memory().unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); insert_login( &store.db.lock(), "changed", @@ -676,7 +652,7 @@ mod tests { #[test] fn test_bad_record() { - let store = LoginStore::new_in_memory().unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); let test_ids = ["dummy_000001", "dummy_000002", "dummy_000003"]; for id in test_ids { insert_login(&store.db.lock(), id, Some("password"), Some("password")); @@ -733,14 +709,14 @@ mod tests { username: username.into(), password: password.into(), } - .encrypt(&TEST_ENCRYPTOR) + .encrypt(&*TEST_ENCDEC) .unwrap(), } } #[test] fn find_dupe_login() { - let store = LoginStore::new_in_memory().unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); let to_add = LoginEntry { fields: LoginFields { @@ -753,11 +729,7 @@ mod tests { password: "test".into(), }, }; - let first_id = store - .add(to_add, &TEST_ENCRYPTION_KEY) - .expect("should insert first") - .record - .id; + let first_id = store.add(to_add).expect("should insert first").record.id; let to_add = LoginEntry { fields: LoginFields { @@ -770,11 +742,7 @@ mod tests { password: "test1".into(), }, }; - let second_id = store - .add(to_add, &TEST_ENCRYPTION_KEY) - .expect("should insert second") - .record - .id; + let second_id = store.add(to_add).expect("should insert second").record.id; let to_add = LoginEntry { fields: LoginFields { @@ -787,16 +755,9 @@ mod tests { password: "test1".into(), }, }; - let no_form_origin_id = store - .add(to_add, &TEST_ENCRYPTION_KEY) - .expect("should insert second") - .record - .id; + let no_form_origin_id = store.add(to_add).expect("should insert second").record.id; - let mut engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); - engine - .set_local_encryption_key(&TEST_ENCRYPTION_KEY) - .unwrap(); + let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); let to_find = make_enc_login("test", "test", Some("https://www.example.com".into()), None); assert_eq!( @@ -889,7 +850,6 @@ mod tests { password: "test".into(), }, }, - &TEST_ENCRYPTION_KEY, ) .unwrap(); let changeset = engine.fetch_outgoing().unwrap(); @@ -898,11 +858,8 @@ mod tests { } // The test itself... - let store = LoginStore::new_in_memory().unwrap(); - let mut engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); - engine - .set_local_encryption_key(&TEST_ENCRYPTION_KEY) - .unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); + let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); apply_incoming_payload( &engine, @@ -982,11 +939,8 @@ mod tests { // The test itself... let (local_timestamp, remote_timestamp) = if local_newer { (123, 0) } else { (0, 123) }; - let store = LoginStore::new_in_memory().unwrap(); - let mut engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); - engine - .set_local_encryption_key(&TEST_ENCRYPTION_KEY) - .unwrap(); + let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); + let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); // apply an incoming record - will be in the mirror. apply_incoming_payload( diff --git a/components/logins/src/sync/merge.rs b/components/logins/src/sync/merge.rs index 2af39a954d..3ec57f4d0e 100644 --- a/components/logins/src/sync/merge.rs +++ b/components/logins/src/sync/merge.rs @@ -144,7 +144,7 @@ impl SyncLoginData { &self.guid } - pub fn from_bso(bso: IncomingBso, encdec: &EncryptorDecryptor) -> Result { + pub fn from_bso(bso: IncomingBso, encdec: &dyn EncryptorDecryptor) -> Result { let guid = bso.envelope.id.clone(); let inbound_ts = bso.envelope.modified; let inbound = match bso.into_content::().kind { @@ -279,7 +279,7 @@ impl EncryptedLogin { pub(crate) fn apply_delta( &mut self, mut delta: LoginDelta, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result<()> { apply_field!(self, delta, origin); @@ -315,7 +315,7 @@ impl EncryptedLogin { pub(crate) fn delta( &self, older: &EncryptedLogin, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { let mut delta = LoginDelta::default(); @@ -380,7 +380,7 @@ impl EncryptedLogin { #[cfg(test)] mod tests { use super::*; - use crate::encryption::test_utils::TEST_ENCRYPTOR; + use crate::encryption::test_utils::TEST_ENCDEC; #[test] fn test_invalid_payload_timestamps() { @@ -396,7 +396,7 @@ mod tests { "timeLastUsed": "some other garbage", "timePasswordChanged": -30, // valid i64 but negative })); - let login = SyncLoginData::from_bso(bad_payload, &TEST_ENCRYPTOR) + let login = SyncLoginData::from_bso(bad_payload, &*TEST_ENCDEC) .unwrap() .inbound .unwrap() @@ -417,7 +417,7 @@ mod tests { "timePasswordChanged": now64 - 25, })); - let login = SyncLoginData::from_bso(good_payload, &TEST_ENCRYPTOR) + let login = SyncLoginData::from_bso(good_payload, &*TEST_ENCDEC) .unwrap() .inbound .unwrap() diff --git a/components/logins/src/sync/payload.rs b/components/logins/src/sync/payload.rs index f60620b7bd..5e3ca3fef2 100644 --- a/components/logins/src/sync/payload.rs +++ b/components/logins/src/sync/payload.rs @@ -19,19 +19,30 @@ use sync_guid::Guid; type UnknownFields = serde_json::Map; trait UnknownFieldsExt { - fn encrypt(&self, encdec: &EncryptorDecryptor) -> Result; - fn decrypt(ciphertext: &str, encdec: &EncryptorDecryptor) -> Result + fn encrypt(&self, encdec: &dyn EncryptorDecryptor) -> Result; + fn decrypt(ciphertext: &str, encdec: &dyn EncryptorDecryptor) -> Result where Self: Sized; } impl UnknownFieldsExt for UnknownFields { - fn encrypt(&self, encdec: &EncryptorDecryptor) -> Result { - encdec.encrypt_struct(&self, "encrypt unknown fields") + fn encrypt(&self, encdec: &dyn EncryptorDecryptor) -> Result { + let string = serde_json::to_string(&self)?; + let cipherbytes = encdec + .encrypt(string.as_bytes().into()) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + let ciphertext = std::str::from_utf8(&cipherbytes) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + Ok(ciphertext.to_owned()) } - fn decrypt(ciphertext: &str, encdec: &EncryptorDecryptor) -> Result { - encdec.decrypt_struct(ciphertext, "decrypt unknown fields") + fn decrypt(ciphertext: &str, encdec: &dyn EncryptorDecryptor) -> Result { + let jsonbytes = encdec + .decrypt(ciphertext.as_bytes().into()) + .map_err(|e| Error::DecryptionFailed(e.to_string()))?; + let json = + std::str::from_utf8(&jsonbytes).map_err(|e| Error::DecryptionFailed(e.to_string()))?; + Ok(serde_json::from_str(json)?) } } @@ -51,7 +62,7 @@ impl IncomingLogin { pub(super) fn from_incoming_payload( p: LoginPayload, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result { let fields = LoginFields { origin: p.hostname, @@ -148,7 +159,7 @@ pub struct LoginPayload { impl EncryptedLogin { pub fn into_bso( self, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, enc_unknown_fields: Option, ) -> Result { let unknown_fields = match enc_unknown_fields { @@ -193,7 +204,7 @@ where #[cfg(test)] mod tests { use super::*; - use crate::encryption::test_utils::{encrypt_struct, TEST_ENCRYPTOR}; + use crate::encryption::test_utils::{encrypt_struct, TEST_ENCDEC}; use crate::sync::merge::SyncLoginData; use crate::{EncryptedLogin, LoginFields, RecordFields, SecureLoginFields}; use sync15::bso::IncomingBso; @@ -209,7 +220,7 @@ mod tests { })); let login = IncomingLogin::from_incoming_payload( bso.into_content::().content().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap() .login; @@ -217,7 +228,7 @@ mod tests { assert_eq!(login.fields.http_realm, Some("test".to_string())); assert_eq!(login.fields.origin, "https://www.example.com"); assert_eq!(login.fields.form_action_origin, None); - let sec_fields = login.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); } @@ -236,7 +247,7 @@ mod tests { })); let login = IncomingLogin::from_incoming_payload( bso.into_content::().content().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap() .login; @@ -244,11 +255,11 @@ mod tests { assert_eq!(login.fields.form_action_origin, Some("".to_string())); assert_eq!(login.fields.http_realm, None); assert_eq!(login.fields.origin, "https://www.example.com"); - let sec_fields = login.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); - let bso = login.into_bso(&TEST_ENCRYPTOR, None).unwrap(); + let bso = login.into_bso(&*TEST_ENCDEC, None).unwrap(); assert_eq!(bso.envelope.id, "123412341234"); let payload_data: serde_json::Value = serde_json::from_str(&bso.payload).unwrap(); assert_eq!(payload_data["httpRealm"], serde_json::Value::Null); @@ -284,19 +295,12 @@ mod tests { "bar" ); // re-serialize it. - let unknown = Some( - TEST_ENCRYPTOR - .encrypt_struct::( - &payload.unknown_fields, - "test encrypt unknown fields", - ) - .unwrap(), - ); - let login = IncomingLogin::from_incoming_payload(payload, &TEST_ENCRYPTOR) + let unknown = Some(encrypt_struct::(&payload.unknown_fields)); + let login = IncomingLogin::from_incoming_payload(payload, &*TEST_ENCDEC) .unwrap() .login; // The raw outgoing payload should have it back. - let outgoing = login.into_bso(&TEST_ENCRYPTOR, unknown).unwrap(); + let outgoing = login.into_bso(&*TEST_ENCDEC, unknown).unwrap(); let json = serde_json::from_str::>(&outgoing.payload) .unwrap(); @@ -315,7 +319,7 @@ mod tests { })); let login = IncomingLogin::from_incoming_payload( bso.into_content::().content().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap() .login; @@ -327,7 +331,7 @@ mod tests { Some("https://www.example.com".to_string()) ); assert_eq!(login.fields.username_field, "username-field"); - let sec_fields = login.decrypt_fields(&TEST_ENCRYPTOR).unwrap(); + let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); } @@ -349,7 +353,7 @@ mod tests { password: "password".into(), }), }; - let bso = login.into_bso(&TEST_ENCRYPTOR, None).unwrap(); + let bso = login.into_bso(&*TEST_ENCDEC, None).unwrap(); assert_eq!(bso.envelope.id, "123412341234"); let payload_data: serde_json::Value = serde_json::from_str(&bso.payload).unwrap(); assert_eq!(payload_data["httpRealm"], "test".to_string()); @@ -377,7 +381,7 @@ mod tests { // Incoming sync data gets fixed automatically. let login = IncomingLogin::from_incoming_payload( bad_bso.into_content::().content().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap() .login; @@ -385,7 +389,7 @@ mod tests { // SyncLoginData::from_payload also fixes up. let bad_bso = IncomingBso::from_test_content(bad_json); - let login = SyncLoginData::from_bso(bad_bso, &TEST_ENCRYPTOR) + let login = SyncLoginData::from_bso(bad_bso, &*TEST_ENCDEC) .unwrap() .inbound .unwrap() @@ -406,7 +410,7 @@ mod tests { let login = IncomingLogin::from_incoming_payload( bad_bso.into_content::().content().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap() .login; diff --git a/components/logins/src/sync/update_plan.rs b/components/logins/src/sync/update_plan.rs index 877d2ebc3f..2b3dbdc41d 100644 --- a/components/logins/src/sync/update_plan.rs +++ b/components/logins/src/sync/update_plan.rs @@ -54,7 +54,7 @@ impl UpdatePlan { upstream: IncomingLogin, upstream_time: ServerTimestamp, server_now: ServerTimestamp, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result<()> { let local_age = SystemTime::now() .duration_since(local.local_modified()) @@ -323,7 +323,7 @@ mod tests { get_server_modified, insert_encrypted_login, insert_login, }; use crate::db::LoginDb; - use crate::encryption::test_utils::TEST_ENCRYPTOR; + use crate::encryption::test_utils::TEST_ENCDEC; use crate::login::test_utils::enc_login; fn inc_login(id: &str, password: &str) -> crate::sync::IncomingLogin { @@ -470,7 +470,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); update_plan @@ -536,7 +536,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); update_plan @@ -607,7 +607,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &TEST_ENCRYPTOR, + &*TEST_ENCDEC, ) .unwrap(); update_plan diff --git a/examples/sync-pass/src/sync-pass.rs b/examples/sync-pass/src/sync-pass.rs index ad0dd2107f..b9ca657f1b 100644 --- a/examples/sync-pass/src/sync-pass.rs +++ b/examples/sync-pass/src/sync-pass.rs @@ -10,13 +10,14 @@ use cli_support::fxa_creds::{ get_account_and_token, get_cli_fxa, get_default_fxa_config, SYNC_SCOPE, }; use cli_support::prompt::{prompt_char, prompt_string, prompt_usize}; -use logins::encryption::{create_key, EncryptorDecryptor}; +use logins::encryption::{create_key, ManagedEncryptorDecryptor, StaticKeyManager}; use logins::{ - EncryptedLogin, LoginEntry, LoginFields, LoginStore, LoginsSyncEngine, SecureLoginFields, + Login, LoginEntry, LoginFields, LoginStore, LoginsSyncEngine, SecureLoginFields, ValidateAndFixup, }; + use prettytable::{row, Cell, Row, Table}; -use rusqlite::OptionalExtension; +use std::fs; use std::sync::Arc; use sync15::{ client::{sync_multiple, MemoryCachedState, Sync15StorageClientInit}, @@ -112,9 +113,9 @@ fn string_opt_or<'a>(o: &'a Option, or: &'a str) -> &'a str { string_opt(o).unwrap_or(or) } -fn update_login(login: EncryptedLogin, encdec: &EncryptorDecryptor) -> LoginEntry { +fn update_login(login: Login) -> LoginEntry { let mut record = LoginEntry { - sec_fields: login.decrypt_fields(encdec).unwrap(), + sec_fields: login.sec_fields, fields: login.fields, }; update_encrypted_fields(&mut record.sec_fields, ", leave blank to keep"); @@ -210,7 +211,7 @@ fn show_sql(conn: &rusqlite::Connection, sql: &str) -> Result<()> { Ok(()) } -fn show_all(store: &LoginStore, encdec: &EncryptorDecryptor) -> Result> { +fn show_all(store: &LoginStore) -> Result> { let logins = store.list()?; let mut table = prettytable::Table::new(); @@ -238,12 +239,11 @@ fn show_all(store: &LoginStore, encdec: &EncryptorDecryptor) -> Resultv.len(), Fr->&login.guid(), - &sec_fields.username, - &sec_fields.password, + &login.sec_fields.username, + &login.sec_fields.password, &login.fields.origin, string_opt_or(&login.fields.form_action_origin, ""), @@ -267,12 +267,8 @@ fn show_all(store: &LoginStore, encdec: &EncryptorDecryptor) -> Result Result> { - let index_to_id = show_all(s, encdec)?; +fn prompt_record_id(s: &LoginStore, action: &str) -> Result> { + let index_to_id = show_all(s)?; let input = if let Some(input) = prompt_usize(format!("Enter (idx) of record to {}", action)) { input } else { @@ -285,51 +281,32 @@ fn prompt_record_id( Ok(Some(index_to_id[input].as_str().into())) } -fn open_database(db_path: &str) -> Result<(LoginStore, EncryptorDecryptor, String)> { - let store = LoginStore::new(db_path)?; - // Get or create an encryption key to use - let encryption_key = match get_encryption_key(&store) { - Some(s) => s, - None => { - log::warn!("Creating new encryption key"); +fn open_database(db_path: &str) -> Result<(LoginStore, String)> { + let encryption_key = get_or_create_encryption_key()?; + let encdec = Arc::new(ManagedEncryptorDecryptor::new(Arc::new( + StaticKeyManager::new(encryption_key.clone()), + ))); + let store = LoginStore::new(db_path, encdec)?; + Ok((store, encryption_key)) +} + +fn get_or_create_encryption_key() -> Result { + match get_encryption_key() { + Ok(encryption_key) => Ok(encryption_key), + Err(_) => { let encryption_key = create_key()?; - set_encryption_key(&store, &encryption_key)?; - encryption_key + set_encryption_key(encryption_key.clone())?; + Ok(encryption_key) } - }; - Ok(( - store, - EncryptorDecryptor::new(&encryption_key)?, - encryption_key, - )) + } } -// Use loginsSyncMeta as a quick and dirty solution to store the encryption key -fn get_encryption_key(store: &LoginStore) -> Option { - store - .db - .lock() - .query_row( - "SELECT value FROM loginsSyncMeta WHERE key = 'sync-pass-key'", - [], - |r| r.get(0), - ) - .optional() - .unwrap() +fn get_encryption_key() -> Result { + fs::read_to_string("logins.jwk") } -fn set_encryption_key(store: &LoginStore, key: &str) -> rusqlite::Result<()> { - store - .db - .lock() - .execute( - " - INSERT INTO loginsSyncMeta (key, value) - VALUES ('sync-pass-key', ?) - ", - [&key], - ) - .map(|_| ()) +fn set_encryption_key(encryption_key: String) -> Result<(), std::io::Error> { + fs::write("logins.jwk", encryption_key) } fn do_sync( @@ -411,12 +388,12 @@ fn main() -> Result<()> { log::debug!("db: {:?}", db_path); // Lets not log the encryption key, it's just not a good habit to be in. - let (store, encdec, encryption_key) = open_database(db_path)?; + let (store, encryption_key) = open_database(db_path)?; let store = Arc::new(store); log::info!("Store has {} passwords", store.list()?.len()); - if let Err(e) = show_all(&store, &encdec) { + if let Err(e) = show_all(&store) { log::warn!("Failed to show initial login data! {}", e); } @@ -425,13 +402,13 @@ fn main() -> Result<()> { 'A' | 'a' => { log::info!("Adding new record"); let record = read_login(); - if let Err(e) = store.add(record, &encryption_key) { + if let Err(e) = store.add(record) { log::warn!("Failed to create record! {}", e); } } 'D' | 'd' => { log::info!("Deleting record"); - match prompt_record_id(&store, &encdec, "delete") { + match prompt_record_id(&store, "delete") { Ok(Some(id)) => { if let Err(e) = store.delete(&id) { log::warn!("Failed to delete record! {}", e); @@ -445,7 +422,7 @@ fn main() -> Result<()> { } 'U' | 'u' => { log::info!("Updating record fields"); - match prompt_record_id(&store, &encdec, "update") { + match prompt_record_id(&store, "update") { Err(e) => { log::warn!("Failed to get record ID! {}", e); } @@ -461,7 +438,7 @@ fn main() -> Result<()> { continue; } }; - if let Err(e) = store.update(&id, update_login(login_record.clone(), &encdec), &encryption_key) { + if let Err(e) = store.update(&id, update_login(login_record.clone())) { log::warn!("Failed to update record! {}", e); } } @@ -501,7 +478,7 @@ fn main() -> Result<()> { } } 'V' | 'v' => { - if let Err(e) = show_all(&store, &encdec) { + if let Err(e) = show_all(&store) { log::warn!("Failed to dump passwords? This is probably bad! {}", e); } } @@ -520,7 +497,7 @@ fn main() -> Result<()> { } 'T' | 't' => { log::info!("Touching (bumping use count) for a record"); - match prompt_record_id(&store, &encdec, "update") { + match prompt_record_id(&store, "update") { Err(e) => { log::warn!("Failed to get record ID! {}", e); } diff --git a/testing/sync-test/src/auth.rs b/testing/sync-test/src/auth.rs index 2a0bf42f2b..510c373344 100644 --- a/testing/sync-test/src/auth.rs +++ b/testing/sync-test/src/auth.rs @@ -5,12 +5,16 @@ use anyhow::Result; use autofill::db::store::Store as AutofillStore; use cli_support::fxa_creds::CliFxa; use fxa_client::{Device, FxaConfig, FxaServer}; +use logins::encryption::{create_key, ManagedEncryptorDecryptor, StaticKeyManager}; use logins::LoginStore; use std::collections::HashMap; use std::sync::Arc; -use sync15::{client::{SetupStorageClient, Sync15StorageClient}, DeviceType}; +use sync15::{ + client::{SetupStorageClient, Sync15StorageClient}, + DeviceType, +}; use sync_manager::{ - manager::SyncManager, SyncEngineSelection, SyncParams, SyncReason, DeviceSettings + manager::SyncManager, DeviceSettings, SyncEngineSelection, SyncParams, SyncReason, }; use tabs::TabsStore; @@ -36,19 +40,32 @@ impl TestClient { pub fn new(cli: Arc, device_name: &str) -> Result { // XXX - not clear if/how this device gets cleaned up - we never disconnect from the account! // And this is messy - I think it reflects that the public device api should be improved? - let device = match cli.account.get_devices(false)?.into_iter().find(|d| d.is_current_device) { + let device = match cli + .account + .get_devices(false)? + .into_iter() + .find(|d| d.is_current_device) + { Some(d) => d, None => { - cli.account.initialize_device(device_name, DeviceType::Desktop, vec![])?; - cli.account.get_devices(true)?.into_iter().find(|d| d.is_current_device).ok_or_else(|| anyhow::Error::msg("can't find new device"))? + cli.account + .initialize_device(device_name, DeviceType::Desktop, vec![])?; + cli.account + .get_devices(true)? + .into_iter() + .find(|d| d.is_current_device) + .ok_or_else(|| anyhow::Error::msg("can't find new device"))? } }; + let key = create_key().unwrap(); + let encdec = Arc::new(ManagedEncryptorDecryptor::new(Arc::new(StaticKeyManager::new(key.clone())))); + Ok(Self { cli, device, autofill_store: Arc::new(AutofillStore::new_shared_memory("sync-test")?), - logins_store: Arc::new(LoginStore::new_in_memory()?), + logins_store: Arc::new(LoginStore::new_in_memory(encdec)?), tabs_store: Arc::new(TabsStore::new_with_mem_path("sync-test-tabs")), sync_manager: SyncManager::new(), persisted_state: None, @@ -104,7 +121,7 @@ impl TestClient { pub fn fully_reset_local_db(&mut self) -> Result<()> { // Not great... self.autofill_store = Arc::new(AutofillStore::new_shared_memory("sync-test")?); - self.logins_store = Arc::new(LoginStore::new_in_memory()?); + self.logins_store = Arc::new(LoginStore::new_in_memory(self.logins_store.encdec.clone())?); self.tabs_store = Arc::new(TabsStore::new_with_mem_path("sync-test-tabs")); Ok(()) } @@ -135,10 +152,12 @@ pub struct TestUser { impl TestUser { pub fn new(cli: Arc, client_count: usize) -> Result { - let clients = (0..client_count).map(|client_num| { - let name = format!("Testing Device {client_num}"); - TestClient::new(cli.clone(), &name) - }).collect::>()?; + let clients = (0..client_count) + .map(|client_num| { + let name = format!("Testing Device {client_num}"); + TestClient::new(cli.clone(), &name) + }) + .collect::>()?; Ok(Self { clients }) } } @@ -159,7 +178,9 @@ impl FxaConfigUrl { FxaConfigUrl::Stage => FxaConfig::stage(client_id, redirect), FxaConfigUrl::Release => FxaConfig::release(client_id, redirect), FxaConfigUrl::Custom(url) => FxaConfig { - server: FxaServer::Custom { url: url.to_string() }, + server: FxaServer::Custom { + url: url.to_string(), + }, client_id: client_id.to_string(), redirect_uri: redirect.to_string(), token_server_url_override: None, diff --git a/testing/sync-test/src/autofill.rs b/testing/sync-test/src/autofill.rs index bdf2de5c7a..1df225f9f0 100644 --- a/testing/sync-test/src/autofill.rs +++ b/testing/sync-test/src/autofill.rs @@ -74,8 +74,7 @@ pub fn add_credit_card( } pub fn scrub_credit_card(s: Arc) -> AutofillResult<()> { - AutofillStore::scrub_encrypted_data(s) - .expect("scrub_encrypted_data() to succeed"); + AutofillStore::scrub_encrypted_data(s).expect("scrub_encrypted_data() to succeed"); Ok(()) } @@ -257,7 +256,7 @@ pub fn get_test_group() -> TestGroup { ), ( "test_autofill_credit_cards_with_scrubbed_cards", - test_autofill_credit_cards_with_scrubbed_cards + test_autofill_credit_cards_with_scrubbed_cards, ), ], ) diff --git a/testing/sync-test/src/logins.rs b/testing/sync-test/src/logins.rs index 2f4c946ef0..1de55c83fe 100644 --- a/testing/sync-test/src/logins.rs +++ b/testing/sync-test/src/logins.rs @@ -4,7 +4,6 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ use crate::auth::TestClient; use crate::testing::TestGroup; use anyhow::Result; -use logins::encryption::{create_key, EncryptorDecryptor}; use logins::{ ApiResult as LoginResult, Login, LoginEntry, LoginFields, LoginStore, SecureLoginFields, }; @@ -26,23 +25,17 @@ pub fn times_used_for_id(s: &LoginStore, id: &str) -> i64 { .times_used } -pub fn add_login(s: &LoginStore, l: LoginEntry, key: &str) -> LoginResult { - let encrypted = s.add(l, key)?; - let fetched = s - .get(&encrypted.guid())? - .expect("Login we just added to exist"); - let encdec = EncryptorDecryptor::new(key).unwrap(); - Ok(fetched.decrypt(&encdec).unwrap()) +pub fn add_login(s: &LoginStore, l: LoginEntry) -> LoginResult { + let login = s.add(l)?; + let fetched = s.get(&login.guid())?.expect("Login we just added to exist"); + Ok(fetched) } -pub fn verify_login(s: &LoginStore, l: &Login, key: &str) { - let encdec = EncryptorDecryptor::new(key).unwrap(); +pub fn verify_login(s: &LoginStore, l: &Login) { let equivalent = s .get(&l.guid()) .expect("get() to succeed") - .expect("Expected login to be present") - .decrypt(&encdec) - .expect("should decrypt"); + .expect("Expected login to be present"); assert_logins_equiv(&equivalent, l); } @@ -58,35 +51,27 @@ pub fn verify_missing_login(s: &LoginStore, id: &str) { pub fn update_login( s: &LoginStore, id: &str, - key: &str, mut callback: F, ) -> LoginResult { - let encdec = EncryptorDecryptor::new(key).unwrap(); - let encrypted = s.get(id)?.expect("No such login!"); - let mut login = encrypted.decrypt(&encdec).unwrap(); + let mut login = s.get(id)?.expect("No such login!"); callback(&mut login); let to_update = LoginEntry { fields: login.fields, sec_fields: login.sec_fields, }; - s.update(id, to_update, key)?; - Ok(s.get(id)? - .expect("Just updated this") - .decrypt(&encdec) - .unwrap()) + s.update(id, to_update)?; + Ok(s.get(id)?.expect("Just updated this")) } -pub fn touch_login(s: &LoginStore, id: &str, times: usize, key: &str) -> LoginResult { +pub fn touch_login(s: &LoginStore, id: &str, times: usize) -> LoginResult { for _ in 0..times { s.touch(id)?; } - let encdec = EncryptorDecryptor::new(key).unwrap(); - Ok(s.get(id)?.unwrap().decrypt(&encdec).unwrap()) + Ok(s.get(id)?.unwrap()) } -pub fn sync_logins(client: &mut TestClient, key: &str) -> Result<()> { - let mut local_encryption_keys = HashMap::new(); - local_encryption_keys.insert("passwords".to_string(), key.to_string()); +pub fn sync_logins(client: &mut TestClient) -> Result<()> { + let local_encryption_keys = HashMap::new(); client.sync(&["passwords".to_string()], local_encryption_keys) } @@ -95,8 +80,6 @@ pub fn sync_logins(client: &mut TestClient, key: &str) -> Result<()> { fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { log::info!("Add some logins to client0"); - let key = create_key().unwrap(); - let l0id = add_login( &c0.logins_store, LoginEntry { @@ -112,12 +95,11 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { password: "hunter2".into(), }, }, - &key, ) .expect("add l0") .guid(); - let login0_c0 = touch_login(&c0.logins_store, &l0id, 2, &key).expect("touch0 c0"); + let login0_c0 = touch_login(&c0.logins_store, &l0id, 2).expect("touch0 c0"); assert_eq!(login0_c0.record.times_used, 3); let login1_c0 = add_login( @@ -133,25 +115,24 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { password: "sekret".into(), }, }, - &key, ) .expect("add l1"); let l1id = login1_c0.guid(); log::info!("Syncing client0"); - sync_logins(c0, &key).expect("c0 sync to work"); + sync_logins(c0).expect("c0 sync to work"); // Should be the same after syncing. - verify_login(&c0.logins_store, &login0_c0, &key); - verify_login(&c0.logins_store, &login1_c0, &key); + verify_login(&c0.logins_store, &login0_c0); + verify_login(&c0.logins_store, &login1_c0); log::info!("Syncing client1"); - sync_logins(c1, &key).expect("c1 sync to work"); + sync_logins(c1).expect("c1 sync to work"); log::info!("Check state"); - verify_login(&c1.logins_store, &login0_c0, &key); - verify_login(&c1.logins_store, &login1_c0, &key); + verify_login(&c1.logins_store, &login0_c0); + verify_login(&c1.logins_store, &login1_c0); assert_eq!( times_used_for_id(&c1.logins_store, &l0id), @@ -162,31 +143,31 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { log::info!("Update logins"); // Change login0 on both - update_login(&c1.logins_store, &l0id, &key, |l| { + update_login(&c1.logins_store, &l0id, |l| { l.sec_fields.password = "testtesttest".into(); }) .unwrap(); - let login0_c0 = update_login(&c0.logins_store, &l0id, &key, |l| { + let login0_c0 = update_login(&c0.logins_store, &l0id, |l| { l.fields.username_field = "users_name".into(); }) .unwrap(); // and login1 on remote. - let login1_c1 = update_login(&c1.logins_store, &l1id, &key, |l| { + let login1_c1 = update_login(&c1.logins_store, &l1id, |l| { l.sec_fields.username = "less_cool_username".into(); }) .unwrap(); log::info!("Sync again"); - sync_logins(c1, &key).expect("c1 sync 2"); - sync_logins(c0, &key).expect("c0 sync 2"); + sync_logins(c1).expect("c1 sync 2"); + sync_logins(c0).expect("c0 sync 2"); log::info!("Check state again"); // Ensure the remotely changed password change made it through - verify_login(&c0.logins_store, &login1_c1, &key); + verify_login(&c0.logins_store, &login1_c1); // And that the conflicting one did too. verify_login( @@ -202,7 +183,6 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { }, record: login0_c0.record, }, - &key, ); assert_eq!( @@ -220,7 +200,6 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { log::info!("Add some logins to client0"); - let key = create_key().unwrap(); let login0 = add_login( &c0.logins_store, @@ -237,7 +216,6 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { password: "hunter2".into(), }, }, - &key, ) .expect("add l0"); let l0id = login0.guid(); @@ -255,7 +233,6 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { password: "sekret".into(), }, }, - &key, ) .expect("add l1"); let l1id = login1.guid(); @@ -273,7 +250,6 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { password: "123454321".into(), }, }, - &key, ) .expect("add l2"); let l2id = login2.guid(); @@ -291,29 +267,28 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { password: "aaaaa".into(), }, }, - &key, ) .expect("add l3"); let l3id = login3.guid(); log::info!("Syncing client0"); - sync_logins(c0, &key).expect("c0 sync to work"); + sync_logins(c0).expect("c0 sync to work"); // Should be the same after syncing. - verify_login(&c0.logins_store, &login0, &key); - verify_login(&c0.logins_store, &login1, &key); - verify_login(&c0.logins_store, &login2, &key); - verify_login(&c0.logins_store, &login3, &key); + verify_login(&c0.logins_store, &login0); + verify_login(&c0.logins_store, &login1); + verify_login(&c0.logins_store, &login2); + verify_login(&c0.logins_store, &login3); log::info!("Syncing client1"); - sync_logins(c1, &key).expect("c1 sync to work"); + sync_logins(c1).expect("c1 sync to work"); log::info!("Check state"); - verify_login(&c1.logins_store, &login0, &key); - verify_login(&c1.logins_store, &login1, &key); - verify_login(&c1.logins_store, &login2, &key); - verify_login(&c1.logins_store, &login3, &key); + verify_login(&c1.logins_store, &login0); + verify_login(&c1.logins_store, &login1); + verify_login(&c1.logins_store, &login2); + verify_login(&c1.logins_store, &login3); // The 4 logins are for the for possible scenarios. All of them should result in the record // being deleted. @@ -335,7 +310,7 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { // case 3a. c0 modifies record (c1 will delete it after c0 syncs so the timestamps line up) log::info!("Updating {} on c0", l2id); - let login2_new = update_login(&c0.logins_store, &l2id, &key, |l| { + let login2_new = update_login(&c0.logins_store, &l2id, |l| { l.sec_fields.username = "foobar".into(); }) .unwrap(); @@ -345,30 +320,30 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { // Sync c1 log::info!("Syncing c1"); - sync_logins(c1, &key).expect("c1 sync to work"); + sync_logins(c1).expect("c1 sync to work"); log::info!("Checking c1 state after sync"); verify_missing_login(&c1.logins_store, &l0id); verify_missing_login(&c1.logins_store, &l1id); - verify_login(&c1.logins_store, &login2, &key); + verify_login(&c1.logins_store, &login2); verify_missing_login(&c1.logins_store, &l3id); log::info!("Update {} on c0", l3id); // 4b - update_login(&c0.logins_store, &l3id, &key, |l| { + update_login(&c0.logins_store, &l3id, |l| { l.sec_fields.password = "quux".into(); }) .unwrap(); // Sync c0 log::info!("Syncing c0"); - sync_logins(c0, &key).expect("c0 sync to work"); + sync_logins(c0).expect("c0 sync to work"); log::info!("Checking c0 state after sync"); verify_missing_login(&c0.logins_store, &l0id); verify_missing_login(&c0.logins_store, &l1id); - verify_login(&c0.logins_store, &login2_new, &key); + verify_login(&c0.logins_store, &login2_new); verify_missing_login(&c0.logins_store, &l3id); log::info!("Delete {} on c1", l2id); @@ -376,14 +351,14 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { assert!(c1.logins_store.delete(&l2id).expect("Delete should work")); log::info!("Syncing c1"); - sync_logins(c1, &key).expect("c1 sync to work"); + sync_logins(c1).expect("c1 sync to work"); log::info!("{} should stay dead", l2id); // Ensure we didn't revive it. verify_missing_login(&c1.logins_store, &l2id); log::info!("Syncing c0"); - sync_logins(c0, &key).expect("c0 sync to work"); + sync_logins(c0).expect("c0 sync to work"); log::info!("Should delete {}", l2id); verify_missing_login(&c0.logins_store, &l2id); } diff --git a/testing/sync-test/src/main.rs b/testing/sync-test/src/main.rs index 5d93fb1325..aaa14fa98a 100644 --- a/testing/sync-test/src/main.rs +++ b/testing/sync-test/src/main.rs @@ -5,11 +5,10 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ #![warn(rust_2018_idioms)] use cli_support::fxa_creds::{get_cli_fxa, get_default_fxa_config, SYNC_SCOPE}; -use std::{collections::HashSet, process}; use std::sync::Arc; +use std::{collections::HashSet, process}; use structopt::StructOpt; - mod auth; mod autofill; mod logins; @@ -78,7 +77,8 @@ pub fn run_test_group(opts: &Opts, group: TestGroup) { } let cfg = get_default_fxa_config(); - let cli_fxa = get_cli_fxa(cfg, &opts.credential_file, &[SYNC_SCOPE]).expect("can't initialize cli"); + let cli_fxa = + get_cli_fxa(cfg, &opts.credential_file, &[SYNC_SCOPE]).expect("can't initialize cli"); let acct = Arc::new(cli_fxa); let mut user = TestUser::new(acct, 2).expect("Failed to get test user."); diff --git a/testing/sync-test/src/sync15.rs b/testing/sync-test/src/sync15.rs index a920d439f8..3692639b80 100644 --- a/testing/sync-test/src/sync15.rs +++ b/testing/sync-test/src/sync15.rs @@ -16,9 +16,7 @@ use std::cell::{Cell, RefCell}; use std::mem; use sync15::bso::{IncomingBso, OutgoingBso}; use sync15::client::{sync_multiple, MemoryCachedState}; -use sync15::engine::{ - CollectionRequest, EngineSyncAssociation, SyncEngine, -}; +use sync15::engine::{CollectionRequest, EngineSyncAssociation, SyncEngine}; use sync15::{telemetry, ServerTimestamp}; use sync_guid::Guid; @@ -87,10 +85,10 @@ impl SyncEngine for TestEngine { // the RefCell. let temp: Vec = mem::take(&mut *self.test_records.borrow_mut()); - Ok(temp.into_iter() - .map(OutgoingBso::from_content_with_id) - .collect::>()? - ) + Ok(temp + .into_iter() + .map(OutgoingBso::from_content_with_id) + .collect::>()?) } fn set_uploaded(