[TrustedVault] DownloadKeys: handle empty trusted vault key

If device was successfully registered with unknown epoch/trusted vault
key, first DownloadKeys call won't have any key available and incoming
keys validation should be skipped.

Note: strictly speaking, there is a constant key; but performing
validation using it doesn't add any value, while would make future
changes harder.

Bug: 1113598
Change-Id: Ib978987b4ba1304f40c54414ea79ef08d7ebb6ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2620262
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842088}

components/sync/trusted_vault/download_keys_response_handler.cc

@@ -146,7 +146,8 @@ DownloadKeysResponseHandler::ProcessedResponse::operator=(
 DownloadKeysResponseHandler::ProcessedResponse::~ProcessedResponse() = default;
 
 DownloadKeysResponseHandler::DownloadKeysResponseHandler(
-    const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+    const base::Optional<TrustedVaultKeyAndVersion>&
+        last_trusted_vault_key_and_version,
     std::unique_ptr<SecureBoxKeyPair> device_key_pair)
     : last_trusted_vault_key_and_version_(last_trusted_vault_key_and_version),
       device_key_pair_(std::move(device_key_pair)) {
@@ -186,15 +187,25 @@ DownloadKeysResponseHandler::ProcessResponse(
 
   std::vector<ExtractedSharedKey> extracted_keys = ExtractAndSortSharedKeys(
       *current_member, device_key_pair_->private_key());
-  if (extracted_keys.empty() ||
-      !IsValidKeyChain(extracted_keys, last_trusted_vault_key_and_version_)) {
+  if (extracted_keys.empty()) {
+    // |current_member| doesn't have any keys, should be treated as not
+    // registered member.
+    return ProcessedResponse(
+        /*status=*/TrustedVaultRequestStatus::kLocalDataObsolete);
+  }
+
+  if (last_trusted_vault_key_and_version_.has_value() &&
+      !IsValidKeyChain(extracted_keys, *last_trusted_vault_key_and_version_)) {
+    // Data corresponding to |current_member| is corrupted or
+    // |last_trusted_vault_key_and_version_| is too old.
     return ProcessedResponse(
         /*status=*/TrustedVaultRequestStatus::kLocalDataObsolete);
   }
 
   std::vector<std::vector<uint8_t>> new_keys;
   for (const ExtractedSharedKey& key : extracted_keys) {
-    if (key.version >= last_trusted_vault_key_and_version_.version) {
+    if (!last_trusted_vault_key_and_version_.has_value() ||
+        key.version >= last_trusted_vault_key_and_version_->version) {
       // Don't include previous keys into the result, because they weren't
       // validated using |last_trusted_vault_key_and_version| and client should
       // be already aware of them.

components/sync/trusted_vault/download_keys_response_handler.h

@@ -46,11 +46,12 @@ class DownloadKeysResponseHandler {
     int last_key_version;
   };
 
-  // |device_key_pair| must not be null. This class doesn't make an assumption
-  // of |last_trusted_vault_key_and_version.key| being non-empty or
-  // |last_trusted_vault_key_version_and_version.version| being non-negative.
+  // |device_key_pair| must not be null. If |last_trusted_vault_key_and_version|
+  // is provided, then it will be verified that the new keys are result of
+  // rotating the provided key.
   DownloadKeysResponseHandler(
-      const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+      const base::Optional<TrustedVaultKeyAndVersion>&
+          last_trusted_vault_key_and_version,
       std::unique_ptr<SecureBoxKeyPair> device_key_pair);
   DownloadKeysResponseHandler(const DownloadKeysResponseHandler& other) =
       delete;
@@ -62,7 +63,8 @@ class DownloadKeysResponseHandler {
                                     const std::string& response_body) const;
 
  private:
-  const TrustedVaultKeyAndVersion last_trusted_vault_key_and_version_;
+  const base::Optional<TrustedVaultKeyAndVersion>
+      last_trusted_vault_key_and_version_;
   const std::unique_ptr<SecureBoxKeyPair> device_key_pair_;
 };
 

components/sync/trusted_vault/download_keys_response_handler_unittest.cc

@@ -475,6 +475,30 @@ TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleEmptyMember) {
               Eq(TrustedVaultRequestStatus::kLocalDataObsolete));
 }
 
+// Test scenario, when no trusted vault keys available on the current device
+// (e.g. device was registered with constant key). In this case new keys
+// shouldn't be validated.
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleEmptyLastKnownKey) {
+  // This test uses custom parameters for the handler ctor, so create new
+  // handler instead of using one from the fixture.
+  DownloadKeysResponseHandler handler(base::nullopt, MakeTestKeyPair());
+
+  const int kLastKeyVersion = 123;
+  const DownloadKeysResponseHandler::ProcessedResponse processed_response =
+      handler.ProcessResponse(
+          /*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
+          /*response_body=*/
+          CreateListSecurityDomainsResponseWithSingleSyncMember(
+              /*trusted_vault_keys=*/{kTrustedVaultKey1},
+              /*trusted_vault_keys_versions=*/{kLastKeyVersion},
+              /*signing_keys=*/{{}}));
+
+  EXPECT_THAT(processed_response.status,
+              Eq(TrustedVaultRequestStatus::kSuccess));
+  EXPECT_THAT(processed_response.keys, ElementsAre(kTrustedVaultKey1));
+  EXPECT_THAT(processed_response.last_key_version, Eq(kLastKeyVersion));
+}
+
 }  // namespace
 
 }  // namespace syncer

components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc

@@ -42,14 +42,6 @@ MATCHER_P(KeyMaterialEq, expected, "") {
   return key_material_as_bytes == expected;
 }
 
-// TODO(crbug.com/1102340): leave only Optional* version of this matcher once
-// TrustedVaultConnection::DownloadKeys() accept optional key.
-MATCHER_P2(TrustedVaultKeyAndVersionEq, expected_key, expected_version, "") {
-  const TrustedVaultKeyAndVersion& key_and_version = arg;
-  return key_and_version.key == expected_key &&
-         key_and_version.version == expected_version;
-}
-
 MATCHER_P2(OptionalTrustedVaultKeyAndVersionEq,
            expected_key,
            expected_version,
@@ -83,11 +75,11 @@ class MockTrustedVaultConnection : public TrustedVaultConnection {
                const SecureBoxPublicKey& authentication_factor_public_key,
                RegisterAuthenticationFactorCallback callback),
               (override));
-  MOCK_METHOD(
-      std::unique_ptr<Request>,
+  MOCK_METHOD(std::unique_ptr<Request>,
               DownloadKeys,
               (const CoreAccountInfo& account_info,
-       const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+               const base::Optional<TrustedVaultKeyAndVersion>&
+                   last_trusted_vault_key_and_version,
                std::unique_ptr<SecureBoxKeyPair> device_key_pair,
                DownloadKeysCallback callback),
               (override));
@@ -515,10 +507,11 @@ TEST_F(StandaloneTrustedVaultBackendTest, ShouldDownloadKeys) {
   TrustedVaultConnection::DownloadKeysCallback download_keys_callback;
   EXPECT_CALL(*connection(),
               DownloadKeys(Eq(account_info),
-                           TrustedVaultKeyAndVersionEq(kInitialVaultKey,
-                                                       kInitialLastKeyVersion),
+                           OptionalTrustedVaultKeyAndVersionEq(
+                               kInitialVaultKey, kInitialLastKeyVersion),
                            _, _))
-      .WillOnce([&](const CoreAccountInfo&, const TrustedVaultKeyAndVersion&,
+      .WillOnce([&](const CoreAccountInfo&,
+                    const base::Optional<TrustedVaultKeyAndVersion>&,
                     std::unique_ptr<SecureBoxKeyPair> key_pair,
                     TrustedVaultConnection::DownloadKeysCallback callback) {
         device_key_pair = std::move(key_pair);
@@ -562,7 +555,8 @@ TEST_F(StandaloneTrustedVaultBackendTest,
   TrustedVaultConnection::DownloadKeysCallback download_keys_callback;
   ON_CALL(*connection(), DownloadKeys(_, _, _, _))
       .WillByDefault(
-          [&](const CoreAccountInfo&, const TrustedVaultKeyAndVersion&,
+          [&](const CoreAccountInfo&,
+              const base::Optional<TrustedVaultKeyAndVersion>&,
               std::unique_ptr<SecureBoxKeyPair> key_pair,
               TrustedVaultConnection::DownloadKeysCallback callback) {
             download_keys_callback = std::move(callback);

components/sync/trusted_vault/trusted_vault_connection.h

@@ -84,7 +84,8 @@ class TrustedVaultConnection {
   // or until request needs to be cancelled.
   virtual std::unique_ptr<Request> DownloadKeys(
       const CoreAccountInfo& account_info,
-      const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+      const base::Optional<TrustedVaultKeyAndVersion>&
+          last_trusted_vault_key_and_version,
       std::unique_ptr<SecureBoxKeyPair> device_key_pair,
       DownloadKeysCallback callback) WARN_UNUSED_RESULT = 0;
 };

components/sync/trusted_vault/trusted_vault_connection_impl.cc

@@ -78,7 +78,8 @@ TrustedVaultConnectionImpl::RegisterAuthenticationFactor(
 std::unique_ptr<TrustedVaultConnection::Request>
 TrustedVaultConnectionImpl::DownloadKeys(
     const CoreAccountInfo& account_info,
-    const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+    const base::Optional<TrustedVaultKeyAndVersion>&
+        last_trusted_vault_key_and_version,
     std::unique_ptr<SecureBoxKeyPair> device_key_pair,
     DownloadKeysCallback callback) {
   auto request = std::make_unique<TrustedVaultRequest>(

components/sync/trusted_vault/trusted_vault_connection_impl.h

@@ -45,7 +45,8 @@ class TrustedVaultConnectionImpl : public TrustedVaultConnection {
 
   std::unique_ptr<Request> DownloadKeys(
       const CoreAccountInfo& account_info,
-      const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
+      const base::Optional<TrustedVaultKeyAndVersion>&
+          last_trusted_vault_key_and_version,
       std::unique_ptr<SecureBoxKeyPair> device_key_pair,
       DownloadKeysCallback callback) override;