Restart the host when the third party auth certificate changes

remoting/host/remoting_me2me_host.cc

Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index 2460328157713065969fef344a444be07d218ae1..0f0beb8e00ec1561f0e78c6f3bf1badfc4db1a94 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -115,6 +115,7 @@
 #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)
@@ -366,6 +367,8 @@ class HostProcess : public ConfigWatcher::Delegate,
   // Error handler for SignalingConnector.
   void OnAuthFailed();
 
+  void OnHostRestartRequested();
+
   void RestartHost(const std::string& host_offline_reason);
   void ShutdownHost(HostExitCodes exit_code);
 
@@ -390,6 +393,11 @@ class HostProcess : public ConfigWatcher::Delegate,
 
   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_;
@@ -807,6 +815,15 @@ void HostProcess::CreateAuthenticatorFactory() {
     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());
@@ -1705,6 +1722,16 @@ void HostProcess::OnCrash(const std::string& function_name,
   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);