Skip to content

GitLab

T
tangled
Commit 0477cee9 authored by Olga Sharonova's avatar Olga Sharonova Committed by Commit Bot
Browse files
Options

Revert "Crostini port forwarding activate and add port" 

This reverts commit c9552c88.

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



Original change's description:
> 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.
> 
> Bug: chromium:848127
> Test: crostini_port_forwarder_unittest.cc and everything builds
> Change-Id: I9a0478f1486252fb09fab18ab3261e03d00eb480
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999972
> Reviewed-by: David Munro <davidmunro@google.com>
> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
> Reviewed-by: Nic Hollingum <hollingum@google.com>
> Commit-Queue: Matthew Chen <matterchen@google.com>
> Cr-Commit-Position: refs/heads/master@{#734303}

TBR=hashimoto@chromium.org,hollingum@google.com,davidmunro@google.com,matterchen@google.com

Change-Id: Iabb6d856090b1f34776b547cd1b6ba652ebfb055
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:848127
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2015085Reviewed-by: default avatarOlga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734416}
parent b32520d2
Show whitespace changes
Inline Side-by-side
Showing with 2 additions and 490 deletions
chrome/browser/chromeos/BUILD.gn
View file @ 0477cee9
......@@ -873,8 +873,6 @@ 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",
......@@ -2715,7 +2713,6 @@ 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",
......
chrome/browser/chromeos/crostini/crostini_port_forwarder.cc deleted 100644 → 0
View file @ b32520d2
// 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,
const 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,
const std::string& label,
const 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
chrome/browser/chromeos/crostini/crostini_port_forwarder.h deleted 100644 → 0
View file @ b32520d2
// 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;
const 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,
const PortRuleKey& key,
bool success);
void OnAddPortCompleted(ResultCallback result_callback,
const std::string& label,
const 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_
chrome/browser/chromeos/crostini/crostini_port_forwarder_unittest.cc deleted 100644 → 0
View file @ b32520d2
// 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
chromeos/dbus/permission_broker/fake_permission_broker_client.cc
View file @ 0477cee9
......@@ -135,20 +135,6 @@ 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,
......@@ -156,10 +142,7 @@ void FakePermissionBrokerClient::RequestTcpPortForward(
uint16_t dst_port,
int lifeline_fd,
ResultCallback callback) {
// 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);
std::move(callback).Run(false);
}
void FakePermissionBrokerClient::RequestUdpPortForward(
......@@ -169,9 +152,7 @@ void FakePermissionBrokerClient::RequestUdpPortForward(
uint16_t dst_port,
int lifeline_fd,
ResultCallback callback) {
auto rule = std::make_pair(in_port, in_interface);
udp_forwarding_set_.insert(rule);
std::move(callback).Run(true);
std::move(callback).Run(false);
}
void FakePermissionBrokerClient::ReleaseTcpPortForward(
......
chromeos/dbus/permission_broker/fake_permission_broker_client.h
View file @ 0477cee9
......@@ -76,12 +76,6 @@ 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 */>>;
......@@ -94,9 +88,6 @@ 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