Add PAM session wrapper

remoting/host/linux/linux_me2me_host.py

 #!/usr/bin/python
 # 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.
 
 # Virtual Me2Me implementation.  This script runs and manages the processes
 # required for a Virtual Me2Me desktop, which are: X server, X desktop
 # session, and Host process.
 # This script is intended to run continuously as a background daemon
 # process, running under an ordinary (non-root) user account.
@@ skipping 341 matching lines @@
 
   @staticmethod
   def get_unused_display_number():
     """Return a candidate display number for which there is currently no
     X Server lock file"""
     display = FIRST_X_DISPLAY_NUMBER
     while os.path.exists(X_LOCK_FILE_TEMPLATE % display):
       display += 1
     return display
 
-  def _init_child_env(self):
-    # Create clean environment for new session, so it is cleanly separated from
-    # the user's console X session.
-    self.child_env = {}
+  def _init_child_env(self, keep_env):
+    if keep_env:
+      self.child_env = dict(os.environ)
+    else:
+      # Create clean environment for new session, so it is cleanly separated
+      # from the user's console X session.
+      self.child_env = {}
 
-    for key in [
-        "HOME",
-        "LANG",
-        "LOGNAME",
-        "PATH",
-        "SHELL",
-        "USER",
-        "USERNAME",
-        LOG_FILE_ENV_VAR]:
-      if key in os.environ:
-        self.child_env[key] = os.environ[key]
+      for key in [
+          "HOME",
+          "LANG",
+          "LOGNAME",
+          "PATH",
+          "SHELL",
+          "USER",
+          "USERNAME",
+          LOG_FILE_ENV_VAR]:
+        if key in os.environ:
+          self.child_env[key] = os.environ[key]
+
+      # Initialize the environment from files that would normally be read in a
+      # PAM-authenticated session.

[Jamie, 2016/11/16 01:27:27]

Is this comment still correct now that we are using PAM so create the session? I
guess we're not using the "auth" module, so it might be.

Moving this block will mean that LD_LIBRARY_PATH will no longer override our
mesa settings, even if specified in /etc/environment or /etc/default/locale.
This seems like a good thing independently of this CL. For future reference,
consider a separate CL when you encounter that sort of tangentially-related bug.
It makes the reviewer's job easier (no need to scan the moved block of code to
see if any changes have snuck in) and results in smaller CLs.

[rkjnsn, 2016/11/18 22:46:48]

The comment is correct because the relevant code is only run in the !keep_env
case (when the script is run directly), and not in the keep_env case (when the
script is run by the PAM wrapper). The code rearrangement here is actually to
implement the new --keep-parent-env option needed by the wrapper. The
LD_LIBRARY_PATH fix is more of a beneficial side effect.

+      for env_filename in [
+        "/etc/environment",
+        "/etc/default/locale",
+        os.path.expanduser("~/.pam_environment")]:
+        if not os.path.exists(env_filename):
+          continue
+        try:
+          with open(env_filename, "r") as env_file:
+            for line in env_file:
+              line = line.rstrip("\n")
+              # Split at the first "=", leaving any further instances in the
+              # value.
+              key_value_pair = line.split("=", 1)
+              if len(key_value_pair) == 2:
+                key, value = tuple(key_value_pair)
+                # The file stores key=value assignments, but the value may be
+                # quoted, so strip leading & trailing quotes from it.
+                value = value.strip("'\"")
+                self.child_env[key] = value
+        except IOError:
+          logging.error("Failed to read file: %s" % env_filename)
 
     # Ensure that the software-rendering GL drivers are loaded by the desktop
     # session, instead of any hardware GL drivers installed on the system.
-    self.child_env["LD_LIBRARY_PATH"] = (
+    library_path = (
         "/usr/lib/%(arch)s-linux-gnu/mesa:"
         "/usr/lib/%(arch)s-linux-gnu/dri:"
         "/usr/lib/%(arch)s-linux-gnu/gallium-pipe" %
         { "arch": platform.machine() })
 
-    # Initialize the environment from files that would normally be read in a
-    # PAM-authenticated session.
-    for env_filename in [
-      "/etc/environment",
-      "/etc/default/locale",
-      os.path.expanduser("~/.pam_environment")]:
-      if not os.path.exists(env_filename):
-        continue
-      try:
-        with open(env_filename, "r") as env_file:
-          for line in env_file:
-            line = line.rstrip("\n")
-            # Split at the first "=", leaving any further instances in the
-            # value.
-            key_value_pair = line.split("=", 1)
-            if len(key_value_pair) == 2:
-              key, value = tuple(key_value_pair)
-              # The file stores key=value assignments, but the value may be
-              # quoted, so strip leading & trailing quotes from it.
-              value = value.strip("'\"")
-              self.child_env[key] = value
-      except IOError:
-        logging.error("Failed to read file: %s" % env_filename)
+    if "LD_LIBRARY_PATH" in self.child_env:

[Lambros, 2016/11/17 21:19:35]

Appreciate the bug-fix :) I agree this should normally be a separate CL, but no
need to change it now.

+      library_path += ":" + self.child_env["LD_LIBRARY_PATH"]
+
+    self.child_env["LD_LIBRARY_PATH"] = library_path
 
   def _setup_pulseaudio(self):
     self.pulseaudio_pipe = None
 
     # pulseaudio uses UNIX sockets for communication. Length of UNIX socket
     # name is limited to 108 characters, so audio will not work properly if
     # the path is too long. To workaround this problem we use only first 10
     # symbols of the host hash.
     pulse_path = os.path.join(CONFIG_DIR,
                               "pulseaudio#%s" % g_host_hash[0:10])
@@ skipping 222 matching lines @@
       raise Exception("Unable to choose suitable X session command.")
 
     logging.info("Launching X session: %s" % xsession_command)
     self.session_proc = subprocess.Popen(xsession_command,
                                          stdin=open(os.devnull, "r"),
                                          cwd=HOME_DIR,
                                          env=self.child_env)
     if not self.session_proc.pid:
       raise Exception("Could not start X session")
 
-  def launch_session(self, x_args):
-    self._init_child_env()
+  def launch_session(self, keep_env, x_args):
+    self._init_child_env(keep_env)
     self._setup_pulseaudio()
     self._setup_gnubby()
     self._launch_x_server(x_args)
     self._launch_x_session()
 
   def launch_host(self, host_config):
     # Start remoting host
     args = [HOST_BINARY_PATH, "--host-config=-"]
     if self.pulseaudio_pipe:
       args.append("--audio-pipe-name=%s" % self.pulseaudio_pipe)
@@ skipping 565 matching lines @@
   parser.add_option("", "--reload", dest="reload", default=False,
                     action="store_true",
                     help="Signal currently running host to reload the config.")
   parser.add_option("", "--add-user", dest="add_user", default=False,
                     action="store_true",
                     help="Add current user to the chrome-remote-desktop group.")
   parser.add_option("", "--add-user-as-root", dest="add_user_as_root",
                     action="store", metavar="USER",
                     help="Adds the specified user to the chrome-remote-desktop "
                     "group (must be run as root).")
+  parser.add_option("", "--keep-parent-env", dest="keep_env", default=False,

[rkjnsn, 2016/11/15 22:52:57]

Should this flag be public and documented?

[Jamie, 2016/11/16 01:27:27]

TBH, I don't see the point of having separate public and secret arguments. The
whole script is not intended to be used directly.

[Lambros, 2016/11/17 21:19:35]

I don't think it matters either way. The help-text is just a developer
convenience - users aren't supposed to run this script directly.

+                    action="store_true",
+                    help=optparse.SUPPRESS_HELP)
   parser.add_option("", "--watch-resolution", dest="watch_resolution",
                     type="int", nargs=2, default=False, action="store",
                     help=optparse.SUPPRESS_HELP)
   (options, args) = parser.parse_args()
 
   # Determine the filename of the host configuration and PID files.
   if not options.config:
     options.config = os.path.join(CONFIG_DIR, "host#%s.json" % g_host_hash)
 
   # Check for a modal command-line option (start, stop, etc.)
@@ skipping 213 matching lines @@
         logging.info("Relaunching self")
         relaunch_self()
       else:
         # If there is a non-zero |failures| count, restarting the whole script
         # would lose this information, so just launch the session as normal.
         if x_server_inhibitor.is_inhibited():
           logging.info("Waiting before launching X server")
           relaunch_times.append(x_server_inhibitor.earliest_relaunch_time)
         else:
           logging.info("Launching X server and X session.")
-          desktop.launch_session(args)
+          desktop.launch_session(options.keep_env, args)
           x_server_inhibitor.record_started(MINIMUM_PROCESS_LIFETIME,
                                             backoff_time)
           allow_relaunch_self = True
 
     if desktop.host_proc is None:
       if host_inhibitor.is_inhibited():
         logging.info("Waiting before launching host process")
         relaunch_times.append(host_inhibitor.earliest_relaunch_time)
       else:
         logging.info("Launching host process")
@@ skipping 53 matching lines @@
         else:
           logging.info("Host exited with status %s." % os.WEXITSTATUS(status))
       elif os.WIFSIGNALED(status):
         logging.info("Host terminated by signal %s." % os.WTERMSIG(status))
 
 
 if __name__ == "__main__":
   logging.basicConfig(level=logging.DEBUG,
                       format="%(asctime)s:%(levelname)s:%(message)s")
   sys.exit(main())