Skip to content

Commit 3c37a69

Browse files
issammaniissammani
committed
fix: address feedback
1 parent 2cf0925 commit 3c37a69

File tree

1 file changed

+115
-89
lines changed

1 file changed

+115
-89
lines changed

‎firefox-ios/Storage/Rust/RustLogins.swift‎

Lines changed: 115 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -828,104 +828,130 @@ public class RustLogins: LoginsProtocol, KeyManager {
828828
}
829829

830830
private func getKeychainData(rustKeys: RustLoginEncryptionKeys) -> (String?, String?) {
831-
let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
832-
let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)
833-
return (key, encryptedCanaryPhrase)
831+
var keychainData: (String?, String?) = (nil, nil)
832+
833+
DispatchQueue.global(qos: .background).sync {
834+
let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
835+
let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)
836+
keychainData = (key, encryptedCanaryPhrase)
837+
}
838+
839+
return keychainData
834840
}
835841

836842
public func getStoredKey(completion: @escaping (Result<String, NSError>) -> Void) {
837-
let rustKeys = RustLoginEncryptionKeys()
838-
let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)
839-
switch(key, encryptedCanaryPhrase) {
840-
case (.some(key), .some(encryptedCanaryPhrase)):
841-
// We expected the key to be present, and it is.
842-
do {
843-
let canaryIsValid = try checkCanary(canary: encryptedCanaryPhrase!,
844-
text: rustKeys.canaryPhrase,
845-
encryptionKey: key!)
846-
if canaryIsValid {
847-
completion(.success(key!))
848-
} else {
849-
self.logger.log("Logins key was corrupted, new one generated",
850-
level: .warning,
851-
category: .storage)
852-
GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record()
853-
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
854-
}
855-
} catch let error as NSError {
856-
self.logger.log("Error validating logins encryption key",
843+
let rustKeys = RustLoginEncryptionKeys()
844+
let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)
845+
switch(key, encryptedCanaryPhrase) {
846+
case (.some(key), .some(encryptedCanaryPhrase)):
847+
// We expected the key to be present, and it is.
848+
do {
849+
let canaryIsValid = try checkCanary(canary: encryptedCanaryPhrase!,
850+
text: rustKeys.canaryPhrase,
851+
encryptionKey: key!)
852+
if canaryIsValid {
853+
completion(.success(key!))
854+
} else {
855+
self.logger.log("Logins key was corrupted, new one generated",
857856
level: .warning,
858-
category: .storage,
859-
description: error.localizedDescription)
860-
completion(.failure(error))
857+
category: .storage)
858+
GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record()
859+
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
861860
}
862-
case (.some(key), .none):
863-
// The key is present, but we didn't expect it to be there.
864-
865-
self.logger.log("Logins key lost due to storage malfunction, new one generated",
866-
level: .warning,
867-
category: .storage)
868-
GleanMetrics.LoginsStoreKeyRegeneration.other.record()
869-
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
870-
case (.none, .some(encryptedCanaryPhrase)):
871-
// We expected the key to be present, but it's gone missing on us.
872-
873-
self.logger.log("Logins key lost, new one generated",
861+
} catch let error as NSError {
862+
self.logger.log("Error validating logins encryption key",
874863
level: .warning,
875-
category: .storage)
876-
GleanMetrics.LoginsStoreKeyRegeneration.lost.record()
877-
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
878-
case (.none, .none):
879-
// We didn't expect the key to be present, which either means this is a first-time
880-
// call or the key data has been cleared from the keychain.
881-
882-
self.hasSyncedLogins().upon { result in
883-
guard result.failureValue == nil else {
884-
completion(.failure(result.failureValue! as NSError))
885-
return
886-
}
864+
category: .storage,
865+
description: error.localizedDescription)
866+
completion(.failure(error))
867+
}
868+
case (.some(key), .none):
869+
// The key is present, but we didn't expect it to be there.
870+
871+
self.logger.log("Logins key lost due to storage malfunction, new one generated",
872+
level: .warning,
873+
category: .storage)
874+
GleanMetrics.LoginsStoreKeyRegeneration.other.record()
875+
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
876+
case (.none, .some(encryptedCanaryPhrase)):
877+
// We expected the key to be present, but it's gone missing on us.
878+
879+
self.logger.log("Logins key lost, new one generated",
880+
level: .warning,
881+
category: .storage)
882+
GleanMetrics.LoginsStoreKeyRegeneration.lost.record()
883+
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
884+
case (.none, .none):
885+
// We didn't expect the key to be present, which either means this is a first-time
886+
// call or the key data has been cleared from the keychain.
887+
888+
self.hasSyncedLogins().upon { result in
889+
guard result.failureValue == nil else {
890+
completion(.failure(result.failureValue! as NSError))
891+
return
892+
}
887893

888-
guard let hasLogins = result.successValue else {
889-
let msg = "Failed to verify logins count before attempting to reset key"
890-
completion(.failure(LoginEncryptionKeyError.dbRecordCountVerificationError(msg) as NSError))
891-
return
892-
}
894+
guard let hasLogins = result.successValue else {
895+
let msg = "Failed to verify logins count before attempting to reset key"
896+
completion(.failure(LoginEncryptionKeyError.dbRecordCountVerificationError(msg) as NSError))
897+
return
898+
}
893899

894-
if hasLogins {
895-
// Since the key data isn't present and we have login records in
896-
// the database, we both clear the database and reset the key.
897-
GleanMetrics.LoginsStoreKeyRegeneration.keychainDataLost.record()
898-
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
899-
} else {
900-
// There are no records in the database so we don't need to wipe any
901-
// existing login records. We just need to create a new key.
902-
do {
903-
let key = try rustKeys.createAndStoreKey()
904-
completion(.success(key))
905-
} catch let error as NSError {
906-
completion(.failure(error))
907-
}
900+
if hasLogins {
901+
// Since the key data isn't present and we have login records in
902+
// the database, we both clear the database and reset the key.
903+
GleanMetrics.LoginsStoreKeyRegeneration.keychainDataLost.record()
904+
self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion)
905+
} else {
906+
// There are no records in the database so we don't need to wipe any
907+
// existing login records. We just need to create a new key.
908+
do {
909+
let key = try rustKeys.createAndStoreKey()
910+
completion(.success(key))
911+
} catch let error as NSError {
912+
completion(.failure(error))
908913
}
909914
}
910-
default:
911-
// If none of the above cases apply, we're in a state that shouldn't be
912-
// possible but is disallowed nonetheless
913-
completion(.failure(LoginEncryptionKeyError.illegalState as NSError))
914915
}
915-
}
916-
917-
// MARK: - KeyManager
918-
// This method will be called internally by rust when it needs to encrypt/decrypt logins.
919-
// NOTE: Since this is called internally by rust and each CRUD operation will acquire a mutex lock on the db
920-
// before doing anything we must make sure that getStoredKey is called before doing any CRUD operation in swift.
921-
public func getKey() throws -> Data {
922-
let rustKeys = RustLoginEncryptionKeys()
923-
let (key, _) = getKeychainData(rustKeys: rustKeys)
924-
925-
guard let keyData = key?.data(using: .utf8) else {
926-
throw LoginsStoreError.MissingKey
927-
}
928-
929-
return keyData
930-
}
916+
default:
917+
// If none of the above cases apply, we're in a state that shouldn't be
918+
// possible but is disallowed nonetheless
919+
completion(.failure(LoginEncryptionKeyError.illegalState as NSError))
920+
}
921+
}
922+
923+
// MARK: - KeyManager
924+
925+
/**
926+
* Retrieves the encryption key used by the Rust logins component for encrypting and decrypting login data.
927+
*
928+
* This method is invoked internally by the Rust component whenever encryption or decryption is required.
929+
*
930+
* **Note on thread safety:**
931+
* Each CRUD operation on the db acquires a mutex lock on the database to ensure thread safety.
932+
* Therefore, it's crucial to call `getStoredKey` before performing any such CRUD operations in Swift that call into internal Rust methods.
933+
* `addLogin` and `updateLogin` are good examples of that.
934+
*
935+
* **Usage Example:**
936+
* ```
937+
* public func methodThatRequiresEncDec() {
938+
* ...
939+
* self.getStoredKey {
940+
* ...
941+
* self.storage.someRustMethod()
942+
* ...
943+
* }
944+
* }
945+
* ```
946+
*/
947+
public func getKey() throws -> Data {
948+
let rustKeys = RustLoginEncryptionKeys()
949+
let (key, _) = getKeychainData(rustKeys: rustKeys)
950+
951+
guard let keyData = key?.data(using: .utf8) else {
952+
throw LoginsStoreError.MissingKey
953+
}
954+
955+
return keyData
956+
}
931957
}

0 commit comments

Comments
 (0)