Commit 621dcebf authored by matterchen's avatar matterchen Committed by Commit Bot

Reland: Crostini port forwarding activate and add port

Port forwarding in Cros will allow users to add new ports and activate,
deactivate and remove existing ports.

When any of the utilities are called, the port forwarding library will
invoke the permission broker client to change the ChromeOS port
forwarding settings.

In future work we will also save port forwarding preferences such that a
user's preferences will persist over container shutdown.

Revert CL comment:
Reason for revert: CrostiniPortForwarderTest.AddPortDuplicateFail failing on Linux Chromium OS ASan LSan Tests (1)

First failing build: https://ci.chromium.org/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/36750

Failure example:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8890488549187658624/+/steps/unit_tests/0/logs/Deterministic_failure:_CrostiniPortForwarderTest.AddPortDuplicateFail__status_CRASH_/0

Actions taken:
Built tests and ran with address sanitizer, confirmed
existence of issue. Made appropriate changes and confirmed address
sanitizer does not raise issues anymore.

everything builds

Bug: chromium:848127
Test: crostini_port_forwarder_unittest.cc (+Address Sanitizer) and
Change-Id: Ib1d1b901b9f9e45fb7e85feaa0b97be026feabac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018363Reviewed-by: default avatarNic Hollingum <hollingum@google.com>
Reviewed-by: default avatarRyo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Matthew Chen <matterchen@google.com>
Cr-Commit-Position: refs/heads/master@{#735761}
parent b934de52
......@@ -883,6 +883,8 @@ source_set("chromeos") {
"crostini/crostini_package_operation_status.h",
"crostini/crostini_package_service.cc",
"crostini/crostini_package_service.h",
"crostini/crostini_port_forwarder.cc",
"crostini/crostini_port_forwarder.h",
"crostini/crostini_pref_names.cc",
"crostini/crostini_pref_names.h",
"crostini/crostini_registry_service.cc",
......@@ -2724,6 +2726,7 @@ source_set("unit_tests") {
"crostini/crostini_mime_types_service_unittest.cc",
"crostini/crostini_package_notification_unittest.cc",
"crostini/crostini_package_service_unittest.cc",
"crostini/crostini_port_forwarder_unittest.cc",
"crostini/crostini_registry_service_unittest.cc",
"crostini/crostini_reporting_util_unittest.cc",
"crostini/crostini_unsupported_action_notifier_unittest.cc",
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <fcntl.h>
#include "chrome/browser/chromeos/crostini/crostini_port_forwarder.h"
#include "base/bind.h"
#include "base/no_destructor.h"
#include "chrome/browser/profiles/profile.h"
#include "chromeos/dbus/permission_broker/permission_broker_client.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
namespace crostini {
// Currently, we are not supporting ethernet/mlan/usb port forwarding.
constexpr char kDefaultInterfaceToForward[] = "wlan0";
class CrostiniPortForwarderFactory : public BrowserContextKeyedServiceFactory {
public:
static CrostiniPortForwarder* GetForProfile(Profile* profile) {
return static_cast<CrostiniPortForwarder*>(
GetInstance()->GetServiceForBrowserContext(profile, true));
}
static CrostiniPortForwarderFactory* GetInstance() {
static base::NoDestructor<CrostiniPortForwarderFactory> factory;
return factory.get();
}
private:
friend class base::NoDestructor<CrostiniPortForwarderFactory>;
CrostiniPortForwarderFactory()
: BrowserContextKeyedServiceFactory(
"CrostiniPortForwarderService",
BrowserContextDependencyManager::GetInstance()) {}
~CrostiniPortForwarderFactory() override = default;
// BrowserContextKeyedServiceFactory:
KeyedService* BuildServiceInstanceFor(
content::BrowserContext* context) const override {
Profile* profile = Profile::FromBrowserContext(context);
return new CrostiniPortForwarder(profile);
}
};
CrostiniPortForwarder* CrostiniPortForwarder::GetForProfile(Profile* profile) {
return CrostiniPortForwarderFactory::GetForProfile(profile);
}
CrostiniPortForwarder::CrostiniPortForwarder(Profile* profile)
: profile_(profile) {
// TODO(matterchen): Profile will be used later, this is to remove warnings.
(void)profile_;
}
CrostiniPortForwarder::~CrostiniPortForwarder() = default;
void CrostiniPortForwarder::OnActivatePortCompleted(
ResultCallback result_callback,
PortRuleKey key,
bool success) {
if (!success) {
forwarded_ports_.erase(key);
LOG(ERROR) << "Failed to activate port, port preference not added: "
<< key.port_number;
std::move(result_callback).Run(success);
return;
}
// TODO(matterchen): Update current port forwarding preference.
std::move(result_callback).Run(success);
}
void CrostiniPortForwarder::OnAddPortCompleted(ResultCallback result_callback,
std::string label,
PortRuleKey key,
bool success) {
if (!success) {
forwarded_ports_.erase(key);
LOG(ERROR) << "Failed to activate port, port preference not added: "
<< key.port_number;
std::move(result_callback).Run(success);
return;
}
// TODO(matterchen): Add new port forwarding preference.
std::move(result_callback).Run(success);
}
void CrostiniPortForwarder::TryActivatePort(
uint16_t port_number,
const Protocol& protocol_type,
const std::string& ipv4_addr,
chromeos::PermissionBrokerClient::ResultCallback result_callback) {
chromeos::PermissionBrokerClient* client =
chromeos::PermissionBrokerClient::Get();
if (!client) {
LOG(ERROR) << "Could not get permission broker client.";
std::move(result_callback).Run(false);
return;
}
int lifeline[2] = {-1, -1};
if (pipe(lifeline) < 0) {
LOG(ERROR) << "Failed to create a lifeline pipe";
std::move(result_callback).Run(false);
return;
}
PortRuleKey port_key = {
.port_number = port_number,
.protocol_type = protocol_type,
.input_ifname = kDefaultInterfaceToForward,
};
base::ScopedFD lifeline_local(lifeline[0]);
base::ScopedFD lifeline_remote(lifeline[1]);
forwarded_ports_[port_key] = std::move(lifeline_local);
if (Protocol::TCP == protocol_type) {
client->RequestTcpPortForward(
port_number, kDefaultInterfaceToForward, ipv4_addr, port_number,
std::move(lifeline_remote.get()), std::move(result_callback));
} else if (Protocol::UDP == protocol_type) {
client->RequestUdpPortForward(
port_number, kDefaultInterfaceToForward, ipv4_addr, port_number,
std::move(lifeline_remote.get()), std::move(result_callback));
}
}
void CrostiniPortForwarder::AddPort(uint16_t port_number,
const Protocol& protocol_type,
const std::string& label,
ResultCallback result_callback) {
PortRuleKey new_port_key = {
.port_number = port_number,
.protocol_type = protocol_type,
.input_ifname = kDefaultInterfaceToForward,
};
if (forwarded_ports_.find(new_port_key) != forwarded_ports_.end()) {
LOG(ERROR) << "Trying to add an already forwarded port.";
std::move(result_callback).Run(false);
return;
}
base::OnceCallback<void(bool)> on_add_port_completed =
base::BindOnce(&CrostiniPortForwarder::OnAddPortCompleted,
weak_ptr_factory_.GetWeakPtr(), std::move(result_callback),
label, new_port_key);
// TODO(matterchen): Extract container IPv4 address.
TryActivatePort(port_number, protocol_type, "PLACEHOLDER_IP_ADDRESS",
std::move(on_add_port_completed));
}
void CrostiniPortForwarder::ActivatePort(uint16_t port_number,
ResultCallback result_callback) {
// TODO(matterchen): Find the current port setting from profile preferences.
PortRuleKey existing_port_key = {
.port_number = port_number,
.protocol_type = Protocol::TCP,
.input_ifname = kDefaultInterfaceToForward,
};
base::OnceCallback<void(bool)> on_activate_port_completed =
base::BindOnce(&CrostiniPortForwarder::OnActivatePortCompleted,
weak_ptr_factory_.GetWeakPtr(), std::move(result_callback),
existing_port_key);
// TODO(matterchen): Extract container IPv4 address.
CrostiniPortForwarder::TryActivatePort(port_number, Protocol::TCP,
"PLACEHOLDER_IP_ADDRESS",
std::move(on_activate_port_completed));
}
} // namespace crostini
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_PORT_FORWARDER_H_
#define CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_PORT_FORWARDER_H_
#include <string>
#include "base/files/scoped_file.h"
#include "base/memory/weak_ptr.h"
#include "base/values.h"
#include "components/keyed_service/core/keyed_service.h"
class Profile;
namespace crostini {
class CrostiniPortForwarder : public KeyedService {
public:
enum class Protocol {
TCP,
UDP,
};
struct PortRuleKey {
uint16_t port_number;
Protocol protocol_type;
std::string input_ifname;
bool operator==(const PortRuleKey& other) const {
return port_number == other.port_number &&
protocol_type == other.protocol_type &&
input_ifname == other.input_ifname;
}
};
// Helper for using PortRuleKey as key entries in std::unordered_maps.
struct PortRuleKeyHasher {
std::size_t operator()(const PortRuleKey& k) const {
return ((std::hash<uint16_t>()(k.port_number) ^
(std::hash<Protocol>()(k.protocol_type) << 1)) >>
1) ^
(std::hash<std::string>()(k.input_ifname) << 1);
}
};
using ResultCallback = base::OnceCallback<void(bool)>;
void ActivatePort(uint16_t port_number, ResultCallback result_callback);
void AddPort(uint16_t port_number,
const Protocol& protocol_type,
const std::string& label,
ResultCallback result_callback);
static CrostiniPortForwarder* GetForProfile(Profile* profile);
explicit CrostiniPortForwarder(Profile* profile);
~CrostiniPortForwarder() override;
private:
FRIEND_TEST_ALL_PREFIXES(CrostiniPortForwarderTest, AddPortDuplicateFail);
FRIEND_TEST_ALL_PREFIXES(CrostiniPortForwarderTest, AddPortMultipleSuccess);
FRIEND_TEST_ALL_PREFIXES(CrostiniPortForwarderTest, AddPortUdpAndTcpSuccess);
FRIEND_TEST_ALL_PREFIXES(CrostiniPortForwarderTest,
TryActivatePortPermissionBrokerClientFail);
void OnActivatePortCompleted(ResultCallback result_callback,
PortRuleKey key,
bool success);
void OnAddPortCompleted(ResultCallback result_callback,
std::string label,
PortRuleKey key,
bool success);
void TryActivatePort(uint16_t port_number,
const Protocol& protocol_type,
const std::string& ipv4_addr,
base::OnceCallback<void(bool)> result_callback);
// For each port rule (protocol, port, interface), keep track of the fd which
// requested it so we can release it on removal / deactivate.
std::unordered_map<PortRuleKey, base::ScopedFD, PortRuleKeyHasher>
forwarded_ports_;
Profile* profile_;
base::WeakPtrFactory<CrostiniPortForwarder> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(CrostiniPortForwarder);
}; // class
} // namespace crostini
#endif // CHROME_BROWSER_CHROMEOS_CROSTINI_CROSTINI_PORT_FORWARDER_H_
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/chromeos/crostini/crostini_port_forwarder.h"
#include "chrome/browser/chromeos/crostini/crostini_test_helper.h"
#include "chrome/test/base/testing_profile.h"
#include "chromeos/dbus/permission_broker/fake_permission_broker_client.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using testing::Mock;
using testing::Return;
void TestingCallback(bool* out, bool in) {
*out = in;
}
namespace crostini {
class CrostiniPortForwarderTest : public testing::Test {
public:
CrostiniPortForwarderTest() {}
~CrostiniPortForwarderTest() override {}
void SetUp() override {
chromeos::PermissionBrokerClient::InitializeFake();
profile_ = std::make_unique<TestingProfile>();
test_helper_ = std::make_unique<CrostiniTestHelper>(profile_.get());
crostini_port_forwarder_ =
std::make_unique<CrostiniPortForwarder>(profile());
}
void TearDown() override {
chromeos::PermissionBrokerClient::Shutdown();
crostini_port_forwarder_.reset();
test_helper_.reset();
profile_.reset();
}
protected:
Profile* profile() { return profile_.get(); }
std::unique_ptr<CrostiniTestHelper> test_helper_;
std::unique_ptr<TestingProfile> profile_;
std::unique_ptr<CrostiniPortForwarder> crostini_port_forwarder_;
content::BrowserTaskEnvironment task_environment_;
private:
DISALLOW_COPY_AND_ASSIGN(CrostiniPortForwarderTest);
};
TEST_F(CrostiniPortForwarderTest, AddPortTcpSuccess) {
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
bool success = false;
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::TCP,
"label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_TRUE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
}
TEST_F(CrostiniPortForwarderTest, AddPortUdpSuccess) {
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
bool success = false;
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::UDP,
"label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_TRUE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
}
TEST_F(CrostiniPortForwarderTest, AddPortDuplicateFail) {
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
bool success = false;
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::UDP,
"label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_TRUE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 1U);
// Leave success as == true.
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::UDP,
"label1",
base::BindOnce(&TestingCallback, &success));
EXPECT_FALSE(success);
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_TRUE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 1U);
}
TEST_F(CrostiniPortForwarderTest, AddPortUdpAndTcpSuccess) {
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
bool success = false;
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::UDP,
"label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
success = false;
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::TCP,
"label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 2U);
}
TEST_F(CrostiniPortForwarderTest, AddPortMultipleSuccess) {
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 0U);
crostini_port_forwarder_->AddPort(5000, CrostiniPortForwarder::Protocol::UDP,
"label0", base::DoNothing());
crostini_port_forwarder_->AddPort(5001, CrostiniPortForwarder::Protocol::TCP,
"label1", base::DoNothing());
crostini_port_forwarder_->AddPort(5002, CrostiniPortForwarder::Protocol::UDP,
"label2", base::DoNothing());
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 3U);
}
TEST_F(CrostiniPortForwarderTest, ActivatePortTcpSuccess) {
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
bool success = false;
crostini_port_forwarder_->ActivatePort(
5000, base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_TRUE(chromeos::FakePermissionBrokerClient::Get()->HasTcpPortForward(
5000, "wlan0"));
EXPECT_FALSE(chromeos::FakePermissionBrokerClient::Get()->HasUdpPortForward(
5000, "wlan0"));
}
TEST_F(CrostiniPortForwarderTest, TryActivatePortPermissionBrokerClientFail) {
bool success = false;
crostini_port_forwarder_->TryActivatePort(
5000, CrostiniPortForwarder::Protocol::TCP, "label0",
base::BindOnce(&TestingCallback, &success));
EXPECT_TRUE(success);
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 1U);
chromeos::PermissionBrokerClient::Shutdown();
// Leave success as == true.
crostini_port_forwarder_->TryActivatePort(
5001, CrostiniPortForwarder::Protocol::TCP, "label1",
base::BindOnce(&TestingCallback, &success));
EXPECT_FALSE(success);
EXPECT_EQ(crostini_port_forwarder_->forwarded_ports_.size(), 1U);
// Re-initialize otherwise Shutdown in TearDown phase will break.
chromeos::PermissionBrokerClient::InitializeFake();
}
} // namespace crostini
......@@ -135,6 +135,20 @@ bool FakePermissionBrokerClient::HasUdpHole(uint16_t port,
return udp_hole_set_.find(rule) != udp_hole_set_.end();
}
bool FakePermissionBrokerClient::HasTcpPortForward(
uint16_t port,
const std::string& interface) {
auto rule = std::make_pair(port, interface);
return tcp_forwarding_set_.find(rule) != tcp_forwarding_set_.end();
}
bool FakePermissionBrokerClient::HasUdpPortForward(
uint16_t port,
const std::string& interface) {
auto rule = std::make_pair(port, interface);
return udp_forwarding_set_.find(rule) != udp_forwarding_set_.end();
}
void FakePermissionBrokerClient::RequestTcpPortForward(
uint16_t in_port,
const std::string& in_interface,
......@@ -142,7 +156,10 @@ void FakePermissionBrokerClient::RequestTcpPortForward(
uint16_t dst_port,
int lifeline_fd,
ResultCallback callback) {
std::move(callback).Run(false);
// TODO(matterchen): Increase logic for adding duplicate ports.
auto rule = std::make_pair(in_port, in_interface);
tcp_forwarding_set_.insert(rule);
std::move(callback).Run(true);
}
void FakePermissionBrokerClient::RequestUdpPortForward(
......@@ -152,7 +169,9 @@ void FakePermissionBrokerClient::RequestUdpPortForward(
uint16_t dst_port,
int lifeline_fd,
ResultCallback callback) {
std::move(callback).Run(false);
auto rule = std::make_pair(in_port, in_interface);
udp_forwarding_set_.insert(rule);
std::move(callback).Run(true);
}
void FakePermissionBrokerClient::ReleaseTcpPortForward(
......
......@@ -76,6 +76,12 @@ class COMPONENT_EXPORT(PERMISSION_BROKER) FakePermissionBrokerClient
// Returns true if UDP port has a hole.
bool HasUdpHole(uint16_t port, const std::string& interface);
// Returns true if TCP port is being forwarded.
bool HasTcpPortForward(uint16_t port, const std::string& interface);
// Returns true if UDP port is being forwarded.
bool HasUdpPortForward(uint16_t port, const std::string& interface);
private:
using RuleSet =
std::set<std::pair<uint16_t /* port */, std::string /* interface */>>;
......@@ -88,6 +94,9 @@ class COMPONENT_EXPORT(PERMISSION_BROKER) FakePermissionBrokerClient
RuleSet tcp_hole_set_;
RuleSet udp_hole_set_;
RuleSet tcp_forwarding_set_;
RuleSet udp_forwarding_set_;
RuleSet tcp_deny_rule_set_;
RuleSet udp_deny_rule_set_;
......
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