diff --git a/chrome/browser/policy/messaging_layer/public/report_client.cc b/chrome/browser/policy/messaging_layer/public/report_client.cc index 79e429fee6c2ea..ae367421ab1dd0 100644 --- a/chrome/browser/policy/messaging_layer/public/report_client.cc +++ b/chrome/browser/policy/messaging_layer/public/report_client.cc @@ -53,16 +53,40 @@ const base::FilePath::CharType kReportingDirectory[] = } // namespace +ReportingClient::AsyncStartUploaderRequest::AsyncStartUploaderRequest( + Priority priority, + bool need_encryption_key, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb) + : priority_(priority), + need_encryption_key_(need_encryption_key), + start_uploader_cb_(std::move(start_uploader_cb)) {} +ReportingClient::AsyncStartUploaderRequest::~AsyncStartUploaderRequest() = + default; + +Priority ReportingClient::AsyncStartUploaderRequest::priority() const { + return priority_; +} +bool ReportingClient::AsyncStartUploaderRequest::need_encryption_key() const { + return need_encryption_key_; +} +UploaderInterface::UploaderInterfaceResultCb& +ReportingClient::AsyncStartUploaderRequest::start_uploader_cb() { + return start_uploader_cb_; +} + // Uploader is passed to Storage in order to upload messages using the // UploadClient. class ReportingClient::Uploader : public UploaderInterface { public: using UploadCallback = - base::OnceCallback>)>; + base::OnceCallback>)>; static StatusOr> Create( + bool need_encryption_key, UploadCallback upload_callback) { - auto uploader = base::WrapUnique(new Uploader(std::move(upload_callback))); + auto uploader = base::WrapUnique( + new Uploader(need_encryption_key, std::move(upload_callback))); return uploader; } @@ -90,7 +114,9 @@ class ReportingClient::Uploader : public UploaderInterface { // Helper class that performs actions, wrapped in SequenceBound by |Uploader|. class Helper { public: - explicit Helper(UploadCallback upload_callback); + Helper(bool need_encryption_key, UploadCallback upload_callback); + Helper(const Helper& other) = delete; + Helper& operator=(const Helper& other) = delete; void ProcessRecord(EncryptedRecord data, base::OnceCallback processed_cb); void ProcessGap(SequencingInformation start, @@ -100,21 +126,25 @@ class ReportingClient::Uploader : public UploaderInterface { private: bool completed_{false}; + const bool need_encryption_key_; std::unique_ptr> encrypted_records_; UploadCallback upload_callback_; }; - explicit Uploader(UploadCallback upload_callback) + Uploader(bool need_encryption_key, UploadCallback upload_callback) : helper_(base::ThreadPool::CreateSequencedTaskRunner({}), + need_encryption_key, std::move(upload_callback)) {} base::SequenceBound helper_; }; ReportingClient::Uploader::Helper::Helper( + bool need_encryption_key, ReportingClient::Uploader::UploadCallback upload_callback) - : encrypted_records_(std::make_unique>()), + : need_encryption_key_(need_encryption_key), + encrypted_records_(std::make_unique>()), upload_callback_(std::move(upload_callback)) {} void ReportingClient::Uploader::Helper::ProcessRecord( @@ -156,12 +186,13 @@ void ReportingClient::Uploader::Helper::Completed(Status final_status) { } completed_ = true; DCHECK(encrypted_records_); - if (encrypted_records_->empty()) { + if (encrypted_records_->empty() && !need_encryption_key_) { return; } DCHECK(upload_callback_); Status upload_status = - std::move(upload_callback_).Run(std::move(encrypted_records_)); + std::move(upload_callback_) + .Run(need_encryption_key_, std::move(encrypted_records_)); if (!upload_status.ok()) { LOG(ERROR) << "Unable to upload records: " << upload_status; } @@ -234,17 +265,9 @@ void ReportingClient::ClientInitializingContext::OnCloudPolicyClientConfigured( } void ReportingClient::ClientInitializingContext::ConfigureStorageModule() { - // Storage location in the local file system (if local storage is enabled). - base::FilePath user_data_dir; - if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) { - Complete( - Status(error::FAILED_PRECONDITION, "Could not retrieve base path")); - return; - } - base::FilePath reporting_path = user_data_dir.Append(kReportingDirectory); - StorageSelector::CreateStorageModule( - reporting_path, std::move(async_start_upload_cb_), + client_->reporting_path_, client_->verification_key_, + std::move(async_start_upload_cb_), base::BindOnce(&ClientInitializingContext::OnStorageModuleConfigured, base::Unretained(this))); } @@ -297,14 +320,23 @@ void ReportingClient::ClientInitializingContext::OnCompleted() { } if (upload_client_) { DCHECK(!client_->upload_client_) << "Upload client already recorded"; - client_->upload_client_ = std::move(upload_client_); + client_->SetUploadClient(std::move(upload_client_)); } DCHECK(!client_->storage_) << "Storage module already recorded"; client_->storage_ = std::move(storage_); } ReportingClient::ReportingClient() - : build_cloud_policy_client_cb_(GetCloudPolicyClientCb()) {} + : verification_key_(SignatureVerifier::VerificationKey()), + build_cloud_policy_client_cb_(GetCloudPolicyClientCb()), + uploaders_queue_task_runner_(base::ThreadPool::CreateSequencedTaskRunner( + {base::TaskPriority::BEST_EFFORT, base::MayBlock()})) { + // Storage location in the local file system (if local storage is enabled). + base::FilePath user_data_dir; + DCHECK(base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) + << "Could not retrieve base path"; + reporting_path_ = user_data_dir.Append(kReportingDirectory); +} ReportingClient::~ReportingClient() = default; @@ -333,18 +365,71 @@ void ReportingClient::AsyncStartUploader( UploaderInterface::UploaderInterfaceResultCb start_uploader_cb) { ReportingClient* const instance = static_cast(GetInstance()); - DCHECK(instance->upload_client_); - auto uploader = Uploader::Create(base::BindOnce( - &UploadClient::EnqueueUpload, - base::Unretained(instance->upload_client_.get()), need_encryption_key)); - std::move(start_uploader_cb).Run(std::move(uploader)); + instance->DeliverAsyncStartUploader(priority, need_encryption_key, + std::move(start_uploader_cb)); +} + +void ReportingClient::DeliverAsyncStartUploader( + Priority priority, + bool need_encryption_key, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb) { + uploaders_queue_task_runner_->PostTask( + FROM_HERE, + base::BindOnce( + [](Priority priority, bool need_encryption_key, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb, + ReportingClient* instance) { + if (instance->upload_client_) { + auto uploader = Uploader::Create( + need_encryption_key, + base::BindOnce( + &UploadClient::EnqueueUpload, + base::Unretained(instance->upload_client_.get()))); + std::move(start_uploader_cb).Run(std::move(uploader)); + return; + } + // Not set yet. Enqueue it. + instance->async_start_uploaders_queue_.emplace( + priority, need_encryption_key, std::move(start_uploader_cb)); + }, + priority, need_encryption_key, std::move(start_uploader_cb), + base::Unretained(this))); +} + +void ReportingClient::FlushAsyncStartUploaderQueue() { + // Executed on sequential task runner. + while (!async_start_uploaders_queue_.empty()) { + auto& request = async_start_uploaders_queue_.front(); + auto uploader = Uploader::Create( + request.need_encryption_key(), + base::BindOnce(&UploadClient::EnqueueUpload, + base::Unretained(upload_client_.get()))); + std::move(request.start_uploader_cb()).Run(std::move(uploader)); + async_start_uploaders_queue_.pop(); + } +} + +void ReportingClient::SetUploadClient( + std::unique_ptr upload_client) { + // This can only happen once. + DCHECK(!upload_client_); + upload_client_ = std::move(upload_client); + uploaders_queue_task_runner_->PostTask( + FROM_HERE, base::BindOnce(&ReportingClient::FlushAsyncStartUploaderQueue, + base::Unretained(this))); } ReportingClient::TestEnvironment::TestEnvironment( + const base::FilePath& reporting_path, + base::StringPiece verification_key, policy::CloudPolicyClient* client) : saved_build_cloud_policy_client_cb_( std::move(static_cast(GetInstance()) ->build_cloud_policy_client_cb_)) { + static_cast(GetInstance())->reporting_path_ = + reporting_path; + static_cast(GetInstance())->verification_key_ = + std::string(verification_key); static_cast(GetInstance())->build_cloud_policy_client_cb_ = base::BindRepeating( [](policy::CloudPolicyClient* client, diff --git a/chrome/browser/policy/messaging_layer/public/report_client.h b/chrome/browser/policy/messaging_layer/public/report_client.h index a07c90811d082a..c6bbe7a0d3953b 100644 --- a/chrome/browser/policy/messaging_layer/public/report_client.h +++ b/chrome/browser/policy/messaging_layer/public/report_client.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_POLICY_MESSAGING_LAYER_PUBLIC_REPORT_CLIENT_H_ #include +#include #include #include "base/memory/singleton.h" @@ -88,11 +89,14 @@ class ReportingClient : public ReportQueueProvider { StatusOr> CreateNewQueue( std::unique_ptr config) override; - // RAII class for testing ReportingClient - substitutes a cloud policy client - // builder to return given client and resets it when destructed. + // RAII class for testing ReportingClient - substitutes reporting files + // location, signature verification public key and a cloud policy client + // builder to return given client. Resets client when destructed. class TestEnvironment { public: - explicit TestEnvironment(policy::CloudPolicyClient* client); + TestEnvironment(const base::FilePath& reporting_path, + base::StringPiece verification_key, + policy::CloudPolicyClient* client); TestEnvironment(const TestEnvironment& other) = delete; TestEnvironment& operator=(const TestEnvironment& other) = delete; ~TestEnvironment(); @@ -108,6 +112,30 @@ class ReportingClient : public ReportQueueProvider { private: class Uploader; + + // Request for async start uploader (to be held in queue until upload_client + // is set). + class AsyncStartUploaderRequest { + public: + AsyncStartUploaderRequest( + Priority priority, + bool need_encryption_key, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb); + AsyncStartUploaderRequest(const AsyncStartUploaderRequest& other) = delete; + AsyncStartUploaderRequest& operator=( + const AsyncStartUploaderRequest& other) = delete; + ~AsyncStartUploaderRequest(); + + Priority priority() const; + bool need_encryption_key() const; + UploaderInterface::UploaderInterfaceResultCb& start_uploader_cb(); + + private: + const Priority priority_; + const bool need_encryption_key_; + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb_; + }; + friend class TestEnvironment; friend class ReportQueueProvider; friend struct base::DefaultSingletonTraits; @@ -130,6 +158,17 @@ class ReportingClient : public ReportQueueProvider { bool need_encryption_key, UploaderInterface::UploaderInterfaceResultCb start_uploader_cb); + void DeliverAsyncStartUploader( + Priority priority, + bool need_encryption_key, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb); + + void FlushAsyncStartUploaderQueue(); + + void SetUploadClient(std::unique_ptr upload_client); + + base::FilePath reporting_path_; + std::string verification_key_; GetCloudPolicyClientCallback build_cloud_policy_client_cb_; // TODO(chromium:1078512) Passing around a raw pointer is unsafe. Wrap @@ -137,6 +176,12 @@ class ReportingClient : public ReportQueueProvider { policy::CloudPolicyClient* cloud_policy_client_ = nullptr; std::unique_ptr upload_client_; scoped_refptr storage_; + + // Queue of async start uploader requests protected by sequenced task runner. + // When new request is posted, upload_client_ might be not set yet; in that + // case it is added to the queue and executed only once upload_client_ is set. + std::queue async_start_uploaders_queue_; + scoped_refptr uploaders_queue_task_runner_; }; } // namespace reporting diff --git a/chrome/browser/policy/messaging_layer/public/report_client_unittest.cc b/chrome/browser/policy/messaging_layer/public/report_client_unittest.cc index 0bea165b708c39..095c963591348d 100644 --- a/chrome/browser/policy/messaging_layer/public/report_client_unittest.cc +++ b/chrome/browser/policy/messaging_layer/public/report_client_unittest.cc @@ -4,6 +4,8 @@ #include "chrome/browser/policy/messaging_layer/public/report_client.h" +#include "base/base64.h" +#include "base/files/scoped_temp_dir.h" #include "base/memory/singleton.h" #include "base/task/post_task.h" #include "base/test/scoped_feature_list.h" @@ -14,6 +16,11 @@ #include "components/policy/core/common/cloud/mock_cloud_policy_client.h" #include "components/reporting/client/report_queue_configuration.h" #include "components/reporting/client/report_queue_provider.h" +#include "components/reporting/encryption/decryption.h" +#include "components/reporting/encryption/encryption_module_interface.h" +#include "components/reporting/encryption/primitives.h" +#include "components/reporting/encryption/testing_primitives.h" +#include "components/reporting/encryption/verification.h" #include "components/reporting/proto/record_constants.pb.h" #include "components/reporting/util/status.h" #include "components/reporting/util/status_macros.h" @@ -31,6 +38,7 @@ #endif // BUILDFLAG(IS_CHROMEOS_ASH) using ::testing::_; +using ::testing::Eq; using ::testing::Invoke; using ::testing::Ne; using ::testing::SizeIs; @@ -39,9 +47,10 @@ using ::testing::WithArgs; namespace reporting { namespace { -class ReportClientTest : public testing::Test { - public: +class ReportClientTest : public ::testing::TestWithParam { + protected: void SetUp() override { + ASSERT_TRUE(location_.CreateUniqueTempDir()); #if BUILDFLAG(IS_CHROMEOS_ASH) // Set up fake primary profile. auto mock_user_manager = @@ -58,30 +67,156 @@ class ReportClientTest : public testing::Test { user_manager_ = std::make_unique( std::move(mock_user_manager)); #endif // BUILDFLAG(IS_CHROMEOS_ASH) + + // Encryption is disabled by default. + ASSERT_FALSE(EncryptionModuleInterface::is_enabled()); + if (is_encryption_enabled()) { + // Enable encryption. + scoped_feature_list_.InitFromCommandLine( + "EncryptedReportingPipeline,EncryptedReporting", ""); + // Generate signing key pair. + test::GenerateSigningKeyPair(signing_private_key_, + signature_verification_public_key_); + // Create decryption module. + auto decryptor_result = test::Decryptor::Create(); + ASSERT_OK(decryptor_result.status()) << decryptor_result.status(); + decryptor_ = std::move(decryptor_result.ValueOrDie()); + // Prepare the key. + signed_encryption_key_ = GenerateAndSignKey(); + } else { + scoped_feature_list_.InitFromCommandLine("EncryptedReportingPipeline", + "EncryptedReporting"); + } + // Provide a mock cloud policy client. client_ = std::make_unique(); client_->SetDMToken("FAKE_DM_TOKEN"); - test_reporting_ = - std::make_unique(client_.get()); - - scoped_feature_list_.InitAndEnableFeature( - ReportQueueProvider::kEncryptedReportingPipeline); + test_reporting_ = std::make_unique( + base::FilePath(location_.GetPath()), + base::StringPiece( + reinterpret_cast(signature_verification_public_key_), + kKeySize), + client_.get()); } void TearDown() override { + // Let everything ongoing to finish. + task_environment_.RunUntilIdle(); + #if BUILDFLAG(IS_CHROMEOS_ASH) user_manager_.reset(); profile_.reset(); #endif // BUILDFLAG(IS_CHROMEOS_ASH) } - protected: + SignedEncryptionInfo GenerateAndSignKey() { + DCHECK(decryptor_) << "Decryptor not created"; + // Generate new pair of private key and public value. + uint8_t private_key[kKeySize]; + Encryptor::PublicKeyId public_key_id; + uint8_t public_value[kKeySize]; + test::GenerateEncryptionKeyPair(private_key, public_value); + test::TestEvent> prepare_key_pair; + decryptor_->RecordKeyPair( + std::string(reinterpret_cast(private_key), kKeySize), + std::string(reinterpret_cast(public_value), kKeySize), + prepare_key_pair.cb()); + auto prepare_key_result = prepare_key_pair.result(); + DCHECK(prepare_key_result.ok()); + public_key_id = prepare_key_result.ValueOrDie(); + // Deliver public key to storage. + SignedEncryptionInfo signed_encryption_key; + signed_encryption_key.set_public_asymmetric_key( + std::string(reinterpret_cast(public_value), kKeySize)); + signed_encryption_key.set_public_key_id(public_key_id); + // Sign public key. + uint8_t value_to_sign[sizeof(Encryptor::PublicKeyId) + kKeySize]; + memcpy(value_to_sign, &public_key_id, sizeof(Encryptor::PublicKeyId)); + memcpy(value_to_sign + sizeof(Encryptor::PublicKeyId), public_value, + kKeySize); + uint8_t signature[kSignatureSize]; + test::SignMessage( + signing_private_key_, + base::StringPiece(reinterpret_cast(value_to_sign), + sizeof(value_to_sign)), + signature); + signed_encryption_key.set_signature( + std::string(reinterpret_cast(signature), kSignatureSize)); + // Double check signature. + DCHECK(VerifySignature( + signature_verification_public_key_, + base::StringPiece(reinterpret_cast(value_to_sign), + sizeof(value_to_sign)), + signature)); + return signed_encryption_key; + } + + std::unique_ptr CreateQueue(bool expect_key_roundtrip) { + auto config_result = ReportQueueConfiguration::Create( + dm_token_, destination_, policy_checker_callback_); + EXPECT_TRUE(config_result.ok()); + + test::TestEvent>> create_queue_event; + if (expect_key_roundtrip) { + EXPECT_CALL(*client_, UploadEncryptedReport(_, _, _)) + .WillOnce(WithArgs<0, 2>(Invoke( + [this](base::Value payload, + policy::CloudPolicyClient::ResponseCallback done_cb) { + base::Optional const attach_encryption_settings = + payload.FindBoolKey("attachEncryptionSettings"); + ASSERT_TRUE(attach_encryption_settings.has_value()); + ASSERT_TRUE(attach_encryption_settings + .value()); // If set, must be true. + ASSERT_TRUE(is_encryption_enabled()); + + base::Value encryption_settings{base::Value::Type::DICTIONARY}; + std::string public_key; + base::Base64Encode( + signed_encryption_key_.public_asymmetric_key(), + &public_key); + encryption_settings.SetStringKey("publicKey", public_key); + encryption_settings.SetIntKey( + "publicKeyId", signed_encryption_key_.public_key_id()); + std::string public_key_signature; + base::Base64Encode(signed_encryption_key_.signature(), + &public_key_signature); + encryption_settings.SetStringKey("publicKeySignature", + public_key_signature); + base::Value response{base::Value::Type::DICTIONARY}; + response.SetPath("encryptionSettings", + std::move(encryption_settings)); + std::move(done_cb).Run(std::move(response)); + }))) + .RetiresOnSaturation(); + } + ReportQueueProvider::CreateQueue(std::move(config_result.ValueOrDie()), + create_queue_event.cb()); + auto result = create_queue_event.result(); + EXPECT_OK(result); + auto report_queue = std::move(result.ValueOrDie()); + + // Let everything ongoing to finish. + task_environment_.RunUntilIdle(); + + return report_queue; + } + + bool is_encryption_enabled() const { return GetParam(); } + base::test::ScopedFeatureList scoped_feature_list_; // BrowserTaskEnvironment must be instantiated before other classes that posts // tasks. content::BrowserTaskEnvironment task_environment_{ base::test::TaskEnvironment::TimeSource::MOCK_TIME}; std::unique_ptr test_reporting_; + + base::ScopedTempDir location_; + + uint8_t signature_verification_public_key_[kKeySize]; + uint8_t signing_private_key_[kSignKeySize]; + scoped_refptr decryptor_; + SignedEncryptionInfo signed_encryption_key_; + #if BUILDFLAG(IS_CHROMEOS_ASH) std::unique_ptr profile_; std::unique_ptr user_manager_; @@ -94,76 +229,76 @@ class ReportClientTest : public testing::Test { }; // Tests that a ReportQueue can be created using the ReportingClient. -TEST_F(ReportClientTest, CreatesReportQueue) { - auto config_result = ReportQueueConfiguration::Create( - dm_token_, destination_, policy_checker_callback_); - ASSERT_OK(config_result); - - test::TestEvent>> a; - ReportQueueProvider::CreateQueue(std::move(config_result.ValueOrDie()), - a.cb()); - ASSERT_OK(a.result()); +TEST_P(ReportClientTest, CreatesReportQueue) { + auto report_queue = CreateQueue(is_encryption_enabled()); + ASSERT_THAT(report_queue.get(), Ne(nullptr)); } // Ensures that created ReportQueues are actually different. -TEST_F(ReportClientTest, CreatesTwoDifferentReportQueues) { - auto config_result = ReportQueueConfiguration::Create( - dm_token_, destination_, policy_checker_callback_); - EXPECT_TRUE(config_result.ok()); - - test::TestEvent>> a1; - ReportQueueProvider::CreateQueue(std::move(config_result.ValueOrDie()), - a1.cb()); - auto result = a1.result(); - ASSERT_OK(result); - auto report_queue_1 = std::move(result.ValueOrDie()); - - test::TestEvent>> a2; - config_result = ReportQueueConfiguration::Create(dm_token_, destination_, - policy_checker_callback_); - ReportQueueProvider::CreateQueue(std::move(config_result.ValueOrDie()), - a2.cb()); - result = a2.result(); - ASSERT_OK(result); - auto report_queue_2 = std::move(result.ValueOrDie()); +TEST_P(ReportClientTest, CreatesTwoDifferentReportQueues) { + // Create first queue. + auto report_queue_1 = CreateQueue(is_encryption_enabled()); + ASSERT_THAT(report_queue_1.get(), Ne(nullptr)); + + // Create second queue. It will reuse the same ReportClient, so even if + // encryption is enabled, there will be no roundtrip to server to get the key. + auto report_queue_2 = CreateQueue(/*expect_key_roundtrip=*/false); + ASSERT_THAT(report_queue_2.get(), Ne(nullptr)); EXPECT_NE(report_queue_1.get(), report_queue_2.get()); } // Creates queue, enqueues messages and verifies they are uploaded. -TEST_F(ReportClientTest, EnqueueMessageAndUpload) { - auto config_result = ReportQueueConfiguration::Create( - dm_token_, destination_, policy_checker_callback_); - EXPECT_TRUE(config_result.ok()); - - test::TestEvent>> create_queue_event; - ReportQueueProvider::CreateQueue(std::move(config_result.ValueOrDie()), - create_queue_event.cb()); - auto result = create_queue_event.result(); - ASSERT_OK(result); - auto report_queue = std::move(result.ValueOrDie()); +TEST_P(ReportClientTest, EnqueueMessageAndUpload) { + // Create queue. + auto report_queue = CreateQueue(is_encryption_enabled()); + // Enqueue event. test::TestEvent enqueue_record_event; report_queue->Enqueue("Record", FAST_BATCH, enqueue_record_event.cb()); - ASSERT_OK(enqueue_record_event.result()); + const auto enqueue_record_result = enqueue_record_event.result(); + EXPECT_OK(enqueue_record_result) << enqueue_record_result; EXPECT_CALL(*client_, UploadEncryptedReport(_, _, _)) .WillOnce(WithArgs<0, 2>( - Invoke([](base::Value payload, - policy::CloudPolicyClient::ResponseCallback done_cb) { + Invoke([this](base::Value payload, + policy::CloudPolicyClient::ResponseCallback done_cb) { base::Value* const records = payload.FindListKey("encryptedRecord"); ASSERT_THAT(records, Ne(nullptr)); base::Value::ListView records_list = records->GetList(); ASSERT_THAT(records_list, SizeIs(1)); + base::Value& record = records_list[0]; + if (is_encryption_enabled()) { + const base::Value* const enctyption_info = + record.FindDictKey("encryptionInfo"); + ASSERT_THAT(enctyption_info, Ne(nullptr)); + const std::string* const encryption_key = + enctyption_info->FindStringKey("encryptionKey"); + ASSERT_THAT(encryption_key, Ne(nullptr)); + const std::string* const public_key_id = + enctyption_info->FindStringKey("publicKeyId"); + ASSERT_THAT(public_key_id, Ne(nullptr)); + int64_t key_id; + ASSERT_TRUE(base::StringToInt64(*public_key_id, &key_id)); + EXPECT_THAT(key_id, Eq(signed_encryption_key_.public_key_id())); + } else { + ASSERT_THAT(record.FindKey("encryptionInfo"), Eq(nullptr)); + } base::Value* const seq_info = - records_list[0].FindDictKey("sequenceInformation"); + record.FindDictKey("sequenceInformation"); ASSERT_THAT(seq_info, Ne(nullptr)); base::Value response{base::Value::Type::DICTIONARY}; response.SetPath("lastSucceedUploadedRecord", std::move(*seq_info)); std::move(done_cb).Run(std::move(response)); }))); + // Trigger upload. task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1)); } + +INSTANTIATE_TEST_SUITE_P(ReportClientTestSuite, + ReportClientTest, + ::testing::Bool() /* true - encryption enabled */); + } // namespace } // namespace reporting diff --git a/chrome/browser/policy/messaging_layer/upload/record_handler_impl.cc b/chrome/browser/policy/messaging_layer/upload/record_handler_impl.cc index 80ad5c1b895525..f49fad9fcbd6b7 100644 --- a/chrome/browser/policy/messaging_layer/upload/record_handler_impl.cc +++ b/chrome/browser/policy/messaging_layer/upload/record_handler_impl.cc @@ -162,7 +162,7 @@ void RecordHandlerImpl::ReportUploader::OnStart() { return; } - if (records_->empty()) { + if (records_->empty() && !need_encryption_key_) { Status empty_records = Status(error::INVALID_ARGUMENT, "records_ was empty"); LOG(ERROR) << empty_records; @@ -232,7 +232,7 @@ void RecordHandlerImpl::ReportUploader::HandleFailedUpload() { } void RecordHandlerImpl::ReportUploader::HandleSuccessfulUpload() { - // Decypher 'response' containing a base::Value dictionary that looks like: + // Decipher 'response' containing a base::Value dictionary that looks like: // { // "lastSucceedUploadedRecord": ... // SequencingInformation proto // "firstFailedUploadedRecord": { diff --git a/chrome/browser/policy/messaging_layer/upload/record_upload_request_builder.cc b/chrome/browser/policy/messaging_layer/upload/record_upload_request_builder.cc index c0775354c61931..462f340d9a71d6 100644 --- a/chrome/browser/policy/messaging_layer/upload/record_upload_request_builder.cc +++ b/chrome/browser/policy/messaging_layer/upload/record_upload_request_builder.cc @@ -43,8 +43,6 @@ constexpr char kPublicKeyId[] = "publicKeyId"; UploadEncryptedReportingRequestBuilder::UploadEncryptedReportingRequestBuilder( bool attach_encryption_settings) { result_ = base::Value{base::Value::Type::DICTIONARY}; - result_.value().SetKey(GetEncryptedRecordListPath(), - base::Value{base::Value::Type::LIST}); if (attach_encryption_settings) { result_.value().SetBoolKey(GetAttachEncryptionSettingsPath(), true); } @@ -60,9 +58,13 @@ UploadEncryptedReportingRequestBuilder::AddRecord( // Some errors were already detected. return *this; } - base::Value* const records_list = + base::Value* records_list = result_.value().FindListKey(GetEncryptedRecordListPath()); - if (!records_list || !records_list->is_list()) { + if (!records_list) { + records_list = result_.value().SetKey(GetEncryptedRecordListPath(), + base::Value{base::Value::Type::LIST}); + } + if (!records_list->is_list()) { NOTREACHED(); // Should not happen. return *this; } @@ -235,8 +237,9 @@ EncryptionInfoDictionaryBuilder::EncryptionInfoDictionaryBuilder( return; } - encryption_info_dictionary.SetStringKey(GetEncryptionKeyPath(), - encryption_info.encryption_key()); + std::string base64_key; + base::Base64Encode(encryption_info.encryption_key(), &base64_key); + encryption_info_dictionary.SetStringKey(GetEncryptionKeyPath(), base64_key); encryption_info_dictionary.SetStringKey( GetPublicKeyIdPath(), base::NumberToString(encryption_info.public_key_id())); diff --git a/chrome/browser/policy/messaging_layer/upload/upload_client.cc b/chrome/browser/policy/messaging_layer/upload/upload_client.cc index 5b16a7684e19ac..9bd8b249376150 100644 --- a/chrome/browser/policy/messaging_layer/upload/upload_client.cc +++ b/chrome/browser/policy/messaging_layer/upload/upload_client.cc @@ -48,7 +48,7 @@ Status UploadClient::EnqueueUpload( std::unique_ptr> records) { DCHECK(records); - if (records->empty()) { + if (records->empty() && !need_encryption_keys) { return Status::StatusOK(); } diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 372c0a9b60ed27..3cd7dfc9f8921f 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -4295,6 +4295,12 @@ test("unit_tests") { "//components/page_load_metrics/common:test_support", "//components/query_tiles:unit_tests", "//components/reporting/client:test_support", + "//components/reporting/encryption:decryption", + "//components/reporting/encryption:encryption", + "//components/reporting/encryption:encryption_module", + "//components/reporting/encryption:encryption_module_interface", + "//components/reporting/encryption:test_support", + "//components/reporting/encryption:testing_primitives", "//components/reporting/storage:storage_uploader_interface", "//components/reporting/storage:test_support", "//components/reporting/util:test_callbacks_support", diff --git a/components/reporting/storage/storage.cc b/components/reporting/storage/storage.cc index 83640c76d7961e..354243db5c5420 100644 --- a/components/reporting/storage/storage.cc +++ b/components/reporting/storage/storage.cc @@ -122,7 +122,7 @@ class Storage::QueueUploaderInterface : public UploaderInterface { static void AsyncProvideUploader( Priority priority, Storage* storage, - UploaderInterface::UploaderInterfaceResultCb start_uploader_cb) { + UploaderInterfaceResultCb start_uploader_cb) { storage->async_start_upload_cb_.Run( priority, /*need_encryption_key=*/EncryptionModuleInterface::is_enabled() && @@ -157,7 +157,7 @@ class Storage::QueueUploaderInterface : public UploaderInterface { private: static void WrapInstantiatedUploader( Priority priority, - UploaderInterface::UploaderInterfaceResultCb start_uploader_cb, + UploaderInterfaceResultCb start_uploader_cb, StatusOr> uploader_result) { if (!uploader_result.ok()) { std::move(start_uploader_cb).Run(uploader_result.status()); @@ -500,11 +500,15 @@ void Storage::Create( } else { if (EncryptionModuleInterface::is_enabled()) { // Initiate upload with need_encryption_key flag and no records. + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb = + base::BindOnce(&StorageInitContext::EncryptionKeyReceiverReady, + base::Unretained(this)); storage_->async_start_upload_cb_.Run( /*priority=*/MANUAL_BATCH, // Any priority would do. /*need_encryption_key=*/true, - base::BindOnce(&StorageInitContext::EncryptionKeyReceiverReady, - base::Unretained(this))); + base::BindOnce(&StorageInitContext::WrapInstantiatedKeyUploader, + /*priority=*/MANUAL_BATCH, + std::move(start_uploader_cb))); // Continue initialization without waiting for it to respond. // Until the response arrives, we will reject Enqueues. } @@ -527,6 +531,19 @@ void Storage::Create( } } + static void WrapInstantiatedKeyUploader( + Priority priority, + UploaderInterface::UploaderInterfaceResultCb start_uploader_cb, + StatusOr> uploader_result) { + if (!uploader_result.ok()) { + std::move(start_uploader_cb).Run(uploader_result.status()); + return; + } + std::move(start_uploader_cb) + .Run(std::make_unique( + priority, std::move(uploader_result.ValueOrDie()))); + } + void EncryptionKeyReceiverReady( StatusOr> uploader_result) { if (uploader_result.ok()) { @@ -645,9 +662,18 @@ void Storage::UpdateEncryptionKey(SignedEncryptionInfo signed_encryption_key) { })); // Serialize whole signed_encryption_key to a new file, discard the old - // one(s). - const Status status = key_in_storage_->UploadKeyFile(signed_encryption_key); - LOG_IF(ERROR, !status.ok()) << "Failed to upload the new encription key."; + // one(s). Do it on a thread which may block doing file operations. + base::ThreadPool::PostTask( + FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()}, + base::BindOnce( + [](SignedEncryptionInfo signed_encryption_key, + KeyInStorage* key_in_storage) { + const Status status = + key_in_storage->UploadKeyFile(signed_encryption_key); + LOG_IF(ERROR, !status.ok()) + << "Failed to upload the new encription key."; + }, + std::move(signed_encryption_key), key_in_storage_.get())); } StatusOr> Storage::GetQueue(Priority priority) { diff --git a/components/reporting/storage_selector/storage_selector.cc b/components/reporting/storage_selector/storage_selector.cc index ef4994ac64a069..0c94d819925918 100644 --- a/components/reporting/storage_selector/storage_selector.cc +++ b/components/reporting/storage_selector/storage_selector.cc @@ -14,7 +14,6 @@ #include "build/build_config.h" #include "build/chromeos_buildflags.h" #include "components/reporting/encryption/encryption_module.h" -#include "components/reporting/encryption/verification.h" #include "components/reporting/storage/storage_module.h" #include "components/reporting/storage/storage_module_interface.h" #include "components/reporting/storage/storage_uploader_interface.h" @@ -60,6 +59,7 @@ bool StorageSelector::is_uploader_required() { // static void StorageSelector::CreateStorageModule( const base::FilePath& local_reporting_path, + base::StringPiece verification_key, UploaderInterface::AsyncStartUploaderCb async_start_upload_cb, base::OnceCallback>)> cb) { @@ -89,12 +89,12 @@ void StorageSelector::CreateStorageModule( #endif // BUILDFLAG(IS_CHROMEOS_ASH) // Use Storage in a local file system. - StorageModule::Create(StorageOptions() - .set_directory(local_reporting_path) - .set_signature_verification_public_key( - SignatureVerifier::VerificationKey()), - std::move(async_start_upload_cb), - EncryptionModule::Create(), std::move(cb)); + StorageModule::Create( + StorageOptions() + .set_directory(local_reporting_path) + .set_signature_verification_public_key(verification_key), + std::move(async_start_upload_cb), EncryptionModule::Create(), + std::move(cb)); } } // namespace reporting diff --git a/components/reporting/storage_selector/storage_selector.h b/components/reporting/storage_selector/storage_selector.h index 807157225fcd68..1b4c40c5823a2f 100644 --- a/components/reporting/storage_selector/storage_selector.h +++ b/components/reporting/storage_selector/storage_selector.h @@ -35,6 +35,7 @@ class StorageSelector { static bool is_uploader_required(); static void CreateStorageModule( const base::FilePath& local_reporting_path, + base::StringPiece verification_key, UploaderInterface::AsyncStartUploaderCb async_start_upload_cb, base::OnceCallback>)> cb);