Add PAM session wrapper

remoting/host/linux/linux_me2me_host.py

Index: remoting/host/linux/linux_me2me_host.py
diff --git a/remoting/host/linux/linux_me2me_host.py b/remoting/host/linux/linux_me2me_host.py
index a40063bf470105b1338d0d97c3d1ba62005d878a..a5e97c097fb38322fa53e3ee66fb0eb6955dcde8 100755
--- a/remoting/host/linux/linux_me2me_host.py
+++ b/remoting/host/linux/linux_me2me_host.py
@@ -359,54 +359,62 @@ class Desktop:
       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 = {}
-
-    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]
+  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]
+
+      # 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
@@ -649,8 +657,8 @@ class Desktop:
     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)
@@ -1236,6 +1244,9 @@ Web Store: https://chrome.google.com/remotedesktop"""
                     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)
@@ -1469,7 +1480,7 @@ Web Store: https://chrome.google.com/remotedesktop"""
           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