[Sync] Get rid of KeyDerivationMethod::UNSUPPORTED

NigoriSyncBridge always rejects remote NigoriSpecifics with unknown key
derivation method (corresponds to KeyDerivationMethod::UNSUPPORTED) and
goes into ModelError state, thus enum uses beyond handling/validation
of the remote NigoriSpecifics can't encounter UNSUPPORTED.

This CL removes this enum value and adapts optional where appropriate.

Fixed: 1379853
Change-Id: I03a9d5ce34a5493cfc18f696ffa347a0bd8854d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4000104
Auto-Submit: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067333}

components/sync/base/passphrase_enums.cc

@@ -6,6 +6,7 @@
 
 #include "base/check_op.h"
 #include "base/notreached.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace syncer {
 
@@ -72,7 +73,7 @@ sync_pb::NigoriSpecifics::PassphraseType EnumPassphraseTypeToProto(
   return sync_pb::NigoriSpecifics::IMPLICIT_PASSPHRASE;
 }
 
-KeyDerivationMethod ProtoKeyDerivationMethodToEnum(
+absl::optional<KeyDerivationMethod> ProtoKeyDerivationMethodToEnum(
     ::google::protobuf::int32 method) {
   DCHECK_GE(method, 0);
 
@@ -89,7 +90,7 @@ KeyDerivationMethod ProtoKeyDerivationMethodToEnum(
 
   // We do not know about this value. It is likely a method added in a newer
   // version of Chrome.
-  return KeyDerivationMethod::UNSUPPORTED;
+  return absl::nullopt;
 }
 
 sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
@@ -99,10 +100,6 @@ sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
       return sync_pb::NigoriSpecifics::PBKDF2_HMAC_SHA1_1003;
     case KeyDerivationMethod::SCRYPT_8192_8_11:
       return sync_pb::NigoriSpecifics::SCRYPT_8192_8_11;
-    case KeyDerivationMethod::UNSUPPORTED:
-      // This value does not have a counterpart in the protocol proto enum,
-      // because it is just a client side abstraction.
-      break;
   }
 
   NOTREACHED();

components/sync/base/passphrase_enums.h

@@ -57,17 +57,16 @@ sync_pb::NigoriSpecifics::PassphraseType EnumPassphraseTypeToProto(
 enum class KeyDerivationMethod {
   PBKDF2_HMAC_SHA1_1003 = 0,  // PBKDF2-HMAC-SHA1 with 1003 iterations.
   SCRYPT_8192_8_11 = 1,  // scrypt with N = 2^13, r = 8, p = 11 and random salt.
-  UNSUPPORTED = 2,       // Unsupported method, likely from a future version.
 };
 
 // This function accepts an integer and not KeyDerivationMethod from the proto
-// in order to be able to handle new, unknown values.
-KeyDerivationMethod ProtoKeyDerivationMethodToEnum(
+// in order to be able to handle new, unknown values. Returns nullopt if value
+// is unknown (indicates protocol violation or value coming from newer version)
+// and PBKDF2_HMAC_SHA1_1003 if value is unspecified (indicates value coming
+// from older version, that is not aware of the field).
+absl::optional<KeyDerivationMethod> ProtoKeyDerivationMethodToEnum(
     ::google::protobuf::int32 method);
-// Note that KeyDerivationMethod::UNSUPPORTED is an invalid input for this
-// function, since it corresponds to a method that we are not aware of and so
-// cannot meaningfully convert. The caller is responsible for ensuring that
-// KeyDerivationMethod::UNSUPPORTED is never passed to this function.
+
 sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
     KeyDerivationMethod method);
 

components/sync/driver/sync_service_crypto.cc

@@ -360,13 +360,6 @@ bool SyncServiceCrypto::SetDecryptionPassphrase(const std::string& passphrase) {
               KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003);
   }
 
-  if (state_.passphrase_key_derivation_params.method() ==
-      KeyDerivationMethod::UNSUPPORTED) {
-    DLOG(ERROR) << "Cannot derive keys using an unsupported key derivation "
-                   "method. Rejecting passphrase.";
-    return false;
-  }
-
   std::unique_ptr<Nigori> nigori = Nigori::CreateByDerivation(
       state_.passphrase_key_derivation_params, passphrase);
   DCHECK(nigori);

components/sync/engine/nigori/key_derivation_params.cc

@@ -41,8 +41,4 @@ KeyDerivationParams KeyDerivationParams::CreateForScrypt(
   return {KeyDerivationMethod::SCRYPT_8192_8_11, salt};
 }
 
-KeyDerivationParams KeyDerivationParams::CreateWithUnsupportedMethod() {
-  return {KeyDerivationMethod::UNSUPPORTED, /*scrypt_salt_=*/""};
-}
-
 }  // namespace syncer

components/sync/engine/nigori/key_derivation_params.h

@@ -22,7 +22,6 @@ class KeyDerivationParams {
  public:
   static KeyDerivationParams CreateForPbkdf2();
   static KeyDerivationParams CreateForScrypt(const std::string& salt);
-  static KeyDerivationParams CreateWithUnsupportedMethod();
 
   KeyDerivationMethod method() const { return method_; }
   const std::string& scrypt_salt() const;

components/sync/engine/nigori/nigori.cc

@@ -76,12 +76,10 @@ const char* GetHistogramSuffixForKeyDerivationMethod(
       return "Pbkdf2";
     case KeyDerivationMethod::SCRYPT_8192_8_11:
       return "Scrypt8192";
-    case KeyDerivationMethod::UNSUPPORTED:
-      break;
   }
 
   NOTREACHED();
-  return "Unsupported";
+  return "";
 }
 
 }  // namespace
@@ -349,8 +347,6 @@ std::unique_ptr<Nigori> Nigori::CreateByDerivationImpl(
       nigori->keys_.InitByDerivationUsingScrypt(
           key_derivation_params.scrypt_salt(), password);
       break;
-    case KeyDerivationMethod::UNSUPPORTED:
-      NOTREACHED();
   }
 
   UmaHistogramTimes(

components/sync/engine/sync_string_conversions.cc

@@ -5,6 +5,7 @@
 #include "components/sync/engine/sync_string_conversions.h"
 
 #include "base/notreached.h"
+#include "components/sync/base/passphrase_enums.h"
 
 #define ENUM_CASE(x) \
   case x:            \
@@ -37,17 +38,6 @@ const char* PassphraseTypeToString(PassphraseType type) {
   return "INVALID_PASSPHRASE_TYPE";
 }
 
-const char* KeyDerivationMethodToString(KeyDerivationMethod method) {
-  switch (method) {
-    ENUM_CASE(KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003);
-    ENUM_CASE(KeyDerivationMethod::SCRYPT_8192_8_11);
-    ENUM_CASE(KeyDerivationMethod::UNSUPPORTED);
-  }
-
-  NOTREACHED();
-  return "INVALID_KEY_DERIVATION_METHOD";
-}
-
 #undef ENUM_CASE
 
 }  // namespace syncer

components/sync/engine/sync_string_conversions.h

@@ -5,9 +5,7 @@
 #ifndef COMPONENTS_SYNC_ENGINE_SYNC_STRING_CONVERSIONS_H_
 #define COMPONENTS_SYNC_ENGINE_SYNC_STRING_CONVERSIONS_H_
 
-#include "components/sync/base/passphrase_enums.h"
 #include "components/sync/engine/connection_status.h"
-#include "components/sync/engine/sync_encryption_handler.h"
 
 namespace syncer {
 
@@ -17,8 +15,6 @@ const char* ConnectionStatusToString(ConnectionStatus status);
 
 const char* PassphraseTypeToString(PassphraseType type);
 
-const char* KeyDerivationMethodToString(KeyDerivationMethod method);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_ENGINE_SYNC_STRING_CONVERSIONS_H_

components/sync/nigori/nigori_state.cc

@@ -10,10 +10,12 @@
 #include "base/notreached.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
+#include "components/sync/engine/nigori/key_derivation_params.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/keystore_keys_cryptographer.h"
 #include "components/sync/protocol/nigori_local_data.pb.h"
+#include "components/sync/protocol/nigori_specifics.pb.h"
 
 namespace syncer {
 
@@ -32,18 +34,18 @@ CustomPassphraseKeyDerivationParamsToProto(const KeyDerivationParams& params) {
 
 KeyDerivationParams CustomPassphraseKeyDerivationParamsFromProto(
     const sync_pb::CustomPassphraseKeyDerivationParams& proto) {
-  switch (ProtoKeyDerivationMethodToEnum(
-      proto.custom_passphrase_key_derivation_method())) {
-    case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
+  switch (proto.custom_passphrase_key_derivation_method()) {
+    case sync_pb::NigoriSpecifics::UNSPECIFIED:
+      [[fallthrough]];
+    case sync_pb::NigoriSpecifics::PBKDF2_HMAC_SHA1_1003:
       return KeyDerivationParams::CreateForPbkdf2();
-    case KeyDerivationMethod::SCRYPT_8192_8_11:
+    case sync_pb::NigoriSpecifics::SCRYPT_8192_8_11:
       return KeyDerivationParams::CreateForScrypt(
           proto.custom_passphrase_key_derivation_salt());
-    case KeyDerivationMethod::UNSUPPORTED:
-      break;
   }
+
   NOTREACHED();
-  return KeyDerivationParams::CreateWithUnsupportedMethod();
+  return KeyDerivationParams::CreateForPbkdf2();
 }
 
 // |encrypted| must not be null.
@@ -91,7 +93,6 @@ void UpdateSpecificsFromKeyDerivationParams(
     sync_pb::NigoriSpecifics* specifics) {
   DCHECK_EQ(specifics->passphrase_type(),
             sync_pb::NigoriSpecifics::CUSTOM_PASSPHRASE);
-  DCHECK_NE(params.method(), KeyDerivationMethod::UNSUPPORTED);
   specifics->set_custom_passphrase_key_derivation_method(
       EnumKeyDerivationMethodToProto(params.method()));
   if (params.method() == KeyDerivationMethod::SCRYPT_8192_8_11) {

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -13,6 +13,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/notreached.h"
 #include "base/observer_list.h"
 #include "components/os_crypt/os_crypt.h"
 #include "components/sync/base/passphrase_enums.h"
@@ -40,7 +41,7 @@ const char kNigoriNonUniqueName[] = "Nigori";
 // added.
 enum class KeyDerivationMethodStateForMetrics {
   NOT_SET = 0,
-  UNSUPPORTED = 1,
+  DEPRECATED_UNSUPPORTED = 1,
   PBKDF2_HMAC_SHA1_1003 = 2,
   SCRYPT_8192_8_11 = 3,
   kMaxValue = SCRYPT_8192_8_11
@@ -56,12 +57,10 @@ KeyDerivationMethodStateForMetrics GetKeyDerivationMethodStateForMetrics(
       return KeyDerivationMethodStateForMetrics::PBKDF2_HMAC_SHA1_1003;
     case KeyDerivationMethod::SCRYPT_8192_8_11:
       return KeyDerivationMethodStateForMetrics::SCRYPT_8192_8_11;
-    case KeyDerivationMethod::UNSUPPORTED:
-      return KeyDerivationMethodStateForMetrics::UNSUPPORTED;
   }
 
   NOTREACHED();
-  return KeyDerivationMethodStateForMetrics::UNSUPPORTED;
+  return KeyDerivationMethodStateForMetrics::NOT_SET;
 }
 
 std::string GetScryptSaltFromSpecifics(
@@ -77,18 +76,22 @@ std::string GetScryptSaltFromSpecifics(
 
 KeyDerivationParams GetKeyDerivationParamsFromSpecifics(
     const sync_pb::NigoriSpecifics& specifics) {
-  switch (ProtoKeyDerivationMethodToEnum(
-      specifics.custom_passphrase_key_derivation_method())) {
+  absl::optional<KeyDerivationMethod> key_derivation_method =
+      ProtoKeyDerivationMethodToEnum(
+          specifics.custom_passphrase_key_derivation_method());
+  // Guaranteed by validations (e.g. SpecificsHasValidKeyDerivationParams()).
+  DCHECK(key_derivation_method);
+
+  switch (*key_derivation_method) {
     case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
       return KeyDerivationParams::CreateForPbkdf2();
     case KeyDerivationMethod::SCRYPT_8192_8_11:
       return KeyDerivationParams::CreateForScrypt(
           GetScryptSaltFromSpecifics(specifics));
-    case KeyDerivationMethod::UNSUPPORTED:
-      break;
   }
 
-  return KeyDerivationParams::CreateWithUnsupportedMethod();
+  NOTREACHED();
+  return KeyDerivationParams::CreateForPbkdf2();
 }
 
 // We need to apply base64 encoding before deriving Nigori keys because the
@@ -104,12 +107,15 @@ std::vector<std::string> Base64EncodeKeys(
 }
 
 bool SpecificsHasValidKeyDerivationParams(const NigoriSpecifics& specifics) {
-  switch (ProtoKeyDerivationMethodToEnum(
-      specifics.custom_passphrase_key_derivation_method())) {
-    case KeyDerivationMethod::UNSUPPORTED:
-      DLOG(ERROR) << "Unsupported key derivation method encountered: "
-                  << specifics.custom_passphrase_key_derivation_method();
-      return false;
+  absl::optional<KeyDerivationMethod> key_derivation_method =
+      ProtoKeyDerivationMethodToEnum(
+          specifics.custom_passphrase_key_derivation_method());
+  if (!key_derivation_method) {
+    DLOG(ERROR) << "Unsupported key derivation method encountered: "
+                << specifics.custom_passphrase_key_derivation_method();
+    return false;
+  }
+  switch (*key_derivation_method) {
     case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
       return true;
     case KeyDerivationMethod::SCRYPT_8192_8_11:
@@ -916,7 +922,7 @@ KeyDerivationParams NigoriSyncBridgeImpl::GetKeyDerivationParamsForPendingKeys()
   switch (state_.passphrase_type) {
     case NigoriSpecifics::UNKNOWN:
       NOTREACHED();
-      return KeyDerivationParams::CreateWithUnsupportedMethod();
+      return KeyDerivationParams::CreateForPbkdf2();
     case NigoriSpecifics::IMPLICIT_PASSPHRASE:
     case NigoriSpecifics::KEYSTORE_PASSPHRASE:
     case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:

components/sync/test/nigori_test_utils.cc

@@ -7,6 +7,7 @@
 #include "base/base64.h"
 #include "base/check.h"
 #include "components/sync/base/time.h"
+#include "components/sync/engine/nigori/key_derivation_params.h"
 #include "components/sync/engine/nigori/nigori.h"
 #include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/nigori_key_bag.h"
@@ -127,10 +128,6 @@ sync_pb::NigoriSpecifics BuildCustomPassphraseNigoriSpecifics(
                          &encoded_salt);
       nigori.set_custom_passphrase_key_derivation_salt(encoded_salt);
       break;
-    case KeyDerivationMethod::UNSUPPORTED:
-      ADD_FAILURE() << "Unsupported method in KeyParamsForTesting, cannot "
-                       "construct Nigori.";
-      break;
   }
 
   // Create the cryptographer, which encrypts with the key derived from
@@ -153,23 +150,25 @@ sync_pb::NigoriSpecifics BuildCustomPassphraseNigoriSpecifics(
 
 KeyDerivationParams InitCustomPassphraseKeyDerivationParamsFromNigori(
     const sync_pb::NigoriSpecifics& nigori) {
-  switch (ProtoKeyDerivationMethodToEnum(
-      nigori.custom_passphrase_key_derivation_method())) {
-    case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003: {
+  absl::optional<KeyDerivationMethod> key_derivation_method =
+      ProtoKeyDerivationMethodToEnum(
+          nigori.custom_passphrase_key_derivation_method());
+  if (!key_derivation_method.has_value()) {
+    // The test cannot pass since we wouldn't know how to decrypt data encrypted
+    // using an unsupported method.
+    ADD_FAILURE() << "Unsupported key derivation method encountered: "
+                  << nigori.custom_passphrase_key_derivation_method();
+    return KeyDerivationParams::CreateForPbkdf2();
+  }
+
+  switch (*key_derivation_method) {
+    case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
       return KeyDerivationParams::CreateForPbkdf2();
-    }
-    case KeyDerivationMethod::SCRYPT_8192_8_11: {
+    case KeyDerivationMethod::SCRYPT_8192_8_11:
       std::string decoded_salt;
       EXPECT_TRUE(base::Base64Decode(
           nigori.custom_passphrase_key_derivation_salt(), &decoded_salt));
       return KeyDerivationParams::CreateForScrypt(decoded_salt);
-    }
-    case KeyDerivationMethod::UNSUPPORTED:
-      // This test cannot pass since we wouldn't know how to decrypt data
-      // encrypted using an unsupported method.
-      ADD_FAILURE() << "Unsupported key derivation method encountered: "
-                    << nigori.custom_passphrase_key_derivation_method();
-      return KeyDerivationParams::CreateForPbkdf2();
   }
 }