Return 401 instead of 404 on unknown backup-ids

signalapp · Apr 4, 2024 · 63c8b27 · 63c8b27
1 parent 1ebc173
commit 63c8b27
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 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
@@ -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<Void> delete(final AuthenticatedBackupUser backupUser,
         .toFuture();
   }
 
+  private static final ECPublicKey INVALID_PUBLIC_KEY = Curve.generateKeyPair().getPublicKey();
   /**
    * Authenticate the ZK anonymous backup credential's presentation
    * <p>
@@ -449,12 +451,13 @@ public CompletableFuture<AuthenticatedBackupUser> 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
@@ -170,7 +170,10 @@ public CompletionStage<BackupAuthCredentialsResponse> 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<ListResponse> 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
@@ -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));
   }
 }