Skip to content

GitLab

T
tangled
Commit ec97e7fa authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot
Browse files
Options

Implement sync Nigori AddTrustedVaultDecryptionKeys() 

This patch changes the implementation within NigoriSyncBridgeImpl to
avoid reusing the codepath for user-entered passphrases, since there
are subtle differences, including:
1. Different observer notifications should be triggered.
2. The selection of the default encryption key is determined via
   Nigori specifics (sync protocol).

Bug: 1834222
Change-Id: Ief85905bb576a82faba515a71c7a6afd9e1612d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834227
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: default avatarMaksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#701981}
parent 9e5fd477
Hide whitespace changes
Inline Side-by-side
Showing with 102 additions and 50 deletions
components/sync/nigori/nigori_sync_bridge_impl.cc
View file @ ec97e7fa
......@@ -677,9 +677,7 @@ void NigoriSyncBridgeImpl::SetDecryptionPassphrase(
return;
}
sync_pb::NigoriKeyBag decrypted_pending_keys;
if (!state_.cryptographer->Decrypt(*state_.pending_keys,
&decrypted_pending_keys)) {
if (!TryDecryptPendingKeys()) {
// TODO(crbug.com/922900): old implementation assumes that pending keys
// encryption key may change in between of OnPassphraseRequired() and
// SetDecryptionPassphrase() calls, verify whether it's really possible.
......@@ -691,10 +689,7 @@ void NigoriSyncBridgeImpl::SetDecryptionPassphrase(
return;
}
state_.cryptographer->EmplaceKeysFrom(
NigoriKeyBag::CreateFromProto(decrypted_pending_keys));
state_.cryptographer->SelectDefaultEncryptionKey(new_key_name);
state_.pending_keys.reset();
storage_->StoreData(SerializeAsNigoriLocalData());
for (auto& observer : observers_) {
......@@ -713,17 +708,41 @@ void NigoriSyncBridgeImpl::SetDecryptionPassphrase(
void NigoriSyncBridgeImpl::AddTrustedVaultDecryptionKeys(
const std::vector<std::string>& keys) {
// This API gets plumbed and ultimately exposed to layers outside the sync
// codebase and even outside the browser, so there are no preconditions and
// instead we ignore invalid or partially invalid input.
if (state_.passphrase_type != NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE ||
!state_.pending_keys) {
!state_.pending_keys || keys.empty()) {
return;
}
// TODO(crbug.com/1010189): Implement this independently of
// SetDecryptionPassphrase() and notify observers via trustedvault-specific
// notification functions.
for (const std::string& key : keys) {
SetDecryptionPassphrase(key);
if (!key.empty()) {
state_.cryptographer->EmplaceKey(key,
GetKeyDerivationParamsForPendingKeys());
}
}
const std::string pending_key_name = state_.pending_keys->key_name();
if (TryDecryptPendingKeys()) {
state_.cryptographer->SelectDefaultEncryptionKey(pending_key_name);
}
storage_->StoreData(SerializeAsNigoriLocalData());
for (auto& observer : observers_) {
observer.OnCryptographerStateChanged(state_.cryptographer.get(),
state_.pending_keys.has_value());
}
if (!state_.pending_keys) {
for (auto& observer : observers_) {
observer.OnTrustedVaultKeyAccepted();
}
}
MaybeNotifyBootstrapTokenUpdated();
}
void NigoriSyncBridgeImpl::EnableEncryptEverything() {
......@@ -925,15 +944,37 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::UpdateLocalState(
observer.OnCryptographerStateChanged(state_.cryptographer.get(),
state_.pending_keys.has_value());
}
if (state_.pending_keys.has_value()) {
// Update with keystore Nigori shouldn't reach this point, since it should
// report model error if it has pending keys.
for (auto& observer : observers_) {
observer.OnPassphraseRequired(REASON_DECRYPTION,
GetKeyDerivationParamsForPendingKeys(),
*state_.pending_keys);
if (!state_.pending_keys.has_value()) {
return base::nullopt;
}
switch (state_.passphrase_type) {
case NigoriSpecifics::UNKNOWN:
case NigoriSpecifics::IMPLICIT_PASSPHRASE:
NOTREACHED();
break;
case NigoriSpecifics::KEYSTORE_PASSPHRASE: {
// Update with keystore Nigori shouldn't reach this point, since it should
// report model error if it has pending keys.
NOTREACHED();
break;
}
case NigoriSpecifics::CUSTOM_PASSPHRASE:
case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
for (auto& observer : observers_) {
observer.OnPassphraseRequired(REASON_DECRYPTION,
GetKeyDerivationParamsForPendingKeys(),
*state_.pending_keys);
}
break;
case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
for (auto& observer : observers_) {
observer.OnTrustedVaultKeyRequired();
}
break;
}
return base::nullopt;
}
......@@ -994,6 +1035,18 @@ void NigoriSyncBridgeImpl::UpdateCryptographerFromNonKeystoreNigori(
state_.cryptographer->EmplaceKeysFrom(NigoriKeyBag::CreateFromProto(key_bag));
}
bool NigoriSyncBridgeImpl::TryDecryptPendingKeys() {
sync_pb::NigoriKeyBag decrypted_pending_keys;
if (!state_.cryptographer->Decrypt(*state_.pending_keys,
&decrypted_pending_keys)) {
return false;
}
state_.cryptographer->EmplaceKeysFrom(
NigoriKeyBag::CreateFromProto(decrypted_pending_keys));
state_.pending_keys.reset();
return true;
}
std::unique_ptr<EntityData> NigoriSyncBridgeImpl::GetData() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (state_.passphrase_type == NigoriSpecifics::UNKNOWN) {
......
components/sync/nigori/nigori_sync_bridge_impl.h
View file @ ec97e7fa
......@@ -104,6 +104,12 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
void UpdateCryptographerFromNonKeystoreNigori(
const sync_pb::EncryptedData& keybag);
// Uses the cryptographer to try to decrypt pending keys. If success, the
// newly decrypted keys are put in the cryptographer's keybag, pending keys
// are cleared and the function returns true. Otherwise, it returns false and
// the state remains unchanged. It does not change the default key.
bool TryDecryptPendingKeys();
base::Time GetExplicitPassphraseTime() const;
// Returns key derivation params based on |passphrase_type_| and
......
components/sync/nigori/nigori_sync_bridge_impl_unittest.cc
View file @ ec97e7fa
......@@ -801,10 +801,10 @@ TEST(NigoriSyncBridgeImplPersistenceTest, ShouldRestoreKeystoreNigori) {
// the sync protocol.
TEST_F(NigoriSyncBridgeImplTest,
ShouldRequireUserActionIfInitiallyUsingTrustedVault) {
const std::string kTrustedVaultPassphrase = "trusted_vault_passphrase";
const std::string kTrustedVaultKey = "trusted_vault_key";
EntityData entity_data;
*entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
{Pbkdf2KeyParams(kTrustedVaultPassphrase)});
*entity_data.specifics.mutable_nigori() =
BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
......@@ -813,9 +813,7 @@ TEST_F(NigoriSyncBridgeImplTest,
EXPECT_CALL(*observer(),
OnPassphraseTypeChanged(PassphraseType::kTrustedVaultPassphrase,
NullTime()));
EXPECT_CALL(*observer(), OnPassphraseRequired(/*reason=*/REASON_DECRYPTION,
/*key_derivation_params=*/_,
/*pending_keys=*/_));
EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired());
EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
Eq(base::nullopt));
EXPECT_THAT(bridge()->GetPassphraseTypeForTesting(),
......@@ -824,11 +822,11 @@ TEST_F(NigoriSyncBridgeImplTest,
Eq(SyncEncryptionHandler::SensitiveTypes()));
EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
EXPECT_CALL(*observer(), OnPassphraseAccepted());
EXPECT_CALL(*observer(), OnTrustedVaultKeyAccepted());
EXPECT_CALL(*observer(), OnCryptographerStateChanged(
NotNull(), /*has_pending_keys=*/false));
EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
}
......@@ -837,7 +835,7 @@ TEST_F(NigoriSyncBridgeImplTest,
// passphrase by means other than the sync protocol.
TEST_F(NigoriSyncBridgeImplTest,
ShouldProcessRemoteTransitionFromKeystoreToTrustedVault) {
const std::string kTrustedVaultPassphrase = "trusted_vault_passphrase";
const std::string kTrustedVaultKey = "trusted_vault_key";
EntityData default_entity_data;
*default_entity_data.specifics.mutable_nigori() =
......@@ -851,8 +849,7 @@ TEST_F(NigoriSyncBridgeImplTest,
EntityData new_entity_data;
*new_entity_data.specifics.mutable_nigori() =
BuildTrustedVaultNigoriSpecifics(
{Pbkdf2KeyParams(kTrustedVaultPassphrase)});
BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
......@@ -861,9 +858,7 @@ TEST_F(NigoriSyncBridgeImplTest,
EXPECT_CALL(*observer(),
OnPassphraseTypeChanged(PassphraseType::kTrustedVaultPassphrase,
NullTime()));
EXPECT_CALL(*observer(), OnPassphraseRequired(/*reason=*/REASON_DECRYPTION,
/*key_derivation_params=*/_,
/*pending_keys=*/_));
EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired());
EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
Eq(base::nullopt));
EXPECT_THAT(bridge()->GetPassphraseTypeForTesting(),
......@@ -872,11 +867,11 @@ TEST_F(NigoriSyncBridgeImplTest,
Eq(SyncEncryptionHandler::SensitiveTypes()));
EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
EXPECT_CALL(*observer(), OnPassphraseAccepted());
EXPECT_CALL(*observer(), OnTrustedVaultKeyAccepted());
EXPECT_CALL(*observer(), OnCryptographerStateChanged(
NotNull(), /*has_pending_keys=*/false));
EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
}
......@@ -884,18 +879,18 @@ TEST_F(NigoriSyncBridgeImplTest,
// vault passphrase.
TEST_F(NigoriSyncBridgeImplTest,
ShouldProcessRemoteKeyRotationForTrustedVault) {
const std::string kTrustedVaultPassphrase = "trusted_vault_passphrase";
const std::string kRotatedTrustedVaultPassphrase = "rotated_vault_passphrase";
const std::string kTrustedVaultKey = "trusted_vault_key";
const std::string kRotatedTrustedVaultKey = "rotated_vault_key";
EntityData entity_data;
*entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
{Pbkdf2KeyParams(kTrustedVaultPassphrase)});
*entity_data.specifics.mutable_nigori() =
BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
Eq(base::nullopt));
ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
......@@ -903,25 +898,23 @@ TEST_F(NigoriSyncBridgeImplTest,
EntityData new_entity_data;
*new_entity_data.specifics.mutable_nigori() =
BuildTrustedVaultNigoriSpecifics(
{Pbkdf2KeyParams(kTrustedVaultPassphrase),
Pbkdf2KeyParams(kRotatedTrustedVaultPassphrase)});
{Pbkdf2KeyParams(kTrustedVaultKey),
Pbkdf2KeyParams(kRotatedTrustedVaultKey)});
EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
EXPECT_CALL(*observer(), OnPassphraseTypeChanged(_, _)).Times(0);
EXPECT_CALL(*observer(), OnCryptographerStateChanged(
NotNull(), /*has_pending_keys=*/true));
EXPECT_CALL(*observer(), OnPassphraseRequired(/*reason=*/REASON_DECRYPTION,
/*key_derivation_params=*/_,
/*pending_keys=*/_));
EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired());
EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
Eq(base::nullopt));
EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
EXPECT_CALL(*observer(), OnPassphraseAccepted());
EXPECT_CALL(*observer(), OnTrustedVaultKeyAccepted());
EXPECT_CALL(*observer(), OnCryptographerStateChanged(
NotNull(), /*has_pending_keys=*/false));
bridge()->SetDecryptionPassphrase(kRotatedTrustedVaultPassphrase);
bridge()->AddTrustedVaultDecryptionKeys({kRotatedTrustedVaultKey});
EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
}
......@@ -929,18 +922,18 @@ TEST_F(NigoriSyncBridgeImplTest,
// passphrase.
TEST_F(NigoriSyncBridgeImplTest,
ShouldTransitionLocallyFromTrustedVaultToCustomPassphrase) {
const std::string kTrustedVaultPassphrase = "trusted_vault_passphrase";
const std::string kTrustedVaultKey = "trusted_vault_key";
const std::string kCustomPassphrase = "custom_passphrase";
EntityData entity_data;
*entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
{Pbkdf2KeyParams(kTrustedVaultPassphrase)});
*entity_data.specifics.mutable_nigori() =
BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
Eq(base::nullopt));
ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment