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}

chromeos/services/device_sync/cryptauth_enrollment_result.cc

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

chromeos/services/device_sync/cryptauth_enrollment_result.h

@@ -79,6 +79,9 @@ 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,6 +14,22 @@ 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,12 +49,27 @@ 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,21 +4,13 @@
 
 #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"
@@ -165,29 +157,41 @@ 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_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,
+    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,
                             handle);
-    } else {
-      DCHECK(IsValidAsymmetricKeyType(key_to_create.second.type));
 
-      secure_message_delegate_->GenerateKeyPair(
-          base::Bind(&CryptAuthKeyCreatorImpl::OnAsymmetricKeyPairGenerated,
-                     base::Unretained(this), key_to_create.first));
+      continue;
     }
+
+    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,8 +5,6 @@
 #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>
@@ -18,6 +16,7 @@
 #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,48 +254,59 @@ 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>
-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.";
+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.";
     return CryptAuthEnrollmentResult::ResultCode::
-        kErrorKeyCreationKeyTypeNotSupported;
+        kErrorUserKeyPairCreationInstructionsInvalid;
   }
 
-  // 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.";
+  // 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.";
     return CryptAuthEnrollmentResult::ResultCode::
-        kErrorSymmetricKeyCreationMissingServerDiffieHellman;
+        kErrorUserKeyPairCreationInstructionsInvalid;
   }
 
-  // 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;
+  // 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.";
 
-  *new_key_to_create = CryptAuthKeyCreator::CreateKeyData(
-      ConvertKeyCreationToKeyStatus(single_key_response.key_creation()),
-      single_key_response.key_type(), new_key_handle);
+    *new_key_to_create = CryptAuthKeyCreator::CreateKeyData(
+        status, type, kCryptAuthFixedUserKeyPairHandle,
+        current_active_key->public_key(), current_active_key->private_key());
 
-  if (single_key_response.has_key_directive())
-    *new_key_directive = single_key_response.key_directive();
+    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);
 
   return base::nullopt;
 }
@@ -620,6 +631,51 @@ 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,6 +120,17 @@ 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(