Add PAM session wrapper

remoting/host/linux/remoting_pam_session.cc

Index: remoting/host/linux/remoting_pam_session.cc
diff --git a/remoting/host/linux/remoting_pam_session.cc b/remoting/host/linux/remoting_pam_session.cc
new file mode 100644
index 0000000000000000000000000000000000000000..94cb2254e2926f49f5112789d3858da2f62b2b6d
--- /dev/null
+++ b/remoting/host/linux/remoting_pam_session.cc
@@ -0,0 +1,409 @@
+// Copyright 2016 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 wrapper to run the virtual me2me session within a
+// proper PAM session. It will generally be run as root and drop privileges to
+// the specified user before running the me2me session script.
+
+#include <sys/types.h>
+#include <grp.h>
+#include <pwd.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <cerrno>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include <security/pam_appl.h>
+
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "base/strings/string_piece.h"
+
+namespace {
+
+const char kPamName[] = "chrome-remote-desktop";
+
+const char kHelpSwitchName[] = "help";
+const char kQuestionSwitchName[] = "?";
+const char kUserSwitchName[] = "user";
+const char kScriptSwitchName[] = "me2me-script";
+
+const char kUsageMessage[] =
+  "Usage: %s [options]\n"
+  "\n"
+  "Options:\n"
+  "  --help, -?               - Print this message.\n"
+  "  --user=<user>            - Create session as the specified user."
+                               "(Must be root.)\n"
+  "  --me2me-script=<script>  - Location of the me2me python script "

[Sergey Ulanov, 2016/09/10 00:23:16]

Do we need this parameter? Can this wrapper discover the daemon script location
same way the script finds the host binary?

[rkjnsn, 2016/09/12 19:04:27]

Using lstat and readlink on /proc/self/exe, along with some path munging, yes.
However, it would add complexity, reduce flexibility, and make testing more
difficult. Further, the init script would still have to know the location of the
me2me script for the stop and reload operations, so there doesn't seem to be
much of a downside to passing that information to the wrapper.

+                               "(required)\n";
+
+void PrintUsage(const base::FilePath& program_name) {
+  std::printf(kUsageMessage, program_name.MaybeAsASCII().c_str());
+}
+
+std::string ShellEscapeArgument(const base::StringPiece argument) {
+  std::string result;
+  for (char character : argument) {
+    // Some shells ascribe special meaning to some escape sequences such as \n,
+    // so don't escape any alphanumerics. (Also cuts down on verbosity.)

[Jamie, 2016/09/09 18:27:12]

This approach (exempting characters for which a preceding backslash escape would
either be correct or ignored) is a bit counter-intuitive. Is this well-defined
behaviour for all shells? I would have thought that a safer approach would be
enclose each "word" (ie, call this for each parameter separately rather than for
the whole strong) in single-quotes and backslash-escape only the backslash and
single-quote characters.

[rkjnsn, 2016/09/09 21:24:31]

This is indeed called separately for each argument (it will backslash escape
space). The problem with single quoting is that different shells treat single
quotes differently. For example, bash will not interpret any backslash escapes
in a single-quoted string, while fish will allow you to backslash-escape a
single quote and a backslash within a single-quoted string. Liberally escaping
everything except these specific characters works in every shell I've tested
with, and is similar to what sudo does.

Single-quoting could work if we replace each inner single quote and backslash
with something like '\'' and '\\' (that is, close the single-quoted string, add
the respective character escaped but not quoted, and then open a new
single-quoted string). That would work with the shells I have tested.

I opted for the solution here because it's sudo's approach, but I can switch if
you'd prefer.

[Sergey Ulanov, 2016/09/10 00:23:16]

Why do we need to support any shell other than bash?

[rkjnsn, 2016/09/10 00:40:56]

We need to invoke the user's login shell to pick up any environment variables
they have set in their shell configuration, including our
CHROME_REMOTE_DESKTOP_* environment variables. This is what sudo does with they
way we currently launch user sessions, so not doing so could break users.

[rkjnsn, 2016/09/12 19:33:05]

In code, this would look something like this:

    std::string result;
    result.push_back('\'');
    for (char character : argument) {
      if (character == '\'' || character == '\\') {
        result.push_back('\'');
        result.push_back('\\');
        result.push_back(character);
        result.push_back('\'');
      } else {
        result.push_back(character);
      }
    }
    result.push_back('\'');

This relies on the shell in question interpreting no other special character in
single quotes. The sudo approach still strikes me as safer because escaping
behavior seems more universal than quoting behavior. If nothing else, using the
sudo approach ensures that a shell that deviated in escaping behavior would
break sudo, too.

[Jamie, 2016/09/12 20:36:51]

If there's precedent for this approach, then I think it's fine. Perhaps add
something to the comment explaining that this is in keeping with sudo's
approach.

+    if (!((character >= '0' && character <= '9') ||

[Lambros, 2016/09/09 22:48:58]

I'm not sure if it's better to use isalnum() from <ctype.h> ? It would be more
readable, but it would be locale-dependent as well.

[rkjnsn, 2016/09/10 00:06:38]

I was wondering the same. Sudo uses isalnum, but isalnum fundamentally can't
function properly with multibyte encodings like UTF-8, which is now standard for
common locales on Linux. I could do some testing to see how it handles getting
one byte of a multibyte sequence, but it seemed safer to explicitly work with
bytes (which seems to be what shells pay attention to) and specify which
characters we want to whitelist.

+          (character >= 'A' && character <= 'Z') ||
+          (character >= 'a' && character <= 'z') ||
+          (character == '-' || character == '_')))

[Jamie, 2016/09/09 18:27:11]

It's not mandated by the style guide (and we're not consistent on the team), but
I prefer braces even for single-line conditionals. In cases like this where the
boolean expression itself spans multiple lines I think it's a fairly
uncontentious opinion.

If you do change it here, please be consistent elsewhere in this file.

[rkjnsn, 2016/09/09 21:24:30]

Acknowledged.

[Lambros, 2016/09/09 22:48:58]

Yep, I think we prefer braces when the 'if' condition or the body spans more
than 1 line.

+      result.push_back('\\');
+    result.push_back(character);
+  }
+  return result;
+}
+
+extern "C" int Converse(int num_msg, const struct pam_message **msg,

[Lambros, 2016/09/09 22:48:58]

nit: Name these parameters consistently with
remoting/host/pam_authorization_factory_posix.cc

Does this function do anything other than log stuff to stdout/stderr ? If it
does, I'm having trouble seeing it through all of the logging noise.

I think there should be a function comment explaining what is implemented here
and why.

[rkjnsn, 2016/09/10 00:06:38]

Yeah, it's basically the same purpose as PamAuthorizer::PamConversation:
output/log messages and errors from PAM, but fail if PAM asks us for a response.
I'll add a comment. The parameters are named to match the PAM documentation, but
I'll change them to match PamAuthorizer::PamConversation, instead.

+                        struct pam_response **resp, void *appdata_ptr) {
+  bool failed = false;
+
+  for (int i = 0; i < num_msg; ++i) {
+    // This is correct for the PAM included with Linux, OS X, and BSD. However,
+    // apparently Solaris and HP/UX require instead `&(*msg)[i]`. That is, they
+    // disagree as to which level of indirection contains the array.
+    const pam_message* message = msg[i];
+
+    switch (message->msg_style) {
+      case PAM_PROMPT_ECHO_OFF:
+      case PAM_PROMPT_ECHO_ON:
+        std::fputs("Unsupported request for user input from PAM\n", stderr);

[Jamie, 2016/09/09 18:27:11]

LOG(ERROR) would be more appropriate here, unless you're specifically trying to
avoid pulling in more libraries than we need.

[rkjnsn, 2016/09/09 21:24:31]

Lambros mentioned that pulling in logging would also bring in all of the unicode
stuff and bloat the executable, but I can definitely do it that way, if you'd
prefer.

[Jamie, 2016/09/12 20:36:51]

It might be worth seeing how much it increases the code size. If it's not
significant, then I prefer code consistency; if it is then please add a comment
explaining why we're not using it.

+        failed = true;
+        break;
+      case PAM_TEXT_INFO:
+        std::fprintf(stdout, "[PAM] %s", message->msg);

[Jamie, 2016/09/09 18:27:11]

LOG(INFO), or maybe even VLOG if most users don't need to see this.

+        if (message->msg[0] == '\0' ||

[Lambros, 2016/09/09 22:48:58]

Maybe:
!StringPiece(message->msg).ends_with('\n')
is more readable because you don't need the separate check for emptiness.

But we're not doing anything with this message so perhaps there's no need to log
it in the first place? A simple log that some PAM module tried to converse with
our binary is maybe sufficient here?

Thinking about it, we perhaps shouldn't log anything at all, except for a fatal
error. This binary will be started by an init.d script, so any output will spew
the user's startup console (where it says "Starting xxx ... [OK]"), or simply
get redirected to /dev/null. You could consider using the syslog() facility
instead?

[rkjnsn, 2016/09/10 00:06:38]

I didn't notice that StringPiece also handles getting passed a NULL pointer.
Cool.

If PAM spits out an error and fails to initialize the session, we want the
message to be retrievable somehow. I'll see how much the standard logging stuff
bloats us, and check out syslog if it's too much.

+            message->msg[std::strlen(message->msg) - 1] != '\n')
+          std::fputc('\n', stdout);
+        break;
+      case PAM_ERROR_MSG:
+        std::fprintf(stderr, "[PAM] %s", message->msg);
+        if (message->msg[0] == '\0' ||
+            message->msg[std::strlen(message->msg) - 1] != '\n')
+          std::fputc('\n', stderr);
+        break;
+      default:
+        std::fputs("Unknown PAM conversation message style\n", stderr);
+        failed = true;
+        break;
+    }
+  }
+
+  if (failed)
+    return PAM_CONV_ERR;
+
+  pam_response* response = static_cast<pam_response*>(
+      calloc(num_msg, sizeof(*response)));
+
+  if (response == nullptr)
+    return PAM_BUF_ERR;
+
+  *resp = response;
+  return PAM_SUCCESS;
+}
+
+const struct pam_conv kPamConversation = {Converse, nullptr};
+
+// Wrapper class for working with PAM
+class PamHandle {
+ public:
+  // Wrapper for environment list to properly free it on scope exit

[Jamie, 2016/09/09 18:27:12]

This shouldn't be a nested class--Env is not an sub-concept of PamHandle. It
should also be called Environment, since abbreviations are discouraged.

The comment should also explain why a simple vector<string> will not suffice
(presumably because PAM needs a raw char** pointer).

[rkjnsn, 2016/09/09 21:24:31]

Acknowledged.

+  class Env {
+   public:
+    Env(Env&& other) = default;

[Lambros, 2016/09/09 22:48:58]

If you define a move-ctor, I think it's standard to provide a
move-assignment-operator as well.

[rkjnsn, 2016/09/10 00:06:38]

It is. On the other hand, this class is private to this file, and nothing needs
move assignment. I figured there wasn't much point implementing dead code, but I
can add it if you think the convention is important.

+
+    ~Env() {
+      for (char* entry : env_) {
+        free(entry);
+      }
+    }
+
+    // Set an variable in the environment. If overwrite is true, any existing
+    // value will be replaced. Otherwise, the old value will be kept.
+

[Jamie, 2016/09/09 18:27:12]

No newline after comments, please.

[rkjnsn, 2016/09/09 21:24:31]

Acknowledged.

+    void SetVariable(const char* variable, const char* value, bool overwrite) {

[Jamie, 2016/09/09 18:27:11]

Rather than an overwrite flag, it might be better to provide a GetVariable
method and have the caller check it for nullptr. It would simplify this code a
little.

[rkjnsn, 2016/09/09 21:24:31]

There should only be one entry for each variable, and I was considering it Env's
responsibility to uphold that invariant, which would require it to loop through
the vector, anyway. This approach takes advantage of that to simplify the client
code (which sets several variables) by obviating the need for separate check
before each variable set. It's also slightly more efficient because we only have
to loop through once instead of twice, though that probably not a necessary
consideration.

To sum up, I think the approach I took trades a slight increase in the
complexity of this method for significantly simplifying the caller. In addition,
nothing else in the code needs a GetVariable, so adding it just to remove this
flag seems an especially poor tradeoff.

[Jamie, 2016/09/12 20:36:51]

The problem is that the API contract is confusing because the name doesn't
really reflect what the function does. When reading the call-site, I expect a
method called SetVariable to set a variable unconditionally; I shouldn't have to
look at the method declaration to see what the additional bool parameter does.

It would be clearer if you used an emum instead of a bool (enum class
ReplaceDisposition { KEEP_EXISTING, REPLACE_EXISTING }, for example), but TBH I
don't think that the performance impact is important, and I think that the
call-site would be a lot more readable written as:

     env->SetVariable("USER", pwinfo->pw_name);
     env->SetVariable("LOGNAME", pwinfo->pw_name);
     env->SetVariable("HOME", pwinfo->pw_dir);
     env->SetVariable("SHELL", pwinfo->pw_shell);
     if (!env->HasVariable("PATH") {
       env->SetVariable("PATH", "/bin:/usr/bin");
     }

Alternatively, Lambros's suggestion of just using std::map (and standard
manipulations to add things to it) and providing a method to convert it to
char** would also work.

+      size_t var_len = strlen(variable);
+      size_t val_len = strlen(value);
+
+      char* new_entry = static_cast<char*>(malloc(var_len + 1 + val_len + 1));

[Lambros, 2016/09/09 22:48:58]

It looks like you're trying to directly operate on a Posix-style env list. This
means the code is unnecessarily low-level, and you have to write your own code
to reject duplicates.

I would suggest instead that you store the name-value pairs into a
std::map<string, string>. Then provide a method that will return this data as an
env-list.

Have a look at base::Environment::AlterEnvironment() to see how it returns the
array (note that it uses std::unique_ptr which we strongly encourage). Does it
already provide what you're looking for? If not, you can copy the bits you need
into here.

[rkjnsn, 2016/09/10 00:06:38]

So PAM returns a bunch of malloc'd c strings. The idea with this class was to
make it easy for the code in ExecuteSession to set additional environment
variables in an encapsulated way, and also ensure that all strings get freed in
an RAII fashion, without doing a bunch of unnecessary copying and allocation.
AlterEnvironment doesn't quite do what I need (there's no way to add a variable
if and only if it isn't already present), and it seems more complicated, so I
don't think copying it here and modifying it would get me anything.

+      if (new_entry == nullptr) {
+        return; // Should never happen in practice, but don't crash if it does.

[Lambros, 2016/09/09 22:48:58]

A graceful "crash" is fine. _exit(1) is a reasonable thing to call if you can't
allocate memory.
It's better to use C++ new/delete instead of malloc/free, so that you get this
"crashing" behavior for free.

[rkjnsn, 2016/09/10 00:06:38]

I'm using malloc/free because PAM gives us malloc'd strings, and I didn't want
to reallocate all of them.

+      }
+
+      // strcpy is safe here because the buffer size is calculated from the
+      // source strings.
+
+      strcpy(new_entry, variable);
+      new_entry[var_len] = '=';
+      strcpy(new_entry + var_len + 1, value);
+
+      // Check if the variable already exists.
+
+      for (char*& entry : env_) {

[Jamie, 2016/09/09 18:27:12]

We don't use non-const references much. They obfuscate the fact that what
appears to be an assignment to a local variable can have wider effect. Writing
this as an old-style for loop of iterators and making the reassignment explicit
(ie, *it = new_entry) would be clearer.

[rkjnsn, 2016/09/09 21:24:31]

I personally find this formulation clearer, but I'll do it your way. :)

+        if (entry != nullptr && strncmp(entry, new_entry, var_len + 1) == 0) {
+          if (overwrite) {
+            free(entry);
+            entry = new_entry;
+          } else {
+            free(new_entry);
+          }
+          return;
+        }
+      }
+      env_.insert(env_.end() - 1, new_entry);  // Insert before terminating null

[Jamie, 2016/09/09 18:27:11]

push_back() would be clearer.

[rkjnsn, 2016/09/09 21:24:31]

push_back would insert the entry after the terminating NULL. We need to insert
it before the NULL (hence `- 1`).

[Jamie, 2016/09/12 20:36:51]

Ack.

+    }
+
+    operator char** () {

[Lambros, 2016/09/09 22:48:58]

Operator-overloading (and overloading in general) is discouraged in Chromium.
Just write a Data() method instead (if you still need it after reading my other
comments).

[rkjnsn, 2016/09/10 00:06:38]

Acknowledged.

+      return env_.data();
+    }
+   private:
+    explicit Env(char** env) {
+      if (env) {
+        for (char** entry = env; *entry != nullptr; ++entry) {
+          env_.push_back(*entry);
+        }
+        env_.push_back(nullptr);
+        free(env);
+      }
+    }
+
+    std::vector<char*> env_;
+
+    DISALLOW_COPY_AND_ASSIGN(Env);
+    friend class PamHandle;

[Lambros, 2016/09/09 22:48:58]

Why does PamHandle need special access? Maybe because of the char** env ctor
above? You could provide that as a public ctor instead.

[rkjnsn, 2016/09/10 00:06:38]

The char** ctor expects to receive specifically the output of pam_getenvlist.
(In detail, it expects env to be a null-terminated, malloc'd array of pointers
to null-terminated, malloc'd strings of the form VAR=value, and is written with
this in mind.) I made the char** ctor private because it breaks the
encapsulation otherwise provided by the class. (@Jamie: This coupling with PAM
is why I put it within PamHandle instead of making it a top-level class.)

+  };
+
+  // Initialize PAM transaction
+  PamHandle(const char* service_name, const char* user,
+            const struct pam_conv* pam_conversation) {
+    last_retcode_ = pam_start(service_name, user, pam_conversation, &pamh_);
+    if (last_retcode_ != PAM_SUCCESS)
+      pamh_ = nullptr;
+  }
+
+  // Terminate PAM transaction
+  ~PamHandle() {
+    if (pamh_ != nullptr)
+      pam_end(pamh_, last_retcode_);
+  }
+
+  // True if the PAM transaction was successfully initialized.
+  operator bool() const {

[Lambros, 2016/09/09 22:48:58]

Same comment about overloading. Maybe call it IsInitialized()?

[rkjnsn, 2016/09/10 00:06:38]

Acknowledged.

+    return pamh_ != nullptr;
+  }
+
+  // Perform account validation
+  int AccountManagement(int flags) {
+    return last_retcode_ = pam_acct_mgmt(pamh_, flags);
+  }
+
+  // Establish / delete PAM user credentials
+  int SetCredentials(int flags) {
+    return last_retcode_ = pam_setcred(pamh_, flags);
+  }
+
+  // Start user session
+  int OpenSession(int flags) {
+    return last_retcode_ = pam_open_session(pamh_, flags);
+  }
+
+  // End user session
+  int CloseSession(int flags) {
+    return last_retcode_ = pam_close_session(pamh_, flags);
+  }
+
+  // Returns the current username according to PAM. It is possible for PAM
+  // modules to change this from the initial value passed to the constructor.
+  base::Optional<std::string> GetUser() {
+    const char* user;
+    last_retcode_ = pam_get_item(pamh_, PAM_USER,
+                                 reinterpret_cast<const void**>(&user));
+    if (last_retcode_ != PAM_SUCCESS || user == nullptr)
+      return base::nullopt;
+    return std::string(user);
+  }
+
+  base::Optional<Env> GetEnv() {
+    char** env = pam_getenvlist(pamh_);
+    if (env == nullptr)
+      return base::nullopt;
+    return Env(env);
+  }
+
+  // Returns a description of the given return code
+  const char* ErrorString(int retcode) {
+    return pam_strerror(pamh_, retcode);
+  }
+ private:
+  pam_handle_t* pamh_ = nullptr;

[Lambros, 2016/09/09 22:48:58]

Name this pam_handle_.

[rkjnsn, 2016/09/10 00:06:38]

Acknowledged.

+  int last_retcode_ = PAM_SUCCESS;

[Lambros, 2016/09/09 22:48:58]

Avoid 'last', because it could mean 'final' or 'previous'. Use 'current' or
'latest' or 'previous' instead.
Don't abbreviate 'retcode' - perhaps use 'return_code' or 'result' or
'return_value' instead.

[rkjnsn, 2016/09/10 00:06:38]

Acknowledged.

+
+  DISALLOW_COPY_AND_ASSIGN(PamHandle);
+};
+
+#define RETURN_ON_PAM_FAILURE(retcode) \

[Jamie, 2016/09/09 18:27:12]

Please don't use macros if you can avoid it. In this case, the code is so short
you can just duplicate it (perhaps moving the error message logging into the
PamHandle class?)

[rkjnsn, 2016/09/09 21:24:30]

I figured this would make the non-error case easier to read, but I can duplicate
it out.

+    do { \
+      int my_retcode = (retcode); \
+      if (my_retcode != PAM_SUCCESS) { \
+        fprintf(stderr, "[PAM] %s\n", pam_handle.ErrorString(my_retcode)); \
+        return false; \
+      } \
+    } while (false)

[Jamie, 2016/09/09 18:27:12]

I've never seen do...while(false) used elsewhere; what's the benefit?

[rkjnsn, 2016/09/09 21:24:30]

It's a trick often used for function-style macros. It makes
RETURN_ON_PAM_FAILURE(foo); a single statement, preventing confusing errors if
someone were to write

if (foo)
  RETURN_ON_PAM_FAILURE(bar);
else
  ...

(With a standard block, the semicolon would make the above a syntax error.)

+
+// Set up the PAM session and run the me2me script. Returns true if the session
+// was executed successfully.
+
+bool ExecuteSession(const char* user, const char* script_path) {
+
+  // First we set up the PAM session
+
+  PamHandle pam_handle(kPamName, user, &kPamConversation);
+
+  if (!pam_handle) {
+    std::fputs("Failed to initialize PAM", stderr);
+    return false;
+  }
+
+  RETURN_ON_PAM_FAILURE(pam_handle.AccountManagement(0));
+
+  // The documentation states that setcred should be called before open_session,
+  // as done here, but it may me worth noting that `login` calls open_session

[Jamie, 2016/09/09 18:27:12]

s/me/be/

Also, move this comment down to the code it applies to.

[rkjnsn, 2016/09/09 21:24:31]

Acknowledged.

+  // first.
+
+  std::string real_user = pam_handle.GetUser().value_or(user);
+
+  errno = 0;
+  struct passwd* pwinfo = getpwnam(real_user.c_str());
+  if (pwinfo == nullptr) {
+    std::perror("getpwnam");
+    return false;
+  }
+  if (setgid(pwinfo->pw_gid) == -1) {
+    std::perror("setgid");
+    return false;
+  }
+  if (initgroups(pwinfo->pw_name, pwinfo->pw_gid) == -1) {
+    std::perror("initgroups");
+    return false;
+  }
+
+  RETURN_ON_PAM_FAILURE(pam_handle.SetCredentials(PAM_ESTABLISH_CRED));
+  RETURN_ON_PAM_FAILURE(pam_handle.OpenSession(0));
+  base::Optional<PamHandle::Env> env = pam_handle.GetEnv();
+  if (!env) {
+    std::fputs("Failed to get environment from PAM", stderr);
+    return false;
+  }
+
+  // The above may have remapped the user or invalidated pwinfo, so get user
+  // info again
+
+  real_user = pam_handle.GetUser().value_or(std::move(real_user));
+  pwinfo = getpwnam(real_user.c_str());
+  if (pwinfo == nullptr) {
+    std::perror("getpwnam");
+    return false;
+  }
+
+  // And now we're ready to fork the child.
+
+  pid_t child = fork();
+  if (child < 0) { // Error
+    std::perror("fork");
+    return false;
+  } else if (child != 0) { // We're the parent
+    int status = 0;
+    do {
+      wait(&status);
+    } while (!WIFEXITED(status) && !WIFSIGNALED(status));
+
+    // Best effort cleanup (ignore errors)
+
+    pam_handle.CloseSession(0);
+    pam_handle.SetCredentials(PAM_DELETE_CRED);
+
+    if (WIFSIGNALED(status) || WEXITSTATUS(status) != EXIT_SUCCESS)
+        return false;
+    return true;
+  } else { // We're the child
+    if (setuid(pwinfo->pw_uid) == -1) { // Group ids were already set above
+      std::perror("setuid");
+      std::exit(EXIT_FAILURE);
+    }
+    if (chdir(pwinfo->pw_dir) == -1) {
+      std::perror("chdir");
+      std::exit(EXIT_FAILURE);
+    }
+
+    // Set various user-related environment variables
+
+    env->SetVariable("USER", pwinfo->pw_name, true);
+    env->SetVariable("LOGNAME", pwinfo->pw_name, true);
+    env->SetVariable("HOME", pwinfo->pw_dir, true);
+    env->SetVariable("SHELL", pwinfo->pw_shell, true);
+
+    // Set a default PATH if one hasn't been provided through PAM
+
+    env->SetVariable("PATH", "/bin:/usr/bin", false);
+
+    // Finally, exec the me2me script

[Jamie, 2016/09/09 18:27:12]

Explain why we're setting argv[0] to '-' (with a reference, if you can find
one).

[rkjnsn, 2016/09/09 21:24:31]

Acknowledged.

+
+    std::string shell_command = ShellEscapeArgument(script_path) +
+                                " --start-foreground --keep-parent-env";
+    const char* args[4] = { "-", "-c", shell_command.c_str(), nullptr };
+
+    execve(pwinfo->pw_shell, const_cast<char * const *>(args), *env);

[Sergey Ulanov, 2016/09/10 00:23:16]

Would it make sense to use base::LaunchProcess() here? It can do a lot of the
stuff you need here, e.g. set environment for the child process.

[Sergey Ulanov, 2016/09/10 00:23:17]

Why do we need to run $SHELL here instead of /bin/sh?
I'm not sure shell is necessary at all. The script is in python, so you could
just run /usr/bin/python here

[rkjnsn, 2016/09/10 00:40:56]

I'll take a look at LaunchProcess. See above for why we are executing through
the user's login shell.

+    std::perror("execve");
+    std::exit(EXIT_FAILURE);
+  }
+}
+
+#undef RETURN_ON_PAM_FAILURE
+
+}  // namespace
+
+int main(int argc, char** argv) {
+  base::CommandLine::Init(argc, argv);
+
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(kHelpSwitchName) ||
+      command_line->HasSwitch(kQuestionSwitchName)) {
+    PrintUsage(command_line->GetProgram());
+    std::exit(EXIT_SUCCESS);
+  }
+
+  base::FilePath script_path =
+      command_line->GetSwitchValuePath(kScriptSwitchName);
+  std::string user = command_line->GetSwitchValueNative(kUserSwitchName);
+
+  if (script_path.empty()) {
+    std::fputs("The path to the me2me python script is required.\n", stderr);
+    std::exit(EXIT_FAILURE);
+  }
+
+  if (user.empty()) {
+    std::fputs("Using the wrapper for the current user is not currently "
+               "implemented.\n", stderr);
+    std::exit(EXIT_FAILURE);
+  }
+
+  // TODO(rkjnsn): daemonize before executing session.
+  if(!ExecuteSession(user.c_str(), script_path.value().c_str()))
+    exit(EXIT_FAILURE);
+  exit(EXIT_SUCCESS);
+}