Skip to content

Commit

Permalink
Refactor FXIOS-7619 [v121] update key retrieval logic (#16973)
Browse files Browse the repository at this point in the history 
(cherry picked from commit 88e6959)
  • Loading branch information
@lougeniaC64 @mergify
lougeniaC64 authored and mergify[bot] committed Nov 22, 2023
1 parent d004679 commit a431063
Show file tree
Hide file tree
Showing 6 changed files with 259 additions and 82 deletions.
73 changes: 44 additions & 29 deletions Providers/RustSyncManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,43 +332,58 @@ public class RustSyncManager: NSObject, SyncManager {
var rustEngines: [String] = []
var registeredPlaces = false

for engine in engines.filter({ syncManagerAPI.rustTogglableEngines.contains($0) }) {
switch engine {
case .tabs:
profile?.tabs.registerWithSyncManager()
rustEngines.append(engine.rawValue)
case .passwords:
profile?.logins.registerWithSyncManager()
if let key = try? profile?.logins.getStoredKey() {
localEncryptionKeys[engine.rawValue] = key
let registerEngines: (String?) -> Void = { loginKey in
for engine in engines.filter({ self.syncManagerAPI.rustTogglableEngines.contains($0) }) {
switch engine {
case .tabs:
self.profile?.tabs.registerWithSyncManager()
rustEngines.append(engine.rawValue)
} else {
logger.log("Login encryption key could not be retrieved for syncing",
level: .warning,
category: .sync)
}
case .creditcards:
if self.creditCardAutofillEnabled {
profile?.autofill.registerWithSyncManager()
if let key = try? profile?.autofill.getStoredKey() {
case .passwords:
self.profile?.logins.registerWithSyncManager()
if let key = loginKey {
localEncryptionKeys[engine.rawValue] = key
rustEngines.append(engine.rawValue)
} else {
logger.log("Credit card encryption key could not be retrieved for syncing",
level: .warning,
category: .sync)
}
case .creditcards:
if self.creditCardAutofillEnabled {
self.profile?.autofill.registerWithSyncManager()
if let key = try? self.profile?.autofill.getStoredKey() {
localEncryptionKeys[engine.rawValue] = key
rustEngines.append(engine.rawValue)
} else {
self.logger.log("Credit card encryption key could not be retrieved for syncing",
level: .warning,
category: .sync)
}
}
case .bookmarks, .history:
if !registeredPlaces {
self.profile?.places.registerWithSyncManager()
registeredPlaces = true
}
rustEngines.append(engine.rawValue)
}
case .bookmarks, .history:
if !registeredPlaces {
profile?.places.registerWithSyncManager()
registeredPlaces = true
}
rustEngines.append(engine.rawValue)
}
}

completion((rustEngines, localEncryptionKeys))
let shouldSyncLogins = engines.contains(RustSyncManagerAPI.TogglableEngine.passwords)
if shouldSyncLogins {
profile?.logins.getStoredKey { result in
switch result {
case .success(let key):
registerEngines(key)
case .failure(let err):
self.logger.log("Login encryption key could not be retrieved for syncing: \(err)",
level: .warning,
category: .sync)
registerEngines(nil)
}
completion((rustEngines, localEncryptionKeys))
}
} else {
registerEngines(nil)
completion((rustEngines, localEncryptionKeys))
}
}

private func doSync(params: SyncParams, completion: @escaping (SyncResult) -> Void) {
Expand Down
143 changes: 93 additions & 50 deletions Storage/Rust/RustLogins.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,9 @@ public extension LoginEntry {
}

public enum LoginEncryptionKeyError: Error {
case illegalState
case noKeyCreated
case illegalState
case dbRecordCountVerificationError(String)
}

public class RustLoginEncryptionKeys {
Expand Down Expand Up @@ -691,15 +692,20 @@ public class RustLogins {
return
}

do {
let key = try self.getStoredKey()
let id = try self.storage?.add(login: login, encryptionKey: key).record.id
deferred.fill(Maybe(success: id!))
} catch let err as NSError {
deferred.fill(Maybe(failure: err))
self.getStoredKey { result in
switch result {
case .success(let key):
do {
let id = try self.storage?.add(login: login, encryptionKey: key).record.id
deferred.fill(Maybe(success: id!))
} catch let err as NSError {
deferred.fill(Maybe(failure: err))
}
case .failure(let err):
deferred.fill(Maybe(failure: err))
}
}
}

return deferred
}

Expand Down Expand Up @@ -734,12 +740,18 @@ public class RustLogins {
return
}

do {
let key = try self.getStoredKey()
_ = try self.storage?.update(id: id, login: login, encryptionKey: key)
deferred.fill(Maybe(success: ()))
} catch let err as NSError {
deferred.fill(Maybe(failure: err))
self.getStoredKey { result in
switch result {
case .success(let key):
do {
_ = try self.storage?.update(id: id, login: login, encryptionKey: key)
deferred.fill(Maybe(success: ()))
} catch let err as NSError {
deferred.fill(Maybe(failure: err))
}
case .failure(let err):
deferred.fill(Maybe(failure: err))
}
}
}

Expand Down Expand Up @@ -815,75 +827,106 @@ public class RustLogins {
keychain.removeObject(forKey: rustKeys.loginsSaltKeychainKey, withAccessibility: .afterFirstUnlock)
}

public func getStoredKey() throws -> String {
private func resetLoginsAndKey(rustKeys: RustLoginEncryptionKeys,
completion: @escaping (Result<String, NSError>) -> Void) {
self.wipeLocalEngine().upon { result in
guard result.isSuccess else {
completion(.failure(result.failureValue! as NSError))
return
}

do {
let key = try rustKeys.createAndStoreKey()
completion(.success(key))
} catch let error as NSError {
self.logger.log("Error creating logins encryption key",
level: .warning,
category: .storage,
description: error.localizedDescription)
completion(.failure(error))
}
}
}

public func getStoredKey(completion: @escaping (Result<String, NSError>) -> Void) {
let rustKeys = RustLoginEncryptionKeys()
let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)

switch(key, encryptedCanaryPhrase) {
case (.some(key), .some(encryptedCanaryPhrase)):
// We expected the key to be present, and it is.
do {
let canaryIsValid = try checkCanary(
canary: encryptedCanaryPhrase!,
text: rustKeys.canaryPhrase,
encryptionKey: key!)

if canaryIsValid {
return key!
completion(.success(key!))
} else {
logger.log("Logins key was corrupted, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record()
_ = self.wipeLocalEngine()

return try rustKeys.createAndStoreKey()
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
}
} catch let error as NSError {
logger.log("Error retrieving logins encryption key",
logger.log("Error validating logins encryption key",
level: .warning,
category: .storage,
description: error.localizedDescription)
completion(.failure(error))
}
case (.some(key), .none):
// The key is present, but we didn't expect it to be there.
do {
logger.log("Logins key lost due to storage malfunction, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.other.record()
_ = self.wipeLocalEngine()

return try rustKeys.createAndStoreKey()
} catch let error as NSError {
throw error
}
logger.log("Logins key lost due to storage malfunction, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.other.record()
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
case (.none, .some(encryptedCanaryPhrase)):
// We expected the key to be present, but it's gone missing on us.
do {
logger.log("Logins key lost, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.lost.record()
_ = self.wipeLocalEngine()

return try rustKeys.createAndStoreKey()
} catch let error as NSError {
throw error
}
logger.log("Logins key lost, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.lost.record()
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
case (.none, .none):
// We didn't expect the key to be present, and it's not (which is the case for first-time calls).
do {
return try rustKeys.createAndStoreKey()
} catch let error as NSError {
throw error
// We didn't expect the key to be present, which either means this is a first-time
// call or the key data has been cleared from the keychain.

self.hasSyncedLogins().upon { result in
guard result.failureValue == nil else {
completion(.failure(result.failureValue! as NSError))
return
}

guard let hasLogins = result.successValue else {
let msg = "Failed to verify logins count before attempting to reset key"
completion(.failure(LoginEncryptionKeyError.dbRecordCountVerificationError(msg) as NSError))
return
}

if hasLogins {
// Since the key data isn't present and we have login records in
// the database, we both clear the databbase and the reset the key.
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
} else {
// There are no records in the database so we don't need to wipe any
// existing login records. We just need to create a new key.
do {
let key = try rustKeys.createAndStoreKey()
completion(.success(key))
} catch let error as NSError {
completion(.failure(error))
}
}
}
default:
// If none of the above cases apply, we're in a state that shouldn't be possible but is disallowed nonetheless
throw LoginEncryptionKeyError.illegalState
completion(.failure(LoginEncryptionKeyError.illegalState as NSError))
}

// This must be declared again for Swift's sake even though the above switch statement handles all cases
throw LoginEncryptionKeyError.illegalState
}
}
11 changes: 8 additions & 3 deletions Tests/ClientTests/PasswordManagerViewModelTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,19 @@ class PasswordManagerViewModelTests: XCTestCase {
"username": "username\(i)",
"password": "password\(i)"
])
let addResult = self.viewModel.profile.logins.addLogin(login: login)
XCTAssertTrue(addResult.value.isSuccess)
XCTAssertNotNil(addResult.value.successValue)
let addExp = expectation(description: "\(#function)\(#line)")
self.viewModel.profile.logins.addLogin(login: login).upon { addResult in
XCTAssertTrue(addResult.isSuccess)
XCTAssertNotNil(addResult.successValue)
addExp.fulfill()
}
}

let logins = self.viewModel.profile.logins.listLogins().value
XCTAssertTrue(logins.isSuccess)
XCTAssertNotNil(logins.successValue)

waitForExpectations(timeout: 10, handler: nil)
}

func testQueryLogins() {
Expand Down
1 change: 1 addition & 0 deletions Tests/ClientTests/RustSyncManagerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class RustSyncManagerTests: XCTestCase {

func testGetEnginesAndKeys() {
let engines: [RustSyncManagerAPI.TogglableEngine] = [.bookmarks, .creditcards, .history, .passwords, .tabs]

rustSyncManager.getEnginesAndKeys(engines: engines) { (engines, keys) in
XCTAssertEqual(engines.count, 5)

Expand Down
Loading

0 comments on commit a431063

Please sign in to comment.