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 96 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>
 #include <base/linux_util.h>
 #include "remoting/host/audio_capturer_linux.h"
+#include "remoting/host/linux/certificate_watcher.h"
+#include "remoting/host/linux/certificate_watcher_inhibitor.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 44 matching lines @@
 const char kDisableAuthenticationSwitchName[] = "disable-authentication";
 
 // Maximum time to wait for clean shutdown to occur, before forcing termination
 // of the process.
 const int kShutdownTimeoutSeconds = 15;
 
 // Maximum time to wait for reporting host-offline-reason to the service,
 // before continuing normal process shutdown.
 const int kHostOfflineReasonTimeoutSeconds = 10;
 
+// Delay time to shutdown the host when a change of NSS database is detected.
+// This is to repeating restarts when continuous writes to the database occur.
+const int kCertUpdateShutdownDelaySeconds = 30;

[Sergey Ulanov, 2016/03/29 19:40:05]

I don't think we need delay restart that long. Currently you have timeout of 30
seconds starting from the first change in the cert directory. I think it's
better to have shorter delay starting at the last change. I.e. if the directory
keeps changing for 5 seconds then you keep the host running and restart it 2
seconds after that 5 second interval. It should be easy to implement this
behavior using base::DelayTimer, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...

[Yuwei, 2016/03/29 19:57:03]

Acknowledged.

[Sergey Ulanov, 2016/03/29 23:09:54]

Normally we use "Acknowledged." response only for optional or no-op comments.
For comments that require some action like this one please mark them as "Done"
once they are done (unless you disagree with the comment, in which case you
should click "Reply" and write your response)

[Yuwei, 2016/03/30 18:47:45]

Done. DelayedTimer is being used.

+
 // Host offline reasons not associated with shutting down the host process
 // and therefore not expressible through HostExitCodes enum.
 const char kHostOfflineReasonPolicyReadError[] = "POLICY_READ_ERROR";
 const char kHostOfflineReasonPolicyChangeRequiresRestart[] =
     "POLICY_CHANGE_REQUIRES_RESTART";
 
 }  // namespace
 
 namespace remoting {
 
@@ skipping 71 matching lines @@
   // HostChangeNotificationListener::Listener overrides.
   void OnHostDeleted() override;
 
   // Handler of the ChromotingDaemonNetworkMsg_InitializePairingRegistry IPC
   // message.
   void OnInitializePairingRegistry(
       IPC::PlatformFileForTransit privileged_key,
       IPC::PlatformFileForTransit unprivileged_key);
 
  private:
+
   // See SetState method for a list of allowed state transitions.
   enum HostState {
     // Waiting for valid config and policies to be read from the disk.
     // Either the host process has just been started, or it is trying to start
     // again after temporarily going offline due to policy change or error.
     HOST_STARTING,
 
     // Host is started and running.
     HOST_STARTED,
 
@@ skipping 100 matching lines @@
 
   // 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 NSS database changes and kill the host when changes occur
+  CertificateWatcher cert_watcher_;
+  CertificateWatcherInhibitor cert_watcher_inhibitor_;
+#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 73 matching lines @@
 
   ShutdownWatchdog* shutdown_watchdog_;
 
   DISALLOW_COPY_AND_ASSIGN(HostProcess);
 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
                          int* exit_code_out,
                          ShutdownWatchdog* shutdown_watchdog)
     : context_(std::move(context)),
+#if defined(OS_LINUX)
+      cert_watcher_(context_->file_task_runner(),
+                    context_->network_task_runner(),
+                    kCertUpdateShutdownDelaySeconds,
+                    base::Bind(&HostProcess::ShutdownHost,
+                               this, kSuccessExitCode)),
+      cert_watcher_inhibitor_(cert_watcher_),
+#endif
       state_(HOST_STARTING),
       use_service_account_(false),
       enable_vp9_(false),
       frame_recorder_buffer_size_(0),
       policy_state_(POLICY_INITIALIZING),
       host_username_match_required_(false),
       allow_nat_traversal_(true),
       allow_relay_(true),
       allow_pairing_(true),
       curtain_required_(false),
@@ skipping 304 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_.Start();
+    host_->AddStatusObserver(&cert_watcher_inhibitor_);

[Sergey Ulanov, 2016/03/29 19:40:05]

You also need to call RemoveStatusObserver() before host_ is destroyed. In some
cases host needs to be restarted, e.g. when system policies change. When this
happens we keep the process running but destroy the old ChromotingHost object
and create a new one. 
I think HostStatusObserver::OnShutdown() is the right place to call
RemoveStatusObserver()

[Yuwei, 2016/03/29 19:57:03]

Acknowledged.

[Yuwei, 2016/03/29 21:37:41]

I am a little bit confused... So the host will not clear the observer list when
it shuts down? It seems a little bit weird to have an observer removing itself
from the host when the host shuts down (the code will also look weird since you
need to pass in a reference to the host or something)...

[Sergey Ulanov, 2016/03/29 23:09:54]

In general when using the observer pattern it's necessary to make sure that if
the observer is deleted before the observable then it is removed from the list
of observers. So to ensure that usually observable objects verify in the
destructor that the observer list is empty (because if it's not empty it
indicates observable/observer are not destroyed properly). ObserverList has a
flag for that (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
but we don't have it enabled in ChromotingHost.
This particular case is a bit different because the observer outlives the
observable. Still as a rule of thumb there should always be one
RemoveStatusObserver() call for each AddStatusObserver()
Actually it's more common to have observer register/unregister itself, e.g. see
remoting/host/host_status_logger.cc .
So I suggest you use the same approach here as well in CertificateWatche. It
just needs to be notified when there is a new instance of ChromotingHost being
created. So it will look something like this:
  cert_watcher_->OnHostCreated(host_.get());

and in cert_watcher.cc:

void CertWatcher::OnHostCreated(Host* host) {
  host_ = host;
  host_->AddObserver(this);
}

void CertWatcher::OnHostShutdown(Host* host) {
  host_->RemoveObserver(this);
}

This also make sense because it hides the fact that the CertWatcher is a
HostStatusObserver, which is an internal detail of its implementation.

[Yuwei, 2016/03/30 18:47:45]

Done. Observer will be added or removed in Start() and Stop() (Stop() is also
called in dtor).

+#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 926 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