system-proxy: Clear user credentials
System-proxy will request and reuse the credentials which a client has
entered in the browser. Users can opt to clear the authentication cache
in the browser.
This CL implements a D-Bus call to clear the proxy credentials from
System-proxy.
BUG=chromium:1098216
TEST=unit tests, manually on DUT
Change-Id: I9a58dd7a4e3e3d7a75d395ce645103d8f798b0a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2308531
Reviewed-by: Hugo Benichi <hugobenichi@google.com>
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 0e9e9c0..3155d04 100644
--- a/system-proxy/dbus/org.chromium.SystemProxy.conf
+++ b/system-proxy/dbus/org.chromium.SystemProxy.conf
@@ -22,6 +22,9 @@
<allow receive_interface="org.chromium.SystemProxy"
receive_member="WorkerActive"
receive_type="signal" />
+ <allow send_destination="org.chromium.SystemProxy"
+ send_interface="org.chromium.SystemProxy"
+ send_member="ClearUserCredentials"/>
</policy>
<!-- For testing. -->
diff --git a/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml b/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
index 55dea65..9964298 100644
--- a/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
+++ b/system-proxy/dbus_bindings/org.chromium.SystemProxy.xml
@@ -1,10 +1,12 @@
<?xml version="1.0" encoding="UTF-8" ?>
-<node name="/org/chromium/SystemProxy" xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">
+<node name="/org/chromium/SystemProxy"
+ xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">
<interface name="org.chromium.SystemProxy">
<method name="SetAuthenticationDetails">
<tp:docstring>
- Sets the credentails for authenticating system services and ARC++ apps to the remote web proxy and the Kerberos availability flag.
+ Sets the credentails for authenticating system services and ARC++ apps
+ to the remote web proxy and the Kerberos availability flag.
</tp:docstring>
<arg name="request" type="ay" direction="in">
<tp:docstring>
@@ -20,7 +22,8 @@
</method>
<method name="ShutDown">
<tp:docstring>
- Shuts down the daemon. The daemon will exit with code 0 which prevents an Upstart respawn.
+ Shuts down the daemon. The daemon will exit with code 0 which prevents
+ an Upstart respawn.
</tp:docstring>
<arg name="response" type="ay" direction="out">
<tp:docstring>
@@ -31,7 +34,8 @@
</method>
<signal name="WorkerActive">
<tp:docstring>
- Signal emitted when a system proxy worker is active and accepting traffic.
+ Signal emitted when a system proxy worker is active and accepting
+ traffic.
</tp:docstring>
<arg name="details" type="ay">
<tp:docstring>
@@ -42,7 +46,8 @@
</signal>
<signal name="AuthenticationRequired">
<tp:docstring>
- Signal emitted when a system proxy worker requires credentials for proxy authentication.
+ Signal emitted when a system proxy worker requires credentials for
+ proxy authentication.
</tp:docstring>
<arg name="details" type="ay">
<tp:docstring>
@@ -51,5 +56,23 @@
</arg>
<annotation name="org.chromium.DBus.Method.Kind" value="simple"/>
</signal>
+ <method name="ClearUserCredentials">
+ <tp:docstring>
+ Removes the user credentials from System-proxy. Credentials set for
+ system services via policy will still be available for proxy
+ authentication.
+ </tp:docstring>
+ <arg name="request" type="ay" direction="in">
+ <tp:docstring>
+ Serialized ClearUserCredentialsRequest message.
+ </tp:docstring>
+ </arg>
+ <arg name="response" type="ay" direction="out">
+ <tp:docstring>
+ Serialized ClearUserCredentialsResponse message.
+ </tp:docstring>
+ </arg>
+ <annotation name="org.chromium.DBus.Method.Kind" value="simple" />
+ </method>
</interface>
</node>
diff --git a/system-proxy/proto/worker_common.proto b/system-proxy/proto/worker_common.proto
index 8f8b097..f733986 100644
--- a/system-proxy/proto/worker_common.proto
+++ b/system-proxy/proto/worker_common.proto
@@ -67,6 +67,8 @@
optional bytes krb5conf_path = 3;
}
+message ClearUserCredentials {}
+
message WorkerConfigs {
oneof params {
Credentials credentials = 1;
@@ -74,5 +76,6 @@
SocketAddress listening_address = 2;
ProxyResolutionReply proxy_resolution_reply = 3;
KerberosConfig kerberos_config = 4;
+ ClearUserCredentials clear_user_credentials = 5;
}
}
diff --git a/system-proxy/sandboxed_worker.cc b/system-proxy/sandboxed_worker.cc
index 8fd772e..d03dc5c 100644
--- a/system-proxy/sandboxed_worker.cc
+++ b/system-proxy/sandboxed_worker.cc
@@ -142,6 +142,19 @@
return true;
}
+bool SandboxedWorker::ClearUserCredentials() {
+ worker::ClearUserCredentials clear_user_credentials;
+ worker::WorkerConfigs configs;
+ *configs.mutable_clear_user_credentials() = clear_user_credentials;
+
+ if (!WriteProtobuf(stdin_pipe_.get(), configs)) {
+ LOG(ERROR) << "Failed to send request to clear user credentials for worker "
+ << pid_;
+ return false;
+ }
+ return true;
+}
+
bool SandboxedWorker::Stop() {
if (is_being_terminated_)
return true;
diff --git a/system-proxy/sandboxed_worker.h b/system-proxy/sandboxed_worker.h
index e631c9d..e4b9e7b 100644
--- a/system-proxy/sandboxed_worker.h
+++ b/system-proxy/sandboxed_worker.h
@@ -45,6 +45,10 @@
// pipes and sets |local_proxy_host_and_port_|.
bool SetListeningAddress(uint32_t addr, int port);
+ // Sends a request to clear the user credentials to the worker via
+ // communication pipes.
+ bool ClearUserCredentials();
+
// Terminates the child process by sending a SIGTERM signal.
virtual bool Stop();
@@ -66,6 +70,7 @@
FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceAuthenticationRequired);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceNoCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ClearUserCredentials);
void OnMessageReceived();
void OnErrorReceived();
diff --git a/system-proxy/server_proxy.cc b/system-proxy/server_proxy.cc
index 8c510ac..1db2772 100644
--- a/system-proxy/server_proxy.cc
+++ b/system-proxy/server_proxy.cc
@@ -198,6 +198,10 @@
unsetenv(kKrb5CCEnvKey);
}
}
+
+ if (config.has_clear_user_credentials()) {
+ auth_cache_.clear();
+ }
}
bool ServerProxy::HandleSignal(const struct signalfd_siginfo& siginfo) {
diff --git a/system-proxy/server_proxy.h b/system-proxy/server_proxy.h
index 4b9a96d..c3ca270 100644
--- a/system-proxy/server_proxy.h
+++ b/system-proxy/server_proxy.h
@@ -77,6 +77,7 @@
FRIEND_TEST(ServerProxyTest, HandlePendingAuthRequests);
FRIEND_TEST(ServerProxyTest, HandlePendingAuthRequestsCachedCredentials);
FRIEND_TEST(ServerProxyTest, HandlePendingAuthRequestsNoCredentials);
+ FRIEND_TEST(ServerProxyTest, ClearUserCredentials);
bool HandleSignal(const struct signalfd_siginfo& siginfo);
diff --git a/system-proxy/server_proxy_test.cc b/system-proxy/server_proxy_test.cc
index 4e01395..c358f1f 100644
--- a/system-proxy/server_proxy_test.cc
+++ b/system-proxy/server_proxy_test.cc
@@ -436,4 +436,27 @@
EXPECT_EQ("test_user:test_pwd", actual_credentials);
}
+// This test verifies that the stored credentials are removed when receiving a
+// |ClearUserCredentials| request.
+TEST_F(ServerProxyTest, ClearUserCredentials) {
+ worker::ProtectionSpace protection_space;
+ protection_space.set_origin(kFakeProxyAddress);
+ protection_space.set_scheme("Basic");
+ protection_space.set_realm("Proxy test realm");
+ // Add an entry in the cache.
+ server_proxy_->auth_cache_[protection_space.SerializeAsString()] =
+ "test_user:test_pwd";
+
+ worker::ClearUserCredentials clear_user_credentials;
+ worker::WorkerConfigs configs;
+ *configs.mutable_clear_user_credentials() = clear_user_credentials;
+ // Redirect the worker stdin and stdout pipes.
+ RedirectStdPipes();
+ // Send the config to the worker's stdin input.
+ EXPECT_TRUE(WriteProtobuf(stdin_write_fd_.get(), configs));
+ brillo_loop_.RunOnce(false);
+ // Expect that the credentials were cleared.
+ EXPECT_EQ(0, server_proxy_->auth_cache_.size());
+}
+
} // namespace system_proxy
diff --git a/system-proxy/system_proxy_adaptor.cc b/system-proxy/system_proxy_adaptor.cc
index 4105130..2e0323f 100644
--- a/system-proxy/system_proxy_adaptor.cc
+++ b/system-proxy/system_proxy_adaptor.cc
@@ -8,6 +8,7 @@
#include <vector>
#include <base/location.h>
+#include <base/strings/stringprintf.h>
#include <base/time/time.h>
#include <brillo/dbus/dbus_object.h>
#include <brillo/message_loops/message_loop.h>
@@ -145,15 +146,13 @@
LOG(INFO) << "Received shutdown request.";
std::string error_message;
- if (system_services_worker_ && system_services_worker_->IsRunning()) {
- if (!system_services_worker_->Stop())
- error_message =
- "Failure to terminate worker process for system services traffic.";
+ if (!ResetWorker(/* user_traffic=*/false)) {
+ error_message =
+ "Failure to terminate worker process for system services traffic.";
}
- if (arc_worker_ && arc_worker_->IsRunning()) {
- if (!arc_worker_->Stop())
- error_message += "Failure to terminate worker process for arc traffic.";
+ if (!ResetWorker(/* user_traffic=*/true)) {
+ error_message += "Failure to terminate worker process for arc traffic.";
}
ShutDownResponse response;
@@ -167,6 +166,35 @@
return SerializeProto(response);
}
+std::vector<uint8_t> SystemProxyAdaptor::ClearUserCredentials(
+ const std::vector<uint8_t>& request_blob) {
+ LOG(INFO) << "Received request to clear user credentials.";
+ std::string error_message;
+ ClearUserCredentials(/*user_traffic=*/false, &error_message);
+ ClearUserCredentials(/*user_traffic=*/true, &error_message);
+
+ ClearUserCredentialsResponse response;
+ if (!error_message.empty())
+ response.set_error_message(error_message);
+ return SerializeProto(response);
+}
+
+void SystemProxyAdaptor::ClearUserCredentials(bool user_traffic,
+ std::string* error_message) {
+ SandboxedWorker* worker = GetWorker(user_traffic);
+ if (!worker) {
+ return;
+ }
+ if (!worker->ClearUserCredentials()) {
+ error_message->append(
+ base::StringPrintf("Failure to clear user credentials for worker with "
+ "pid %s. Restarting worker.",
+ std::to_string(worker->pid()).c_str()));
+ ResetWorker(user_traffic);
+ CreateWorkerIfNeeded(user_traffic);
+ }
+}
+
void SystemProxyAdaptor::GetChromeProxyServersAsync(
const std::string& target_url,
const brillo::http::GetChromeProxyServersCallback& callback) {
@@ -234,6 +262,27 @@
return worker->Start();
}
+bool SystemProxyAdaptor::ResetWorker(bool user_traffic) {
+ SandboxedWorker* worker =
+ user_traffic ? arc_worker_.get() : system_services_worker_.get();
+ if (!worker) {
+ return true;
+ }
+ if (!worker->Stop()) {
+ return false;
+ }
+ if (user_traffic) {
+ arc_worker_.reset();
+ } else {
+ system_services_worker_.reset();
+ }
+ return true;
+}
+
+SandboxedWorker* SystemProxyAdaptor::GetWorker(bool user_traffic) {
+ return user_traffic ? arc_worker_.get() : system_services_worker_.get();
+}
+
// Called when the patchpanel D-Bus service becomes available.
void SystemProxyAdaptor::OnPatchpanelServiceAvailable(bool is_available) {
if (!is_available) {
@@ -247,7 +296,7 @@
void SystemProxyAdaptor::ConnectNamespace(SandboxedWorker* worker,
bool user_traffic) {
- DCHECK(system_services_worker_->IsRunning());
+ DCHECK(worker->IsRunning());
DCHECK_GT(netns_reconnect_attempts_available_, 0);
--netns_reconnect_attempts_available_;
// TODO(b/160736881, acostinas): Remove the delay after patchpanel
@@ -255,8 +304,7 @@
brillo::MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&SystemProxyAdaptor::ConnectNamespaceTask,
- weak_ptr_factory_.GetWeakPtr(), worker,
- /* user_traffic= */ false),
+ weak_ptr_factory_.GetWeakPtr(), worker, user_traffic),
kConnectNamespaceDelay);
}
diff --git a/system-proxy/system_proxy_adaptor.h b/system-proxy/system_proxy_adaptor.h
index 8772112..942fa26 100644
--- a/system-proxy/system_proxy_adaptor.h
+++ b/system-proxy/system_proxy_adaptor.h
@@ -48,6 +48,8 @@
std::vector<uint8_t> SetAuthenticationDetails(
const std::vector<uint8_t>& request_blob) override;
std::vector<uint8_t> ShutDown() override;
+ std::vector<uint8_t> ClearUserCredentials(
+ const std::vector<uint8_t>& request_blob) override;
void GetChromeProxyServersAsync(
const std::string& target_url,
@@ -71,6 +73,8 @@
FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceAuthenticationRequired);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceNoCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ClearUserCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ClearUserCredentialsRestartService);
void SetCredentialsTask(SandboxedWorker* worker,
const worker::Credentials& credentials);
@@ -85,11 +89,24 @@
bool StartWorker(SandboxedWorker* worker, bool user_traffic);
+ // Terminates the worker process for traffic indicated by |user_traffic| and
+ // frees the SandboxedWorker associated with it.
+ bool ResetWorker(bool user_traffic);
+
+ // Returns a pointer to the worker process associated with |user_traffic|. Can
+ // return nullptr.
+ SandboxedWorker* GetWorker(bool user_traffic);
+
// Checks if a worker process exists and if not creates one and sends a
// request to patchpanel to setup the network namespace for it. Returns true
// if the worker exists or was created successfully, false otherwise.
bool CreateWorkerIfNeeded(bool user_traffic);
+ // Sends a request to the worker process associated with |user_traffic| to
+ // clear the cached user credentials. If sending the request fails, the worker
+ // will be restarted.
+ void ClearUserCredentials(bool user_traffic, std::string* error_message);
+
// Called when the patchpanel D-Bus service becomes available.
void OnPatchpanelServiceAvailable(bool is_available);
diff --git a/system-proxy/system_proxy_adaptor_test.cc b/system-proxy/system_proxy_adaptor_test.cc
index e5a4a69..e5fa515 100644
--- a/system-proxy/system_proxy_adaptor_test.cc
+++ b/system-proxy/system_proxy_adaptor_test.cc
@@ -61,7 +61,10 @@
~FakeSandboxedWorker() override = default;
bool Start() override { return is_running_ = true; }
- bool Stop() override { return is_running_ = false; }
+ bool Stop() override {
+ is_running_ = false;
+ return true;
+ }
bool IsRunning() override { return is_running_; }
std::string local_proxy_host_and_port() override {
@@ -83,6 +86,7 @@
protected:
std::unique_ptr<SandboxedWorker> CreateWorker() override {
+ ++create_worker_count_;
return std::make_unique<FakeSandboxedWorker>(
weak_ptr_factory_.GetWeakPtr());
}
@@ -96,7 +100,10 @@
FRIEND_TEST(SystemProxyAdaptorTest, ProxyResolutionFilter);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceAuthenticationRequired);
FRIEND_TEST(SystemProxyAdaptorTest, ProtectionSpaceNoCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ClearUserCredentials);
+ FRIEND_TEST(SystemProxyAdaptorTest, ClearUserCredentialsRestartService);
+ int create_worker_count_ = 0;
base::WeakPtrFactory<FakeSystemProxyAdaptor> weak_ptr_factory_;
};
@@ -297,7 +304,7 @@
EXPECT_TRUE(adaptor_->system_services_worker_->IsRunning());
adaptor_->ShutDown();
- EXPECT_FALSE(adaptor_->system_services_worker_->IsRunning());
+ EXPECT_FALSE(adaptor_->system_services_worker_);
}
TEST_F(SystemProxyAdaptorTest, ConnectNamespace) {
@@ -414,4 +421,48 @@
protection_space.SerializeAsString());
}
+TEST_F(SystemProxyAdaptorTest, ClearUserCredentials) {
+ adaptor_->system_services_worker_ = adaptor_->CreateWorker();
+ ASSERT_TRUE(adaptor_->system_services_worker_.get());
+
+ 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]);
+
+ ClearUserCredentialsRequest request;
+ std::vector<uint8_t> proto_blob(request.ByteSizeLong());
+ request.SerializeToArray(proto_blob.data(), proto_blob.size());
+ adaptor_->ClearUserCredentials(proto_blob);
+ brillo_loop_.RunOnce(false);
+
+ // Expect that a request to clear user credentials has been sent to the
+ // worker.
+ worker::WorkerConfigs config;
+ ASSERT_TRUE(ReadProtobuf(read_scoped_fd.get(), &config));
+ EXPECT_TRUE(config.has_clear_user_credentials());
+}
+
+// Tests that the sandboxed worker is restarted if the request to clear the
+// credentials fails.
+TEST_F(SystemProxyAdaptorTest, ClearUserCredentialsRestartService) {
+ EXPECT_CALL(*bus_, GetObjectProxy(patchpanel::kPatchPanelServiceName, _))
+ .WillOnce(Return(mock_patchpanel_proxy_.get()));
+
+ adaptor_->system_services_worker_ = adaptor_->CreateWorker();
+ ASSERT_TRUE(adaptor_->system_services_worker_.get());
+ EXPECT_EQ(1, adaptor_->create_worker_count_);
+
+ ClearUserCredentialsRequest request;
+ std::vector<uint8_t> proto_blob(request.ByteSizeLong());
+ request.SerializeToArray(proto_blob.data(), proto_blob.size());
+ // This request will fail because we didn't set up a communication pipe.
+ adaptor_->ClearUserCredentials(proto_blob);
+ brillo_loop_.RunOnce(false);
+
+ ASSERT_TRUE(adaptor_->system_services_worker_.get());
+ EXPECT_EQ(2, adaptor_->create_worker_count_);
+}
+
} // namespace system_proxy
