Export ECPrivateKey directly from SyncService

- As a follow-up of https://crrev.com/c/1841451, return the ECPrivateKey
  directly to avoid unnecessary hashing

Bug: 1010968
Change-Id: Iff13d775f511ba031eba33d63e4d47403d205280
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1863297
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706396}

chrome/browser/sync/test/integration/single_client_custom_passphrase_sync_test.cc

@@ -13,7 +13,7 @@
 #include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/nigori/cryptographer.h"
 #include "content/public/test/test_launcher.h"
-#include "crypto/sha2.h"
+#include "crypto/ec_private_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace {
@@ -273,7 +273,7 @@ IN_PROC_BROWSER_TEST_P(SingleClientCustomPassphraseSyncTestWithUssTests,
 }
 
 IN_PROC_BROWSER_TEST_P(SingleClientCustomPassphraseSyncTestWithUssTests,
-                       ShouldExposeExperimentalAuthenticationId) {
+                       ShouldExposeExperimentalAuthenticationKey) {
   const std::vector<std::string>& keystore_keys =
       GetFakeServer()->GetKeystoreKeys();
   ASSERT_THAT(keystore_keys, SizeIs(1));
@@ -284,27 +284,38 @@ IN_PROC_BROWSER_TEST_P(SingleClientCustomPassphraseSyncTestWithUssTests,
   SetupSyncNoWaitingForCompletion();
   ASSERT_TRUE(WaitForPassphraseRequiredState(/*desired_state=*/true));
 
-  // WARNING: Do *NOT* change these values since the authentication ID should be
-  // stable across different browser versions.
+  // WARNING: Do *NOT* change these values since the authentication key should
+  // be stable across different browser versions.
 
   // Default birthday determined by LoopbackServer.
   const std::string kDefaultBirthday = "0";
   const std::string kSeparator("|");
   std::string base64_encoded_keystore_key;
   base::Base64Encode(keystore_keys.back(), &base64_encoded_keystore_key);
-  const std::string authentication_id_before_hashing =
+  const std::string expected_authentication_secret =
       std::string("gaia_id_for_user_gmail.com") + kSeparator +
       kDefaultBirthday + kSeparator + base64_encoded_keystore_key;
 
-  EXPECT_EQ(GetSyncService()->GetExperimentalAuthenticationId(),
-            crypto::SHA256HashString(authentication_id_before_hashing));
+  EXPECT_EQ(GetSyncService()->GetExperimentalAuthenticationSecretForTest(),
+            expected_authentication_secret);
+  std::unique_ptr<crypto::ECPrivateKey> actual_key_1 =
+      GetSyncService()->GetExperimentalAuthenticationKey();
+  ASSERT_TRUE(actual_key_1);
+  std::vector<uint8_t> actual_private_key_1;
+  EXPECT_TRUE(actual_key_1->ExportPrivateKey(&actual_private_key_1));
 
-  // Entering the passphrase should not influence the authentication ID.
+  // Entering the passphrase should not influence the authentication key.
   ASSERT_TRUE(
       GetSyncService()->GetUserSettings()->SetDecryptionPassphrase("hunter2"));
   ASSERT_TRUE(WaitForPassphraseRequiredState(/*desired_state=*/false));
-  EXPECT_EQ(GetSyncService()->GetExperimentalAuthenticationId(),
-            crypto::SHA256HashString(authentication_id_before_hashing));
+  EXPECT_EQ(GetSyncService()->GetExperimentalAuthenticationSecretForTest(),
+            expected_authentication_secret);
+  std::unique_ptr<crypto::ECPrivateKey> actual_key_2 =
+      GetSyncService()->GetExperimentalAuthenticationKey();
+  ASSERT_TRUE(actual_key_2);
+  std::vector<uint8_t> actual_private_key_2;
+  EXPECT_TRUE(actual_key_2->ExportPrivateKey(&actual_private_key_2));
+  EXPECT_EQ(actual_private_key_1, actual_private_key_2);
 }
 
 INSTANTIATE_TEST_SUITE_P(USS,

chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc

@@ -17,7 +17,7 @@
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/nigori.h"
-#include "crypto/sha2.h"
+#include "crypto/ec_private_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace {
@@ -199,7 +199,7 @@ IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
 }
 
 IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
-                       ShouldExposeExperimentalAuthenticationId) {
+                       ShouldExposeExperimentalAuthenticationKey) {
   const std::vector<std::string>& keystore_keys =
       GetFakeServer()->GetKeystoreKeys();
   ASSERT_THAT(keystore_keys, SizeIs(1));
@@ -212,8 +212,8 @@ IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
 
   ASSERT_TRUE(SetupSync());
 
-  // WARNING: Do *NOT* change these values since the authentication ID should be
-  // stable across different browser versions.
+  // WARNING: Do *NOT* change these values since the authentication key should
+  // be stable across different browser versions.
 
   // Default birthday determined by LoopbackServer.
   const std::string kDefaultBirthday = "0";
@@ -224,8 +224,10 @@ IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
       std::string("gaia_id_for_user_gmail.com") + kSeparator +
       kDefaultBirthday + kSeparator + base64_encoded_keystore_key;
 
-  EXPECT_EQ(GetSyncService(/*index=*/0)->GetExperimentalAuthenticationId(),
-            crypto::SHA256HashString(authentication_id_before_hashing));
+  EXPECT_EQ(
+      GetSyncService(/*index=*/0)->GetExperimentalAuthenticationSecretForTest(),
+      authentication_id_before_hashing);
+  EXPECT_TRUE(GetSyncService(/*index=*/0)->GetExperimentalAuthenticationKey());
 }
 
 INSTANTIATE_TEST_SUITE_P(USS,

components/sync/driver/fake_sync_service.cc

@@ -9,6 +9,7 @@
 #include "components/sync/driver/sync_token_status.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 #include "components/sync/syncable/user_share.h"
+#include "crypto/ec_private_key.h"
 
 namespace syncer {
 
@@ -94,8 +95,9 @@ bool FakeSyncService::RequiresClientUpgrade() const {
   return false;
 }
 
-std::string FakeSyncService::GetExperimentalAuthenticationId() const {
-  return std::string();
+std::unique_ptr<crypto::ECPrivateKey>
+FakeSyncService::GetExperimentalAuthenticationKey() const {
+  return nullptr;
 }
 
 UserShare* FakeSyncService::GetUserShare() const {

components/sync/driver/fake_sync_service.h

@@ -46,7 +46,8 @@ class FakeSyncService : public SyncService {
   GoogleServiceAuthError GetAuthError() const override;
   base::Time GetAuthErrorTime() const override;
   bool RequiresClientUpgrade() const override;
-  std::string GetExperimentalAuthenticationId() const override;
+  std::unique_ptr<crypto::ECPrivateKey> GetExperimentalAuthenticationKey()
+      const override;
   UserShare* GetUserShare() const override;
   void DataTypePreconditionChanged(syncer::ModelType type) override;
   SyncTokenStatus GetSyncTokenStatusForDebugging() const override;

components/sync/driver/mock_sync_service.h

@@ -14,6 +14,7 @@
 #include "components/sync/driver/sync_token_status.h"
 #include "components/sync/driver/sync_user_settings_mock.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
+#include "crypto/ec_private_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace syncer {
@@ -40,7 +41,8 @@ class MockSyncService : public SyncService {
   MOCK_CONST_METHOD0(GetAuthError, GoogleServiceAuthError());
   MOCK_CONST_METHOD0(GetAuthErrorTime, base::Time());
   MOCK_CONST_METHOD0(RequiresClientUpgrade, bool());
-  MOCK_CONST_METHOD0(GetExperimentalAuthenticationId, std::string());
+  MOCK_CONST_METHOD0(GetExperimentalAuthenticationKey,
+                     std::unique_ptr<crypto::ECPrivateKey>());
 
   MOCK_METHOD0(GetSetupInProgressHandle,
                std::unique_ptr<SyncSetupInProgressHandle>());

components/sync/driver/profile_sync_service.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/strcat.h"
 #include "components/invalidation/public/invalidation_service.h"
 #include "components/signin/public/base/signin_metrics.h"
 #include "components/signin/public/identity_manager/account_info.h"
@@ -41,7 +42,7 @@
 #include "components/sync/model/sync_error.h"
 #include "components/sync/syncable/user_share.h"
 #include "components/version_info/version_info_values.h"
-#include "crypto/sha2.h"
+#include "crypto/ec_private_key.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace syncer {
@@ -1119,24 +1120,15 @@ bool ProfileSyncService::RequiresClientUpgrade() const {
   return last_actionable_error_.action == UPGRADE_CLIENT;
 }
 
-std::string ProfileSyncService::GetExperimentalAuthenticationId() const {
-  // Dependent fields are first populated when the sync engine is initialized,
-  // when usually all except keystore keys are guaranteed to be available.
-  // Keystore keys are usually available initially too, but in rare cases they
-  // should arrive in later sync cycles.
-  if (last_keystore_key_.empty()) {
-    return std::string();
+std::unique_ptr<crypto::ECPrivateKey>
+ProfileSyncService::GetExperimentalAuthenticationKey() const {
+  std::string secret = GetExperimentalAuthenticationSecret();
+  if (secret.empty()) {
+    return nullptr;
   }
 
-  // A separator is not strictly needed but it's adopted here as good practice.
-  const std::string kSeparator("|");
-  const std::string gaia_id = GetAuthenticatedAccountInfo().gaia;
-  const std::string birthday = sync_prefs_.GetBirthday();
-  DCHECK(!gaia_id.empty());
-  DCHECK(!birthday.empty());
-
-  return crypto::SHA256HashString(gaia_id + kSeparator + birthday + kSeparator +
-                                  last_keystore_key_);
+  return crypto::ECPrivateKey::DeriveFromSecret(
+      base::as_bytes(base::make_span(secret)));
 }
 
 bool ProfileSyncService::CanConfigureDataTypes(
@@ -1929,4 +1921,29 @@ void ProfileSyncService::ReconfigureDueToPassphrase(ConfigureReason reason) {
   NotifyObservers();
 }
 
+std::string ProfileSyncService::GetExperimentalAuthenticationSecretForTest()
+    const {
+  return GetExperimentalAuthenticationSecret();
+}
+
+std::string ProfileSyncService::GetExperimentalAuthenticationSecret() const {
+  // Dependent fields are first populated when the sync engine is initialized,
+  // when usually all except keystore keys are guaranteed to be available.
+  // Keystore keys are usually available initially too, but in rare cases they
+  // should arrive in later sync cycles.
+  if (last_keystore_key_.empty()) {
+    return std::string();
+  }
+
+  // A separator is not strictly needed but it's adopted here as good practice.
+  const std::string kSeparator("|");
+  const std::string gaia_id = GetAuthenticatedAccountInfo().gaia;
+  const std::string birthday = sync_prefs_.GetBirthday();
+  DCHECK(!gaia_id.empty());
+  DCHECK(!birthday.empty());
+
+  return base::StrCat(
+      {gaia_id, kSeparator, birthday, kSeparator, last_keystore_key_});
+}
+
 }  // namespace syncer

components/sync/driver/profile_sync_service.h

@@ -126,7 +126,8 @@ class ProfileSyncService : public SyncService,
   bool RequiresClientUpgrade() const override;
   std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle()
       override;
-  std::string GetExperimentalAuthenticationId() const override;
+  std::unique_ptr<crypto::ECPrivateKey> GetExperimentalAuthenticationKey()
+      const override;
   bool IsSetupInProgress() const override;
   ModelTypeSet GetRegisteredDataTypes() const override;
   ModelTypeSet GetPreferredDataTypes() const override;
@@ -260,6 +261,10 @@ class ProfileSyncService : public SyncService,
 
   SyncClient* GetSyncClientForTest();
 
+  // Combines GAIA ID, sync birthday and keystore key with '|' sepearator to
+  // generate a secret. Returns empty string if keystore key is not available.
+  std::string GetExperimentalAuthenticationSecretForTest() const;
+
  private:
   // Passed as an argument to StopImpl to control whether or not the sync
   // engine should clear its data directory when it shuts down. See StopImpl
@@ -361,6 +366,8 @@ class ProfileSyncService : public SyncService,
   // Called by SyncServiceCrypto when a passphrase is required or accepted.
   void ReconfigureDueToPassphrase(ConfigureReason reason);
 
+  std::string GetExperimentalAuthenticationSecret() const;
+
   // This profile's SyncClient, which abstracts away non-Sync dependencies and
   // the Sync API component factory.
   const std::unique_ptr<SyncClient> sync_client_;

components/sync/driver/profile_sync_service_unittest.cc

@@ -36,7 +36,7 @@
 #include "components/sync/engine/fake_sync_engine.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/version_info/version_info_values.h"
-#include "crypto/sha2.h"
+#include "crypto/ec_private_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/metrics_proto/user_demographics.pb.h"
@@ -1465,7 +1465,21 @@ TEST_F(ProfileSyncServiceTest,
   EXPECT_TRUE(HasBirthYearOffset(prefs()));
 }
 
-TEST_F(ProfileSyncServiceTest, GetExperimentalAuthenticationId) {
+TEST_F(ProfileSyncServiceTest, GetExperimentalAuthenticationKey) {
+  const std::vector<uint8_t> kExpectedPrivateKeyInfo = {
+      0x30, 0x81, 0x87, 0x02, 0x01, 0x00, 0x30, 0x13, 0x06, 0x07, 0x2a, 0x86,
+      0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d,
+      0x03, 0x01, 0x07, 0x04, 0x6d, 0x30, 0x6b, 0x02, 0x01, 0x01, 0x04, 0x20,
+      0xae, 0xf3, 0x15, 0x62, 0x31, 0x99, 0x3f, 0xe2, 0x96, 0xd4, 0xe6, 0x9c,
+      0x33, 0x25, 0x38, 0x58, 0x97, 0xcc, 0x40, 0x0d, 0xab, 0xbf, 0x2b, 0xb7,
+      0xd4, 0xcd, 0x79, 0xb9, 0x1f, 0x95, 0x19, 0x66, 0xa1, 0x44, 0x03, 0x42,
+      0x00, 0x04, 0x5e, 0xf4, 0x5d, 0x00, 0xaa, 0xea, 0xc9, 0x33, 0xed, 0xcd,
+      0xe5, 0xaf, 0xe6, 0x42, 0xef, 0x2b, 0xd2, 0xe0, 0xd6, 0x74, 0x5c, 0x90,
+      0x45, 0xad, 0x3f, 0x60, 0xfd, 0xc1, 0xcd, 0x09, 0x0a, 0x9a, 0xda, 0x3d,
+      0xf8, 0x18, 0xc6, 0x16, 0x46, 0x79, 0x53, 0x75, 0x92, 0xf2, 0x77, 0xcc,
+      0x38, 0x65, 0xa1, 0xcc, 0x79, 0xb3, 0x06, 0xd9, 0x9c, 0xb6, 0x8b, 0x96,
+      0x33, 0x88, 0x09, 0xc4, 0x07, 0x44};
+
   SignIn();
   CreateService(ProfileSyncService::AUTO_START);
   InitializeForNthSync();
@@ -1474,13 +1488,19 @@ TEST_F(ProfileSyncServiceTest, GetExperimentalAuthenticationId) {
 
   const std::string kSeparator("|");
   const std::string kGaiaId = signin::GetTestGaiaIdForEmail(kTestUser);
-
-  const std::string authentication_id_before_hashing =
+  const std::string expected_secret =
       kGaiaId + kSeparator + FakeSyncEngine::kTestBirthday + kSeparator +
       FakeSyncEngine::kTestKeystoreKey;
 
-  EXPECT_EQ(crypto::SHA256HashString(authentication_id_before_hashing),
-            service()->GetExperimentalAuthenticationId());
+  EXPECT_EQ(expected_secret,
+            service()->GetExperimentalAuthenticationSecretForTest());
+
+  std::unique_ptr<crypto::ECPrivateKey> actual_key =
+      service()->GetExperimentalAuthenticationKey();
+  ASSERT_TRUE(actual_key);
+  std::vector<uint8_t> actual_private_key;
+  EXPECT_TRUE(actual_key->ExportPrivateKey(&actual_private_key));
+  EXPECT_EQ(kExpectedPrivateKeyInfo, actual_private_key);
 }
 
 }  // namespace

components/sync/driver/sync_service.h

@@ -22,6 +22,10 @@ struct CoreAccountInfo;
 class GoogleServiceAuthError;
 class GURL;
 
+namespace crypto {
+class ECPrivateKey;
+}  // namespace crypto
+
 namespace syncer {
 
 class JsController;
@@ -240,13 +244,14 @@ class SyncService : public KeyedService {
   // Sync to work.
   virtual bool RequiresClientUpgrade() const = 0;
 
-  // Returns a high-entropy randomly-generated ID that is unique to a user and
-  // sync-ed across devices via Nigori. Populated when the transport state
-  // becomes CONFIGURING. Returns an empty string if not available. Consumers
-  // of this ID should observe for changes via
+  // Returns a high-entropy elliptic curve (EC) private key that is unique to a
+  // user and sync-ed across devices via Nigori. Populated when the transport
+  // state becomes CONFIGURING. Returns nullptr if not available. Consumers of
+  // this key should observe for changes via
   // SyncServiceObserver::OnSyncCycleCompleted().
   // TODO(crbug.com/1012226): Remove when VAPID migration is over.
-  virtual std::string GetExperimentalAuthenticationId() const = 0;
+  virtual std::unique_ptr<crypto::ECPrivateKey>
+  GetExperimentalAuthenticationKey() const = 0;
 
   //////////////////////////////////////////////////////////////////////////////
   // DERIVED STATE ACCESS

components/sync/driver/test_sync_service.cc

@@ -11,6 +11,7 @@
 #include "components/sync/base/progress_marker_map.h"
 #include "components/sync/driver/sync_token_status.h"
 #include "components/sync/engine/cycle/model_neutral_state.h"
+#include "crypto/ec_private_key.h"
 
 namespace syncer {
 
@@ -170,8 +171,9 @@ bool TestSyncService::RequiresClientUpgrade() const {
          syncer::UPGRADE_CLIENT;
 }
 
-std::string TestSyncService::GetExperimentalAuthenticationId() const {
-  return std::string();
+std::unique_ptr<crypto::ECPrivateKey>
+TestSyncService::GetExperimentalAuthenticationKey() const {
+  return nullptr;
 }
 
 std::unique_ptr<SyncSetupInProgressHandle>

components/sync/driver/test_sync_service.h

@@ -63,7 +63,8 @@ class TestSyncService : public SyncService {
   GoogleServiceAuthError GetAuthError() const override;
   base::Time GetAuthErrorTime() const override;
   bool RequiresClientUpgrade() const override;
-  std::string GetExperimentalAuthenticationId() const override;
+  std::unique_ptr<crypto::ECPrivateKey> GetExperimentalAuthenticationKey()
+      const override;
 
   std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle()
       override;