Revert "Use key bundle name instead of key handle for HKDF "info" parameter"

This reverts commit c439e081.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Use key bundle name instead of key handle for HKDF "info" parameter
> 
> In the CryptAuth v2 Enrollment flow, a key derivation function (HKDF) is
> needed for symmetric key generation and symmetric key proof computation.
> The v2 Enrollment protocol specifies that the "info" input parameter to
> HKDF should be the key bundle name, e.g. "PublicKey" or "authzen";
> however, we were previously using the key handle.
> 
> We verified this through manual testing.
> 
> Bug: 899080
> Change-Id: I31e6d5d4aba819f17b960744bd70f80c07ea9867
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504310
> Commit-Queue: Josh Nohle <nohle@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#638378}

TBR=khorimoto@chromium.org,nohle@chromium.org

Change-Id: Ic53495e0755ed26a6ec121bac81dad7ee92ed341
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 899080
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1507278Reviewed-by: James Hawkins <jhawkins@chromium.org>
Commit-Queue: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638390}

chromeos/services/device_sync/cryptauth_key_creator.h

@@ -38,7 +38,7 @@ namespace device_sync {
 //   the CryptAuth server. Specifically,
 //
 //   |derived_key| =
-//       Hkdf(|secret|, salt="CryptAuth Enrollment", info=|key_bundle_name|).
+//       Hkdf(|secret|, salt="CryptAuth Enrollment", info=|derived_key_handle|).
 //
 //   The CryptAuth server's Diffie-Hellman key is passed into CreateKeys(),
 //   CreateKeys() generates the client side of the Diffie-Hellman handshake, and

chromeos/services/device_sync/cryptauth_key_creator_impl.cc

@@ -4,6 +4,7 @@
 
 #include "chromeos/services/device_sync/cryptauth_key_creator_impl.h"
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/no_destructor.h"
@@ -12,6 +13,7 @@
 #include "chromeos/services/device_sync/cryptauth_constants.h"
 #include "chromeos/services/device_sync/proto/cryptauth_common.pb.h"
 #include "crypto/hkdf.h"
+#include "crypto/random.h"
 
 namespace chromeos {
 
@@ -55,6 +57,17 @@ size_t NumBytesForSymmetricKeyType(cryptauthv2::KeyType key_type) {
   }
 }
 
+// Creates a handle by base64-encoding 32 random bytes.
+std::string CreateRandomHandle() {
+  std::string bytes(32, '\0');
+  crypto::RandBytes(base::WriteInto(&bytes, bytes.size()), bytes.size());
+
+  std::string handle;
+  base::Base64Encode(bytes, &handle);
+
+  return handle;
+}
+
 }  // namespace
 
 // static
@@ -153,13 +166,15 @@ void CryptAuthKeyCreatorImpl::StartKeyCreation() {
     // Enrollment" and the info should be the key handle. This process is
     // synchronous, unlike SecureMessageDelegate calls.
     if (IsValidSymmetricKeyType(key_data.type)) {
-      std::string derived_symmetric_key_material = crypto::HkdfSha256(
-          dh_handshake_secret_->symmetric_key(),
-          kCryptAuthSymmetricKeyDerivationSalt,
-          CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name),
-          NumBytesForSymmetricKeyType(key_data.type));
+      std::string handle =
+          key_data.handle ? *key_data.handle : CreateRandomHandle();
+      std::string derived_symmetric_key_material =
+          crypto::HkdfSha256(dh_handshake_secret_->symmetric_key(),
+                             kCryptAuthSymmetricKeyDerivationSalt, handle,
+                             NumBytesForSymmetricKeyType(key_data.type));
 
-      OnSymmetricKeyDerived(bundle_name, derived_symmetric_key_material);
+      OnSymmetricKeyDerived(bundle_name, derived_symmetric_key_material,
+                            handle);
 
       continue;
     }
@@ -201,7 +216,8 @@ void CryptAuthKeyCreatorImpl::OnAsymmetricKeyPairGenerated(
 
 void CryptAuthKeyCreatorImpl::OnSymmetricKeyDerived(
     CryptAuthKeyBundle::Name bundle_name,
-    const std::string& symmetric_key) {
+    const std::string& symmetric_key,
+    const std::string& handle) {
   DCHECK(keys_to_create_.size() > 0);
   DCHECK(!symmetric_key.empty());
 
@@ -209,7 +225,7 @@ void CryptAuthKeyCreatorImpl::OnSymmetricKeyDerived(
       keys_to_create_.find(bundle_name)->second;
 
   new_keys_.try_emplace(bundle_name, symmetric_key, create_key_data.status,
-                        create_key_data.type, create_key_data.handle);
+                        create_key_data.type, handle);
 
   keys_to_create_.erase(bundle_name);
   if (keys_to_create_.empty())

chromeos/services/device_sync/cryptauth_key_creator_impl.h

@@ -59,7 +59,8 @@ class CryptAuthKeyCreatorImpl : public CryptAuthKeyCreator {
                                     const std::string& public_key,
                                     const std::string& private_key);
   void OnSymmetricKeyDerived(CryptAuthKeyBundle::Name bundle_name,
-                             const std::string& symmetric_key);
+                             const std::string& symmetric_key,
+                             const std::string& handle);
 
   base::flat_map<CryptAuthKeyBundle::Name, CreateKeyData> keys_to_create_;
   base::flat_map<CryptAuthKeyBundle::Name, CryptAuthKey> new_keys_;

chromeos/services/device_sync/cryptauth_key_creator_impl_unittest.cc

@@ -138,7 +138,7 @@ class DeviceSyncCryptAuthKeyCreatorImplTest : public testing::Test {
 
     std::string expected_symmetric_key_material = crypto::HkdfSha256(
         expected_handshake_secret, kCryptAuthSymmetricKeyDerivationSalt,
-        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name),
+        key_to_create.handle ? *key_to_create.handle : "",
         NumBytesForSymmetricKeyType(key_to_create.type));
 
     return CryptAuthKey(expected_symmetric_key_material, key_to_create.status,

chromeos/services/device_sync/cryptauth_key_proof_computer.h

@@ -9,7 +9,6 @@
 
 #include "base/macros.h"
 #include "base/optional.h"
-#include "chromeos/services/device_sync/cryptauth_key_bundle.h"
 
 namespace chromeos {
 
@@ -26,7 +25,7 @@ class CryptAuthKey;
 //   Symmetric keys: The HMAC-SHA256 of the payload, using a key derived from
 //                   the input symmetric key. Schematically,
 //
-//       HMAC(HKDF(|symmetric_key|, |salt|, |info|), |payload|)
+//       HMAC(HKDF(|symmetric_key|, |salt|, info=|key_handle|), |payload|)
 //
 //   Asymmetric keys: A DER-encoded ECDSA signature (RFC 3279) of the
 //                    concatenation of the salt and payload strings.
@@ -34,12 +33,10 @@ class CryptAuthKey;
 //
 //       Sign(|private_key|, |salt| + |payload|)
 //
-// Specifically, the CryptAuth v2 Enrollment protocol states that
-//   1) |payload| = SyncKeysResponse::random_session_id,
-//   2)    |salt| = "CryptAuth Key Proof",
-//   3)    |info| = key bundle name (needed for symmetric keys only).
-// In the future, key crossproofs might be employed, where the payload will
-// consist of other key proofs.
+// The CryptAuth v2 Enrollment protocol states that the
+// SyncKeysResponse::random_session_id, sent by the CryptAuth server, be used as
+// the payload for key proofs. In the future, key crossproofs might be employed,
+// where the payload will consist of other key proofs.
 //
 // Requirements:
 //   - Currently, the only supported key types are RAW128 and RAW256 for
@@ -50,13 +47,10 @@ class CryptAuthKeyProofComputer {
   virtual ~CryptAuthKeyProofComputer() = default;
 
   // Returns null if key proof computation failed.
-  // Note: The parameter |info| must be non-null for symmetric keys, but it is
-  // not used for asymmetric keys.
   virtual base::Optional<std::string> ComputeKeyProof(
       const CryptAuthKey& key,
       const std::string& payload,
-      const std::string& salt,
-      const base::Optional<std::string>& info) = 0;
+      const std::string& salt) = 0;
 
   DISALLOW_COPY_AND_ASSIGN(CryptAuthKeyProofComputer);
 };

chromeos/services/device_sync/cryptauth_key_proof_computer_impl.cc

@@ -83,24 +83,21 @@ CryptAuthKeyProofComputerImpl::~CryptAuthKeyProofComputerImpl() = default;
 base::Optional<std::string> CryptAuthKeyProofComputerImpl::ComputeKeyProof(
     const CryptAuthKey& key,
     const std::string& payload,
-    const std::string& salt,
-    const base::Optional<std::string>& info) {
-  if (key.IsAsymmetricKey())
-    return ComputeAsymmetricKeyProof(key, payload, salt);
+    const std::string& salt) {
+  if (key.IsSymmetricKey())
+    return ComputeSymmetricKeyProof(key, payload, salt);
 
-  DCHECK(info);
-  return ComputeSymmetricKeyProof(key, payload, salt, *info);
+  return ComputeAsymmetricKeyProof(key, payload, salt);
 }
 
 base::Optional<std::string>
 CryptAuthKeyProofComputerImpl::ComputeSymmetricKeyProof(
     const CryptAuthKey& symmetric_key,
     const std::string& payload,
-    const std::string& salt,
-    const std::string& info) {
-  std::string derived_symmetric_key_material =
-      crypto::HkdfSha256(symmetric_key.symmetric_key(), salt, info,
-                         NumBytesForSymmetricKeyType(symmetric_key.type()));
+    const std::string& salt) {
+  std::string derived_symmetric_key_material = crypto::HkdfSha256(
+      symmetric_key.symmetric_key(), salt, symmetric_key.handle(),
+      NumBytesForSymmetricKeyType(symmetric_key.type()));
 
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);
   std::vector<unsigned char> signed_payload(hmac.DigestLength());

chromeos/services/device_sync/cryptauth_key_proof_computer_impl.h

@@ -10,7 +10,6 @@
 
 #include "base/macros.h"
 #include "base/optional.h"
-#include "chromeos/services/device_sync/cryptauth_key_bundle.h"
 #include "chromeos/services/device_sync/cryptauth_key_proof_computer.h"
 
 namespace chromeos {
@@ -35,11 +34,9 @@ class CryptAuthKeyProofComputerImpl : public CryptAuthKeyProofComputer {
   ~CryptAuthKeyProofComputerImpl() override;
 
   // CryptAuthKeyProofComputer:
-  base::Optional<std::string> ComputeKeyProof(
-      const CryptAuthKey& key,
-      const std::string& payload,
-      const std::string& salt,
-      const base::Optional<std::string>& info) override;
+  base::Optional<std::string> ComputeKeyProof(const CryptAuthKey& key,
+                                              const std::string& payload,
+                                              const std::string& salt) override;
 
  private:
   CryptAuthKeyProofComputerImpl();
@@ -47,8 +44,7 @@ class CryptAuthKeyProofComputerImpl : public CryptAuthKeyProofComputer {
   base::Optional<std::string> ComputeSymmetricKeyProof(
       const CryptAuthKey& symmetric_key,
       const std::string& payload,
-      const std::string& salt,
-      const std::string& info);
+      const std::string& salt);
   base::Optional<std::string> ComputeAsymmetricKeyProof(
       const CryptAuthKey& asymmetric_key,
       const std::string& payload,

chromeos/services/device_sync/cryptauth_key_proof_computer_impl_unittest.cc

@@ -107,8 +107,7 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
-          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt,
-                            base::nullopt /* info */);
+          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt);
   EXPECT_TRUE(key_proof);
 
   // Verify the key proof which should be of the form:
@@ -127,19 +126,18 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
 TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
      Symmetric256KeyProofComputation_Success) {
   CryptAuthKey key(ByteVectorToString(kTestSymmetricKeyBytes),
-                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW256);
+                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW256,
+                   ByteVectorToString(kSymmetricTestInfoBytes));
 
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
           ->ComputeKeyProof(key, kTestPayload,
-                            ByteVectorToString(kSymmetricTestSaltBytes),
-                            ByteVectorToString(kSymmetricTestInfoBytes));
-
+                            ByteVectorToString(kSymmetricTestSaltBytes));
   EXPECT_TRUE(key_proof);
 
   // Verify the key proof which should be of the form:
-  //     HMAC(HKDF(|key|, |salt|, |info|), |payload|)
+  //     HMAC(HKDF(|key|, |salt|, info=<key handle>), |payload|)
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);
   EXPECT_TRUE(
       hmac.Init(ByteVectorToString(kExpectedDerivedSymmetricKey32Bytes)));
@@ -149,14 +147,14 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
 TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
      Symmetric128KeyProofComputation_Success) {
   CryptAuthKey key(ByteVectorToString(kTestSymmetricKeyBytes),
-                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW128);
+                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW128,
+                   ByteVectorToString(kSymmetricTestInfoBytes));
 
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
           ->ComputeKeyProof(key, kTestPayload,
-                            ByteVectorToString(kSymmetricTestSaltBytes),
-                            ByteVectorToString(kSymmetricTestInfoBytes));
+                            ByteVectorToString(kSymmetricTestSaltBytes));
   EXPECT_TRUE(key_proof);
 
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);
@@ -173,8 +171,7 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
-          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt,
-                            base::nullopt /* info */);
+          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt);
 
   EXPECT_FALSE(key_proof);
 }

chromeos/services/device_sync/cryptauth_v2_enroller_impl.cc

@@ -701,12 +701,10 @@ void CryptAuthV2EnrollerImpl::OnKeysCreated(
     const CryptAuthKeyBundle::Name& bundle_name = name_key_pair.first;
     const CryptAuthKey& new_key = name_key_pair.second;
 
-    std::string bundle_name_str =
-        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name);
-
     EnrollSingleKeyRequest* single_key_request =
         request.add_enroll_single_key_requests();
-    single_key_request->set_key_name(bundle_name_str);
+    single_key_request->set_key_name(
+        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name));
     single_key_request->set_new_key_handle(new_key.handle());
     if (new_key.IsAsymmetricKey())
       single_key_request->set_key_material(new_key.public_key());
@@ -715,7 +713,7 @@ void CryptAuthV2EnrollerImpl::OnKeysCreated(
     // SyncKeysResponse as the payload and the particular salt specified by the
     // v2 Enrollment protocol.
     base::Optional<std::string> key_proof = key_proof_computer->ComputeKeyProof(
-        new_key, session_id, kCryptAuthKeyProofSalt, bundle_name_str);
+        new_key, session_id, kCryptAuthKeyProofSalt);
     if (!key_proof || key_proof->empty()) {
       FinishAttempt(CryptAuthEnrollmentResult::ResultCode::
                         kErrorKeyProofComputationFailed);

chromeos/services/device_sync/cryptauth_v2_enroller_impl_unittest.cc

@@ -529,10 +529,8 @@ class DeviceSyncCryptAuthV2EnrollerImplTest
         enroll_keys_request()->enroll_single_key_requests(
             GetKeyBundleIndex(bundle_name));
 
-    std::string bundle_name_str =
-        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name);
-
-    EXPECT_EQ(bundle_name_str, single_request_user_key_pair.key_name());
+    EXPECT_EQ(CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name),
+              single_request_user_key_pair.key_name());
 
     EXPECT_EQ(new_key.handle(), single_request_user_key_pair.new_key_handle());
 
@@ -544,7 +542,7 @@ class DeviceSyncCryptAuthV2EnrollerImplTest
     EXPECT_EQ(CryptAuthKeyProofComputerImpl::Factory::Get()
                   ->BuildInstance()
                   ->ComputeKeyProof(new_key, kRandomSessionId,
-                                    kCryptAuthKeyProofSalt, bundle_name_str),
+                                    kCryptAuthKeyProofSalt),
               single_request_user_key_pair.key_proof());
   }
 

chromeos/services/device_sync/fake_cryptauth_key_proof_computer.cc

@@ -24,13 +24,12 @@ FakeCryptAuthKeyProofComputer::~FakeCryptAuthKeyProofComputer() = default;
 base::Optional<std::string> FakeCryptAuthKeyProofComputer::ComputeKeyProof(
     const CryptAuthKey& key,
     const std::string& payload,
-    const std::string& salt,
-    const base::Optional<std::string>& info) {
+    const std::string& salt) {
   if (should_return_null_)
     return base::nullopt;
 
-  return kFakeKeyProofPrefix + std::string("_") + std::string("_") + payload +
-         std::string("_") + salt + (info ? "_" + *info : "");
+  return kFakeKeyProofPrefix + std::string("_") + key.handle() +
+         std::string("_") + payload + std::string("_") + salt;
 }
 
 }  // namespace device_sync

chromeos/services/device_sync/fake_cryptauth_key_proof_computer.h

@@ -8,8 +8,6 @@
 #include <string>
 
 #include "base/macros.h"
-#include "base/optional.h"
-#include "chromeos/services/device_sync/cryptauth_key_bundle.h"
 #include "chromeos/services/device_sync/cryptauth_key_proof_computer.h"
 
 namespace chromeos {
@@ -24,12 +22,10 @@ class FakeCryptAuthKeyProofComputer : public CryptAuthKeyProofComputer {
   ~FakeCryptAuthKeyProofComputer() override;
 
   // CryptAuthKeyProofComputer:
-  // Returns "fake_key_proof_|payload|>_<|salt|_|info (if not null)|".
-  base::Optional<std::string> ComputeKeyProof(
-      const CryptAuthKey& key,
-      const std::string& payload,
-      const std::string& salt,
-      const base::Optional<std::string>& info) override;
+  // Returns "fake_key_proof_<key handle>_<payload>_<salt>".
+  base::Optional<std::string> ComputeKeyProof(const CryptAuthKey& key,
+                                              const std::string& payload,
+                                              const std::string& salt) override;
 
   void set_should_return_null(bool should_return_null) {
     should_return_null_ = should_return_null;