[Enrollment v2] Re-enroll if v1 user key pair replaces v2 user key pair

In a rollback-rollforward scenario, it is possible that the persisted v1
and v2 user key pairs can differ when the v2 enrollment manager is
created. To get in that state, consider the following:
  - the device enrolls with v2, creating a user key pair,
  - Enrollment v2 is rolled back,
  - the device enrolls with Enrollment v1, creating a new user key pair
  - Enrollment v2 is rolled forward.

Because an existing v1-enrolled key is always preferred (for continuity
reasons), the v2 key will be overwritten by the v1 key on start-up.
However, the v2 scheduler does not necessarily know to re-enroll (now
with the v1 key) since it still considers the v2 enrollment before the
rollback the last successful enrollment. Therefore, the v2 enrollment
manager needs to force a v2 re-enrollment.

Bug: 899080
Change-Id: I1d5c9511996210a1852efc1da842370d291898e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1686774
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684478}

chromeos/services/device_sync/cryptauth_v2_enrollment_manager_impl.cc

@@ -53,6 +53,27 @@ enum class UserKeyPairState {
   kMaxValue = kYesV1KeyYesV2KeyDisagree
 };
 
+UserKeyPairState GetUserKeyPairState(const std::string& public_key_v1,
+                                     const std::string& private_key_v1,
+                                     const CryptAuthKey* key_v2) {
+  bool v1_key_exists = !public_key_v1.empty() && !private_key_v1.empty();
+
+  if (v1_key_exists && key_v2) {
+    if (public_key_v1 == key_v2->public_key() &&
+        private_key_v1 == key_v2->private_key()) {
+      return UserKeyPairState::kYesV1KeyYesV2KeyAgree;
+    } else {
+      return UserKeyPairState::kYesV1KeyYesV2KeyDisagree;
+    }
+  } else if (v1_key_exists && !key_v2) {
+    return UserKeyPairState::kYesV1KeyNoV2Key;
+  } else if (!v1_key_exists && key_v2) {
+    return UserKeyPairState::kNoV1KeyYesV2Key;
+  } else {
+    return UserKeyPairState::kNoV1KeyNoV2Key;
+  }
+}
+
 cryptauthv2::ClientMetadata::InvocationReason ConvertInvocationReasonV1ToV2(
     cryptauth::InvocationReason invocation_reason_v1) {
   switch (invocation_reason_v1) {
@@ -98,31 +119,6 @@ void RecordEnrollmentResult(CryptAuthEnrollmentResult result) {
                                 result.result_code());
 }
 
-void RecordUserKeyPairState(const std::string& public_key_v1,
-                            const std::string& private_key_v1,
-                            const CryptAuthKey* key_v2) {
-  bool v1_key_exists = !public_key_v1.empty() && !private_key_v1.empty();
-
-  UserKeyPairState key_pair_state;
-  if (v1_key_exists && key_v2) {
-    if (public_key_v1 == key_v2->public_key() &&
-        private_key_v1 == key_v2->private_key()) {
-      key_pair_state = UserKeyPairState::kYesV1KeyYesV2KeyAgree;
-    } else {
-      key_pair_state = UserKeyPairState::kYesV1KeyYesV2KeyDisagree;
-    }
-  } else if (v1_key_exists && !key_v2) {
-    key_pair_state = UserKeyPairState::kYesV1KeyNoV2Key;
-  } else if (!v1_key_exists && key_v2) {
-    key_pair_state = UserKeyPairState::kNoV1KeyYesV2Key;
-  } else {
-    key_pair_state = UserKeyPairState::kNoV1KeyNoV2Key;
-  }
-
-  base::UmaHistogramEnumeration("CryptAuth.EnrollmentV2.UserKeyPairState",
-                                key_pair_state);
-}
-
 }  // namespace
 
 // static
@@ -235,6 +231,14 @@ void CryptAuthV2EnrollmentManagerImpl::Start() {
   scheduler_->StartEnrollmentScheduling(
       scheduler_weak_ptr_factory_.GetWeakPtr());
 
+  // If the v1 and v2 user key pairs initially disagreed, force a re-enrollment
+  // with the v1 user key pair that replaced the v2 user key pair.
+  if (initial_v1_and_v2_user_key_pairs_disagree_) {
+    ForceEnrollmentNow(
+        cryptauth::InvocationReason::INVOCATION_REASON_INITIALIZATION,
+        base::nullopt /* session_id */);
+  }
+
   // It is possible, though unlikely, that |scheduler_| has previously enrolled
   // successfully but |key_registry_| no longer holds the enrolled keys, for
   // example, if keys are deleted from the key registry or if the persisted key
@@ -509,24 +513,31 @@ void CryptAuthV2EnrollmentManagerImpl::AddV1UserKeyPairToRegistryIfNecessary() {
   std::string private_key_v1 = GetV1UserPrivateKey();
   const CryptAuthKey* key_v2 =
       key_registry_->GetActiveKey(CryptAuthKeyBundle::Name::kUserKeyPair);
+  UserKeyPairState user_key_pair_state =
+      GetUserKeyPairState(public_key_v1, private_key_v1, key_v2);
 
-  RecordUserKeyPairState(public_key_v1, private_key_v1, key_v2);
-
-  // If the v1 user key pair does not exist, no action is needed.
-  if (public_key_v1.empty() || private_key_v1.empty())
-    return;
-
-  // If the v1 and v2 user key pairs already agree, no action is needed.
-  if (key_v2 && key_v2->public_key() == public_key_v1 &&
-      key_v2->private_key() == private_key_v1) {
-    return;
-  }
-
-  key_registry_->AddKey(
-      CryptAuthKeyBundle::Name::kUserKeyPair,
-      CryptAuthKey(public_key_v1, private_key_v1, CryptAuthKey::Status::kActive,
-                   cryptauthv2::KeyType::P256,
-                   kCryptAuthFixedUserKeyPairHandle));
+  base::UmaHistogramEnumeration("CryptAuth.EnrollmentV2.UserKeyPairState",
+                                user_key_pair_state);
+
+  initial_v1_and_v2_user_key_pairs_disagree_ =
+      user_key_pair_state == UserKeyPairState::kYesV1KeyYesV2KeyDisagree;
+
+  switch (user_key_pair_state) {
+    case (UserKeyPairState::kNoV1KeyNoV2Key):
+      FALLTHROUGH;
+    case (UserKeyPairState::kNoV1KeyYesV2Key):
+      FALLTHROUGH;
+    case (UserKeyPairState::kYesV1KeyYesV2KeyAgree):
+      return;
+    case (UserKeyPairState::kYesV1KeyNoV2Key):
+      FALLTHROUGH;
+    case (UserKeyPairState::kYesV1KeyYesV2KeyDisagree):
+      key_registry_->AddKey(CryptAuthKeyBundle::Name::kUserKeyPair,
+                            CryptAuthKey(public_key_v1, private_key_v1,
+                                         CryptAuthKey::Status::kActive,
+                                         cryptauthv2::KeyType::P256,
+                                         kCryptAuthFixedUserKeyPairHandle));
+  };
 }
 
 std::ostream& operator<<(std::ostream& stream,

chromeos/services/device_sync/cryptauth_v2_enrollment_manager_impl.h

@@ -167,6 +167,7 @@ class CryptAuthV2EnrollmentManagerImpl
   base::Clock* clock_;
   std::unique_ptr<base::OneShotTimer> timer_;
 
+  bool initial_v1_and_v2_user_key_pairs_disagree_ = false;
   State state_ = State::kIdle;
   base::Optional<cryptauthv2::ClientMetadata> current_client_metadata_;
   base::Optional<cryptauthv2::PolicyReference>

chromeos/services/device_sync/cryptauth_v2_enrollment_manager_impl_unittest.cc

@@ -727,6 +727,24 @@ TEST_F(DeviceSyncCryptAuthV2EnrollmentManagerImplTest,
             enrollment_manager()->GetUserPublicKey());
   EXPECT_EQ(expected_user_key_pair_v1.private_key(),
             enrollment_manager()->GetUserPrivateKey());
+
+  // Expect re-enrollment using newly added v1 key.
+  enrollment_manager()->Start();
+  cryptauthv2::ClientMetadata expected_client_metadata =
+      cryptauthv2::BuildClientMetadata(
+          0 /* retry_count */, cryptauthv2::ClientMetadata::INITIALIZATION,
+          base::nullopt /* session_id */);
+  CryptAuthEnrollmentResult expected_enrollment_result(
+      CryptAuthEnrollmentResult::ResultCode::kSuccessNewKeysEnrolled,
+      base::nullopt /* client_directive */);
+  CompleteGcmRegistration(true /* success */);
+  HandleGetClientAppMetadataRequest(true /* success */);
+  FinishEnrollmentAttempt(0u /* expected_enroller_instance_index */,
+                          expected_client_metadata, expected_enrollment_result);
+
+  VerifyInvocationReasonHistogram(
+      {expected_client_metadata.invocation_reason()});
+  VerifyEnrollmentResults({expected_enrollment_result});
 }
 
 TEST_F(DeviceSyncCryptAuthV2EnrollmentManagerImplTest,