From 63c8b275d14e1862e43879dd5edf6ed672efc3d0 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Tue, 2 Apr 2024 15:03:24 -0500 Subject: [PATCH] Return 401 instead of 404 on unknown backup-ids --- .../textsecuregcm/backup/BackupManager.java | 7 +++++-- .../controllers/ArchiveController.java | 7 +++++-- .../textsecuregcm/backup/BackupManagerTest.java | 13 +++++++------ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java index 4e37fd56e..071a3b97d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java @@ -23,6 +23,7 @@ import java.util.concurrent.CompletionStage; import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; +import org.signal.libsignal.protocol.ecc.Curve; import org.signal.libsignal.protocol.ecc.ECPublicKey; import org.signal.libsignal.zkgroup.GenericServerSecretParams; import org.signal.libsignal.zkgroup.VerificationFailedException; @@ -428,6 +429,7 @@ public CompletableFuture delete(final AuthenticatedBackupUser backupUser, .toFuture(); } + private static final ECPublicKey INVALID_PUBLIC_KEY = Curve.generateKeyPair().getPublicKey(); /** * Authenticate the ZK anonymous backup credential's presentation *

@@ -449,12 +451,13 @@ public CompletableFuture authenticateBackupUser( .retrieveAuthenticationData(presentation.getBackupId()) .thenApply(optionalAuthenticationData -> { final BackupsDb.AuthenticationData authenticationData = optionalAuthenticationData - .orElseThrow(() -> { + .orElseGet(() -> { Metrics.counter(ZK_AUTHN_COUNTER_NAME, SUCCESS_TAG_NAME, String.valueOf(false), FAILURE_REASON_TAG_NAME, "missing_public_key") .increment(); - return Status.NOT_FOUND.withDescription("Backup not found").asRuntimeException(); + // There was no stored public key, use a bunk public key so that validation will fail + return new BackupsDb.AuthenticationData(INVALID_PUBLIC_KEY, null, null); }); return new AuthenticatedBackupUser( presentation.getBackupId(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java index b7c24e403..36dd12f76 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java @@ -170,7 +170,10 @@ public CompletionStage getBackupZKCredentials( @ApiResponse( responseCode = "403", description = "Forbidden. The request had insufficient permissions to perform the requested action") - @ApiResponse(responseCode = "401", description = "The provided backup auth credential presentation could not be verified") + @ApiResponse(responseCode = "401", description = """ + The provided backup auth credential presentation could not be verified or + The public key signature was invalid or + There is no backup associated with the backup-id in the presentation""") @ApiResponse(responseCode = "400", description = "Bad arguments. The request may have been made on an authenticated channel") @interface ApiResponseZkAuth {} @@ -695,7 +698,7 @@ public CompletionStage listMedia( } return backupManager .authenticateBackupUser(presentation.presentation, signature.signature) - .thenCompose(backupUser ->backupManager.list(backupUser, cursor, limit.orElse(1000)) + .thenCompose(backupUser -> backupManager.list(backupUser, cursor, limit.orElse(1000)) .thenApply(result -> new ListResponse( result.media() .stream().map(entry -> new StoredMediaObject(entry.cdn(), entry.key(), entry.length())) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java index b08be325a..7478c8772 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java @@ -226,7 +226,7 @@ public void unknownPublicKey() throws VerificationFailedException { StatusRuntimeException.class, backupManager.authenticateBackupUser(presentation, signature)) .getStatus().getCode()) - .isEqualTo(Status.NOT_FOUND.getCode()); + .isEqualTo(Status.UNAUTHENTICATED.getCode()); } @Test @@ -306,10 +306,10 @@ public void credentialExpiration() throws VerificationFailedException { // should be rejected the day after that testClock.pin(Instant.ofEpochSecond(1).plus(Duration.ofDays(3))); - assertThat(CompletableFutureTestUtil.assertFailsWithCause( - StatusRuntimeException.class, - backupManager.authenticateBackupUser(oldCredential, signature)) - .getStatus().getCode()) + assertThatExceptionOfType(StatusRuntimeException.class) + .isThrownBy(() -> backupManager.authenticateBackupUser(oldCredential, signature)) + .extracting(StatusRuntimeException::getStatus) + .extracting(Status::getCode) .isEqualTo(Status.UNAUTHENTICATED.getCode()); } @@ -752,6 +752,7 @@ private AuthenticatedBackupUser backupUser(final byte[] backupId, final BackupTi } catch (InvalidKeyException e) { throw new RuntimeException(e); } - return new AuthenticatedBackupUser(backupId, backupTier, BackupsDb.generateDirName(secureRandom), BackupsDb.generateDirName(secureRandom)); + return new AuthenticatedBackupUser(backupId, backupTier, BackupsDb.generateDirName(secureRandom), + BackupsDb.generateDirName(secureRandom)); } }