Restart the host when the third party auth certificate changes

remoting/host/remoting_me2me_host.cc

 // Copyright (c) 2012 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.
 //
 // This file implements a standalone host process for Me2Me.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <string>
@@ skipping 97 matching lines @@
 #if defined(OS_MACOSX)
 #include "base/mac/scoped_cftyperef.h"
 #endif  // defined(OS_MACOSX)
 
 #if defined(OS_LINUX)
 #include <gtk/gtk.h>
 #include <X11/Xlib.h>
 #undef Status  // Xlib.h #defines this, which breaks protobuf headers.
 #include <base/linux_util.h>
 #include "remoting/host/audio_capturer_linux.h"
+#include "remoting/host/linux/certificate_watcher.h"
 #endif  // defined(OS_LINUX)
 
 #if defined(OS_WIN)
 #include <commctrl.h>
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
 #include "remoting/host/pairing_registry_delegate_win.h"
 #include "remoting/host/win/session_desktop_environment.h"
 #endif  // defined(OS_WIN)
 
@@ skipping 231 matching lines @@
   void StartHostIfReady();
   void StartHost();
 
   // Error handler for HeartbeatSender.
   void OnHeartbeatSuccessful();
   void OnUnknownHostIdError();
 
   // Error handler for SignalingConnector.
   void OnAuthFailed();
 
+  void OnHostRestartRequested();
+
   void RestartHost(const std::string& host_offline_reason);
   void ShutdownHost(HostExitCodes exit_code);
 
   // Helper methods doing the work needed by RestartHost and ShutdownHost.
   void GoOffline(const std::string& host_offline_reason);
   void OnHostOfflineReasonAck(bool success);
 
 #if defined(OS_WIN)
   // Initializes the pairing registry on Windows. This should be invoked on the
   // network thread.
   void InitializePairingRegistry(
       IPC::PlatformFileForTransit privileged_key,
       IPC::PlatformFileForTransit unprivileged_key);
 #endif  // defined(OS_WIN)
 
   // Crashes the process in response to a daemon's request. The daemon passes
   // the location of the code that detected the fatal error resulted in this
   // request.
   void OnCrash(const std::string& function_name,
                const std::string& file_name,
                const int& line_number);
 
   scoped_ptr<ChromotingHostContext> context_;
 
+#if defined(OS_LINUX)
+  // Watch for certificate changes and kill the host when changes occur
+  scoped_ptr<CertificateWatcher> cert_watcher_;
+#endif
+
   // XMPP server/remoting bot configuration (initialized from the command line).
   XmppSignalStrategy::XmppServerConfig xmpp_server_config_;
   std::string directory_bot_jid_;
 
   // Created on the UI thread but used from the network thread.
   base::FilePath host_config_path_;
   std::string host_config_;
   scoped_ptr<DesktopEnvironmentFactory> desktop_environment_factory_;
 
   // Accessed on the network thread.
@@ skipping 397 matching lines @@
         use_service_account_, host_owner_, local_certificate, key_pair_,
         client_domain_, pin_hash_, pairing_registry);
 
     host_->set_pairing_registry(pairing_registry);
   } else {
     // ThirdPartyAuthConfig::Parse() leaves the config in a valid state, so
     // these URLs are both valid.
     DCHECK(third_party_auth_config_.token_url.is_valid());
     DCHECK(third_party_auth_config_.token_validation_url.is_valid());
 
+#if defined(OS_LINUX)
+    cert_watcher_.reset(new CertificateWatcher(

[Sergey Ulanov, 2016/03/31 22:36:16]

CreateAuthenticatorFactory() is called every time the host is restarted, but we
don't want to create a new instance of CertificateWatcher every time - certs may
be updated while the host is being restarted and we would loose the state in
that case.

[Yuwei, 2016/03/31 23:08:33]

Currently the watcher get constructed just before the authorization starts and
destructed when the host dies. Not sure why we need to care about during the
interval of (old host died, new host authorized)... We can keep using the same
watcher for the whole process then it will need to reset the host reference
anyways...

[Sergey Ulanov, 2016/04/01 17:49:23]

This code also resets it every time the host is restarted (without restarting
the process), and we want to avoid that.
We need to care about it because restarting the host doesn't refresh the caches
that NSS may hold internally.
CreateAuthenticatorFactory() is called in two case:
 1. Host is being started the first time.
 2. Host is being restarted, e.g. because policies have been updated.

In the second case we don't want to create new instance of CertificateWatcher.
So I think here you want to create the object if it didn't exist before and then
then give it reference to the host, i.e.:

 if (!cert_watcher_) {
   cert_watcher_.reset(new CertificateWatcher(
       base::Bind(&HostProcess::OnHostRestartRequested, this)));
 }
 cert_watcher_->Start(host_->AsWeakPtr());

No Start() will be called every time host is restarted.

[Yuwei, 2016/04/01 18:28:04]

Okay. I guess I had mixed up the concept of HostProcess and the host...

[Yuwei, 2016/04/01 23:41:03]

Done.

+        host_->AsWeakPtr(),
+        base::Bind(&HostProcess::OnHostRestartRequested, this)));
+    context_->file_task_runner()->PostTask(

[Sergey Ulanov, 2016/03/31 22:36:16]

I don't think you need to use file_task_runner() here. CertificateWatcher should
work just fin on the network thread.

[Yuwei, 2016/03/31 23:08:33]

I tried this but it complained something like it's not on IO thread...

[Sergey Ulanov, 2016/04/01 17:49:23]

Network thread is an IO thread (see
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/chro...),
so it should work. Could it be that you tried it on main thread, instead of the
network thread?

[Yuwei, 2016/04/01 18:28:04]

Interesting... I tried

    context_->network_task_runner()->PostTask(
        FROM_HERE, base::Bind(&CertificateWatcher::Start,
                              base::Unretained(cert_watcher_.get())));

Then I got

[0401/112513:FATAL:thread_restrictions.cc(38)] Function marked as IO-only was
called from a thread that disallows IO!  If this thread really should be allowed
to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in
this thread's startup.

[Yuwei, 2016/04/01 18:44:34]

Line 114: network_task_runner->PostTask(FROM_HERE,
base::Bind(&DisallowBlockingOperations));

And:

void DisallowBlockingOperations() {
  base::ThreadRestrictions::SetIOAllowed(false);
  base::ThreadRestrictions::DisallowWaiting();
}

Looks like we have explicitly disallowed IO's on network thread...

[Sergey Ulanov, 2016/04/01 20:11:00]

I see. This error message is somewhat wrong. Network thread is an IO thread, but
we disallow _blocking_ IO. Looks like FilePathWatcher needs to use blocking IO
and that's the reason we cannot run it on network_thread. Sorry I assumed it
doesn't require blocking IO. 

So given that FilePathWatcher needs to run on the file thread then I think it's
better if CertificateWatcher was responsible of posting the task to the file
thread. Pass the file_task_runner() and store the reference there. Then
internally it should use that task_runner to call FilePathWatcher::Watch().
Whenever it receives notification from the watcher it will need to post a task
back to the network thread to handle it there.

In the current version of this CL CertificateWatcher uses inhibit flag on two
threads and this is not safe (timer works on the file thread, while the host
notifications are handled on the network thread). Keep as much logic as possible
on the network thread and use file thread only to receive notification from the
file watcher.

Let's discuss this offline if what I wrote above doesn't make sense.

[Yuwei, 2016/04/01 20:53:22]

That sounds like a problem... I think I can start the timer on the network
thread and always post task to network if we need to reset the timer, then now
the inhibit flag will only be accessed on the network thread.

+        FROM_HERE, base::Bind(&CertificateWatcher::Start,
+                              base::Unretained(cert_watcher_.get())));
+#endif
+
     scoped_refptr<protocol::TokenValidatorFactory> token_validator_factory =
         new TokenValidatorFactoryImpl(third_party_auth_config_, key_pair_,
                                       context_->url_request_context_getter());
     factory = protocol::Me2MeHostAuthenticatorFactory::CreateWithThirdPartyAuth(
         use_service_account_, host_owner_, local_certificate, key_pair_,
         client_domain_, token_validator_factory);
   }
 
 #if defined(OS_POSIX)
   // On Linux and Mac, perform a PAM authorization step after authentication.
@@ skipping 878 matching lines @@
   char message[1024];
   base::snprintf(message, sizeof(message),
                  "Requested by %s at %s, line %d.",
                  function_name.c_str(), file_name.c_str(), line_number);
   base::debug::Alias(message);
 
   // The daemon requested us to crash the process.
   CHECK(false) << message;
 }
 
+void HostProcess::OnHostRestartRequested() {
+  // restarts(shutdowns) the server when the certificate is updated
+  if (!context_->network_task_runner()->BelongsToCurrentThread()) {
+    context_->network_task_runner()->PostTask(FROM_HERE,
+        base::Bind(&HostProcess::OnHostRestartRequested, this));
+    return;
+  }
+  ShutdownHost(kSuccessExitCode);
+}
+
 int HostProcessMain() {
   HOST_LOG << "Starting host process: version " << STRINGIZE(VERSION);
 
 #if defined(OS_LINUX)
   // Required in order for us to run multiple X11 threads.
   XInitThreads();
 
   // Required for any calls into GTK functions, such as the Disconnect and
   // Continue windows, though these should not be used for the Me2Me case
   // (crbug.com/104377).
@@ skipping 28 matching lines @@
       base::TimeDelta::FromSeconds(kShutdownTimeoutSeconds));
   new HostProcess(std::move(context), &exit_code, &shutdown_watchdog);
 
   // Run the main (also UI) message loop until the host no longer needs it.
   message_loop.Run();
 
   return exit_code;
 }
 
 }  // namespace remoting