From 0e43524dacee105b33b2f47f11effa029017d6b1 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Fri, 17 May 2024 15:05:08 -0400 Subject: [PATCH] Remove client public keys when deleting accounts/devices --- .../textsecuregcm/WhisperServerService.java | 2 +- .../storage/AccountsManager.java | 35 +++++++++++++------ .../workers/CommandDependencies.java | 9 ++++- ...ccountCreationDeletionIntegrationTest.java | 11 ++++++ ...ntsManagerChangeNumberIntegrationTest.java | 7 ++++ ...ConcurrentModificationIntegrationTest.java | 1 + .../storage/AccountsManagerTest.java | 4 +++ ...ccountsManagerUsernameIntegrationTest.java | 1 + .../AddRemoveDeviceIntegrationTest.java | 19 +++++++++- 9 files changed, 75 insertions(+), 14 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 3e643eb46..bd514ecd4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -587,7 +587,7 @@ public class WhisperServerService extends Application { account.removeDevice(deviceId); - return accounts.updateTransactionallyAsync(account, keysManager.buildWriteItemsForRemovedDevice( - account.getIdentifier(IdentityType.ACI), - account.getIdentifier(IdentityType.PNI), - deviceId)) + final List additionalWriteItems = new ArrayList<>( + keysManager.buildWriteItemsForRemovedDevice( + account.getIdentifier(IdentityType.ACI), + account.getIdentifier(IdentityType.PNI), + deviceId)); + + additionalWriteItems.add(clientPublicKeysManager.buildTransactWriteItemForDeletion(account.getIdentifier(IdentityType.ACI), deviceId)); + + return accounts.updateTransactionallyAsync(account, additionalWriteItems) .thenApply(ignored -> account); }) .thenCompose(updatedAccount -> redisDeleteAsync(updatedAccount).thenApply(ignored -> updatedAccount)) @@ -956,12 +963,18 @@ public class AccountsManager { } private CompletableFuture delete(final Account account) { - final List additionalWriteItems = - account.getDevices().stream().flatMap(device -> keysManager.buildWriteItemsForRemovedDevice( - account.getIdentifier(IdentityType.ACI), - account.getIdentifier(IdentityType.PNI), - device.getId()).stream()) - .toList(); + final List additionalWriteItems = Stream.concat( + account.getDevices().stream() + .flatMap(device -> keysManager.buildWriteItemsForRemovedDevice( + account.getIdentifier(IdentityType.ACI), + account.getIdentifier(IdentityType.PNI), + device.getId()) + .stream()), + account.getDevices().stream() + .map(device -> clientPublicKeysManager.buildTransactWriteItemForDeletion( + account.getIdentifier(IdentityType.ACI), + device.getId()))) + .toList(); return CompletableFuture.allOf( secureStorageClient.deleteStoredData(account.getUuid()), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java index ed91149f4..1fa08eaf4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java @@ -37,6 +37,8 @@ import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2 import org.whispersystems.textsecuregcm.storage.AccountLockManager; import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.storage.AccountsManager; +import org.whispersystems.textsecuregcm.storage.ClientPublicKeys; +import org.whispersystems.textsecuregcm.storage.ClientPublicKeysManager; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.KeysManager; import org.whispersystems.textsecuregcm.storage.MessagesCache; @@ -145,6 +147,11 @@ record CommandDependencies( RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager = new RegistrationRecoveryPasswordsManager( registrationRecoveryPasswords); + ClientPublicKeys clientPublicKeys = + new ClientPublicKeys(dynamoDbAsyncClient, configuration.getDynamoDbTables().getClientPublicKeys().getTableName()); + + ClientPublicKeysManager clientPublicKeysManager = new ClientPublicKeysManager(clientPublicKeys); + Accounts accounts = new Accounts( dynamoDbClient, dynamoDbAsyncClient, @@ -197,7 +204,7 @@ record CommandDependencies( AccountsManager accountsManager = new AccountsManager(accounts, phoneNumberIdentifiers, cacheCluster, accountLockManager, keys, messagesManager, profilesManager, secureStorageClient, secureValueRecovery2Client, clientPresenceManager, - registrationRecoveryPasswordsManager, accountLockExecutor, clientPresenceExecutor, + registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, clock); RateLimiters rateLimiters = RateLimiters.createAndValidate(configuration.getLimitsConfiguration(), dynamicConfigurationManager, rateLimitersCluster); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java index 9dca82692..d1d7c8334 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java @@ -53,6 +53,7 @@ public class AccountCreationDeletionIntegrationTest { @RegisterExtension static final DynamoDbExtension DYNAMO_DB_EXTENSION = new DynamoDbExtension( DynamoDbExtensionSchema.Tables.ACCOUNTS, + DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS, DynamoDbExtensionSchema.Tables.DELETED_ACCOUNTS, DynamoDbExtensionSchema.Tables.DELETED_ACCOUNTS_LOCK, DynamoDbExtensionSchema.Tables.NUMBERS, @@ -74,6 +75,7 @@ public class AccountCreationDeletionIntegrationTest { private AccountsManager accountsManager; private KeysManager keysManager; + private ClientPublicKeysManager clientPublicKeysManager; record DeliveryChannels(boolean fetchesMessages, String apnsToken, String apnsVoipToken, String fcmToken) {} @@ -93,6 +95,11 @@ public class AccountCreationDeletionIntegrationTest { DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName() ); + final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), + DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); + + clientPublicKeysManager = new ClientPublicKeysManager(clientPublicKeys); + final Accounts accounts = new Accounts( DYNAMO_DB_EXTENSION.getDynamoDbClient(), DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), @@ -142,6 +149,7 @@ public class AccountCreationDeletionIntegrationTest { svr2Client, mock(ClientPresenceManager.class), registrationRecoveryPasswordsManager, + clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, CLOCK); @@ -451,6 +459,8 @@ public class AccountCreationDeletionIntegrationTest { aciPqLastResortPreKey, pniPqLastResortPreKey)); + clientPublicKeysManager.setPublicKey(account.getIdentifier(IdentityType.ACI), Device.PRIMARY_ID, Curve.generateKeyPair().getPublicKey()).join(); + final UUID aci = account.getIdentifier(IdentityType.ACI); assertTrue(accountsManager.getByAccountIdentifier(aci).isPresent()); @@ -462,6 +472,7 @@ public class AccountCreationDeletionIntegrationTest { assertFalse(keysManager.getEcSignedPreKey(account.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); assertFalse(keysManager.getLastResort(account.getUuid(), Device.PRIMARY_ID).join().isPresent()); assertFalse(keysManager.getLastResort(account.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); + assertFalse(clientPublicKeysManager.findPublicKey(account.getUuid(), Device.PRIMARY_ID).join().isPresent()); } @SuppressWarnings("OptionalUsedAsFieldOrParameterType") diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java index eee6c5744..e8627e8ae 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -48,6 +48,7 @@ class AccountsManagerChangeNumberIntegrationTest { @RegisterExtension static final DynamoDbExtension DYNAMO_DB_EXTENSION = new DynamoDbExtension( Tables.ACCOUNTS, + Tables.CLIENT_PUBLIC_KEYS, Tables.DELETED_ACCOUNTS, Tables.DELETED_ACCOUNTS_LOCK, Tables.NUMBERS, @@ -87,6 +88,11 @@ class AccountsManagerChangeNumberIntegrationTest { Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName() ); + final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), + DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); + + final ClientPublicKeysManager clientPublicKeysManager = new ClientPublicKeysManager(clientPublicKeys); + final Accounts accounts = new Accounts( DYNAMO_DB_EXTENSION.getDynamoDbClient(), DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), @@ -137,6 +143,7 @@ class AccountsManagerChangeNumberIntegrationTest { svr2Client, clientPresenceManager, registrationRecoveryPasswordsManager, + clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, mock(Clock.class)); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index dd2274660..e297e1c7f 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -131,6 +131,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { mock(SecureValueRecovery2Client.class), mock(ClientPresenceManager.class), mock(RegistrationRecoveryPasswordsManager.class), + mock(ClientPublicKeysManager.class), mock(Executor.class), mock(Executor.class), mock(Clock.class) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index e3154a6b6..284773535 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -105,6 +105,7 @@ class AccountsManagerTest { private MessagesManager messagesManager; private ProfilesManager profilesManager; private ClientPresenceManager clientPresenceManager; + private ClientPublicKeysManager clientPublicKeysManager; private Map phoneNumberIdentifiersByE164; @@ -137,6 +138,7 @@ class AccountsManagerTest { messagesManager = mock(MessagesManager.class); profilesManager = mock(ProfilesManager.class); clientPresenceManager = mock(ClientPresenceManager.class); + clientPublicKeysManager = mock(ClientPublicKeysManager.class); final Executor clientPresenceExecutor = mock(Executor.class); @@ -231,6 +233,7 @@ class AccountsManagerTest { svr2Client, clientPresenceManager, registrationRecoveryPasswordsManager, + clientPublicKeysManager, mock(Executor.class), clientPresenceExecutor, clock); @@ -740,6 +743,7 @@ class AccountsManagerTest { verify(messagesManager, times(2)).clear(account.getUuid(), linkedDevice.getId()); verify(keysManager, times(2)).deleteSingleUsePreKeys(account.getUuid(), linkedDevice.getId()); verify(keysManager).buildWriteItemsForRemovedDevice(account.getUuid(), account.getPhoneNumberIdentifier(), linkedDevice.getId()); + verify(clientPublicKeysManager).buildTransactWriteItemForDeletion(account.getUuid(), linkedDevice.getId()); verify(clientPresenceManager).disconnectPresence(account.getUuid(), linkedDevice.getId()); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java index c97c4ab25..176781d4d 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -143,6 +143,7 @@ class AccountsManagerUsernameIntegrationTest { mock(SecureValueRecovery2Client.class), mock(ClientPresenceManager.class), mock(RegistrationRecoveryPasswordsManager.class), + mock(ClientPublicKeysManager.class), Executors.newSingleThreadExecutor(), Executors.newSingleThreadExecutor(), mock(Clock.class)); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java index a01f25423..51d873d9e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java @@ -42,6 +42,7 @@ public class AddRemoveDeviceIntegrationTest { @RegisterExtension static final DynamoDbExtension DYNAMO_DB_EXTENSION = new DynamoDbExtension( DynamoDbExtensionSchema.Tables.ACCOUNTS, + DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS, DynamoDbExtensionSchema.Tables.DELETED_ACCOUNTS, DynamoDbExtensionSchema.Tables.DELETED_ACCOUNTS_LOCK, DynamoDbExtensionSchema.Tables.NUMBERS, @@ -62,6 +63,7 @@ public class AddRemoveDeviceIntegrationTest { private ExecutorService clientPresenceExecutor; private KeysManager keysManager; + private ClientPublicKeysManager clientPublicKeysManager; private MessagesManager messagesManager; private AccountsManager accountsManager; @@ -81,6 +83,11 @@ public class AddRemoveDeviceIntegrationTest { DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName() ); + final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), + DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); + + clientPublicKeysManager = new ClientPublicKeysManager(clientPublicKeys); + final Accounts accounts = new Accounts( DYNAMO_DB_EXTENSION.getDynamoDbClient(), DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), @@ -130,6 +137,7 @@ public class AddRemoveDeviceIntegrationTest { svr2Client, mock(ClientPresenceManager.class), registrationRecoveryPasswordsManager, + clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, CLOCK); @@ -221,6 +229,9 @@ public class AddRemoveDeviceIntegrationTest { final byte addedDeviceId = updatedAccountAndDevice.second().getId(); + clientPublicKeysManager.setPublicKey(account.getUuid(), Device.PRIMARY_ID, Curve.generateKeyPair().getPublicKey()).join(); + clientPublicKeysManager.setPublicKey(account.getUuid(), addedDeviceId, Curve.generateKeyPair().getPublicKey()).join(); + final Account updatedAccount = accountsManager.removeDevice(updatedAccountAndDevice.first(), addedDeviceId).join(); assertEquals(1, updatedAccount.getDevices().size()); @@ -229,11 +240,13 @@ public class AddRemoveDeviceIntegrationTest { assertFalse(keysManager.getEcSignedPreKey(updatedAccount.getPhoneNumberIdentifier(), addedDeviceId).join().isPresent()); assertFalse(keysManager.getLastResort(updatedAccount.getUuid(), addedDeviceId).join().isPresent()); assertFalse(keysManager.getLastResort(updatedAccount.getPhoneNumberIdentifier(), addedDeviceId).join().isPresent()); + assertFalse(clientPublicKeysManager.findPublicKey(updatedAccount.getUuid(), addedDeviceId).join().isPresent()); assertTrue(keysManager.getEcSignedPreKey(updatedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getEcSignedPreKey(updatedAccount.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getLastResort(updatedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getLastResort(updatedAccount.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); + assertTrue(clientPublicKeysManager.findPublicKey(updatedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); } @Test @@ -249,7 +262,6 @@ public class AddRemoveDeviceIntegrationTest { assertEquals(1, accountsManager.getByAccountIdentifier(account.getUuid()).orElseThrow().getDevices().size()); final UUID aci = account.getIdentifier(IdentityType.ACI); - final UUID pni = account.getIdentifier(IdentityType.PNI); final Pair updatedAccountAndDevice = accountsManager.addDevice(account, new DeviceSpec( @@ -278,16 +290,21 @@ public class AddRemoveDeviceIntegrationTest { final Account retrievedAccount = accountsManager.getByAccountIdentifierAsync(aci).join().orElseThrow(); + clientPublicKeysManager.setPublicKey(retrievedAccount.getUuid(), Device.PRIMARY_ID, Curve.generateKeyPair().getPublicKey()).join(); + clientPublicKeysManager.setPublicKey(retrievedAccount.getUuid(), addedDeviceId, Curve.generateKeyPair().getPublicKey()).join(); + assertEquals(2, retrievedAccount.getDevices().size()); assertTrue(keysManager.getEcSignedPreKey(retrievedAccount.getUuid(), addedDeviceId).join().isPresent()); assertTrue(keysManager.getEcSignedPreKey(retrievedAccount.getPhoneNumberIdentifier(), addedDeviceId).join().isPresent()); assertTrue(keysManager.getLastResort(retrievedAccount.getUuid(), addedDeviceId).join().isPresent()); assertTrue(keysManager.getLastResort(retrievedAccount.getPhoneNumberIdentifier(), addedDeviceId).join().isPresent()); + assertTrue(clientPublicKeysManager.findPublicKey(retrievedAccount.getUuid(), addedDeviceId).join().isPresent()); assertTrue(keysManager.getEcSignedPreKey(retrievedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getEcSignedPreKey(retrievedAccount.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getLastResort(retrievedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); assertTrue(keysManager.getLastResort(retrievedAccount.getPhoneNumberIdentifier(), Device.PRIMARY_ID).join().isPresent()); + assertTrue(clientPublicKeysManager.findPublicKey(retrievedAccount.getUuid(), Device.PRIMARY_ID).join().isPresent()); } }