Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FXIOS-8297 [v124] Logins Key Retrieval Logic #18401

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Refactor FXIOS-8297 [v124] Logins Key Retrieval Logic #18401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96ce580
Re-merge logins key logic refactor
lougeniaC64 Jan 26, 2024
ece499c
Refactored RustSyncManager getEnginesAndKeys function
lougeniaC64 Jan 31, 2024
1766bbf
Removed unused code from RustLoginsTests.swift
lougeniaC64 Feb 1, 2024
d12483b
Fixed failing tests
lougeniaC64 Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 53 additions & 44 deletions firefox-ios/Providers/RustSyncManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,50 +330,59 @@ public class RustSyncManager: NSObject, SyncManager {

func getEnginesAndKeys(engines: [RustSyncManagerAPI.TogglableEngine],
completion: @escaping (([String], [String: String])) -> Void) {
var localEncryptionKeys: [String: String] = [:]
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
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() {
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 .addresses:
profile?.autofill.registerWithSyncManager()
rustEngines.append(engine.rawValue)
case .bookmarks, .history:
if !registeredPlaces {
profile?.places.registerWithSyncManager()
registeredPlaces = true
}
rustEngines.append(engine.rawValue)
}
}

completion((rustEngines, localEncryptionKeys))
var localEncryptionKeys: [String: String] = [:]
var rustEngines: [String] = []
var registeredPlaces = false

profile?.logins.getStoredKey { loginsResult in
var loginsKey: String?

switch loginsResult {
case .success(let key):
loginsKey = key
case .failure(let err):
self.logger.log("Login encryption key could not be retrieved for syncing: \(err)",
level: .warning,
category: .sync)
}

for engine in engines.filter({ self.syncManagerAPI.rustTogglableEngines.contains($0) }) {
switch engine {
case .tabs:
self.profile?.tabs.registerWithSyncManager()
rustEngines.append(engine.rawValue)
case .passwords:
self.profile?.logins.registerWithSyncManager()
if let key = loginsKey {
localEncryptionKeys[engine.rawValue] = key
rustEngines.append(engine.rawValue)
}
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 .addresses:
self.profile?.autofill.registerWithSyncManager()
rustEngines.append(engine.rawValue)
case .bookmarks, .history:
if !registeredPlaces {
self.profile?.places.registerWithSyncManager()
registeredPlaces = true
}
rustEngines.append(engine.rawValue)
}
}

completion((rustEngines, localEncryptionKeys))
}
}

private func doSync(params: SyncParams, completion: @escaping (SyncResult) -> Void) {
Expand Down
220 changes: 134 additions & 86 deletions firefox-ios/Storage/Rust/RustLogins.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,9 @@ public extension LoginEntry {
}

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

public class RustLoginEncryptionKeys {
Expand All @@ -416,17 +417,14 @@ public class RustLoginEncryptionKeys {
let secret = try createKey()
let canary = try createCanary(text: canaryPhrase, encryptionKey: secret)

keychain.set(
secret,
forKey: loginPerFieldKeychainKey,
withAccessibility: MZKeychainItemAccessibility.afterFirstUnlock
)
keychain.set(
canary,
forKey: canaryPhraseKey,
withAccessibility: MZKeychainItemAccessibility.afterFirstUnlock
)

DispatchQueue.global(qos: .background).sync {
self.keychain.set(secret,
forKey: self.loginPerFieldKeychainKey,
withAccessibility: MZKeychainItemAccessibility.afterFirstUnlock)
self.keychain.set(canary,
forKey: self.canaryPhraseKey,
withAccessibility: MZKeychainItemAccessibility.afterFirstUnlock)
}
return secret
} catch let err as NSError {
if let loginsStoreError = err as? LoginsStoreError {
Expand Down Expand Up @@ -728,12 +726,18 @@ 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))
}
}
}

Expand Down Expand Up @@ -771,12 +775,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 @@ -835,76 +845,114 @@ public class RustLogins {
}
}

public func getStoredKey() throws -> String {
let rustKeys = RustLoginEncryptionKeys()
let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)
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
}

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!
} else {
logger.log("Logins key was corrupted, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record()
_ = self.wipeLocalEngine()

return try rustKeys.createAndStoreKey()
}
let key = try rustKeys.createAndStoreKey()
completion(.success(key))
} catch let error as NSError {
logger.log("Error retrieving logins encryption key",
level: .warning,
category: .storage,
description: error.localizedDescription)
self.logger.log("Error creating 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)
}
}

private func getKeychainData(rustKeys: RustLoginEncryptionKeys, completion: @escaping (String?, String?) -> Void) {
DispatchQueue.global(qos: .background).sync {
let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)
completion(key, encryptedCanaryPhrase)
}
}

public func getStoredKey(completion: @escaping (Result<String, NSError>) -> Void) {
let rustKeys = RustLoginEncryptionKeys()
getKeychainData(rustKeys: rustKeys) { (key, encryptedCanaryPhrase) in
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 {
completion(.success(key!))
} else {
self.logger.log("Logins key was corrupted, new one generated",
level: .warning,
category: .storage)
GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record()
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
}
} catch let error as NSError {
self.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.

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

return try rustKeys.createAndStoreKey()
} catch let error as NSError {
throw error
}
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)
self.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
}
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
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
case (.none, .none):
// 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.
GleanMetrics.LoginsStoreKeyRegeneration.keychainDataLost.record()
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
completion(.failure(LoginEncryptionKeyError.illegalState as NSError))
}
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
}

// 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 firefox-ios/firefox-ios-tests/Tests/ClientTests/PasswordManagerViewModelTests.swift
View file
Open in desktop
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.0, handler: nil)
}

func testQueryLogins() {
Expand Down
Loading