Revert "Use existing kUserKeyPair keys during key creation, if possible."

This reverts commit 657a8c53.

Reason for revert: ASAN errors; container overflow

Original change's description:
> Use existing kUserKeyPair keys during key creation, if possible.
> 
> In CyptAuth v2 Enrollment, the user key pair--also known as
> CryptAuthKeyBundle::Name::kUserKeyPair or "PublicKey"--has special
> standing in order to 1) accommodate any existing key from v1 Enrollment
> and 2) enforce that the key is not rotated. Only one user key pair
> should exist in its key bundle, and it should be an active, P-256 key
> with handle "device_key".
> 
> It is possible that CryptAuth could request the creation of a new user
> key pair even if the client sends information about an existing key in
> the SyncKeysRequest. If this happens, the client should re-use the
> existing user key pair key material when creating a new key. At the end
> of the enrollment flow, the existing key will be replaced with this new
> key that has the same public/private keys, but possibly with a new key
> directive.
> 
> It might seem more efficient to simply ignore the key-creation request,
> but the method outlined above natually fits into the enrollment flow,
> the key directive will be updated, and the client will be able to
> provide CryptAuth with an EnrollKeys request, which it might be
> expected.
> 
> Bug: 899080
> Change-Id: I3a4a63aa902090698ecb619bc7af78ff1e790c23
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504121
> Commit-Queue: Josh Nohle <nohle@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#638327}

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

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

chromeos/services/device_sync/cryptauth_enrollment_result.cc

@@ -116,11 +116,7 @@ std::ostream& operator<<(
       stream << "[Error: KeyActions do not specify an active key]";
       break;
     case ResultCode::kErrorKeyCreationKeyTypeNotSupported:
-      stream << "[Error: Key-creation instructions specify unsupported "
-             << "KeyType]";
-      break;
-    case ResultCode::kErrorUserKeyPairCreationInstructionsInvalid:
-      stream << "[Error: Key-creation instructions for user key pair invalid]";
+      stream << "[Error: KeyCreation instructions specify unsupported KeyType]";
       break;
     case ResultCode::kErrorSymmetricKeyCreationMissingServerDiffieHellman:
       stream << "[Error: Cannot create symmetric key; missing server "

chromeos/services/device_sync/cryptauth_enrollment_result.h

@@ -79,9 +79,6 @@ class CryptAuthEnrollmentResult {
     kErrorKeyActionsDoNotSpecifyAnActiveKey,
     // KeyCreation instructions specify an unsupported KeyType.
     kErrorKeyCreationKeyTypeNotSupported,
-    // Invalid key-creation instructions for user key pair. It must be P256 and
-    // active.
-    kErrorUserKeyPairCreationInstructionsInvalid,
     // Cannot create a symmetric key without the server's Diffie-Hellman key.
     kErrorSymmetricKeyCreationMissingServerDiffieHellman,
     // Failed to compute at least one key proof.

chromeos/services/device_sync/cryptauth_key_creator.cc

@@ -14,22 +14,6 @@ CryptAuthKeyCreator::CreateKeyData::CreateKeyData(
     base::Optional<std::string> handle)
     : status(status), type(type), handle(handle) {}
 
-CryptAuthKeyCreator::CreateKeyData::CreateKeyData(
-    CryptAuthKey::Status status,
-    cryptauthv2::KeyType type,
-    const std::string& handle,
-    const std::string& public_key,
-    const std::string& private_key)
-    : status(status),
-      type(type),
-      handle(handle),
-      public_key(public_key),
-      private_key(private_key) {
-  DCHECK(!handle.empty());
-  DCHECK(!public_key.empty());
-  DCHECK(!private_key.empty());
-}
-
 CryptAuthKeyCreator::CreateKeyData::~CreateKeyData() = default;
 
 CryptAuthKeyCreator::CreateKeyData::CreateKeyData(const CreateKeyData&) =

chromeos/services/device_sync/cryptauth_key_creator.h

@@ -49,27 +49,12 @@ class CryptAuthKeyCreator {
     CreateKeyData(CryptAuthKey::Status status,
                   cryptauthv2::KeyType type,
                   base::Optional<std::string> handle = base::nullopt);
-
-    // Special constructor needed to handle existing user key pair. The input
-    // strings cannot be empty.
-    CreateKeyData(CryptAuthKey::Status status,
-                  cryptauthv2::KeyType type,
-                  const std::string& handle,
-                  const std::string& public_key,
-                  const std::string& private_key);
-
     ~CreateKeyData();
     CreateKeyData(const CreateKeyData&);
 
     CryptAuthKey::Status status;
     cryptauthv2::KeyType type;
     base::Optional<std::string> handle;
-    // Special data needed to handle existing user key pair. If these are both
-    // non-empty strings and the key type is asymmetric, then the key creator
-    // will bypass the standard key creation and simply return
-    // CryptAuthKey(|public_key|, |private_key|, |status|, |type|, |handle|).
-    base::Optional<std::string> public_key;
-    base::Optional<std::string> private_key;
   };
 
   CryptAuthKeyCreator();

chromeos/services/device_sync/cryptauth_key_creator_impl.cc

@@ -4,13 +4,21 @@
 
 #include "chromeos/services/device_sync/cryptauth_key_creator_impl.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+
 #include "base/base64.h"
 #include "base/bind.h"
+#include "base/containers/flat_map.h"
 #include "base/memory/ptr_util.h"
 #include "base/no_destructor.h"
+#include "base/optional.h"
 #include "base/strings/string_util.h"
+#include "chromeos/components/multidevice/logging/logging.h"
 #include "chromeos/components/multidevice/secure_message_delegate_impl.h"
 #include "chromeos/services/device_sync/cryptauth_constants.h"
+#include "chromeos/services/device_sync/cryptauth_key.h"
 #include "chromeos/services/device_sync/proto/cryptauth_common.pb.h"
 #include "crypto/hkdf.h"
 #include "crypto/random.h"
@@ -157,41 +165,29 @@ void CryptAuthKeyCreatorImpl::OnDiffieHellmanHandshakeSecretDerived(
 
 void CryptAuthKeyCreatorImpl::StartKeyCreation() {
   for (const auto& key_to_create : keys_to_create_) {
-    const CryptAuthKeyBundle::Name& bundle_name = key_to_create.first;
-    const CreateKeyData& key_data = key_to_create.second;
-
     // If the key to create is symmetric, derive a symmetric key from the
     // Diffie-Hellman handshake secrect using HKDF. The CryptAuth v2
     // Enrollment protocol specifies that the salt should be "CryptAuth
     // 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,
-                             NumBytesForSymmetricKeyType(key_data.type));
-
-      OnSymmetricKeyDerived(bundle_name, derived_symmetric_key_material,
+    if (IsValidSymmetricKeyType(key_to_create.second.type)) {
+      std::string handle = key_to_create.second.handle.has_value()
+                               ? *key_to_create.second.handle
+                               : CreateRandomHandle();
+      std::string derived_symmetric_key_material = crypto::HkdfSha256(
+          dh_handshake_secret_->symmetric_key(),
+          kCryptAuthSymmetricKeyDerivationSalt, handle,
+          NumBytesForSymmetricKeyType(key_to_create.second.type));
+
+      OnSymmetricKeyDerived(key_to_create.first, derived_symmetric_key_material,
                             handle);
+    } else {
+      DCHECK(IsValidAsymmetricKeyType(key_to_create.second.type));
 
-      continue;
+      secure_message_delegate_->GenerateKeyPair(
+          base::Bind(&CryptAuthKeyCreatorImpl::OnAsymmetricKeyPairGenerated,
+                     base::Unretained(this), key_to_create.first));
     }
-
-    DCHECK(IsValidAsymmetricKeyType(key_data.type));
-
-    // If the key material was explicitly set in CreateKeyData, bypass the
-    // standard key creation.
-    if (key_data.public_key && key_data.private_key) {
-      OnAsymmetricKeyPairGenerated(bundle_name, *key_data.public_key,
-                                   *key_data.private_key);
-      continue;
-    }
-
-    secure_message_delegate_->GenerateKeyPair(
-        base::Bind(&CryptAuthKeyCreatorImpl::OnAsymmetricKeyPairGenerated,
-                   base::Unretained(this), key_to_create.first));
   }
 }
 

chromeos/services/device_sync/cryptauth_key_creator_impl.h

@@ -5,6 +5,8 @@
 #ifndef CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_KEY_CREATOR_IMPL_H_
 #define CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_KEY_CREATOR_IMPL_H_
 
+#include "chromeos/services/device_sync/cryptauth_key_creator.h"
+
 #include <memory>
 #include <string>
 #include <utility>
@@ -16,7 +18,6 @@
 #include "base/optional.h"
 #include "chromeos/services/device_sync/cryptauth_key.h"
 #include "chromeos/services/device_sync/cryptauth_key_bundle.h"
-#include "chromeos/services/device_sync/cryptauth_key_creator.h"
 
 namespace chromeos {
 

chromeos/services/device_sync/cryptauth_v2_enroller_impl.cc

@@ -254,59 +254,48 @@ bool IsSupportedKeyType(const cryptauthv2::KeyType& key_type) {
          key_type == cryptauthv2::KeyType::P256;
 }
 
-// The key bundle kUserKeyPair has special standing in order to 1) accommodate
-// any existing key from v1 Enrollment and 2) enforce that the key is not
-// rotated. As such, only one user key pair should exist in the key bundle, and
-// it should be an active, P-256 key with handle
-// kCryptAuthFixedUserKeyPairHandle.
-//
-// It is possible that CryptAuth could request the creation of a new user key
-// pair even if the client sends information about an existing key in the
-// SyncKeysRequest. If this happens, the client should re-use the existing user
-// key pair key material when creating a new key. At the end of the enrollment
-// flow, the existing key will be replaced with this new key that has the same
-// public/private keys.
-//
 // Returns an error code if the key-creation instructions are invalid and null
 // otherwise.
 base::Optional<CryptAuthEnrollmentResult::ResultCode>
-ProcessNewUserKeyPairInstructions(
-    CryptAuthKey::Status status,
-    cryptauthv2::KeyType type,
-    const CryptAuthKey* current_active_key,
-    base::Optional<CryptAuthKeyCreator::CreateKeyData>* new_key_to_create) {
-  if (type != cryptauthv2::KeyType::P256) {
-    PA_LOG(ERROR) << "User key pair must have KeyType P256.";
+ProcessKeyCreationInstructions(
+    const CryptAuthKeyBundle::Name& bundle_name,
+    const SyncSingleKeyResponse& single_key_response,
+    const std::string& server_ephemeral_dh,
+    base::Optional<CryptAuthKeyCreator::CreateKeyData>* new_key_to_create,
+    base::Optional<cryptauthv2::KeyDirective>* new_key_directive) {
+  if (single_key_response.key_creation() == SyncSingleKeyResponse::NONE)
+    return base::nullopt;
+
+  if (!IsSupportedKeyType(single_key_response.key_type())) {
+    PA_LOG(ERROR) << "KeyType " << single_key_response.key_type() << " "
+                  << "not supported.";
     return CryptAuthEnrollmentResult::ResultCode::
-        kErrorUserKeyPairCreationInstructionsInvalid;
+        kErrorKeyCreationKeyTypeNotSupported;
   }
 
-  // Because no more than one user key pair can exist in the bundle, the newly
-  // created key must be active.
-  if (status != CryptAuthKey::Status::kActive) {
-    PA_LOG(ERROR) << "New user key pair must be active.";
+  // Symmetric keys cannot be created without the server's Diffie-Hellman key.
+  if (server_ephemeral_dh.empty() &&
+      (single_key_response.key_type() == cryptauthv2::KeyType::RAW128 ||
+       single_key_response.key_type() == cryptauthv2::KeyType::RAW256)) {
+    PA_LOG(ERROR)
+        << "Missing server's Diffie-Hellman key. Cannot create symmetric keys.";
     return CryptAuthEnrollmentResult::ResultCode::
-        kErrorUserKeyPairCreationInstructionsInvalid;
+        kErrorSymmetricKeyCreationMissingServerDiffieHellman;
   }
 
-  // If a user key pair already exists in the registry, reuse the same key data.
-  if (current_active_key && current_active_key->IsAsymmetricKey() &&
-      !current_active_key->private_key().empty()) {
-    PA_LOG(WARNING) << "Received request to create new user key pair while one "
-                    << "already exists in the key registry. Reusing existing "
-                    << "key material.";
+  // CryptAuth demands that the key in the kUserKeyPair bundle has a fixed
+  // handle name. For other key bundles, do not specify a handle name; let
+  // CryptAuthKey generate a handle for us.
+  base::Optional<std::string> new_key_handle;
+  if (bundle_name == CryptAuthKeyBundle::Name::kUserKeyPair)
+    new_key_handle = kCryptAuthFixedUserKeyPairHandle;
 
-    *new_key_to_create = CryptAuthKeyCreator::CreateKeyData(
-        status, type, kCryptAuthFixedUserKeyPairHandle,
-        current_active_key->public_key(), current_active_key->private_key());
-
-    return base::nullopt;
-  }
-
-  // If there is no user key pair in the registry, then the user has never
-  // successfully enrolled via v1 or v2 Enrollment. Generate a new key pair.
   *new_key_to_create = CryptAuthKeyCreator::CreateKeyData(
-      status, type, kCryptAuthFixedUserKeyPairHandle);
+      ConvertKeyCreationToKeyStatus(single_key_response.key_creation()),
+      single_key_response.key_type(), new_key_handle);
+
+  if (single_key_response.has_key_directive())
+    *new_key_directive = single_key_response.key_directive();
 
   return base::nullopt;
 }
@@ -631,51 +620,6 @@ CryptAuthV2EnrollerImpl::ProcessSingleKeyResponses(
   return error_code;
 }
 
-base::Optional<CryptAuthEnrollmentResult::ResultCode>
-CryptAuthV2EnrollerImpl::ProcessKeyCreationInstructions(
-    const CryptAuthKeyBundle::Name& bundle_name,
-    const SyncSingleKeyResponse& single_key_response,
-    const std::string& server_ephemeral_dh,
-    base::Optional<CryptAuthKeyCreator::CreateKeyData>* new_key_to_create,
-    base::Optional<cryptauthv2::KeyDirective>* new_key_directive) {
-  if (single_key_response.key_creation() == SyncSingleKeyResponse::NONE)
-    return base::nullopt;
-
-  CryptAuthKey::Status status =
-      ConvertKeyCreationToKeyStatus(single_key_response.key_creation());
-  cryptauthv2::KeyType type = single_key_response.key_type();
-
-  if (!IsSupportedKeyType(type)) {
-    PA_LOG(ERROR) << "KeyType " << type << " not supported.";
-    return CryptAuthEnrollmentResult::ResultCode::
-        kErrorKeyCreationKeyTypeNotSupported;
-  }
-
-  // Symmetric keys cannot be created without the server's Diffie-Hellman key.
-  if (server_ephemeral_dh.empty() && (type == cryptauthv2::KeyType::RAW128 ||
-                                      type == cryptauthv2::KeyType::RAW256)) {
-    PA_LOG(ERROR)
-        << "Missing server's Diffie-Hellman key. Cannot create symmetric keys.";
-    return CryptAuthEnrollmentResult::ResultCode::
-        kErrorSymmetricKeyCreationMissingServerDiffieHellman;
-  }
-
-  if (single_key_response.has_key_directive())
-    *new_key_directive = single_key_response.key_directive();
-
-  // Handle the user key pair special case separately below.
-  if (bundle_name != CryptAuthKeyBundle::Name::kUserKeyPair) {
-    *new_key_to_create = CryptAuthKeyCreator::CreateKeyData(status, type);
-
-    return base::nullopt;
-  }
-
-  DCHECK(bundle_name == CryptAuthKeyBundle::Name::kUserKeyPair);
-  return ProcessNewUserKeyPairInstructions(
-      status, type, key_registry_->GetActiveKey(bundle_name),
-      new_key_to_create);
-}
-
 void CryptAuthV2EnrollerImpl::OnSyncKeysFailure(NetworkRequestError error) {
   FinishAttempt(SyncKeysNetworkRequestErrorToResultCode(error));
 }

chromeos/services/device_sync/cryptauth_v2_enroller_impl.h

@@ -120,17 +120,6 @@ class CryptAuthV2EnrollerImpl : public CryptAuthV2Enroller {
       base::flat_map<CryptAuthKeyBundle::Name, cryptauthv2::KeyDirective>*
           new_key_directives);
 
-  // A function to help ProcessSingleKeyResponse() handle the key-creation
-  // instructions.
-  base::Optional<CryptAuthEnrollmentResult::ResultCode>
-  ProcessKeyCreationInstructions(
-      const CryptAuthKeyBundle::Name& bundle_name,
-      const cryptauthv2::SyncKeysResponse::SyncSingleKeyResponse&
-          single_key_response,
-      const std::string& server_ephemeral_dh,
-      base::Optional<CryptAuthKeyCreator::CreateKeyData>* new_key_to_create,
-      base::Optional<cryptauthv2::KeyDirective>* new_key_directive);
-
   void OnSyncKeysFailure(NetworkRequestError error);
 
   void OnKeysCreated(