From 76e06bfa4c369635604468aa81e6e1594a79abf0 Mon Sep 17 00:00:00 2001 From: "zea@chromium.org" Date: Wed, 10 Jul 2013 19:47:42 +0000 Subject: [Sync] Add password support to sync api This change modifies the password protobuf definition and the sync generic change processor in order to support passwords custom encryption scheme. BUG=117445 Review URL: https://chromiumcodereview.appspot.com/18551007 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@210915 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/sync/glue/generic_change_processor.cc | 48 +++++-- .../sync/glue/generic_change_processor_unittest.cc | 148 ++++++++++++++++++++- sync/engine/build_commit_command.cc | 1 + sync/internal_api/write_node.cc | 1 + sync/protocol/password_specifics.proto | 8 +- sync/syncable/mutable_entry.cc | 1 + sync/test/fake_sync_encryption_handler.cc | 4 +- 7 files changed, 196 insertions(+), 15 deletions(-) diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc index 591e717..fe11df0 100644 --- a/chrome/browser/sync/glue/generic_change_processor.cc +++ b/chrome/browser/sync/glue/generic_change_processor.cc @@ -24,6 +24,39 @@ using content::BrowserThread; namespace browser_sync { +namespace { + +void SetNodeSpecifics(const sync_pb::EntitySpecifics& entity_specifics, + syncer::WriteNode* write_node) { + if (syncer::GetModelTypeFromSpecifics(entity_specifics) == + syncer::PASSWORDS) { + write_node->SetPasswordSpecifics( + entity_specifics.password().client_only_encrypted_data()); + } else { + write_node->SetEntitySpecifics(entity_specifics); + } +} + +syncer::SyncData BuildRemoteSyncData( + int64 sync_id, + const syncer::BaseNode& read_node) { + // Use the specifics of non-password datatypes directly (encryption has + // already been handled). + if (read_node.GetModelType() != syncer::PASSWORDS) { + return syncer::SyncData::CreateRemoteData(sync_id, + read_node.GetEntitySpecifics()); + } + + // Passwords must be accessed differently, to account for their encryption, + // and stored into a temporary EntitySpecifics. + sync_pb::EntitySpecifics password_holder; + password_holder.mutable_password()->mutable_client_only_encrypted_data()-> + CopyFrom(read_node.GetPasswordSpecifics()); + return syncer::SyncData::CreateRemoteData(sync_id, password_holder); +} + +} // namespace + GenericChangeProcessor::GenericChangeProcessor( DataTypeErrorHandler* error_handler, const base::WeakPtr& local_service, @@ -71,8 +104,7 @@ void GenericChangeProcessor::ApplyChangesFromSyncModel( syncer::SyncChange( FROM_HERE, action, - syncer::SyncData::CreateRemoteData( - it->id, read_node.GetEntitySpecifics()))); + BuildRemoteSyncData(it->id, read_node))); } } } @@ -136,8 +168,8 @@ syncer::SyncError GenericChangeProcessor::GetSyncDataForType( type); return error; } - current_sync_data->push_back(syncer::SyncData::CreateRemoteData( - sync_child_node.GetId(), sync_child_node.GetEntitySpecifics())); + current_sync_data->push_back(BuildRemoteSyncData(sync_child_node.GetId(), + sync_child_node)); } return syncer::SyncError(); } @@ -317,8 +349,8 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( } syncer::WriteNode::InitUniqueByCreationResult result = sync_node.InitUniqueByCreation(change.sync_data().GetDataType(), - root_node, - change.sync_data().GetTag()); + root_node, + change.sync_data().GetTag()); if (result != syncer::WriteNode::INIT_SUCCESS) { std::string error_prefix = "Failed to create " + type_str + " node: " + change.location().ToString() + ", "; @@ -368,7 +400,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( } } sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle())); - sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics()); + SetNodeSpecifics(change.sync_data().GetSpecifics(), &sync_node); if (merge_result_.get()) { merge_result_->set_num_items_added(merge_result_->num_items_added() + 1); @@ -461,7 +493,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( } sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle())); - sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics()); + SetNodeSpecifics(change.sync_data().GetSpecifics(), &sync_node); if (merge_result_.get()) { merge_result_->set_num_items_modified( merge_result_->num_items_modified() + 1); diff --git a/chrome/browser/sync/glue/generic_change_processor_unittest.cc b/chrome/browser/sync/glue/generic_change_processor_unittest.cc index bb90a50..25b562c 100644 --- a/chrome/browser/sync/glue/generic_change_processor_unittest.cc +++ b/chrome/browser/sync/glue/generic_change_processor_unittest.cc @@ -10,9 +10,11 @@ #include "base/strings/stringprintf.h" #include "chrome/browser/sync/glue/data_type_error_handler_mock.h" #include "sync/api/fake_syncable_service.h" +#include "sync/api/sync_change.h" #include "sync/api/sync_merge_result.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/read_node.h" +#include "sync/internal_api/public/read_transaction.h" #include "sync/internal_api/public/test/test_user_share.h" #include "sync/internal_api/public/user_share.h" #include "sync/internal_api/public/write_node.h" @@ -23,12 +25,12 @@ namespace browser_sync { namespace { -class GenericChangeProcessorTest : public testing::Test { +class SyncGenericChangeProcessorTest : public testing::Test { public: // It doesn't matter which type we use. Just pick one. static const syncer::ModelType kType = syncer::PREFERENCES; - GenericChangeProcessorTest() : + SyncGenericChangeProcessorTest() : loop_(base::MessageLoop::TYPE_UI), sync_merge_result_(kType), merge_result_ptr_factory_(&sync_merge_result_), @@ -37,7 +39,13 @@ class GenericChangeProcessorTest : public testing::Test { virtual void SetUp() OVERRIDE { test_user_share_.SetUp(); - syncer::TestUserShare::CreateRoot(kType, test_user_share_.user_share()); + syncer::ModelTypeSet types = syncer::ProtocolTypes(); + for (syncer::ModelTypeSet::Iterator iter = types.First(); iter.Good(); + iter.Inc()) { + syncer::TestUserShare::CreateRoot(iter.Get(), + test_user_share_.user_share()); + } + test_user_share_.encryption_handler()->Init(); change_processor_.reset( new GenericChangeProcessor( &data_type_error_handler_, @@ -88,7 +96,7 @@ class GenericChangeProcessorTest : public testing::Test { // This test exercises GenericChangeProcessor's GetSyncDataForType function. // It's not a great test, but, by modifying some of the parameters, you could // turn it into a micro-benchmark for model association. -TEST_F(GenericChangeProcessorTest, StressGetSyncDataForType) { +TEST_F(SyncGenericChangeProcessorTest, StressGetSyncDataForType) { const int kNumChildNodes = 1000; const int kRepeatCount = 1; @@ -103,6 +111,138 @@ TEST_F(GenericChangeProcessorTest, StressGetSyncDataForType) { } } +TEST_F(SyncGenericChangeProcessorTest, SetGetPasswords) { + const int kNumPasswords = 10; + sync_pb::PasswordSpecificsData password_data; + password_data.set_username_value("user"); + + sync_pb::EntitySpecifics password_holder; + + syncer::SyncChangeList change_list; + for (int i = 0; i < kNumPasswords; ++i) { + password_data.set_password_value( + base::StringPrintf("password%i", i)); + password_holder.mutable_password()->mutable_client_only_encrypted_data()-> + CopyFrom(password_data); + change_list.push_back( + syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_ADD, + syncer::SyncData::CreateLocalData( + base::StringPrintf("tag%i", i), + base::StringPrintf("title%i", i), + password_holder))); + } + + ASSERT_FALSE( + change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); + + syncer::SyncDataList password_list; + ASSERT_FALSE( + change_processor()->GetSyncDataForType(syncer::PASSWORDS, &password_list). + IsSet()); + + ASSERT_EQ(password_list.size(), change_list.size()); + for (int i = 0; i < kNumPasswords; ++i) { + // Verify the password is returned properly. + ASSERT_TRUE(password_list[i].GetSpecifics().has_password()); + ASSERT_TRUE(password_list[i].GetSpecifics().password(). + has_client_only_encrypted_data()); + ASSERT_FALSE(password_list[i].GetSpecifics().password().has_encrypted()); + const sync_pb::PasswordSpecificsData& sync_password = + password_list[i].GetSpecifics().password().client_only_encrypted_data(); + const sync_pb::PasswordSpecificsData& change_password = + change_list[i].sync_data().GetSpecifics().password(). + client_only_encrypted_data(); + ASSERT_EQ(sync_password.password_value(), change_password.password_value()); + ASSERT_EQ(sync_password.username_value(), change_password.username_value()); + + // Verify the raw sync data was stored securely. + syncer::ReadTransaction read_transaction(FROM_HERE, user_share()); + syncer::ReadNode node(&read_transaction); + ASSERT_EQ(node.InitByClientTagLookup(syncer::PASSWORDS, + base::StringPrintf("tag%i", i)), + syncer::BaseNode::INIT_OK); + ASSERT_EQ(node.GetTitle(), "encrypted"); + const sync_pb::EntitySpecifics& raw_specifics = node.GetEntitySpecifics(); + ASSERT_TRUE(raw_specifics.has_password()); + ASSERT_TRUE(raw_specifics.password().has_encrypted()); + ASSERT_FALSE(raw_specifics.password().has_client_only_encrypted_data()); + } +} + +TEST_F(SyncGenericChangeProcessorTest, UpdatePasswords) { + const int kNumPasswords = 10; + sync_pb::PasswordSpecificsData password_data; + password_data.set_username_value("user"); + + sync_pb::EntitySpecifics password_holder; + + syncer::SyncChangeList change_list; + syncer::SyncChangeList change_list2; + for (int i = 0; i < kNumPasswords; ++i) { + password_data.set_password_value( + base::StringPrintf("password%i", i)); + password_holder.mutable_password()->mutable_client_only_encrypted_data()-> + CopyFrom(password_data); + change_list.push_back( + syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_ADD, + syncer::SyncData::CreateLocalData( + base::StringPrintf("tag%i", i), + base::StringPrintf("title%i", i), + password_holder))); + password_data.set_password_value( + base::StringPrintf("password_m%i", i)); + password_holder.mutable_password()->mutable_client_only_encrypted_data()-> + CopyFrom(password_data); + change_list2.push_back( + syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_UPDATE, + syncer::SyncData::CreateLocalData( + base::StringPrintf("tag%i", i), + base::StringPrintf("title_m%i", i), + password_holder))); + } + + ASSERT_FALSE( + change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); + ASSERT_FALSE( + change_processor()->ProcessSyncChanges(FROM_HERE, change_list2).IsSet()); + + syncer::SyncDataList password_list; + ASSERT_FALSE( + change_processor()->GetSyncDataForType(syncer::PASSWORDS, &password_list). + IsSet()); + + ASSERT_EQ(password_list.size(), change_list2.size()); + for (int i = 0; i < kNumPasswords; ++i) { + // Verify the password is returned properly. + ASSERT_TRUE(password_list[i].GetSpecifics().has_password()); + ASSERT_TRUE(password_list[i].GetSpecifics().password(). + has_client_only_encrypted_data()); + ASSERT_FALSE(password_list[i].GetSpecifics().password().has_encrypted()); + const sync_pb::PasswordSpecificsData& sync_password = + password_list[i].GetSpecifics().password().client_only_encrypted_data(); + const sync_pb::PasswordSpecificsData& change_password = + change_list2[i].sync_data().GetSpecifics().password(). + client_only_encrypted_data(); + ASSERT_EQ(sync_password.password_value(), change_password.password_value()); + ASSERT_EQ(sync_password.username_value(), change_password.username_value()); + + // Verify the raw sync data was stored securely. + syncer::ReadTransaction read_transaction(FROM_HERE, user_share()); + syncer::ReadNode node(&read_transaction); + ASSERT_EQ(node.InitByClientTagLookup(syncer::PASSWORDS, + base::StringPrintf("tag%i", i)), + syncer::BaseNode::INIT_OK); + ASSERT_EQ(node.GetTitle(), "encrypted"); + const sync_pb::EntitySpecifics& raw_specifics = node.GetEntitySpecifics(); + ASSERT_TRUE(raw_specifics.has_password()); + ASSERT_TRUE(raw_specifics.password().has_encrypted()); + ASSERT_FALSE(raw_specifics.password().has_client_only_encrypted_data()); + } +} + } // namespace } // namespace browser_sync diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc index 0f16b80..a846cfd 100644 --- a/sync/engine/build_commit_command.cc +++ b/sync/engine/build_commit_command.cc @@ -104,6 +104,7 @@ void SetEntrySpecifics(Entry* meta_entry, sync_entry->mutable_specifics()->CopyFrom(meta_entry->Get(SPECIFICS)); sync_entry->set_folder(meta_entry->Get(syncable::IS_DIR)); + CHECK(!sync_entry->specifics().password().has_client_only_encrypted_data()); DCHECK_EQ(meta_entry->GetModelType(), GetModelType(*sync_entry)); } } // namespace diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc index 44a2a50..17a7fdb 100644 --- a/sync/internal_api/write_node.cc +++ b/sync/internal_api/write_node.cc @@ -225,6 +225,7 @@ void WriteNode::SetEntitySpecifics( const sync_pb::EntitySpecifics& new_value) { ModelType new_specifics_type = GetModelTypeFromSpecifics(new_value); + CHECK(!new_value.password().has_client_only_encrypted_data()); DCHECK_NE(new_specifics_type, UNSPECIFIED); DVLOG(1) << "Writing entity specifics of type " << ModelTypeToString(new_specifics_type); diff --git a/sync/protocol/password_specifics.proto b/sync/protocol/password_specifics.proto index 769914433..ad5a512 100644 --- a/sync/protocol/password_specifics.proto +++ b/sync/protocol/password_specifics.proto @@ -33,9 +33,13 @@ message PasswordSpecificsData { optional bool blacklisted = 12; } -// Properties of password sync objects. The actual password data is held in a -// PasswordSpecificsData that is encrypted into |encrypted|. +// Properties of password sync objects. message PasswordSpecifics { + // The actual password data. Contains an encrypted PasswordSpecificsData + // message. optional EncryptedData encrypted = 1; + // An unsynced field for use internally on the client. This field should + // never be set in any network-based communications. + optional PasswordSpecificsData client_only_encrypted_data = 2; } diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index 1d5524d..9562f31 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -261,6 +261,7 @@ bool MutableEntry::Put(StringField field, const string& value) { bool MutableEntry::Put(ProtoField field, const sync_pb::EntitySpecifics& value) { DCHECK(kernel_); + CHECK(!value.password().has_client_only_encrypted_data()); write_transaction_->SaveOriginal(kernel_); // TODO(ncarter): This is unfortunately heavyweight. Can we do // better? diff --git a/sync/test/fake_sync_encryption_handler.cc b/sync/test/fake_sync_encryption_handler.cc index 1a83e55..3a60b07 100644 --- a/sync/test/fake_sync_encryption_handler.cc +++ b/sync/test/fake_sync_encryption_handler.cc @@ -18,7 +18,9 @@ FakeSyncEncryptionHandler::FakeSyncEncryptionHandler() FakeSyncEncryptionHandler::~FakeSyncEncryptionHandler() {} void FakeSyncEncryptionHandler::Init() { - // Do nothing. + // Set up a basic cryptographer. + KeyParams keystore_params = {"localhost", "dummy", "keystore_key"}; + cryptographer_.AddKey(keystore_params); } void FakeSyncEncryptionHandler::ApplyNigoriUpdate( -- cgit v1.1