update_engine: Attach session ID to HTTP header of binary download
In order for Omaha to correlate Omaha Client requests with the actual
binary download, the session ID must be attached to the HTTP header of
the binary download in the X-Goog-Update-SessionId.
Also, remove the HTTP header of X-Goog-Update-SessionId added into the
Omaha requests.
BUG=chromium:940515
TEST=unittests # new unittests
Change-Id: I0759562f2d1c8c003064ad976ca1ae6ce039b960
diff --git a/Android.bp b/Android.bp
index d9f3524..b91e883 100644
--- a/Android.bp
+++ b/Android.bp
@@ -622,6 +622,7 @@
"common/terminator_unittest.cc",
"common/test_utils.cc",
"common/utils_unittest.cc",
+ "libcurl_http_fetcher_unittest.cc",
"payload_consumer/bzip_extent_writer_unittest.cc",
"payload_consumer/cached_file_descriptor_unittest.cc",
"payload_consumer/delta_performer_integration_test.cc",
diff --git a/BUILD.gn b/BUILD.gn
index 2d4e9a4..224ad45 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -478,6 +478,7 @@
"connection_manager_unittest.cc",
"hardware_chromeos_unittest.cc",
"image_properties_chromeos_unittest.cc",
+ "libcurl_http_fetcher_unittest.cc",
"metrics_reporter_omaha_unittest.cc",
"metrics_utils_unittest.cc",
"omaha_request_action_unittest.cc",
diff --git a/common/constants.cc b/common/constants.cc
index 5ab96b0..87bdf91 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -126,4 +126,10 @@
const char kOmahaUpdaterVersion[] = "0.1.0.0";
+// X-Goog-Update headers.
+const char kXGoogleUpdateInteractivity[] = "X-Goog-Update-Interactivity";
+const char kXGoogleUpdateAppId[] = "X-Goog-Update-AppId";
+const char kXGoogleUpdateUpdater[] = "X-Goog-Update-Updater";
+const char kXGoogleUpdateSessionId[] = "X-Goog-SessionId";
+
} // namespace chromeos_update_engine
diff --git a/common/constants.h b/common/constants.h
index 9b4623f..d95a56a 100644
--- a/common/constants.h
+++ b/common/constants.h
@@ -110,6 +110,12 @@
extern const char kOmahaUpdaterVersion[];
+// X-Goog-Update headers.
+extern const char kXGoogleUpdateInteractivity[];
+extern const char kXGoogleUpdateAppId[];
+extern const char kXGoogleUpdateUpdater[];
+extern const char kXGoogleUpdateSessionId[];
+
// A download source is any combination of protocol and server (that's of
// interest to us when looking at UMA metrics) using which we may download
// the payload.
diff --git a/common/file_fetcher.h b/common/file_fetcher.h
index fbdfc32..bd39007 100644
--- a/common/file_fetcher.h
+++ b/common/file_fetcher.h
@@ -59,6 +59,12 @@
void SetHeader(const std::string& header_name,
const std::string& header_value) override {}
+ bool GetHeader(const std::string& header_name,
+ std::string* header_value) const override {
+ header_value->clear();
+ return false;
+ }
+
// Suspend the asynchronous file read.
void Pause() override;
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 93b0e24..94f31d7 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -100,6 +100,14 @@
virtual void SetHeader(const std::string& header_name,
const std::string& header_value) = 0;
+ // Only used for testing.
+ // If |header_name| is set, the value will be set into |header_value|.
+ // On success the boolean true will be returned, hoewever on failture to find
+ // the |header_name| in the header the return value will be false. The state
+ // in which |header_value| is left in for failures is an empty string.
+ virtual bool GetHeader(const std::string& header_name,
+ std::string* header_value) const = 0;
+
// If data is coming in too quickly, you can call Pause() to pause the
// transfer. The delegate will not have ReceivedBytes() called while
// an HttpFetcher is paused.
diff --git a/common/mock_http_fetcher.h b/common/mock_http_fetcher.h
index 492e6ce..0f04319 100644
--- a/common/mock_http_fetcher.h
+++ b/common/mock_http_fetcher.h
@@ -89,6 +89,12 @@
void SetHeader(const std::string& header_name,
const std::string& header_value) override;
+ bool GetHeader(const std::string& header_name,
+ std::string* header_value) const override {
+ header_value->clear();
+ return false;
+ }
+
// Return the value of the header |header_name| or the empty string if not
// set.
std::string GetHeader(const std::string& header_name) const;
diff --git a/common/multi_range_http_fetcher.h b/common/multi_range_http_fetcher.h
index f57ea7f..ef32f0d 100644
--- a/common/multi_range_http_fetcher.h
+++ b/common/multi_range_http_fetcher.h
@@ -83,6 +83,11 @@
base_fetcher_->SetHeader(header_name, header_value);
}
+ bool GetHeader(const std::string& header_name,
+ std::string* header_value) const override {
+ return base_fetcher_->GetHeader(header_name, header_value);
+ }
+
void Pause() override { base_fetcher_->Pause(); }
void Unpause() override { base_fetcher_->Unpause(); }
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index d39351c..06722fd 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -26,6 +26,7 @@
#include <base/format_macros.h>
#include <base/location.h>
#include <base/logging.h>
+#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
@@ -392,6 +393,37 @@
extra_headers_[base::ToLowerASCII(header_name)] = header_line;
}
+// Inputs: header_name, header_value
+// Example:
+// extra_headers_ = { {"foo":"foo: 123"}, {"bar":"bar:"} }
+// string tmp = "gibberish";
+// Case 1:
+// GetHeader("foo", &tmp) -> tmp = "123", return true.
+// Case 2:
+// GetHeader("bar", &tmp) -> tmp = "", return true.
+// Case 3:
+// GetHeader("moo", &tmp) -> tmp = "", return false.
+bool LibcurlHttpFetcher::GetHeader(const string& header_name,
+ string* header_value) const {
+ // Initially clear |header_value| to handle both success and failures without
+ // leaving |header_value| in a unclear state.
+ header_value->clear();
+ auto header_key = base::ToLowerASCII(header_name);
+ auto header_line_itr = extra_headers_.find(header_key);
+ // If the |header_name| was never set, indicate so by returning false.
+ if (header_line_itr == extra_headers_.end())
+ return false;
+ // From |SetHeader()| the check for |header_name| to not include ":" is
+ // verified, so finding the first index of ":" is a safe operation.
+ auto header_line = header_line_itr->second;
+ *header_value = header_line.substr(header_line.find(':') + 1);
+ // The following is neccessary to remove the leading ' ' before the header
+ // value that was place only if |header_value| passed to |SetHeader()| was
+ // a non-empty string.
+ header_value->erase(0, 1);
+ return true;
+}
+
void LibcurlHttpFetcher::CurlPerformOnce() {
CHECK(transfer_in_progress_);
int running_handles = 0;
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index 24103de..3978b70 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -61,6 +61,9 @@
void SetHeader(const std::string& header_name,
const std::string& header_value) override;
+ bool GetHeader(const std::string& header_name,
+ std::string* header_value) const override;
+
// Suspend the transfer by calling curl_easy_pause(CURLPAUSE_ALL).
void Pause() override;
diff --git a/libcurl_http_fetcher_unittest.cc b/libcurl_http_fetcher_unittest.cc
new file mode 100644
index 0000000..88e48fa
--- /dev/null
+++ b/libcurl_http_fetcher_unittest.cc
@@ -0,0 +1,81 @@
+//
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "update_engine/libcurl_http_fetcher.h"
+
+#include <string>
+
+#include <brillo/message_loops/fake_message_loop.h>
+#include <gtest/gtest.h>
+
+#include "update_engine/common/fake_hardware.h"
+#include "update_engine/common/mock_proxy_resolver.h"
+
+using std::string;
+
+namespace chromeos_update_engine {
+
+namespace {
+constexpr char kHeaderName[] = "X-Goog-Test-Header";
+}
+
+class LibcurlHttpFetcherTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ loop_.SetAsCurrent();
+ fake_hardware_.SetIsOfficialBuild(true);
+ fake_hardware_.SetIsOOBEEnabled(false);
+ }
+
+ brillo::FakeMessageLoop loop_{nullptr};
+ FakeHardware fake_hardware_;
+ LibcurlHttpFetcher libcurl_fetcher_{nullptr, &fake_hardware_};
+};
+
+TEST_F(LibcurlHttpFetcherTest, GetEmptyHeaderValueTest) {
+ const string header_value = "";
+ string actual_header_value;
+ libcurl_fetcher_.SetHeader(kHeaderName, header_value);
+ EXPECT_TRUE(libcurl_fetcher_.GetHeader(kHeaderName, &actual_header_value));
+ EXPECT_EQ("", actual_header_value);
+}
+
+TEST_F(LibcurlHttpFetcherTest, GetHeaderTest) {
+ const string header_value = "This-is-value 123";
+ string actual_header_value;
+ libcurl_fetcher_.SetHeader(kHeaderName, header_value);
+ EXPECT_TRUE(libcurl_fetcher_.GetHeader(kHeaderName, &actual_header_value));
+ EXPECT_EQ(header_value, actual_header_value);
+}
+
+TEST_F(LibcurlHttpFetcherTest, GetNonExistentHeaderValueTest) {
+ string actual_header_value;
+ // Skip |SetHeaader()| call.
+ EXPECT_FALSE(libcurl_fetcher_.GetHeader(kHeaderName, &actual_header_value));
+ // Even after a failed |GetHeaderValue()|, enforce that the passed pointer to
+ // modifiable string was cleared to be empty.
+ EXPECT_EQ("", actual_header_value);
+}
+
+TEST_F(LibcurlHttpFetcherTest, GetHeaderEdgeCaseTest) {
+ const string header_value = "\a\b\t\v\f\r\\ edge:-case: \a\b\t\v\f\r\\";
+ string actual_header_value;
+ libcurl_fetcher_.SetHeader(kHeaderName, header_value);
+ EXPECT_TRUE(libcurl_fetcher_.GetHeader(kHeaderName, &actual_header_value));
+ EXPECT_EQ(header_value, actual_header_value);
+}
+
+} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 40e52f0..6c67a3b 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -107,12 +107,6 @@
constexpr char kValPostInstall[] = "postinstall";
constexpr char kValNoUpdate[] = "noupdate";
-// X-Goog-Update headers.
-constexpr char kXGoogleUpdateInteractivity[] = "X-Goog-Update-Interactivity";
-constexpr char kXGoogleUpdateAppId[] = "X-Goog-Update-AppId";
-constexpr char kXGoogleUpdateUpdater[] = "X-Goog-Update-Updater";
-constexpr char kXGoogleUpdateSessionId[] = "X-Goog-Update-SessionId";
-
// updatecheck attributes (without the underscore prefix).
constexpr char kAttrEol[] = "eol";
constexpr char kAttrRollback[] = "rollback";
@@ -444,7 +438,6 @@
kXGoogleUpdateUpdater,
base::StringPrintf(
"%s-%s", constants::kOmahaUpdaterID, kOmahaUpdaterVersion));
- http_fetcher_->SetHeader(kXGoogleUpdateSessionId, session_id_);
http_fetcher_->SetPostData(
request_post.data(), request_post.size(), kHttpContentTypeTextXml);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index e13d10e..41b2520 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -84,7 +84,6 @@
const char kTestAppId[] = "test-app-id";
const char kTestAppId2[] = "test-app2-id";
const char kTestAppIdSkipUpdatecheck[] = "test-app-id-skip-updatecheck";
-const char kTestSessionId[] = "12341234-1234-1234-1234-1234123412341234";
// This is a helper struct to allow unit tests build an update response with the
// values they care about.
@@ -297,8 +296,6 @@
fetcher->GetHeader("X-Goog-Update-Interactivity"));
EXPECT_EQ(kTestAppId, fetcher->GetHeader("X-Goog-Update-AppId"));
EXPECT_NE("", fetcher->GetHeader("X-Goog-Update-Updater"));
- EXPECT_EQ(kTestSessionId,
- fetcher->GetHeader("X-Goog-Update-SessionId"));
}
post_data_ = fetcher->post_data();
} else if (action->Type() ==
@@ -370,7 +367,6 @@
.expected_check_result = metrics::CheckResult::kUpdateAvailable,
.expected_check_reaction = metrics::CheckReaction::kUpdating,
.expected_download_error_code = metrics::DownloadErrorCode::kUnset,
- .session_id = kTestSessionId,
};
}
diff --git a/update_attempter.cc b/update_attempter.cc
index 50aa9f4..b3cb3c3 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -683,6 +683,7 @@
download_fetcher->set_server_to_check(ServerToCheck::kDownload);
if (interactive)
download_fetcher->set_max_retry_count(kDownloadMaxRetryCountInteractive);
+ download_fetcher->SetHeader(kXGoogleUpdateSessionId, session_id_);
auto download_action =
std::make_unique<DownloadAction>(prefs_,
system_state_->boot_control(),
diff --git a/update_attempter.h b/update_attempter.h
index 1c3abe1..b0654d8 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -269,7 +269,6 @@
FRIEND_TEST(UpdateAttempterTest, RollbackMetricsRollbackSuccess);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionNoEventTest);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionTest);
- FRIEND_TEST(UpdateAttempterTest, SessionIdTestOnUpdateCheck);
FRIEND_TEST(UpdateAttempterTest, SessionIdTestEnforceEmptyStrPingOmaha);
FRIEND_TEST(UpdateAttempterTest, SessionIdTestOnOmahaRequestActions);
FRIEND_TEST(UpdateAttempterTest, SetRollbackHappenedNotRollback);
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 8a896da..8b8e390 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -31,6 +31,7 @@
#include <policy/mock_device_policy.h>
#include <policy/mock_libpolicy.h>
+#include "update_engine/common/constants.h"
#include "update_engine/common/dlcservice_interface.h"
#include "update_engine/common/fake_clock.h"
#include "update_engine/common/fake_prefs.h"
@@ -43,6 +44,7 @@
#include "update_engine/common/test_utils.h"
#include "update_engine/common/utils.h"
#include "update_engine/fake_system_state.h"
+#include "update_engine/libcurl_http_fetcher.h"
#include "update_engine/mock_p2p_manager.h"
#include "update_engine/mock_payload_state.h"
#include "update_engine/mock_service_observer.h"
@@ -195,6 +197,7 @@
void SessionIdTestChange();
void SessionIdTestEnforceEmptyStrPingOmaha();
void SessionIdTestConsistencyInUpdateFlow();
+ void SessionIdTestInDownloadAction();
void UpdateToQuickFixBuildStart(bool set_token);
void ResetRollbackHappenedStart(bool is_consumer,
bool is_policy_available,
@@ -305,6 +308,32 @@
loop_.Run();
}
+void UpdateAttempterTest::SessionIdTestInDownloadAction() {
+ // The session ID passed into |DownloadAction|'s |LibcurlHttpFetcher| should
+ // be enforced to be included in the HTTP header as X-Goog-Update-SessionId.
+ string header_value;
+ auto CheckSessionIdInDownloadAction = [&header_value](AbstractAction* aa) {
+ if (aa->Type() == DownloadAction::StaticType()) {
+ DownloadAction* da = static_cast<DownloadAction*>(aa);
+ EXPECT_TRUE(da->http_fetcher()->GetHeader(kXGoogleUpdateSessionId,
+ &header_value));
+ }
+ };
+ EXPECT_CALL(*processor_, EnqueueAction(Pointee(_)))
+ .WillRepeatedly(Invoke(CheckSessionIdInDownloadAction));
+ attempter_.BuildUpdateActions(false);
+ // Validate that X-Goog-Update_SessionId is set correctly in HTTP Header.
+ EXPECT_EQ(attempter_.session_id_, header_value);
+ ScheduleQuitMainLoop();
+}
+
+TEST_F(UpdateAttempterTest, SessionIdTestInDownloadAction) {
+ loop_.PostTask(FROM_HERE,
+ base::Bind(&UpdateAttempterTest::SessionIdTestInDownloadAction,
+ base::Unretained(this)));
+ loop_.Run();
+}
+
TEST_F(UpdateAttempterTest, ActionCompletedDownloadTest) {
unique_ptr<MockHttpFetcher> fetcher(new MockHttpFetcher("", 0, nullptr));
fetcher->FailTransfer(503); // Sets the HTTP response code.