diff --git a/Signal/Backups/BackupDisablingManager.swift b/Signal/Backups/BackupDisablingManager.swift index ebb3ef5b90..cd06ad21c3 100644 --- a/Signal/Backups/BackupDisablingManager.swift +++ b/Signal/Backups/BackupDisablingManager.swift @@ -225,7 +225,6 @@ final class BackupDisablingManager { accountEntropyPoolManager.setAccountEntropyPool( newAccountEntropyPool: try! AccountEntropyPool(key: aepBeingRotatedString), - disablePIN: false, tx: tx, ) } diff --git a/Signal/Backups/BackupSettingsViewController.swift b/Signal/Backups/BackupSettingsViewController.swift index 34c41a75cf..8bd04d39c9 100644 --- a/Signal/Backups/BackupSettingsViewController.swift +++ b/Signal/Backups/BackupSettingsViewController.swift @@ -1390,7 +1390,6 @@ class BackupSettingsViewController: accountEntropyPoolManager.setAccountEntropyPool( newAccountEntropyPool: newCandidateAEP, - disablePIN: false, tx: tx, ) case .disabling, .free, .paid, .paidExpiringSoon, .paidAsTester: diff --git a/Signal/Provisioning/ProvisioningCoordinatorImpl.swift b/Signal/Provisioning/ProvisioningCoordinatorImpl.swift index ffcc6ad8f1..71d20cc3bb 100644 --- a/Signal/Provisioning/ProvisioningCoordinatorImpl.swift +++ b/Signal/Provisioning/ProvisioningCoordinatorImpl.swift @@ -402,7 +402,6 @@ class ProvisioningCoordinatorImpl: ProvisioningCoordinator { userProfileWriter: .linking, transaction: tx, ) - self.svr.clearKeys(transaction: tx) // reset to default (false) self.receiptManager.setAreReadReceiptsEnabled( diff --git a/Signal/Registration/RegistrationCoordinatorImpl.swift b/Signal/Registration/RegistrationCoordinatorImpl.swift index d467b2f802..ca184b59cd 100644 --- a/Signal/Registration/RegistrationCoordinatorImpl.swift +++ b/Signal/Registration/RegistrationCoordinatorImpl.swift @@ -424,7 +424,6 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { case .changingNumber: break case .registering, .reRegistering: - deps.svr.clearKeys(transaction: tx) deps.ows2FAManager.clearLocalPinCode(tx) } } @@ -485,9 +484,6 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { case .changingNumber: break case .registering, .reRegistering: - // Whenever we do this, wipe the keys we've got. - // We don't want to have them and use then implicitly later. - deps.svr.clearKeys(transaction: tx) deps.ows2FAManager.clearLocalPinCode(tx) } } @@ -524,9 +520,6 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { case .changingNumber: break case .registering, .reRegistering: - // Whenever we do this, wipe the keys we've got. - // We don't want to have them and use them implicitly later. - deps.svr.clearKeys(transaction: tx) deps.ows2FAManager.clearLocalPinCode(tx) } } @@ -2043,9 +2036,6 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { // Its possible we tried svr2 and kbs has the right info, or vice versa, but this is all // best effort anyway; just fall back to session-based registration. deps.svrAuthCredentialStore.removeSVR2CredentialsForCurrentUser(tx) - // Clear the SVR master key locally; we failed reglock so we know its wrong - // and useless anyway. - deps.svr.clearKeys(transaction: tx) deps.ows2FAManager.clearLocalPinCode(tx) self.updatePersistedState(tx) { $0.e164WithKnownReglockEnabled = e164 @@ -4040,6 +4030,7 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { try await deps.svr.backupMasterKey( pin: pin, masterKey: masterKey, + force: true, authMethod: authMethod, ) diff --git a/Signal/src/ViewControllers/AppSettings/Account/AdvancedPinSettingsTableViewController.swift b/Signal/src/ViewControllers/AppSettings/Account/AdvancedPinSettingsTableViewController.swift index 7c18787585..fc6c1ef01c 100644 --- a/Signal/src/ViewControllers/AppSettings/Account/AdvancedPinSettingsTableViewController.swift +++ b/Signal/src/ViewControllers/AppSettings/Account/AdvancedPinSettingsTableViewController.swift @@ -145,13 +145,12 @@ class AdvancedPinSettingsTableViewController: OWSTableViewController2 { db.write { tx in Logger.warn("Rotating AEP: disabling PIN!") + ows2FAManager.markDisabled(transaction: tx) + accountEntropyPoolManager.setAccountEntropyPool( newAccountEntropyPool: AccountEntropyPool(), - disablePIN: true, tx: tx, ) - - ows2FAManager.markDisabled(transaction: tx) } self.updateTableContents() diff --git a/Signal/test/Registration/RegistrationCoordinatorTest.swift b/Signal/test/Registration/RegistrationCoordinatorTest.swift index 45901df7cd..9e27d17423 100644 --- a/Signal/test/Registration/RegistrationCoordinatorTest.swift +++ b/Signal/test/Registration/RegistrationCoordinatorTest.swift @@ -407,12 +407,12 @@ public class RegistrationCoordinatorTest { } // We haven't done a SVR backup; that should happen now. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in #expect(pin == Stubs.pinCode) - // We don't have a SVR auth credential, it should use chat server creds. #expect(masterKey.rawData == finalMasterKey.rawData) + #expect(force) + // We don't have a SVR auth credential, it should use chat server creds. #expect(authMethod == .chatServerAuth(expectedAuthedAccount())) - self.svr.hasMasterKey = true return .value(()) } @@ -544,12 +544,12 @@ public class RegistrationCoordinatorTest { }) // We haven't done a SVR backup; that should happen now. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in #expect(pin == Stubs.pinCode) #expect(masterKey.rawData == finalMasterKey.rawData) + #expect(force) // We don't have a SVR auth credential, it should use chat server creds. #expect(authMethod == .chatServerAuth(expectedAuthedAccount())) - self.svr.hasMasterKey = true return .value(()) } @@ -628,11 +628,12 @@ public class RegistrationCoordinatorTest { // Set a PIN on disk. ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + var didClearPinCode = false + ows2FAManagerMock.clearLocalPinCodeMock = { didClearPinCode = true } // Make SVR give us back a reg recovery password. let masterKey = AccountEntropyPool().getMasterKey() await db.awaitableWrite { accountKeyStore.setMasterKey(masterKey, tx: $0) } - svr.hasMasterKey = true // NOTE: We expect to skip opening path steps because // if we have a SVR master key locally, this _must_ be @@ -695,7 +696,7 @@ public class RegistrationCoordinatorTest { ) // Check we have the master key now, to be safe. - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) // Give it the pin code, which should make it try and register. // Now we should expect to be at verification code entry since we already set the phone number. @@ -710,7 +711,7 @@ public class RegistrationCoordinatorTest { // We want to have kept the master key; we failed the reg recovery pw check // but that could happen even if the key is valid. Once we finish session based // re-registration we want to be able to recover the key. - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) } @MainActor @Test(arguments: Self.testCases()) @@ -723,11 +724,12 @@ public class RegistrationCoordinatorTest { // Set a PIN on disk. ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + var didClearPinCode = false + ows2FAManagerMock.clearLocalPinCodeMock = { didClearPinCode = true } // Make SVR give us back a reg recovery password. let masterKey = AccountEntropyPool().getMasterKey() db.write { accountKeyStore.setMasterKey(masterKey, tx: $0) } - svr.hasMasterKey = true // NOTE: We expect to skip opening path steps because // if we have a SVR master key locally, this _must_ be @@ -809,7 +811,7 @@ public class RegistrationCoordinatorTest { .pinEntry(Stubs.pinEntryStateForRegRecoveryPath(mode: mode)), ) - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) // Give it the pin code, which should make it try and register. // Now we should expect to be at verification code entry since we already set the phone number. @@ -822,7 +824,7 @@ public class RegistrationCoordinatorTest { ), ) - #expect(svr.hasMasterKey.negated) + #expect(didClearPinCode) } @MainActor @Test(arguments: Self.testCases()) @@ -837,7 +839,6 @@ public class RegistrationCoordinatorTest { ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } let (initialMasterKey, finalMasterKey) = buildKeyDataMocks(testCase) - svr.hasMasterKey = true // NOTE: We expect to skip opening path steps because // if we have a SVR master key locally, this _must_ be @@ -913,13 +914,13 @@ public class RegistrationCoordinatorTest { }) // We haven't done a SVR backup; that should happen. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in self.testRun.addObservedStep(.backupMasterKey) #expect(pin == Stubs.pinCode) #expect(masterKey.rawData == finalMasterKey.rawData) + #expect(force) // We don't have a SVR auth credential, it should use chat server creds. #expect(authMethod == .chatServerAuth(expectedAuthedAccount())) - self.svr.hasMasterKey = true return .value(()) } @@ -1037,12 +1038,13 @@ public class RegistrationCoordinatorTest { // Set a PIN on disk. ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + var didClearPinCode = false + ows2FAManagerMock.clearLocalPinCodeMock = { didClearPinCode = true } ows2FAManagerMock.isReglockEnabledMock = { true } // Make SVR give us back a reg recovery password. let masterKey = AccountEntropyPool().getMasterKey() db.write { accountKeyStore.setMasterKey(masterKey, tx: $0) } - svr.hasMasterKey = true // NOTE: We expect to skip opening path steps because // if we have a SVR master key locally, this _must_ be @@ -1116,7 +1118,7 @@ public class RegistrationCoordinatorTest { ), )) - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) let acknowledgeAction: RegistrationReglockTimeoutAcknowledgeAction = switch testCase.mode { case .registering: .resetPhoneNumber @@ -1150,7 +1152,7 @@ public class RegistrationCoordinatorTest { ) // We want to have wiped our master key; we failed reglock, which means the key itself is wrong. - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) } // Test the path where a the local masterkey is no longer in sync with the one storedin SVR @@ -1166,6 +1168,8 @@ public class RegistrationCoordinatorTest { // Set a PIN on disk. ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + var didClearPinCode = false + ows2FAManagerMock.clearLocalPinCodeMock = { didClearPinCode = true } ows2FAManagerMock.isReglockEnabledMock = { true } // Make SVR give us back a reg recovery password. @@ -1174,7 +1178,6 @@ public class RegistrationCoordinatorTest { // For non-AEP, we will replace the local key with the remote key. // For AEP, we'll rotate to a new AEP (or use the existing local AEP if it's present) let finalMasterKey = testCase.newKey == .masterKey ? remoteMasterKey : newMasterKey - svr.hasMasterKey = true // Put some auth credentials in storage. let svr2CredentialCandidates: [SVR2AuthCredential] = [ @@ -1203,7 +1206,6 @@ public class RegistrationCoordinatorTest { svr.restoreKeysMock = { pin, authMethod in #expect(pin == Stubs.pinCode) #expect(authMethod == .svrAuth(Stubs.svr2AuthCredential, backup: nil)) - self.svr.hasMasterKey = true return .value(.success(remoteMasterKey)) } @@ -1289,15 +1291,15 @@ public class RegistrationCoordinatorTest { }) // We haven't done a SVR backup; that should happen now. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in #expect(pin == Stubs.pinCode) - // We don't have a SVR auth credential, it should use chat server creds. #expect(masterKey.rawData == finalMasterKey.rawData) + #expect(force) + // We don't have a SVR auth credential, it should use chat server creds. #expect(authMethod == .svrAuth( Stubs.svr2AuthCredential, backup: .chatServerAuth(expectedAuthedAccount()), )) - self.svr.hasMasterKey = true return .value(()) } @@ -1357,12 +1359,12 @@ public class RegistrationCoordinatorTest { #expect(svrAuthCredentialStore.svr2Dict[Stubs.svr2AuthCredential.credential.username] != nil) - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) // Give it the pin code, which should make it try and register. #expect(await coordinator.submitPINCode(Stubs.pinCode).awaitable() == .done) - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) } /// Test the path where both local and remote RRP are rejected due to a reglock challenge @@ -1383,6 +1385,8 @@ public class RegistrationCoordinatorTest { // Set a PIN on disk. ows2FAManagerMock.pinCodeMock = { Stubs.pinCode } + var didClearPinCode = false + ows2FAManagerMock.clearLocalPinCodeMock = { didClearPinCode = true } ows2FAManagerMock.isReglockEnabledMock = { true } // Make SVR give us back a reg recovery password. @@ -1390,7 +1394,6 @@ public class RegistrationCoordinatorTest { let remoteMasterKey = MasterKey() // For non-AEP, we will replace the local key with the remote key. // For AEP, we'll rotate to a new AEP (or use the existing local AEP if it's present) - svr.hasMasterKey = true // Put some auth credentials in storage. let svr2CredentialCandidates: [SVR2AuthCredential] = [ @@ -1420,7 +1423,6 @@ public class RegistrationCoordinatorTest { svr.restoreKeysMock = { pin, authMethod in #expect(pin == Stubs.pinCode) #expect(authMethod == .svrAuth(Stubs.svr2AuthCredential, backup: nil)) - self.svr.hasMasterKey = true return .value(.success(remoteMasterKey)) } @@ -1512,7 +1514,7 @@ public class RegistrationCoordinatorTest { ), ) - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) // Submit verification code #expect( @@ -1526,7 +1528,7 @@ public class RegistrationCoordinatorTest { ) // We want to have wiped our master key; we failed reglock, which means the key itself is wrong. - #expect(svr.hasMasterKey) + #expect(!didClearPinCode) } // MARK: - SVR Auth Credential Path @@ -1557,7 +1559,6 @@ public class RegistrationCoordinatorTest { self.testRun.addObservedStep(.restoreKeys) #expect(pin == Stubs.pinCode) #expect(authMethod == .svrAuth(Stubs.svr2AuthCredential, backup: nil)) - self.svr.hasMasterKey = true return .value(.success(initialMasterKey)) } @@ -1606,10 +1607,11 @@ public class RegistrationCoordinatorTest { }) // Once we create pre-keys, we should back up to svr. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in self.testRun.addObservedStep(.backupMasterKey) #expect(pin == Stubs.pinCode) #expect(masterKey.rawData == finalMasterKey.rawData) + #expect(force) #expect(authMethod == .svrAuth( Stubs.svr2AuthCredential, backup: .chatServerAuth(expectedAuthedAccount()), @@ -1917,9 +1919,10 @@ public class RegistrationCoordinatorTest { }) // Finish the validation. - svr.backupMasterKeyMock = { pin, masterKey, authMethod in + svr.backupMasterKeyMock = { pin, masterKey, force, authMethod in #expect(pin == Stubs.pinCode) #expect(masterKey.rawData == newMasterKey.rawData) + #expect(force) #expect(authMethod == .chatServerAuth(expectedAuthedAccount())) return .value(()) } @@ -2951,7 +2954,7 @@ public class RegistrationCoordinatorTest { }) // When we skip the pin, it should skip any SVR backups. - svr.backupMasterKeyMock = { _, _, _ in + svr.backupMasterKeyMock = { _, _, _, _ in Issue.record("Shouldn't talk to SVR with skipped PIN!") return .value(()) } @@ -3082,7 +3085,7 @@ public class RegistrationCoordinatorTest { }) // When we skip the pin, it should skip any SVR backups. - svr.backupMasterKeyMock = { _, _, _ in + svr.backupMasterKeyMock = { _, _, _, _ in Issue.record("Shouldn't talk to SVR with skipped PIN!") return .value(()) } diff --git a/SignalServiceKit/Environment/AppSetup.swift b/SignalServiceKit/Environment/AppSetup.swift index b28ee77c9f..64a1ce8429 100644 --- a/SignalServiceKit/Environment/AppSetup.swift +++ b/SignalServiceKit/Environment/AppSetup.swift @@ -445,7 +445,6 @@ extension AppSetup.GlobalsContinuation { pinHasher: LibSignalPinHasher(), storageServiceManager: storageServiceManager, svrLocalStorage: svrLocalStorage, - tsAccountManager: tsAccountManager, tsConstants: tsConstants, twoFAManager: SVR2.Wrappers.OWS2FAManager(ows2FAManager), ) diff --git a/SignalServiceKit/Mocks/KeyBackupService/SecureValueRecoveryMock.swift b/SignalServiceKit/Mocks/KeyBackupService/SecureValueRecoveryMock.swift index 14252c125d..5ef54bfb19 100644 --- a/SignalServiceKit/Mocks/KeyBackupService/SecureValueRecoveryMock.swift +++ b/SignalServiceKit/Mocks/KeyBackupService/SecureValueRecoveryMock.swift @@ -17,8 +17,6 @@ public class SecureValueRecoveryMock: SecureValueRecovery { public func refreshCredentialsIfNecessary() async throws { } - public var hasMasterKey = false - public var hasBackedUpMasterKey: Bool = false public func hasBackedUpMasterKey(transaction: DBReadTransaction) -> Bool { @@ -26,15 +24,15 @@ public class SecureValueRecoveryMock: SecureValueRecovery { } public func hasMasterKey(transaction: DBReadTransaction) -> Bool { - return hasMasterKey + return SVRLocalStorage().getIsMasterKeyBackedUp(transaction) } public var reglockToken: String? - public var backupMasterKeyMock: ((_ pin: String, _ masterKey: MasterKey, _ authMethod: SVR.AuthMethod) -> Promise)? + public var backupMasterKeyMock: ((_ pin: String, _ masterKey: MasterKey, _ force: Bool, _ authMethod: SVR.AuthMethod) -> Promise)? - public func backupMasterKey(pin: String, masterKey: MasterKey, authMethod: SVR.AuthMethod) async throws { - try await backupMasterKeyMock!(pin, masterKey, authMethod).awaitable() + public func backupMasterKey(pin: String, masterKey: MasterKey, force: Bool, authMethod: SVR.AuthMethod) async throws { + try await backupMasterKeyMock!(pin, masterKey, force, authMethod).awaitable() } public var restoreKeysMock: ((_ pin: String, _ authMethod: SVR.AuthMethod) -> Guarantee)? @@ -43,10 +41,6 @@ public class SecureValueRecoveryMock: SecureValueRecovery { return await restoreKeysMock!(pin, authMethod).awaitable() } - public func clearKeys(transaction: DBWriteTransaction) { - hasMasterKey = false - } - public var syncedMasterKey: MasterKey? public func storeKeys( @@ -82,10 +76,6 @@ public class SecureValueRecoveryMock: SecureValueRecovery { public func clearPendingRestoration(transaction: DBWriteTransaction) { doesHavePendingRestoration = false } - - public func handleMasterKeyUpdated(newMasterKey: MasterKey, disablePIN: Bool, tx: DBWriteTransaction) { - // Do nothing - } } #endif diff --git a/SignalServiceKit/SecureValueRecovery/AccountEntropyPool/AccountEntropyPoolManager.swift b/SignalServiceKit/SecureValueRecovery/AccountEntropyPool/AccountEntropyPoolManager.swift index 208ac3526c..959c8331ed 100644 --- a/SignalServiceKit/SecureValueRecovery/AccountEntropyPool/AccountEntropyPoolManager.swift +++ b/SignalServiceKit/SecureValueRecovery/AccountEntropyPool/AccountEntropyPoolManager.swift @@ -8,7 +8,6 @@ public protocol AccountEntropyPoolManager { func setAccountEntropyPool( newAccountEntropyPool: AccountEntropyPool, - disablePIN: Bool, tx: DBWriteTransaction, ) } @@ -75,7 +74,6 @@ class AccountEntropyPoolManagerImpl: AccountEntropyPoolManager { setAccountEntropyPool( newAccountEntropyPool: AccountEntropyPool(), - disablePIN: false, tx: tx, ) } @@ -84,7 +82,6 @@ class AccountEntropyPoolManagerImpl: AccountEntropyPoolManager { func setAccountEntropyPool( newAccountEntropyPool: AccountEntropyPool, - disablePIN: Bool, tx: DBWriteTransaction, ) { logger.warn("Setting new AEP!") @@ -117,18 +114,22 @@ class AccountEntropyPoolManagerImpl: AccountEntropyPoolManager { accountKeyStore.setAccountEntropyPool(newAccountEntropyPool, tx: tx) - svr.handleMasterKeyUpdated( - newMasterKey: newAccountEntropyPool.getMasterKey(), - disablePIN: disablePIN, - tx: tx, - ) - // Skip the steps below if we're not yet registered. This check matters // because one of our big callers is registration itself. guard isRegisteredPrimaryDevice else { return } + tx.addSyncCompletion { [svr] in + Task { + do { + try await svr.refreshBackupIfNecessary() + } catch { + Logger.warn("couldn't refresh svr after rotating aep: \(error)") + } + } + } + // Schedule an account attributes update, since we need to update the // reglock and reg recovery password downstream of the master key // changing. @@ -170,7 +171,7 @@ class MockAccountEntropyPoolManager: AccountEntropyPoolManager { func generateIfMissing() async {} var setAccountEntropyPoolMock: (() -> Void)? - func setAccountEntropyPool(newAccountEntropyPool: AccountEntropyPool, disablePIN: Bool, tx: DBWriteTransaction) { + func setAccountEntropyPool(newAccountEntropyPool: AccountEntropyPool, tx: DBWriteTransaction) { setAccountEntropyPoolMock?() } } diff --git a/SignalServiceKit/SecureValueRecovery/LocalStorage/SVRLocalStorage.swift b/SignalServiceKit/SecureValueRecovery/LocalStorage/SVRLocalStorage.swift index 94b3a69943..20e7f90aff 100644 --- a/SignalServiceKit/SecureValueRecovery/LocalStorage/SVRLocalStorage.swift +++ b/SignalServiceKit/SecureValueRecovery/LocalStorage/SVRLocalStorage.swift @@ -5,63 +5,11 @@ import Foundation -/// Stores state related to SVR independent of enclave; e.g. do we have backups at all, -/// what type is our pin, etc. +/// Stores state related to SVR; e.g. do we have backups at all, etc. struct SVRLocalStorage { - private let svrKvStore: KeyValueStore - - init() { - // Collection name must not be changed; matches that historically kept in KeyBackupServiceImpl. - self.svrKvStore = KeyValueStore(collection: "kOWSKeyBackupService_Keys") - } - - // MARK: - Getters - - func getNeedsMasterKeyBackup(_ transaction: DBReadTransaction) -> Bool { - return svrKvStore.getBool(Keys.needsMasterKeyBackup, defaultValue: false, transaction: transaction) - } + let completedBackupStore = KeyValueStore(collection: "SVR.Completed") func getIsMasterKeyBackedUp(_ transaction: DBReadTransaction) -> Bool { - return svrKvStore.getBool(Keys.isMasterKeyBackedUp, defaultValue: false, transaction: transaction) - } - - func getSVR2MrEnclaveStringValue(_ transaction: DBReadTransaction) -> String? { - return svrKvStore.getString(Keys.svr2MrEnclaveStringValue, transaction: transaction) - } - - // MARK: - Setters - - func setNeedsMasterKeyBackup(_ value: Bool, _ transaction: DBWriteTransaction) { - return svrKvStore.setBool(value, key: Keys.needsMasterKeyBackup, transaction: transaction) - } - - func setIsMasterKeyBackedUp(_ value: Bool, _ transaction: DBWriteTransaction) { - svrKvStore.setBool(value, key: Keys.isMasterKeyBackedUp, transaction: transaction) - } - - func setSVR2MrEnclaveStringValue(_ value: String?, _ transaction: DBWriteTransaction) { - svrKvStore.setString(value, key: Keys.svr2MrEnclaveStringValue, transaction: transaction) - } - - // MARK: - Clearing Keys - - func clearSVRKeys(_ transaction: DBWriteTransaction) { - svrKvStore.removeValues( - forKeys: [ - Keys.isMasterKeyBackedUp, - Keys.svr2MrEnclaveStringValue, - Keys.needsMasterKeyBackup, - ], - transaction: transaction, - ) - } - - // MARK: - Identifiers - - private enum Keys { - // These must not change, they match what was historically in KeyBackupServiceImpl. - static let isMasterKeyBackedUp = "isMasterKeyBackedUp" - static let needsMasterKeyBackup = "needsMasterKeyBackup" - static let svr2MrEnclaveStringValue = "svr2_mrenclaveStringValue" + return !completedBackupStore.allKeys(transaction: transaction).isEmpty } } diff --git a/SignalServiceKit/SecureValueRecovery/SVR2/SVR2Shims.swift b/SignalServiceKit/SecureValueRecovery/SVR2/SVR2Shims.swift index 427ccff8f8..db0a989477 100644 --- a/SignalServiceKit/SecureValueRecovery/SVR2/SVR2Shims.swift +++ b/SignalServiceKit/SecureValueRecovery/SVR2/SVR2Shims.swift @@ -20,7 +20,6 @@ extension SVR2 { public protocol _SVR2_OWS2FAManagerShim { func pinCode(transaction: DBReadTransaction) -> String? - func markDisabled(transaction: DBWriteTransaction) } public class _SVR2_OWS2FAManagerWrapper: SVR2.Shims.OWS2FAManager { @@ -30,8 +29,4 @@ public class _SVR2_OWS2FAManagerWrapper: SVR2.Shims.OWS2FAManager { public func pinCode(transaction: DBReadTransaction) -> String? { return manager.pinCode(transaction: transaction) } - - public func markDisabled(transaction: DBWriteTransaction) { - manager.markDisabled(transaction: transaction) - } } diff --git a/SignalServiceKit/SecureValueRecovery/SVR2/SecureValueRecovery2Impl.swift b/SignalServiceKit/SecureValueRecovery/SVR2/SecureValueRecovery2Impl.swift index 4b406ddb71..3bc2591fe2 100644 --- a/SignalServiceKit/SecureValueRecovery/SVR2/SecureValueRecovery2Impl.swift +++ b/SignalServiceKit/SecureValueRecovery/SVR2/SecureValueRecovery2Impl.swift @@ -16,7 +16,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { private let accountKeyStore: AccountKeyStore private let localStorage: SVRLocalStorage private let storageServiceManager: StorageServiceManager - private let tsAccountManager: TSAccountManager private let tsConstants: TSConstantsProtocol private let twoFAManager: SVR2.Shims.OWS2FAManager @@ -28,7 +27,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { pinHasher: any SVR2PinHasher, storageServiceManager: StorageServiceManager, svrLocalStorage: SVRLocalStorage, - tsAccountManager: TSAccountManager, tsConstants: TSConstantsProtocol, twoFAManager: SVR2.Shims.OWS2FAManager, ) { @@ -39,7 +37,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { self.localStorage = svrLocalStorage self.pinHasher = pinHasher self.storageServiceManager = storageServiceManager - self.tsAccountManager = tsAccountManager self.tsConstants = tsConstants self.twoFAManager = twoFAManager } @@ -74,55 +71,14 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { return localStorage.getIsMasterKeyBackedUp(transaction) } - // MARK: - - - /// Takes SVR-related actions when the master key changes, such as updating - /// local state and updating remote SVR state. - /// - /// - Parameter disablePIN - /// If `true`, wipes local state related to the PIN; no remote actions are - /// taken, since the master key the PIN protects is being updated anyway. - public func handleMasterKeyUpdated( - newMasterKey: MasterKey, - disablePIN: Bool, - tx transaction: DBWriteTransaction, - ) { - // clearInProgressBackup will clear any in progress backup state. - // This will prevent us continuing any in progress backups/exposes. - clearInProgressBackup(tx: transaction) - - updateLocalSVRState( - isMasterKeyBackedUp: localStorage.getIsMasterKeyBackedUp(transaction), - mrEnclaveStringValue: nil, - transaction: transaction, - ) - - if disablePIN { - Logger.info("Disabling PIN.") - - // Disable the PIN locally. - twoFAManager.markDisabled(transaction: transaction) - // Wipe credentials; they're now useless. - credentialStorage.removeSVR2CredentialsForCurrentUser(transaction) - } else if let pin = twoFAManager.pinCode(transaction: transaction) { - Logger.info("Scheduling master key backup with PIN.") - - // Record that the master key needs to be backed up. - localStorage.setNeedsMasterKeyBackup(true, transaction) - transaction.addSyncCompletion { - Task { try await self.backupMasterKey(pin: pin, masterKey: newMasterKey, authMethod: .implicit) } - } - } - } - // MARK: - Key Management private let backupQueue = ConcurrentTaskQueue(concurrentLimit: 1) - public func backupMasterKey(pin: String, masterKey: MasterKey, authMethod: SVR.AuthMethod) async throws { + public func backupMasterKey(pin: String, masterKey: MasterKey, force: Bool, authMethod: SVR.AuthMethod) async throws { Logger.info("") try await backupQueue.run { - try await doBackupAndExpose(pin: pin, masterKey: masterKey, authMethod: authMethod) + try await doBackupAndExpose(pin: pin, masterKey: masterKey, force: force, authMethod: authMethod) } } @@ -142,15 +98,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { } } - public func clearKeys(transaction: DBWriteTransaction) { - Logger.info("") - // This will prevent us continuing any in progress backups/exposes. - // If either are in flight, they will no-op when they get a response - // and see no in progress backup state. - clearInProgressBackup(tx: transaction) - localStorage.clearSVRKeys(transaction) - } - public func storeKeys( fromProvisioningMessage provisioningMessage: LinkingProvisioningMessage, authedDevice: AuthedDevice, @@ -220,129 +167,136 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { // MARK: - Backup/Expose Request - private lazy var kvStore = KeyValueStore(collection: "SecureValueRecovery2Impl") + private let kvStore = KeyValueStore(collection: "SecureValueRecovery2Impl") - /// We must be careful to never repeat a backup request when an expose request fails, or - /// even if an expose request was made. Once we get a success response from a backup - /// request, we create and persist one of these to track that, and only ever make expose - /// requests from then on until either: - /// 1. The expose requests succeeeds (we are done backing up and can wipe this) - /// 2. The user chooses a different PIN (we will make a new backup request) - /// 3. The user wipes SVR2 backups - private struct InProgressBackup: Codable { + /// We must be careful to never repeat a backup request after sending an + /// expose request for the first time. We must do this even if the + /// connection dies and we lose the response to the expose request. + /// After we get a success response from a backup request, we create and + /// persist one of these to track that we've started making expose requests, + /// and we only ever make expose requests from then on until: + /// 1. The user chooses a different PIN (we will make a new backup request) + /// 2. The user rotates their master key (we will make a new backup request) + /// 3. We roll out a new enclave (we will make a new backup request) + /// 3. The user disables their PIN (we will make a delete request) + private struct CompletedBackup: Codable { let masterKey: Data let encryptedMasterKey: Data - // TODO: Remove. - let rawPinType: Int let encodedPINVerificationString: String - // If we make a backup to one mrenclave, then update the mrenclave, - // we are safe to drop it and start again with a backup to the new - // mrenclave. - let mrEnclaveStringValue: String + var isExposed: Bool - func matches( - pin: String, - masterKey: MasterKey, - mrEnclave: MrEnclave, - ) -> Bool { + func matches(pin: String, masterKey: MasterKey) -> Bool { return ( SVRUtil.verifyPIN(pin: pin, againstEncodedPINVerificationString: self.encodedPINVerificationString) - && masterKey.rawData.ows_constantTimeIsEqual(to: self.masterKey) - && mrEnclave.stringValue == self.mrEnclaveStringValue, + && masterKey.rawData.ows_constantTimeIsEqual(to: self.masterKey), ) } } - private static let inProgressBackupKey = "InProgressBackup" - - private func getInProgressBackup(tx: DBReadTransaction) throws -> InProgressBackup? { - return try kvStore.getCodableValue(forKey: Self.inProgressBackupKey, transaction: tx) - } - - private func setInProgressBackup(_ value: InProgressBackup, tx: DBWriteTransaction) { - failIfThrows { - try kvStore.setCodable(optional: value, key: Self.inProgressBackupKey, transaction: tx) + private func getCompletedBackup(forEnclave enclave: MrEnclave, tx: DBReadTransaction) -> CompletedBackup? { + do { + return try localStorage.completedBackupStore.getCodableValue(forKey: enclave.stringValue, transaction: tx) + } catch { + // If we fail to decode, something has gone wrong locally. But we can + // treat this like if we never had a backup; after all the user may uninstall, + // reinstall, and do a backup again with the same PIN. This, like that, is + // a local-only trigger. + Logger.error("couldn't decode CompletedBackup") + return nil } } - private func clearInProgressBackup(tx: DBWriteTransaction) { - kvStore.removeValue(forKey: Self.inProgressBackupKey, transaction: tx) + private func setCompletedBackup(_ value: CompletedBackup, forEnclave enclave: MrEnclave, tx: DBWriteTransaction) { + failIfThrows { + try localStorage.completedBackupStore.setCodable(optional: value, key: enclave.stringValue, transaction: tx) + } + } + + private func removeCompletedBackup(forEnclave enclave: String, tx: DBWriteTransaction) { + localStorage.completedBackupStore.removeValue(forKey: enclave, transaction: tx) } private func doBackupAndExpose( pin: String, masterKey: MasterKey, + force: Bool, authMethod: SVR2.AuthMethod, ) async throws { - let config = SVR2WebsocketConfigurator(mrenclave: tsConstants.svr2Enclave, authMethod: authMethod) + let mrEnclave = tsConstants.svr2Enclave + + let priorBackup = self.db.read { tx in self.getCompletedBackup(forEnclave: mrEnclave, tx: tx) } + + let priorMatchingBackup: CompletedBackup? + if force { + // We're forcing a backup, so *nothing* matches. + priorMatchingBackup = nil + } else if let priorBackup, priorBackup.matches(pin: pin, masterKey: masterKey) { + // We're trying to back up what's already backed up. + priorMatchingBackup = priorBackup + } else { + priorMatchingBackup = nil + } + + if let priorMatchingBackup, priorMatchingBackup.isExposed { + return + } + + let config = SVR2WebsocketConfigurator(mrenclave: mrEnclave, authMethod: authMethod) let connection = try await makeHandshakeAndOpenConnection(config) defer { connection.disconnect(code: .normalClosure) } Logger.info("Connection open; beginning backup/expose") - // Check if we had an in flight backup. - let inProgressBackup: InProgressBackup? - do { - inProgressBackup = try self.db.read(block: self.getInProgressBackup(tx:)) - } catch { - // If we fail to decode, something has gone wrong locally. But we can - // treat this like if we never had a backup; after all the user may uninstall, - // reinstall, and do a backup again with the same PIN. This, like that, is - // a local-only trigger. - Logger.error("Failed to decode in progress backup state") - inProgressBackup = nil + // After this point, we might have data stored in this enclave. (For + // example, the network may drop before we receive the response.) Store + // this enclave so that we eventually clean it up. + await self.db.awaitableWrite { tx in + self.addEnclaveToPotentiallyDeleteFrom(mrEnclave, tx) } - let completedInProgressBackup: InProgressBackup - if let inProgressBackup, inProgressBackup.matches(pin: pin, masterKey: masterKey, mrEnclave: config.mrenclave) { - // Continue the backup from where we left off. + var completedBackup: CompletedBackup + if let priorMatchingBackup { + // We already completed a backup for this (pin, masterKey, enclave) triple, + // so we only want to perform an expose. Logger.warn("Skipping backup that was already completed") - completedInProgressBackup = inProgressBackup + completedBackup = priorMatchingBackup } else { - // We don't have a backup, or we're trying to back up something else; start - // fresh in both cases. - completedInProgressBackup = try await self.performBackupRequest( + // We don't have a backup, or we're trying to back up something else, or + // we're trying to back up to somewhere else; start fresh in these cases. + completedBackup = try await self.performBackupRequest( pin: pin, masterKey: masterKey, - mrEnclave: config.mrenclave, connection: connection, ) - // Write the in progress state to disk; we want to continue - // from here and not redo the backup request. await self.db.awaitableWrite { tx in - self.setInProgressBackup(completedInProgressBackup, tx: tx) + // Write that we've finished to disk; we never want to repeat the prior + // request after we start sending expose requests. + self.setCompletedBackup(completedBackup, forEnclave: mrEnclave, tx: tx) } } + // This must be true because it's checked earlier for existing backups and + // starts out false for new backups. + owsPrecondition(!completedBackup.isExposed) + try await self.performExposeRequest( - backup: completedInProgressBackup, + backup: completedBackup, authedAccount: authMethod.authedAccount, connection: connection, ) + completedBackup.isExposed = true await self.db.awaitableWrite { tx in - let hasInProgressBackup = (try? self.getInProgressBackup(tx: tx)) != nil - guard hasInProgressBackup else { - Logger.info("backup state cleared while expose ongoing; throwing away results") - return - } - self.localStorage.setNeedsMasterKeyBackup(false, tx) - self.clearInProgressBackup(tx: tx) - self.updateLocalSVRState( - isMasterKeyBackedUp: true, - mrEnclaveStringValue: completedInProgressBackup.mrEnclaveStringValue, - transaction: tx, - ) + self.setCompletedBackup(completedBackup, forEnclave: mrEnclave, tx: tx) } } private func performBackupRequest( pin: String, masterKey: MasterKey, - mrEnclave: MrEnclave, connection: SgxWebsocketConnection, - ) async throws -> InProgressBackup { + ) async throws -> CompletedBackup { Logger.info("Performing backup") let encodedPINVerificationString = try SVRUtil.deriveEncodedPINVerificationString(pin: pin) @@ -364,12 +318,11 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { switch response.backup.status { case .ok: Logger.info("Backup success!") - return InProgressBackup( + return CompletedBackup( masterKey: masterKey.rawData, encryptedMasterKey: encryptedMasterKey, - rawPinType: 0, encodedPINVerificationString: encodedPINVerificationString, - mrEnclaveStringValue: mrEnclave.stringValue, + isExposed: false, ) case .UNRECOGNIZED, .unset: throw OWSGenericError("backup status response unknown") @@ -377,7 +330,7 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { } private func performExposeRequest( - backup: InProgressBackup, + backup: CompletedBackup, authedAccount: AuthedAccount, connection: SgxWebsocketConnection, ) async throws { @@ -411,7 +364,7 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { // // 3 could be a legitimate server error or a compromised server; in either // case we do NOT want to make another backup; report a failure but keep - // any InProgressBackup state around so that retries just retry the expose. + // any CompletedBackup state around so that retries just retry the expose. // This prevents any possibility of repeated PIN guessing by a compromised server. throw OWSGenericError("Got error response when exposing on SVR2 server; something has gone horribly wrong.") case .UNRECOGNIZED, .unset: @@ -515,13 +468,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { let encryptedMasterKey = response.restore.data let masterKeyData = try pinHash.decryptMasterKey(encryptedMasterKey) let masterKey = try MasterKey(data: masterKeyData) - await self.db.awaitableWrite { tx in - self.updateLocalSVRState( - isMasterKeyBackedUp: true, - mrEnclaveStringValue: mrEnclave.stringValue, - transaction: tx, - ) - } return .success(masterKey: masterKey, mrEnclave: mrEnclave) } } @@ -535,6 +481,12 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { let config = SVR2WebsocketConfigurator(mrenclave: mrEnclave, authMethod: authMethod) let connection = try await makeHandshakeAndOpenConnection(config) defer { connection.disconnect(code: .normalClosure) } + await db.awaitableWrite { tx in + // If send a request to delete this, it may be deleted even if we don't get + // back a response (e.g., the connection fails, the app exits). By clearing + // this now, we ensure resiliency for new backups after these edge cases. + removeCompletedBackup(forEnclave: mrEnclave.stringValue, tx: tx) + } return try await self.performDeleteRequest( mrEnclave: mrEnclave, connection: connection, @@ -558,59 +510,63 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { // MARK: Durable deletes - private static let oldEnclavesToDeleteFromKey = "OldEnclavesToDeleteFrom" + private static let enclavesToPotentiallyDeleteFromKey = "OldEnclavesToDeleteFrom" - private func getOldEnclavesToDeleteFrom(_ tx: DBReadTransaction) -> Set { + private func getEnclavesToPotentiallyDeleteFrom(_ tx: DBReadTransaction) -> Set { // This is decoding a Set. It won't actually ever fail, so just eat up errors. let enclaveStrings: Set = (try? kvStore.getCodableValue( - forKey: Self.oldEnclavesToDeleteFromKey, + forKey: Self.enclavesToPotentiallyDeleteFromKey, transaction: tx, )) ?? Set() return enclaveStrings } - private func addOldEnclaveToDeleteFrom(_ enclave: MrEnclave, _ tx: DBWriteTransaction) { + private func addEnclaveToPotentiallyDeleteFrom(_ enclave: MrEnclave, _ tx: DBWriteTransaction) { // This is (en/de)coding a Set. It won't actually ever fail, so just eat up errors. var enclaveStrings: Set = (try? kvStore.getCodableValue( - forKey: Self.oldEnclavesToDeleteFromKey, + forKey: Self.enclavesToPotentiallyDeleteFromKey, transaction: tx, )) ?? Set() enclaveStrings.insert(enclave.stringValue) - try? kvStore.setCodable(enclaveStrings, key: Self.oldEnclavesToDeleteFromKey, transaction: tx) + try? kvStore.setCodable(enclaveStrings, key: Self.enclavesToPotentiallyDeleteFromKey, transaction: tx) } - private func markOldEnclaveDeleted(_ enclave: String, _ tx: DBWriteTransaction) { + private func markEnclaveDeleted(_ enclave: String, _ tx: DBWriteTransaction) { // This is (en/de)coding a Set. It won't actually ever fail, so just eat up errors. var enclaveStrings: Set = (try? kvStore.getCodableValue( - forKey: Self.oldEnclavesToDeleteFromKey, + forKey: Self.enclavesToPotentiallyDeleteFromKey, transaction: tx, )) ?? Set() enclaveStrings.remove(enclave) - try? kvStore.setCodable(enclaveStrings, key: Self.oldEnclavesToDeleteFromKey, transaction: tx) + try? kvStore.setCodable(enclaveStrings, key: Self.enclavesToPotentiallyDeleteFromKey, transaction: tx) } - private func wipeOldEnclavesIfNeeded() async throws { + private func wipeObsoleteEnclaves(allEnclaves: some Sequence, enclavesToKeep: Int) async throws { var firstError: (any Error)? - var oldEnclaves = db.read(block: { tx in self.getOldEnclavesToDeleteFrom(tx) }) - for obsoleteEnclave in tsConstants.svr2PreviousEnclaves { - guard oldEnclaves.remove(obsoleteEnclave.stringValue) != nil else { + var potentialEnclaves = db.read(block: { tx in self.getEnclavesToPotentiallyDeleteFrom(tx) }) + for keptEnclave in allEnclaves.prefix(enclavesToKeep) { + potentialEnclaves.remove(keptEnclave.stringValue) + } + for obsoleteEnclave in allEnclaves.dropFirst(enclavesToKeep) { + guard potentialEnclaves.remove(obsoleteEnclave.stringValue) != nil else { continue } - Logger.info("wiping old enclave: \(obsoleteEnclave)") + Logger.info("wiping enclave: \(obsoleteEnclave)") do { try await self.doDelete(mrEnclave: obsoleteEnclave, authMethod: .implicit) await db.awaitableWrite { tx in - markOldEnclaveDeleted(obsoleteEnclave.stringValue, tx) + markEnclaveDeleted(obsoleteEnclave.stringValue, tx) } } catch { - Logger.warn("couldn't wipe old enclave; may retry eventually: \(error)") + Logger.warn("couldn't wipe enclave; may retry eventually: \(error)") firstError = firstError ?? error } } - for unknownEnclave in oldEnclaves { + for unknownEnclave in potentialEnclaves { Logger.warn("pruning unknown enclave: \(unknownEnclave)") await db.awaitableWrite { tx in - markOldEnclaveDeleted(unknownEnclave, tx) + removeCompletedBackup(forEnclave: unknownEnclave, tx: tx) + markEnclaveDeleted(unknownEnclave, tx) } } if let firstError { @@ -621,89 +577,26 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { // MARK: - Migrations public func refreshBackupIfNecessary() async throws { - // If a migration isn't needed, this returns a success immediately. - try await migrateEnclavesIfNecessary() - - // If a backup isn't needed, this returns a success immediately. - try await backupMasterKeyIfNecessary() - - // Require migrations/backups to succeed before we check for old stuff to - // wipe because (a) migrations add old stuff to be wiped and (b) backups - // are more important. - try await wipeOldEnclavesIfNeeded() - } - - private func backupMasterKeyIfNecessary() async throws { try await backupQueue.run { - try await _backupMasterKeyIfNecessary() - } - } + let pin = db.read(block: twoFAManager.pinCode(transaction:)) - private func _backupMasterKeyIfNecessary() async throws { - guard db.read(block: localStorage.getNeedsMasterKeyBackup(_:)) else { - return - } - let (pin, masterKey) = db.read { tx -> (String?, MasterKey?) in - return (twoFAManager.pinCode(transaction: tx), accountKeyStore.getMasterKey(tx: tx)) - } - guard let pin, let masterKey else { - Logger.warn("skipping; hasPin? \(pin != nil); hasMasterKey? \(masterKey != nil)") - return - } - try await doBackupAndExpose(pin: pin, masterKey: masterKey, authMethod: .implicit) - } - - /// If there is a newer enclave than the one we most recently backed up to, backs up known - /// master key data to it instead, marking the old enclave for deletion. - /// If there is no migration needed, returns a success promise immediately. - private func migrateEnclavesIfNecessary() async throws { - try await backupQueue.run { - try await _migrateEnclavesIfNecessary() - } - } - - private func _migrateEnclavesIfNecessary() async throws { - let (isBackedUp, oldEnclave) = db.read { tx -> (Bool, String?) in - return (self.localStorage.getIsMasterKeyBackedUp(tx), self.localStorage.getSVR2MrEnclaveStringValue(tx)) - } - guard isBackedUp else { - // "isMasterKeyBackedUp" is shared between svr2 and kbs; if its - // false that means we had no backups to begin with and therefore - // should not back up to any new enclave. - return - } - guard oldEnclave != tsConstants.svr2Enclave.stringValue else { - // The "old" enclave is already the current enclave. - return - } - let (pin, masterKey) = db.read { tx -> (String?, MasterKey?) in - return (twoFAManager.pinCode(transaction: tx), accountKeyStore.getMasterKey(tx: tx)) - } - guard let pin, let masterKey else { - // We don't have anything that *can* be backed up. - return - } - - Logger.info("Migrating SVR2 Enclaves") - do { - try await self.doBackupAndExpose(pin: pin, masterKey: masterKey, authMethod: .implicit) - Logger.info("Successfully migrated SVR2 enclave") - } catch { - owsFailDebug("Failed to migrate SVR2 enclave") - throw error - } - - let backedUpEnclave = self.tsConstants.svr2PreviousEnclaves.first(where: { $0.stringValue == oldEnclave }) - if let backedUpEnclave { - Logger.info("Adding old enclave to be deleted") - // Strictly speaking, this happens in a separate transaction from when we mark the - // backup/expose complete. But no matter what this is best effort; the client - // can be uninstalled before it gets a chance to delete the old backup, for example. - await self.db.awaitableWrite { tx in - self.addOldEnclaveToDeleteFrom(backedUpEnclave, tx) + if let pin { + let masterKey = db.read(block: accountKeyStore.getMasterKey(tx:)) + if let masterKey { + // If a backup isn't needed, this returns a success immediately. + try await doBackupAndExpose(pin: pin, masterKey: masterKey, force: false, authMethod: .implicit) + } else { + Logger.warn("can't back up master key without master key") + } } - // We start wiping any old enclaves right after doing this migration, - // no need to kick it off here. + + let allEnclaves = [tsConstants.svr2Enclave] + tsConstants.svr2PreviousEnclaves + // If we don't have a PIN, we shouldn't keep any enclaves. + let enclavesToKeep = pin == nil ? 0 : 1 + + // Require backups (i.e., migrations) to succeed before deleting from old + // enclaves to ensure we're always backed up to at least one enclave. + try await wipeObsoleteEnclaves(allEnclaves: allEnclaves, enclavesToKeep: enclavesToKeep) } } @@ -773,21 +666,6 @@ public class SecureValueRecovery2Impl: SecureValueRecovery { } } } - - // MARK: - Local key storage helpers - - private func updateLocalSVRState( - isMasterKeyBackedUp: Bool, - mrEnclaveStringValue: String?, - transaction: DBWriteTransaction, - ) { - if isMasterKeyBackedUp != localStorage.getIsMasterKeyBackedUp(transaction) { - localStorage.setIsMasterKeyBackedUp(isMasterKeyBackedUp, transaction) - } - if mrEnclaveStringValue != localStorage.getSVR2MrEnclaveStringValue(transaction) { - localStorage.setSVR2MrEnclaveStringValue(mrEnclaveStringValue, transaction) - } - } } private extension SVR2.AuthMethod { diff --git a/SignalServiceKit/SecureValueRecovery/SecureValueRecovery.swift b/SignalServiceKit/SecureValueRecovery/SecureValueRecovery.swift index 4f82f720f7..2dd228c1d5 100644 --- a/SignalServiceKit/SecureValueRecovery/SecureValueRecovery.swift +++ b/SignalServiceKit/SecureValueRecovery/SecureValueRecovery.swift @@ -113,11 +113,7 @@ public protocol SecureValueRecovery { func restoreKeys(pin: String, authMethod: SVR.AuthMethod) async -> SVR.RestoreKeysResult /// Backs up the user's master key to SVR. - func backupMasterKey(pin: String, masterKey: MasterKey, authMethod: SVR.AuthMethod) async throws - - /// Removes the SVR keys locally from the device, they can still be - /// restored from the server if you know the pin. - func clearKeys(transaction: DBWriteTransaction) + func backupMasterKey(pin: String, masterKey: MasterKey, force: Bool, authMethod: SVR.AuthMethod) async throws func storeKeys( fromKeysSyncMessage syncMessage: SSKProtoSyncMessageKeys, @@ -130,10 +126,4 @@ public protocol SecureValueRecovery { authedDevice: AuthedDevice, tx: DBWriteTransaction, ) throws(SVR.KeysError) - - func handleMasterKeyUpdated( - newMasterKey: MasterKey, - disablePIN: Bool, - tx: DBWriteTransaction, - ) } diff --git a/SignalServiceKit/Util/OWS2FAManager.swift b/SignalServiceKit/Util/OWS2FAManager.swift index f5c6851e14..b028339993 100644 --- a/SignalServiceKit/Util/OWS2FAManager.swift +++ b/SignalServiceKit/Util/OWS2FAManager.swift @@ -241,6 +241,7 @@ public class OWS2FAManager { try await svr.backupMasterKey( pin: pin, masterKey: masterKey, + force: false, authMethod: .implicit, ) diff --git a/SignalServiceKit/tests/SecureValueRecovery/SVR2/SVR2ConcurrencyTests.swift b/SignalServiceKit/tests/SecureValueRecovery/SVR2/SVR2ConcurrencyTests.swift index 120791667a..5f41b3ab71 100644 --- a/SignalServiceKit/tests/SecureValueRecovery/SVR2/SVR2ConcurrencyTests.swift +++ b/SignalServiceKit/tests/SecureValueRecovery/SVR2/SVR2ConcurrencyTests.swift @@ -40,7 +40,6 @@ struct SVR2ConcurrencyTests { pinHasher: MockPinHasher(), storageServiceManager: FakeStorageServiceManager(), svrLocalStorage: localStorage, - tsAccountManager: MockTSAccountManager(), tsConstants: TSConstants.shared, twoFAManager: SVR2.TestMocks.OWS2FAManager(), ) @@ -94,7 +93,7 @@ struct SVR2ConcurrencyTests { } let firstMasterKey = MasterKey() - async let firstBackupResult: Void = svr.backupMasterKey(pin: "1234", masterKey: firstMasterKey, authMethod: .implicit) + async let firstBackupResult: Void = svr.backupMasterKey(pin: "1234", masterKey: firstMasterKey, force: false, authMethod: .implicit) // Let the first backup succeed and start the expose, then make the second request. firstBackupFuture.resolve(backupResponse()) @@ -103,7 +102,7 @@ struct SVR2ConcurrencyTests { try await madeRequestContinuations[1].wait() let secondMasterKey = MasterKey() - async let secondBackupResult: Void = svr.backupMasterKey(pin: "abcd", masterKey: secondMasterKey, authMethod: .implicit) + async let secondBackupResult: Void = svr.backupMasterKey(pin: "abcd", masterKey: secondMasterKey, force: false, authMethod: .implicit) firstExposeFuture.resolve(exposeResponse()) secondBackupFuture.resolve(backupResponse()) @@ -165,7 +164,7 @@ struct SVR2ConcurrencyTests { } let firstMasterKey = MasterKey() - async let firstBackupResult: Void = svr.backupMasterKey(pin: "1234", masterKey: firstMasterKey, authMethod: .implicit) + async let firstBackupResult: Void = svr.backupMasterKey(pin: "1234", masterKey: firstMasterKey, force: false, authMethod: .implicit) let backupError = WebSocketError.closeError(statusCode: 400, closeReason: nil) firstBackupFuture.reject(backupError) @@ -198,7 +197,7 @@ struct SVR2ConcurrencyTests { } let secondMasterKey = MasterKey() - try await svr.backupMasterKey(pin: "zzzz", masterKey: secondMasterKey, authMethod: .implicit) + try await svr.backupMasterKey(pin: "zzzz", masterKey: secondMasterKey, force: false, authMethod: .implicit) #expect(numOpenedConnections == 2) } diff --git a/SignalServiceKit/tests/SecureValueRecovery/SVR2/SecureValueRecovery2Tests.swift b/SignalServiceKit/tests/SecureValueRecovery/SVR2/SecureValueRecovery2Tests.swift index 183afb4ae5..096d36995f 100644 --- a/SignalServiceKit/tests/SecureValueRecovery/SVR2/SecureValueRecovery2Tests.swift +++ b/SignalServiceKit/tests/SecureValueRecovery/SVR2/SecureValueRecovery2Tests.swift @@ -20,7 +20,6 @@ class SecureValueRecovery2Tests: XCTestCase { private var localStorage: SVRLocalStorage! private var mockConnectionFactory: MockSgxWebsocketConnectionFactory! private var mockConnection: MockSgxWebsocketConnection! - private var mockTSAccountManager: MockTSAccountManager! private var mockTSConstants: TSConstantsMock! override func setUp() { @@ -38,7 +37,6 @@ class SecureValueRecovery2Tests: XCTestCase { self.mockConnection = mockConnection mockConnectionFactory = MockSgxWebsocketConnectionFactory() - mockTSAccountManager = .init() mockTSConstants = TSConstantsMock() self.svr = SecureValueRecovery2Impl( @@ -49,7 +47,6 @@ class SecureValueRecovery2Tests: XCTestCase { pinHasher: MockPinHasher(), storageServiceManager: FakeStorageServiceManager(), svrLocalStorage: localStorage, - tsAccountManager: mockTSAccountManager, tsConstants: mockTSConstants, twoFAManager: mock2FAManager, ) @@ -59,14 +56,17 @@ class SecureValueRecovery2Tests: XCTestCase { func testMigration() async throws { // Set up the connections to both the old and new enclaves. let mockAuth = RemoteAttestation.Auth(username: "username", password: "password") + let oldEnclave = MrEnclave("0000000000000000000000000000000000000000000000000000000000000000") let oldEnclaveConnection = MockSgxWebsocketConnection() oldEnclaveConnection.mockEnclave = oldEnclave oldEnclaveConnection.mockAuth = mockAuth + let newEnclave = MrEnclave("0101010101010101010101010101010101010101010101010101010101010101") let newEnclaveConnection = MockSgxWebsocketConnection() newEnclaveConnection.mockEnclave = newEnclave newEnclaveConnection.mockAuth = mockAuth + mockConnectionFactory.setOnConnectAndPerformHandshake { (config: SVR2WebsocketConfigurator) in switch config.mrenclave.stringValue { case oldEnclave.stringValue: @@ -84,16 +84,10 @@ class SecureValueRecovery2Tests: XCTestCase { // Set up the local data needed. db.write { tx in - localStorage.setIsMasterKeyBackedUp(true, tx) accountKeyStore.setMasterKey(masterKey, tx: tx) - localStorage.setSVR2MrEnclaveStringValue(oldEnclave.stringValue, tx) } - mockTSAccountManager.registrationStateMock = { .registered } mock2FAManager.pinCode = pin - mockTSConstants.svr2Enclave = newEnclave - mockTSConstants.svr2PreviousEnclaves = [oldEnclave] - // Expect backup and expose to the new enclave. var newEnclaveRequestCount = 0 newEnclaveConnection.onSendRequestAndReadResponse = { request in @@ -131,6 +125,24 @@ class SecureValueRecovery2Tests: XCTestCase { var response = SVR2Proto_Response() switch oldEnclaveRequestCount { case 0: + // First it should issue a backup to the old enclave + XCTAssert(request.hasBackup) + XCTAssertEqual(request.backup.data.count, 48) + XCTAssertEqual(request.backup.pin.count, 32) + + var backupResponse = SVR2Proto_BackupResponse() + backupResponse.status = .ok + response.backup = backupResponse + case 1: + // Then an expose + XCTAssert(request.hasExpose) + XCTAssertEqual(request.expose.data.count, 48) + + var exposeResponse = SVR2Proto_ExposeResponse() + exposeResponse.status = .ok + response.expose = exposeResponse + case 2: + // Then a delete XCTAssert(request.hasDelete) // New enclave should be all backed up by now. XCTAssertEqual(newEnclaveRequestCount, 2) @@ -143,36 +155,54 @@ class SecureValueRecovery2Tests: XCTestCase { return .value(response) } - // Kick off the migration. + // Migrate to the old enclave. + mockTSConstants.svr2Enclave = oldEnclave + mockTSConstants.svr2PreviousEnclaves = [] _ = try await svr.refreshBackupIfNecessary() - - XCTAssertEqual(newEnclaveRequestCount, 2) - XCTAssertEqual(oldEnclaveRequestCount, 1) - + XCTAssertEqual(newEnclaveRequestCount, 0) + XCTAssertEqual(oldEnclaveRequestCount, 2) db.read { tx in - XCTAssertEqual(localStorage.getSVR2MrEnclaveStringValue(tx), newEnclave.stringValue) + XCTAssertEqual(localStorage.completedBackupStore.allKeys(transaction: tx), [oldEnclave.stringValue]) } - // If we try to migrate again, it does nothing because we are at the newest enclave. + // Migrate to the new enclave. + mockTSConstants.svr2Enclave = newEnclave + mockTSConstants.svr2PreviousEnclaves = [oldEnclave] _ = try await svr.refreshBackupIfNecessary() XCTAssertEqual(newEnclaveRequestCount, 2) - XCTAssertEqual(oldEnclaveRequestCount, 1) + XCTAssertEqual(oldEnclaveRequestCount, 3) + db.read { tx in + XCTAssertEqual(localStorage.completedBackupStore.allKeys(transaction: tx), [newEnclave.stringValue]) + } + + // Migrate again; nothing should change. + _ = try await svr.refreshBackupIfNecessary() + XCTAssertEqual(newEnclaveRequestCount, 2) + XCTAssertEqual(oldEnclaveRequestCount, 3) + db.read { tx in + XCTAssertEqual(localStorage.completedBackupStore.allKeys(transaction: tx), [newEnclave.stringValue]) + } } @MainActor func testMigration_forgottenEnclave() async throws { // Set up the connections to both the old and new enclaves. let mockAuth = RemoteAttestation.Auth(username: "username", password: "password") + let oldEnclave = MrEnclave("0000000000000000000000000000000000000000000000000000000000000000") let oldEnclaveConnection = MockSgxWebsocketConnection() oldEnclaveConnection.mockEnclave = oldEnclave oldEnclaveConnection.mockAuth = mockAuth + let newEnclave = MrEnclave("0101010101010101010101010101010101010101010101010101010101010101") let newEnclaveConnection = MockSgxWebsocketConnection() newEnclaveConnection.mockEnclave = newEnclave newEnclaveConnection.mockAuth = mockAuth + mockConnectionFactory.setOnConnectAndPerformHandshake { (config: SVR2WebsocketConfigurator) in switch config.mrenclave.stringValue { + case oldEnclave.stringValue: + return oldEnclaveConnection case newEnclave.stringValue: return newEnclaveConnection default: @@ -186,17 +216,10 @@ class SecureValueRecovery2Tests: XCTestCase { // Set up the local data needed. db.write { tx in - localStorage.setIsMasterKeyBackedUp(true, tx) accountKeyStore.setMasterKey(masterKey, tx: tx) - localStorage.setSVR2MrEnclaveStringValue(oldEnclave.stringValue, tx) } - mockTSAccountManager.registrationStateMock = { .registered } mock2FAManager.pinCode = pin - mockTSConstants.svr2Enclave = newEnclave - // No old enclaves to know about. - mockTSConstants.svr2PreviousEnclaves = [] - // Expect backup and expose to the new enclave. var newEnclaveRequestCount = 0 newEnclaveConnection.onSendRequestAndReadResponse = { request in @@ -227,20 +250,55 @@ class SecureValueRecovery2Tests: XCTestCase { return .value(response) } - // NOTE: the old enclave should get no requests, its considered dead. + // The old enclave should just get a delete. + var oldEnclaveRequestCount = 0 + oldEnclaveConnection.onSendRequestAndReadResponse = { request in + defer { oldEnclaveRequestCount += 1 } + var response = SVR2Proto_Response() + switch oldEnclaveRequestCount { + case 0: + // First it should issue a backup to the old enclave + XCTAssert(request.hasBackup) + XCTAssertEqual(request.backup.data.count, 48) + XCTAssertEqual(request.backup.pin.count, 32) - // Kick off the migration. - _ = try await svr.refreshBackupIfNecessary() + var backupResponse = SVR2Proto_BackupResponse() + backupResponse.status = .ok + response.backup = backupResponse + case 1: + // Then an expose + XCTAssert(request.hasExpose) + XCTAssertEqual(request.expose.data.count, 48) - XCTAssertEqual(newEnclaveRequestCount, 2) - - db.read { tx in - XCTAssertEqual(localStorage.getSVR2MrEnclaveStringValue(tx), newEnclave.stringValue) + var exposeResponse = SVR2Proto_ExposeResponse() + exposeResponse.status = .ok + response.expose = exposeResponse + default: + XCTFail("Unexpected request") + return .init(error: OWSAssertionError("")) + } + return .value(response) } - // If we try to migrate again, it does nothing because we are at the newest enclave. + // Migrate to the old enclave. + mockTSConstants.svr2Enclave = oldEnclave + mockTSConstants.svr2PreviousEnclaves = [] + _ = try await svr.refreshBackupIfNecessary() + XCTAssertEqual(newEnclaveRequestCount, 0) + XCTAssertEqual(oldEnclaveRequestCount, 2) + db.read { tx in + XCTAssertEqual(localStorage.completedBackupStore.allKeys(transaction: tx), [oldEnclave.stringValue]) + } + + // Migrate to the new enclave. + mockTSConstants.svr2Enclave = newEnclave + mockTSConstants.svr2PreviousEnclaves = [] _ = try await svr.refreshBackupIfNecessary() XCTAssertEqual(newEnclaveRequestCount, 2) + XCTAssertEqual(oldEnclaveRequestCount, 2) + db.read { tx in + XCTAssertEqual(localStorage.completedBackupStore.allKeys(transaction: tx), [newEnclave.stringValue]) + } } func testPinHashingNumeric() throws { @@ -328,6 +386,4 @@ public class _SVR2_OWS2FAManagerTestMock: SVR2.Shims.OWS2FAManager { public func pinCode(transaction: DBReadTransaction) -> String? { return pinCode } - - public func markDisabled(transaction: DBWriteTransaction) {} }