Skip to content

Commit

Permalink
bruschetta: Use custom network context to download the installer
Browse files Browse the repository at this point in the history 
Per discussion on crrev/c/4511794 and the linked bug, the default SSL
certificate handling for the installer (which runs in views) won't work
- it'll always fail. The agreed-upon approach was to create a new
network context for Bruschetta which implements our custom certificate
handling.

Bug: b:270656010
Test: New browser test fails before, passes after
Change-Id: I10af0b74c03812a59f316b8a03454815362fd7e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4576581
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Fergus Dall <sidereal@google.com>
Commit-Queue: David Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/main@{#1153639}
  • Loading branch information
David Munro authored and Chromium LUCI CQ committed Jun 6, 2023
1 parent fd43e6c commit d63cd3c
Show file tree
Hide file tree
Showing 10 changed files with 554 additions and 34 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/ash/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,8 @@ source_set("ash") {
"bruschetta/bruschetta_launcher.h",
"bruschetta/bruschetta_mount_provider.cc",
"bruschetta/bruschetta_mount_provider.h",
"bruschetta/bruschetta_network_context.cc",
"bruschetta/bruschetta_network_context.h",
"bruschetta/bruschetta_policy_handler.cc",
"bruschetta/bruschetta_policy_handler.h",
"bruschetta/bruschetta_pref_names.cc",
Expand Down
14 changes: 9 additions & 5 deletions chrome/browser/ash/bruschetta/bruschetta_download.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "base/memory/ptr_util.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "chrome/browser/ash/bruschetta/bruschetta_network_context.h"
#include "chrome/browser/extensions/cws_info_service.h"
#include "chrome/browser/profiles/profile.h"
#include "content/public/browser/storage_partition.h"
Expand Down Expand Up @@ -107,11 +108,14 @@ SimpleURLLoaderDownload::SimpleURLLoaderDownload(
Profile* profile,
GURL url,
base::OnceCallback<void(base::FilePath path, std::string sha256)> callback)
: profile_(profile), url_(std::move(url)), callback_(std::move(callback)) {
: url_(std::move(url)), callback_(std::move(callback)) {
// We're owned (through a few levels of owning class) by the installer view
// which won't outlive the profile, so it's safe to pass around the raw
// pointer.
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock()}, base::BindOnce(&MakeTempDir),
base::BindOnce(&SimpleURLLoaderDownload::Download,
weak_ptr_factory_.GetWeakPtr()));
weak_ptr_factory_.GetWeakPtr(), profile));
}

SimpleURLLoaderDownload::~SimpleURLLoaderDownload() {
Expand All @@ -123,16 +127,16 @@ SimpleURLLoaderDownload::~SimpleURLLoaderDownload() {
}

void SimpleURLLoaderDownload::Download(
Profile* profile,
std::unique_ptr<base::ScopedTempDir> dir) {
scoped_temp_dir_ = std::move(dir);
auto path = scoped_temp_dir_->GetPath().Append("download");
auto req = std::make_unique<network::ResourceRequest>();
req->url = url_;
loader_ = network::SimpleURLLoader::Create(std::move(req),
kBruschettaTrafficAnnotation);
auto factory = profile_->GetDefaultStoragePartition()
->GetURLLoaderFactoryForBrowserProcess();
loader_->DownloadToFile(factory.get(),
network_context_ = std::make_unique<BruschettaNetworkContext>(profile);
loader_->DownloadToFile(network_context_->GetURLLoaderFactory(),
base::BindOnce(&SimpleURLLoaderDownload::Finished,
weak_ptr_factory_.GetWeakPtr()),
std::move(path));
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ash/bruschetta/bruschetta_download.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "base/files/scoped_temp_dir.h"
#include "base/functional/callback_forward.h"
#include "base/functional/callback_helpers.h"
#include "base/memory/raw_ptr.h"
#include "services/network/public/cpp/simple_url_loader.h"
#include "url/gurl.h"
Expand All @@ -18,6 +17,7 @@ class SimpleURLLoader;
} // namespace network

namespace bruschetta {
class BruschettaNetworkContext;

extern const net::NetworkTrafficAnnotationTag kBruschettaTrafficAnnotation;

Expand Down Expand Up @@ -51,15 +51,15 @@ class SimpleURLLoaderDownload {
GURL url,
base::OnceCallback<void(base::FilePath path, std::string sha256)>
callback);
void Download(std::unique_ptr<base::ScopedTempDir> dir);
void Download(Profile* profile, std::unique_ptr<base::ScopedTempDir> dir);
void Finished(base::FilePath path);

base::raw_ptr<Profile> profile_;
GURL url_;
std::unique_ptr<base::ScopedTempDir> scoped_temp_dir_;
base::OnceCallback<void(base::FilePath path, std::string sha256)> callback_;
std::unique_ptr<network::SimpleURLLoader> loader_;
base::OnceClosure post_deletion_closure_for_testing_;
std::unique_ptr<BruschettaNetworkContext> network_context_;

base::WeakPtrFactory<SimpleURLLoaderDownload> weak_ptr_factory_{this};
};
Expand Down
154 changes: 133 additions & 21 deletions chrome/browser/ash/bruschetta/bruschetta_download_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,54 +9,158 @@
#include "base/strings/string_util.h"
#include "base/test/test_future.h"
#include "base/threading/thread_restrictions.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/net/profile_network_context_service.h"
#include "chrome/browser/net/profile_network_context_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "content/public/test/browser_test.h"
#include "net/dns/mock_host_resolver.h"
#include "net/ssl/client_cert_identity_test_util.h"
#include "net/ssl/client_cert_store.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/test_data_directory.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace bruschetta {
namespace {

class BruschettaDownloadBrowserTest : public InProcessBrowserTest {
class BruschettaDownloadBrowserTest : public InProcessBrowserTest {};

class BruschettaHttpsDownloadBrowserTest
: public BruschettaDownloadBrowserTest {
public:
BruschettaDownloadBrowserTest() = default;
~BruschettaDownloadBrowserTest() override = default;
BruschettaHttpsDownloadBrowserTest() = default;
~BruschettaHttpsDownloadBrowserTest() override = default;

BruschettaDownloadBrowserTest(const BruschettaDownloadBrowserTest&) = delete;
BruschettaDownloadBrowserTest& operator=(
const BruschettaDownloadBrowserTest&) = delete;
BruschettaHttpsDownloadBrowserTest(
const BruschettaHttpsDownloadBrowserTest&) = delete;
BruschettaHttpsDownloadBrowserTest& operator=(
const BruschettaHttpsDownloadBrowserTest&) = delete;

protected:
void SetUpInProcessBrowserTestFixture() override {
InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
BruschettaDownloadBrowserTest::SetUpInProcessBrowserTestFixture();

// Set up the test server, with the download URL and to require SSL cert.
net::SSLServerConfig ssl_config;
ssl_config.client_cert_type =
net::SSLServerConfig::ClientCertType::REQUIRE_CLIENT_CERT;
server_.SetSSLConfig(net::EmbeddedTestServer::CERT_OK, ssl_config);
server_.ServeFilesFromSourceDirectory("chrome/test/data/bruschetta");
server_handle_ = server_.StartAndReturnHandle();
ASSERT_TRUE(server_handle_);
url_ = server_.GetURL("/download_file.img");
}

void SetUpOnMainThread() override {
host_resolver()->AddRule("*", "127.0.0.1");

// Add an entry into AutoSelectCertificateForUrls policy for automatic
// client cert selection.
content::WebContents* tab =
browser()->tab_strip_model()->GetActiveWebContents();
Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
DCHECK(profile);
base::Value::List filters;
filters.Append(base::Value::Dict());
base::Value::Dict setting;
setting.Set("filters", std::move(filters));
HostContentSettingsMapFactory::GetForProfile(profile)
->SetWebsiteSettingDefaultScope(
url_, GURL(), ContentSettingsType::AUTO_SELECT_CERTIFICATE,
base::Value(std::move(setting)));
}

net::EmbeddedTestServer server_{net::EmbeddedTestServer::TYPE_HTTPS};
net::test_server::EmbeddedTestServerHandle server_handle_;
GURL url_;
};

bool PathExistsBlockingAllowed(const base::FilePath& path) {
base::ScopedAllowBlockingForTesting allow_blocking;
return base::PathExists(path);
}

IN_PROC_BROWSER_TEST_F(BruschettaDownloadBrowserTest, TestHappyPath) {
auto expected_hash = base::ToUpperASCII(
// A stub ClientCertStore which returns the list of client certs it was
// initialised with.
class StubClientCertStore : public net::ClientCertStore {
public:
explicit StubClientCertStore(net::ClientCertIdentityList list)
: list_(std::move(list)) {}

~StubClientCertStore() override = default;

// net::ClientCertStore override.
void GetClientCerts(const net::SSLCertRequestInfo& cert_request_info,
ClientCertListCallback callback) override {
std::move(callback).Run(std::move(list_));
}

private:
net::ClientCertIdentityList list_;
};

// Creates a stub client cert store which returns a single test certificate.
std::unique_ptr<net::ClientCertStore> CreateStubClientCertStore() {
base::FilePath certs_dir = net::GetTestCertsDirectory();

net::ClientCertIdentityList cert_identity_list;
base::ScopedAllowBlockingForTesting allow_blocking;
std::unique_ptr<net::FakeClientCertIdentity> cert_identity =
net::FakeClientCertIdentity::CreateFromCertAndKeyFiles(
certs_dir, "client_1.pem", "client_1.pk8");
EXPECT_TRUE(cert_identity.get());
if (cert_identity) {
cert_identity_list.push_back(std::move(cert_identity));
}

return std::unique_ptr<net::ClientCertStore>(
new StubClientCertStore(std::move(cert_identity_list)));
}

std::unique_ptr<net::ClientCertStore> CreateEmptyClientCertStore() {
return std::unique_ptr<net::ClientCertStore>(new StubClientCertStore({}));
}

// Tests that we get an empty path and hash for a network error (in this case
// a bad URL).
IN_PROC_BROWSER_TEST_F(BruschettaHttpsDownloadBrowserTest,
TestDownloadUrlNotFound) {
base::test::TestFuture<base::FilePath, std::string> future;
auto download = SimpleURLLoaderDownload::StartDownload(
browser()->profile(), GURL("bad url"), future.GetCallback());

auto path = future.Get<base::FilePath>();
auto hash = future.Get<std::string>();

ASSERT_TRUE(path.empty());
EXPECT_EQ(hash, "");
}

// Tests that the `url_` is downloaded successfully.
// Because the `server_` is configured to require a client certificate
// this involves selecting a client certificate using the auto-selection
// setting configured in `SetUpOnMainThread`
IN_PROC_BROWSER_TEST_F(BruschettaHttpsDownloadBrowserTest, TestHappyPath) {
const auto kExpectedHash = base::ToUpperASCII(
"f54d00e6d24844ee3b1d0d8c2b9d2ed80b967e94eb1055bb1fd43eb9522908cc");

net::EmbeddedTestServer server(net::EmbeddedTestServer::TYPE_HTTPS);
server.ServeFilesFromSourceDirectory("chrome/test/data/bruschetta");
auto server_handle = server.StartAndReturnHandle();
ASSERT_TRUE(server_handle);
GURL url = server.GetURL("/download_file.img");
// Make the browser use the ClientCertStoreStub instead of the regular one.
ProfileNetworkContextServiceFactory::GetForContext(browser()->profile())
->set_client_cert_store_factory_for_testing(
base::BindRepeating(&CreateStubClientCertStore));

base::test::TestFuture<base::FilePath, std::string> future;
base::FilePath path;
auto download = SimpleURLLoaderDownload::StartDownload(
browser()->profile(), url, future.GetCallback());
browser()->profile(), url_, future.GetCallback());

path = future.Get<base::FilePath>();
auto path = future.Get<base::FilePath>();
auto hash = future.Get<std::string>();

ASSERT_FALSE(path.empty());
EXPECT_EQ(hash, expected_hash);
EXPECT_EQ(hash, kExpectedHash);

// Deleting the download should clean up downloaded files. I found
// RunUntilIdle to be flaky hence we have an explicit callback for after
Expand All @@ -68,16 +172,24 @@ IN_PROC_BROWSER_TEST_F(BruschettaDownloadBrowserTest, TestHappyPath) {
EXPECT_FALSE(PathExistsBlockingAllowed(path));
}

IN_PROC_BROWSER_TEST_F(BruschettaDownloadBrowserTest, TestDownloadFailed) {
// Tests downloading from url_, which requires an SSL cert for auth, when the
// cert isn't available. This should fail and signal that by returning an empty
// path.
IN_PROC_BROWSER_TEST_F(BruschettaHttpsDownloadBrowserTest,
TestDownloadNoMatchingCert) {
// Make the browser use an empty client cert store
ProfileNetworkContextServiceFactory::GetForContext(browser()->profile())
->set_client_cert_store_factory_for_testing(
base::BindRepeating(&CreateEmptyClientCertStore));

base::test::TestFuture<base::FilePath, std::string> future;
auto download = SimpleURLLoaderDownload::StartDownload(
browser()->profile(), GURL("bad url"), future.GetCallback());
browser()->profile(), url_, future.GetCallback());

auto path = future.Get<base::FilePath>();
auto hash = future.Get<std::string>();

ASSERT_TRUE(path.empty());
EXPECT_EQ(hash, "");
}

} // namespace
Expand Down

0 comments on commit d63cd3c

Please sign in to comment.