system-proxy: Notify proxy worker active
This CL implements the |WorkerActive| dbus signal. Clients subscribed
to this event can get the address of the local proxy from the signal
parameters.
It also fixes a couple of other issues:
- naming the ShutDown method in the dbus conf file
- formatting the target URL sent to Chrome for proxy resolution
BUG=chromium:1042642
TEST=unittests, manual test on DUT
Change-Id: I8dd33e9c3be7ddb2669c155850dcb02ba37aae4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2190633
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Tested-by: Andreea-Elena Costinas <acostinas@google.com>
Commit-Queue: Andreea-Elena Costinas <acostinas@google.com>
diff --git a/system-proxy/dbus/org.chromium.SystemProxy.conf b/system-proxy/dbus/org.chromium.SystemProxy.conf
index 5b10d64..8a45080 100644
--- a/system-proxy/dbus/org.chromium.SystemProxy.conf
+++ b/system-proxy/dbus/org.chromium.SystemProxy.conf
@@ -12,13 +12,16 @@
</policy>
<policy user="chronos">
- <!-- Kerberos -->
+ <!-- System-proxy -->
<allow send_destination="org.chromium.SystemProxy"
send_interface="org.chromium.SystemProxy"
send_member="SetSystemTrafficCredentials"/>
<allow send_destination="org.chromium.SystemProxy"
send_interface="org.chromium.SystemProxy"
- send_member="ShutDownRequest"/>
+ send_member="ShutDown"/>
+ <allow receive_interface="org.chromium.SystemProxy"
+ receive_member="WorkerActive"
+ receive_type="signal" />
</policy>
<!-- For testing. -->
diff --git a/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml b/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
index e748533..4d287da 100644
--- a/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
+++ b/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
@@ -30,5 +30,16 @@
</arg>
<annotation name="org.chromium.DBus.Method.Kind" value="simple"/>
</method>
+ <signal name="WorkerActive">
+ <tp:docstring>
+ Signal emitted when a system proxy worker is active and accepting traffic.
+ </tp:docstring>
+ <arg name="details" type="ay">
+ <tp:docstring>
+ Serialized WorkerActiveSignalDetails message.
+ </tp:docstring>
+ </arg>
+ <annotation name="org.chromium.DBus.Method.Kind" value="simple"/>
+ </signal>
</interface>
</node>
diff --git a/system-proxy/proxy_connect_job.cc b/system-proxy/proxy_connect_job.cc
index 96da8c1..4c9e342 100644
--- a/system-proxy/proxy_connect_job.cc
+++ b/system-proxy/proxy_connect_job.cc
@@ -208,9 +208,13 @@
return;
}
+ // The proxy resolution service in Chrome expects a proper URL, formatted as
+ // scheme://host:port. It's safe to assume only https will be used for the
+ // target url.
std::move(resolve_proxy_callback_)
- .Run(target_url_, base::Bind(&ProxyConnectJob::OnProxyResolution,
- base::Unretained(this)));
+ .Run(base::StringPrintf("https://%s", target_url_.c_str()),
+ base::Bind(&ProxyConnectJob::OnProxyResolution,
+ base::Unretained(this)));
}
bool ProxyConnectJob::TryReadHttpHeader(std::vector<char>* raw_request) {
diff --git a/system-proxy/sandboxed_worker.cc b/system-proxy/sandboxed_worker.cc
index a86f11f..6ed24ea 100644
--- a/system-proxy/sandboxed_worker.cc
+++ b/system-proxy/sandboxed_worker.cc
@@ -16,7 +16,9 @@
#include <base/callback_helpers.h>
#include <base/files/file_util.h>
#include <base/strings/string_util.h>
+#include <base/strings/stringprintf.h>
#include <brillo/http/http_transport.h>
+#include <chromeos/patchpanel/net_util.h>
#include <google/protobuf/repeated_field.h>
#include "bindings/worker_common.pb.h"
@@ -114,6 +116,10 @@
LOG(ERROR) << "Failed to set local proxy address for worker " << pid_;
return false;
}
+ local_proxy_host_and_port_ = base::StringPrintf(
+ "%s:%d", patchpanel::IPv4AddressToString(addr).c_str(), port);
+ LOG(INFO) << "Set proxy address " << local_proxy_host_and_port_
+ << " for worker " << pid_;
return true;
}
@@ -227,7 +233,10 @@
base::CompareCase::INSENSITIVE_ASCII) ||
base::StartsWith(proxy, kPrefixDirect,
base::CompareCase::INSENSITIVE_ASCII)) {
- reply.add_proxy_servers(proxy);
+ // Make sure the local proxy doesn't try to connect to itself.
+ if (proxy.find(local_proxy_host_and_port()) == std::string::npos) {
+ reply.add_proxy_servers(proxy);
+ }
}
}
diff --git a/system-proxy/sandboxed_worker.h b/system-proxy/sandboxed_worker.h
index acfee2a..a80682c 100644
--- a/system-proxy/sandboxed_worker.h
+++ b/system-proxy/sandboxed_worker.h
@@ -34,7 +34,7 @@
void SetUsernameAndPassword(const std::string& username,
const std::string& password);
// Sends the listening address and port to the worker via communication
- // pipes.
+ // pipes and sets |local_proxy_host_and_port_|.
bool SetListeningAddress(uint32_t addr, int port);
// Terminates the child process by sending a SIGTERM signal.
@@ -46,9 +46,15 @@
pid_t pid() { return pid_; }
+ // Returns the address of the local proxy as host:port.
+ virtual std::string local_proxy_host_and_port() {
+ return local_proxy_host_and_port_;
+ }
+
private:
friend class SystemProxyAdaptorTest;
FRIEND_TEST(SystemProxyAdaptorTest, SetSystemTrafficCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
void OnMessageReceived();
void OnErrorReceived();
@@ -59,6 +65,7 @@
bool success,
const std::vector<std::string>& proxy_servers);
+ std::string local_proxy_host_and_port_;
bool is_being_terminated_ = false;
ScopedMinijail jail_;
base::ScopedFD stdin_pipe_;
diff --git a/system-proxy/server_proxy_test.cc b/system-proxy/server_proxy_test.cc
index c69afcb..34824e7 100644
--- a/system-proxy/server_proxy_test.cc
+++ b/system-proxy/server_proxy_test.cc
@@ -184,7 +184,7 @@
ASSERT_TRUE(ReadProtobuf(stdout_read_fd_.get(), &request));
ASSERT_TRUE(request.has_proxy_resolution_request());
- EXPECT_EQ("www.example.server.com:443",
+ EXPECT_EQ("https://www.example.server.com:443",
request.proxy_resolution_request().target_url());
EXPECT_EQ(1, server_proxy_->pending_proxy_resolution_requests_.size());
diff --git a/system-proxy/system_proxy_adaptor.cc b/system-proxy/system_proxy_adaptor.cc
index 1427ea6..eb7d01d 100644
--- a/system-proxy/system_proxy_adaptor.cc
+++ b/system-proxy/system_proxy_adaptor.cc
@@ -193,8 +193,21 @@
}
worker->SetNetNamespaceLifelineFd(std::move(result.first));
- return worker->SetListeningAddress(result.second.peer_ipv4_address(),
- kProxyPort);
+ if (!worker->SetListeningAddress(result.second.host_ipv4_address(),
+ kProxyPort)) {
+ return false;
+ }
+ OnNamespaceConnected(worker, user_traffic);
+ return true;
+}
+
+void SystemProxyAdaptor::OnNamespaceConnected(SandboxedWorker* worker,
+ bool user_traffic) {
+ WorkerActiveSignalDetails details;
+ details.set_traffic_origin(user_traffic ? TrafficOrigin::USER
+ : TrafficOrigin::SYSTEM);
+ details.set_local_proxy_url(worker->local_proxy_host_and_port());
+ SendWorkerActiveSignal(SerializeProto(details));
}
} // namespace system_proxy
diff --git a/system-proxy/system_proxy_adaptor.h b/system-proxy/system_proxy_adaptor.h
index d1ca0c8..18db8e2 100644
--- a/system-proxy/system_proxy_adaptor.h
+++ b/system-proxy/system_proxy_adaptor.h
@@ -54,11 +54,15 @@
protected:
virtual std::unique_ptr<SandboxedWorker> CreateWorker();
virtual bool ConnectNamespace(SandboxedWorker* worker, bool user_traffic);
+ // Triggers the |WorkerActive| signal.
+ void OnNamespaceConnected(SandboxedWorker* worker, bool user_traffic);
private:
friend class SystemProxyAdaptorTest;
FRIEND_TEST(SystemProxyAdaptorTest, SetSystemTrafficCredentials);
FRIEND_TEST(SystemProxyAdaptorTest, ShutDown);
+ FRIEND_TEST(SystemProxyAdaptorTest, ConnectNamespace);
+ FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
void SetCredentialsTask(SandboxedWorker* worker,
const std::string& username,
@@ -68,7 +72,7 @@
bool StartWorker(SandboxedWorker* worker, bool user_traffic);
-// Called when the patchpanel D-Bus service becomes available.
+ // Called when the patchpanel D-Bus service becomes available.
void OnPatchpanelServiceAvailable(bool is_available);
// The callback of |GetChromeProxyServersAsync|.
diff --git a/system-proxy/system_proxy_adaptor_test.cc b/system-proxy/system_proxy_adaptor_test.cc
index b4d16a2..5aa8369 100644
--- a/system-proxy/system_proxy_adaptor_test.cc
+++ b/system-proxy/system_proxy_adaptor_test.cc
@@ -6,6 +6,7 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <list>
#include <utility>
#include <base/bind.h>
@@ -15,12 +16,14 @@
#include <base/files/scoped_file.h>
#include <base/memory/weak_ptr.h>
#include <base/message_loop/message_loop.h>
+#include <base/strings/stringprintf.h>
#include <brillo/dbus/async_event_sequencer.h>
#include <brillo/dbus/dbus_object.h>
#include <brillo/message_loops/base_message_loop.h>
#include <dbus/object_path.h>
#include <dbus/message.h>
#include <dbus/mock_bus.h>
+#include <dbus/mock_exported_object.h>
#include <dbus/mock_object_proxy.h>
#include <chromeos/dbus/service_constants.h>
@@ -36,6 +39,12 @@
namespace {
const char kUser[] = "proxy_user";
const char kPassword[] = "proxy_password";
+const char kLocalProxyHostPort[] = "local.proxy.url:3128";
+const char kObjectPath[] = "/object/path";
+
+// Stub completion callback for RegisterAsync().
+void DoNothing(bool /* unused */) {}
+
} // namespace
class FakeSandboxedWorker : public SandboxedWorker {
@@ -50,6 +59,10 @@
bool Stop() override { return is_running_ = false; }
bool IsRunning() override { return is_running_; }
+ std::string local_proxy_host_and_port() override {
+ return kLocalProxyHostPort;
+ }
+
private:
bool is_running_;
};
@@ -69,21 +82,36 @@
weak_ptr_factory_.GetWeakPtr());
}
bool ConnectNamespace(SandboxedWorker* worker, bool user_traffic) override {
+ OnNamespaceConnected(worker, user_traffic);
return true;
}
private:
+ FRIEND_TEST(SystemProxyAdaptorTest, ConnectNamespace);
+ FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
+
base::WeakPtrFactory<FakeSystemProxyAdaptor> weak_ptr_factory_;
};
class SystemProxyAdaptorTest : public ::testing::Test {
public:
SystemProxyAdaptorTest() {
- const dbus::ObjectPath object_path("/object/path");
+ const dbus::ObjectPath object_path(kObjectPath);
+
+ // Mock out D-Bus initialization.
+ mock_exported_object_ =
+ base::MakeRefCounted<dbus::MockExportedObject>(bus_.get(), object_path);
+
+ EXPECT_CALL(*bus_, GetExportedObject(_))
+ .WillRepeatedly(Return(mock_exported_object_.get()));
+
+ EXPECT_CALL(*mock_exported_object_, ExportMethod(_, _, _, _))
+ .Times(testing::AnyNumber());
adaptor_.reset(new FakeSystemProxyAdaptor(
std::make_unique<brillo::dbus_utils::DBusObject>(nullptr, bus_,
object_path)));
+ adaptor_->RegisterAsync(base::BindRepeating(&DoNothing));
mock_patchpanel_proxy_ = base::MakeRefCounted<dbus::MockObjectProxy>(
bus_.get(), patchpanel::kPatchPanelServiceName,
dbus::ObjectPath(patchpanel::kPatchPanelServicePath));
@@ -93,10 +121,24 @@
SystemProxyAdaptorTest& operator=(const SystemProxyAdaptorTest&) = delete;
~SystemProxyAdaptorTest() override = default;
+ void OnWorkerActive(dbus::Signal* signal) {
+ EXPECT_EQ(signal->GetInterface(), "org.chromium.SystemProxy");
+ EXPECT_EQ(signal->GetMember(), "WorkerActive");
+
+ dbus::MessageReader signal_reader(signal);
+ system_proxy::WorkerActiveSignalDetails details;
+ EXPECT_TRUE(signal_reader.PopArrayOfBytesAsProto(&details));
+ EXPECT_EQ(kLocalProxyHostPort, details.local_proxy_url());
+ active_worker_signal_called_ = true;
+ }
+
protected:
+ bool active_worker_signal_called_ = false;
+ scoped_refptr<dbus::MockBus> bus_ = new dbus::MockBus(dbus::Bus::Options());
+ scoped_refptr<dbus::MockExportedObject> mock_exported_object_;
// SystemProxyAdaptor instance that creates fake worker processes.
std::unique_ptr<FakeSystemProxyAdaptor> adaptor_;
- scoped_refptr<dbus::MockBus> bus_ = new dbus::MockBus(dbus::Bus::Options());
+
scoped_refptr<dbus::MockObjectProxy> mock_patchpanel_proxy_;
base::MessageLoopForIO loop_;
brillo::BaseMessageLoop brillo_loop_{&loop_};
@@ -157,4 +199,44 @@
EXPECT_FALSE(adaptor_->system_services_worker_->IsRunning());
}
+TEST_F(SystemProxyAdaptorTest, ConnectNamespace) {
+ EXPECT_FALSE(active_worker_signal_called_);
+ EXPECT_CALL(*mock_exported_object_, SendSignal(_))
+ .WillOnce(Invoke(this, &SystemProxyAdaptorTest::OnWorkerActive));
+
+ adaptor_->system_services_worker_ = adaptor_->CreateWorker();
+ adaptor_->ConnectNamespace(adaptor_->system_services_worker_.get(),
+ /* user_traffic= */ false);
+ EXPECT_TRUE(active_worker_signal_called_);
+}
+
+TEST_F(SystemProxyAdaptorTest, ProxyResolutionFilter) {
+ std::vector<std::string> proxy_list = {
+ base::StringPrintf("%s%s", "http://", kLocalProxyHostPort),
+ "http://test.proxy.com", "https://test.proxy.com", "direct://"};
+
+ adaptor_->system_services_worker_ = adaptor_->CreateWorker();
+ int fds[2];
+ ASSERT_TRUE(base::CreateLocalNonBlockingPipe(fds));
+ base::ScopedFD read_scoped_fd(fds[0]);
+ // Reset the worker stdin pipe to read the input from the other endpoint.
+ adaptor_->system_services_worker_->stdin_pipe_.reset(fds[1]);
+ adaptor_->system_services_worker_->OnProxyResolved("target_url", true,
+ proxy_list);
+
+ brillo_loop_.RunOnce(false);
+
+ worker::WorkerConfigs config;
+ ASSERT_TRUE(ReadProtobuf(read_scoped_fd.get(), &config));
+ EXPECT_TRUE(config.has_proxy_resolution_reply());
+ std::list<std::string> proxies;
+ const worker::ProxyResolutionReply& reply = config.proxy_resolution_reply();
+ for (auto const& proxy : reply.proxy_servers())
+ proxies.push_back(proxy);
+
+ EXPECT_EQ("target_url", reply.target_url());
+ EXPECT_EQ(2, proxies.size());
+ EXPECT_EQ("http://test.proxy.com", proxies.front());
+}
+
} // namespace system_proxy