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}

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=|derived_key_handle|).
+//       Hkdf(|secret|, salt="CryptAuth Enrollment", info=|key_bundle_name|).
 //
 //   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,7 +4,6 @@
 
 #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"
@@ -13,7 +12,6 @@
 #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 {
 
@@ -57,17 +55,6 @@ 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
@@ -166,15 +153,13 @@ 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 handle =
-          key_data.handle ? *key_data.handle : CreateRandomHandle();
-      std::string derived_symmetric_key_material =
-          crypto::HkdfSha256(dh_handshake_secret_->symmetric_key(),
-                             kCryptAuthSymmetricKeyDerivationSalt, handle,
+      std::string derived_symmetric_key_material = crypto::HkdfSha256(
+          dh_handshake_secret_->symmetric_key(),
+          kCryptAuthSymmetricKeyDerivationSalt,
+          CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name),
           NumBytesForSymmetricKeyType(key_data.type));
 
-      OnSymmetricKeyDerived(bundle_name, derived_symmetric_key_material,
-                            handle);
+      OnSymmetricKeyDerived(bundle_name, derived_symmetric_key_material);
 
       continue;
     }
@@ -216,8 +201,7 @@ void CryptAuthKeyCreatorImpl::OnAsymmetricKeyPairGenerated(
 
 void CryptAuthKeyCreatorImpl::OnSymmetricKeyDerived(
     CryptAuthKeyBundle::Name bundle_name,
-    const std::string& symmetric_key,
-    const std::string& handle) {
+    const std::string& symmetric_key) {
   DCHECK(keys_to_create_.size() > 0);
   DCHECK(!symmetric_key.empty());
 
@@ -225,7 +209,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, handle);
+                        create_key_data.type, create_key_data.handle);
 
   keys_to_create_.erase(bundle_name);
   if (keys_to_create_.empty())

chromeos/services/device_sync/cryptauth_key_creator_impl.h

@@ -59,8 +59,7 @@ 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& handle);
+                             const std::string& symmetric_key);
 
   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,
-        key_to_create.handle ? *key_to_create.handle : "",
+        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name),
         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,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/optional.h"
+#include "chromeos/services/device_sync/cryptauth_key_bundle.h"
 
 namespace chromeos {
 
@@ -25,7 +26,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=|key_handle|), |payload|)
+//       HMAC(HKDF(|symmetric_key|, |salt|, |info|), |payload|)
 //
 //   Asymmetric keys: A DER-encoded ECDSA signature (RFC 3279) of the
 //                    concatenation of the salt and payload strings.
@@ -33,10 +34,12 @@ class CryptAuthKey;
 //
 //       Sign(|private_key|, |salt| + |payload|)
 //
-// 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.
+// 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.
 //
 // Requirements:
 //   - Currently, the only supported key types are RAW128 and RAW256 for
@@ -47,10 +50,13 @@ 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) = 0;
+      const std::string& salt,
+      const base::Optional<std::string>& info) = 0;
 
   DISALLOW_COPY_AND_ASSIGN(CryptAuthKeyProofComputer);
 };

chromeos/services/device_sync/cryptauth_key_proof_computer_impl.cc

@@ -83,20 +83,23 @@ CryptAuthKeyProofComputerImpl::~CryptAuthKeyProofComputerImpl() = default;
 base::Optional<std::string> CryptAuthKeyProofComputerImpl::ComputeKeyProof(
     const CryptAuthKey& key,
     const std::string& payload,
-    const std::string& salt) {
-  if (key.IsSymmetricKey())
-    return ComputeSymmetricKeyProof(key, payload, salt);
-
+    const std::string& salt,
+    const base::Optional<std::string>& info) {
+  if (key.IsAsymmetricKey())
     return ComputeAsymmetricKeyProof(key, payload, salt);
+
+  DCHECK(info);
+  return ComputeSymmetricKeyProof(key, payload, salt, *info);
 }
 
 base::Optional<std::string>
 CryptAuthKeyProofComputerImpl::ComputeSymmetricKeyProof(
     const CryptAuthKey& symmetric_key,
     const std::string& payload,
-    const std::string& salt) {
-  std::string derived_symmetric_key_material = crypto::HkdfSha256(
-      symmetric_key.symmetric_key(), salt, symmetric_key.handle(),
+    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()));
 
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);

chromeos/services/device_sync/cryptauth_key_proof_computer_impl.h

@@ -10,6 +10,7 @@
 
 #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 {
@@ -34,9 +35,11 @@ class CryptAuthKeyProofComputerImpl : public CryptAuthKeyProofComputer {
   ~CryptAuthKeyProofComputerImpl() override;
 
   // CryptAuthKeyProofComputer:
-  base::Optional<std::string> ComputeKeyProof(const CryptAuthKey& key,
+  base::Optional<std::string> ComputeKeyProof(
+      const CryptAuthKey& key,
       const std::string& payload,
-                                              const std::string& salt) override;
+      const std::string& salt,
+      const base::Optional<std::string>& info) override;
 
  private:
   CryptAuthKeyProofComputerImpl();
@@ -44,7 +47,8 @@ class CryptAuthKeyProofComputerImpl : public CryptAuthKeyProofComputer {
   base::Optional<std::string> ComputeSymmetricKeyProof(
       const CryptAuthKey& symmetric_key,
       const std::string& payload,
-      const std::string& salt);
+      const std::string& salt,
+      const std::string& info);
   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,7 +107,8 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
-          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt);
+          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt,
+                            base::nullopt /* info */);
   EXPECT_TRUE(key_proof);
 
   // Verify the key proof which should be of the form:
@@ -126,18 +127,19 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
 TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
      Symmetric256KeyProofComputation_Success) {
   CryptAuthKey key(ByteVectorToString(kTestSymmetricKeyBytes),
-                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW256,
-                   ByteVectorToString(kSymmetricTestInfoBytes));
+                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW256);
 
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
           ->ComputeKeyProof(key, kTestPayload,
-                            ByteVectorToString(kSymmetricTestSaltBytes));
+                            ByteVectorToString(kSymmetricTestSaltBytes),
+                            ByteVectorToString(kSymmetricTestInfoBytes));
+
   EXPECT_TRUE(key_proof);
 
   // Verify the key proof which should be of the form:
-  //     HMAC(HKDF(|key|, |salt|, info=<key handle>), |payload|)
+  //     HMAC(HKDF(|key|, |salt|, |info|), |payload|)
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);
   EXPECT_TRUE(
       hmac.Init(ByteVectorToString(kExpectedDerivedSymmetricKey32Bytes)));
@@ -147,14 +149,14 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
 TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
      Symmetric128KeyProofComputation_Success) {
   CryptAuthKey key(ByteVectorToString(kTestSymmetricKeyBytes),
-                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW128,
-                   ByteVectorToString(kSymmetricTestInfoBytes));
+                   CryptAuthKey::Status::kActive, cryptauthv2::KeyType::RAW128);
 
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
           ->ComputeKeyProof(key, kTestPayload,
-                            ByteVectorToString(kSymmetricTestSaltBytes));
+                            ByteVectorToString(kSymmetricTestSaltBytes),
+                            ByteVectorToString(kSymmetricTestInfoBytes));
   EXPECT_TRUE(key_proof);
 
   crypto::HMAC hmac(crypto::HMAC::HashAlgorithm::SHA256);
@@ -171,7 +173,8 @@ TEST(DeviceSyncCryptAuthKeyProofComputerImplTest,
   base::Optional<std::string> key_proof =
       CryptAuthKeyProofComputerImpl::Factory::Get()
           ->BuildInstance()
-          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt);
+          ->ComputeKeyProof(key, kTestPayload, kAsymmetricTestSalt,
+                            base::nullopt /* info */);
 
   EXPECT_FALSE(key_proof);
 }

chromeos/services/device_sync/cryptauth_v2_enroller_impl.cc

@@ -701,10 +701,12 @@ 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(
-        CryptAuthKeyBundle::KeyBundleNameEnumToString(bundle_name));
+    single_key_request->set_key_name(bundle_name_str);
     single_key_request->set_new_key_handle(new_key.handle());
     if (new_key.IsAsymmetricKey())
       single_key_request->set_key_material(new_key.public_key());
@@ -713,7 +715,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);
+        new_key, session_id, kCryptAuthKeyProofSalt, bundle_name_str);
     if (!key_proof || key_proof->empty()) {
       FinishAttempt(CryptAuthEnrollmentResult::ResultCode::
                         kErrorKeyProofComputationFailed);

chromeos/services/device_sync/cryptauth_v2_enroller_impl_unittest.cc

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

chromeos/services/device_sync/fake_cryptauth_key_proof_computer.cc

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

chromeos/services/device_sync/fake_cryptauth_key_proof_computer.h

@@ -8,6 +8,8 @@
 #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 {
@@ -22,10 +24,12 @@ class FakeCryptAuthKeyProofComputer : public CryptAuthKeyProofComputer {
   ~FakeCryptAuthKeyProofComputer() override;
 
   // CryptAuthKeyProofComputer:
-  // Returns "fake_key_proof_<key handle>_<payload>_<salt>".
-  base::Optional<std::string> ComputeKeyProof(const CryptAuthKey& key,
+  // 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) override;
+      const std::string& salt,
+      const base::Optional<std::string>& info) override;
 
   void set_should_return_null(bool should_return_null) {
     should_return_null_ = should_return_null;