Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2323153002:
Add PAM session wrapper (Closed)
DescriptionAdd PAM session wrapper
Adds a wrapper to launch me2me script in a proper PAM session.
BUG=616962
Committed: https://crrev.com/cbef66778a13c926d0518b269928a288777cbbfc
Cr-Commit-Position: refs/heads/master@{#435044}
Patch Set 1 : Add PAM session wrapper #
Total comments: 63
Patch Set 2 : Switch to using LaunchProcess and logging #Patch Set 3 : Option A: Use wrapper for boot only #
Total comments: 33
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Fix unused result error #
Total comments: 5
Patch Set 7 : Address feedback #
Messages
Total messages: 62 (25 generated)
Patchset #1 (id:1) has been deleted
rkjnsn@chromium.org changed reviewers: + jamiewalch@chromium.org, lambroslambrou@chromium.org
Here's my first go at the PAM session wrapper. I've done some testing with it, and it seems to do the right thing, so I'm putting it up for review. Use `ninja remoting_pam_session` to build. Note that the current patch does not yet use the wrapper during session startup. The following changes still need to be made to do so: 1. Add --keep-parent-env option to Python script. 2. Modify init script to use wrapper instead of sudo when launching the session. 3. Modify the chrome-remote-desktop PAM configuration to invoke pam_env on session creation. 4. Make the wrapper daemonize before starting the session. Thanks!
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn File remoting/host/BUILD.gn (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn#... remoting/host/BUILD.gn:1364: executable("remoting_pam_session") { I think it would be better to name this according to what it does, not how it's implemented. If it's something we expect primarily to be useful for starting the host at boot, then perhaps something like remoting_boot_launcher; or since it's primarily responsible for setting up the environment (in the generic, not the Posix sense of the word), remoting_env_setup might be appropriate. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:64: (character == '-' || character == '_'))) 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:84: std::fputs("Unsupported request for user input from PAM\n", stderr); LOG(ERROR) would be more appropriate here, unless you're specifically trying to avoid pulling in more libraries than we need. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:88: std::fprintf(stdout, "[PAM] %s", message->msg); LOG(INFO), or maybe even VLOG if most users don't need to see this. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:124: // Wrapper for environment list to properly free it on scope exit 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). https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:137: No newline after comments, please. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:138: void SetVariable(const char* variable, const char* value, bool overwrite) { 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:156: for (char*& entry : env_) { 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:167: env_.insert(env_.end() - 1, new_entry); // Insert before terminating null push_back() would be clearer. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:258: #define RETURN_ON_PAM_FAILURE(retcode) \ 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?) https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:265: } while (false) I've never seen do...while(false) used elsewhere; what's the benefit? https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:284: // as done here, but it may me worth noting that `login` calls open_session s/me/be/ Also, move this comment down to the code it applies to. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:363: // Finally, exec the me2me script Explain why we're setting argv[0] to '-' (with a reference, if you can find one).
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn File remoting/host/BUILD.gn (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn#... remoting/host/BUILD.gn:1364: executable("remoting_pam_session") { On 2016/09/09 18:27:11, Jamie wrote: > I think it would be better to name this according to what it does, not how it's > implemented. If it's something we expect primarily to be useful for starting the > host at boot, then perhaps something like remoting_boot_launcher; or since it's > primarily responsible for setting up the environment (in the generic, not the > Posix sense of the word), remoting_env_setup might be appropriate. remoting_user_session? I think the essence of what it does is start the me2me script as the target user, doing whatever it needs to do to initialize a session for that user. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) On 2016/09/09 18:27:12, Jamie wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:64: (character == '-' || character == '_'))) On 2016/09/09 18:27:11, Jamie wrote: > 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. Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:84: std::fputs("Unsupported request for user input from PAM\n", stderr); On 2016/09/09 18:27:11, Jamie wrote: > LOG(ERROR) would be more appropriate here, unless you're specifically trying to > avoid pulling in more libraries than we need. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:124: // Wrapper for environment list to properly free it on scope exit On 2016/09/09 18:27:12, Jamie wrote: > 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). Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:137: On 2016/09/09 18:27:12, Jamie wrote: > No newline after comments, please. Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:138: void SetVariable(const char* variable, const char* value, bool overwrite) { On 2016/09/09 18:27:11, Jamie wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:156: for (char*& entry : env_) { On 2016/09/09 18:27:12, Jamie wrote: > 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. I personally find this formulation clearer, but I'll do it your way. :) https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:167: env_.insert(env_.end() - 1, new_entry); // Insert before terminating null On 2016/09/09 18:27:11, Jamie wrote: > push_back() would be clearer. push_back would insert the entry after the terminating NULL. We need to insert it before the NULL (hence `- 1`). https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:258: #define RETURN_ON_PAM_FAILURE(retcode) \ On 2016/09/09 18:27:12, Jamie wrote: > 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?) I figured this would make the non-error case easier to read, but I can duplicate it out. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:265: } while (false) On 2016/09/09 18:27:12, Jamie wrote: > I've never seen do...while(false) used elsewhere; what's the benefit? 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.) https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:284: // as done here, but it may me worth noting that `login` calls open_session On 2016/09/09 18:27:12, Jamie wrote: > s/me/be/ > > Also, move this comment down to the code it applies to. Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:363: // Finally, exec the me2me script On 2016/09/09 18:27:12, Jamie wrote: > Explain why we're setting argv[0] to '-' (with a reference, if you can find > one). Acknowledged.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:61: if (!((character >= '0' && character <= '9') || 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:64: (character == '-' || character == '_'))) On 2016/09/09 18:27:11, Jamie wrote: > 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. Yep, I think we prefer braces when the 'if' condition or the body spans more than 1 line. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:71: extern "C" int Converse(int num_msg, const struct pam_message **msg, 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:89: if (message->msg[0] == '\0' || 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? https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:127: Env(Env&& other) = default; If you define a move-ctor, I think it's standard to provide a move-assignment-operator as well. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:142: char* new_entry = static_cast<char*>(malloc(var_len + 1 + val_len + 1)); 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:144: return; // Should never happen in practice, but don't crash if it does. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:170: operator char** () { 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). https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:187: friend class PamHandle; Why does PamHandle need special access? Maybe because of the char** env ctor above? You could provide that as a public ctor instead. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:205: operator bool() const { Same comment about overloading. Maybe call it IsInitialized()? https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:252: pam_handle_t* pamh_ = nullptr; Name this pam_handle_. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:253: int last_retcode_ = PAM_SUCCESS; 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.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:61: if (!((character >= '0' && character <= '9') || On 2016/09/09 22:48:58, Lambros wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:71: extern "C" int Converse(int num_msg, const struct pam_message **msg, On 2016/09/09 22:48:58, Lambros wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:89: if (message->msg[0] == '\0' || On 2016/09/09 22:48:58, Lambros wrote: > 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? > 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:127: Env(Env&& other) = default; On 2016/09/09 22:48:58, Lambros wrote: > If you define a move-ctor, I think it's standard to provide a > move-assignment-operator as well. > 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:142: char* new_entry = static_cast<char*>(malloc(var_len + 1 + val_len + 1)); On 2016/09/09 22:48:58, Lambros wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:144: return; // Should never happen in practice, but don't crash if it does. On 2016/09/09 22:48:58, Lambros wrote: > 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. I'm using malloc/free because PAM gives us malloc'd strings, and I didn't want to reallocate all of them. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:170: operator char** () { On 2016/09/09 22:48:58, Lambros wrote: > 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). Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:187: friend class PamHandle; On 2016/09/09 22:48:58, Lambros wrote: > Why does PamHandle need special access? Maybe because of the char** env ctor > above? You could provide that as a public ctor instead. 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.) https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:205: operator bool() const { On 2016/09/09 22:48:58, Lambros wrote: > Same comment about overloading. Maybe call it IsInitialized()? Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:252: pam_handle_t* pamh_ = nullptr; On 2016/09/09 22:48:58, Lambros wrote: > Name this pam_handle_. Acknowledged. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:253: int last_retcode_ = PAM_SUCCESS; On 2016/09/09 22:48:58, Lambros wrote: > 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. Acknowledged.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:49: " --me2me-script=<script> - Location of the me2me python script " Do we need this parameter? Can this wrapper discover the daemon script location same way the script finds the host binary? https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) On 2016/09/09 21:24:31, rkjnsn wrote: > On 2016/09/09 18:27:12, Jamie wrote: > > 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. > > 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. Why do we need to support any shell other than bash? > > 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:369: execve(pwinfo->pw_shell, const_cast<char * const *>(args), *env); 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 https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:369: execve(pwinfo->pw_shell, const_cast<char * const *>(args), *env); 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.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) On 2016/09/10 00:23:16, Sergey Ulanov wrote: > On 2016/09/09 21:24:31, rkjnsn wrote: > > On 2016/09/09 18:27:12, Jamie wrote: > > > 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. > > > > 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. > > Why do we need to support any shell other than bash? > > > > > > 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. > 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:369: execve(pwinfo->pw_shell, const_cast<char * const *>(args), *env); On 2016/09/10 00:23:16, Sergey Ulanov wrote: > 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. I'll take a look at LaunchProcess. See above for why we are executing through the user's login shell.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:49: " --me2me-script=<script> - Location of the me2me python script " On 2016/09/10 00:23:16, Sergey Ulanov wrote: > Do we need this parameter? Can this wrapper discover the daemon script location > same way the script finds the host binary? 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.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) On 2016/09/09 21:24:31, rkjnsn wrote: > On 2016/09/09 18:27:12, Jamie wrote: > > 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. > > 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. 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.
https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn File remoting/host/BUILD.gn (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/BUILD.gn#... remoting/host/BUILD.gn:1364: executable("remoting_pam_session") { On 2016/09/09 21:24:30, rkjnsn wrote: > On 2016/09/09 18:27:11, Jamie wrote: > > I think it would be better to name this according to what it does, not how > it's > > implemented. If it's something we expect primarily to be useful for starting > the > > host at boot, then perhaps something like remoting_boot_launcher; or since > it's > > primarily responsible for setting up the environment (in the generic, not the > > Posix sense of the word), remoting_env_setup might be appropriate. > > remoting_user_session? I think the essence of what it does is start the me2me > script as the target user, doing whatever it needs to do to initialize a session > for that user. SGTM. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... File remoting/host/linux/remoting_pam_session.cc (right): https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:60: // so don't escape any alphanumerics. (Also cuts down on verbosity.) On 2016/09/12 19:33:05, rkjnsn wrote: > On 2016/09/09 21:24:31, rkjnsn wrote: > > On 2016/09/09 18:27:12, Jamie wrote: > > > 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. > > > > 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. > > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:84: std::fputs("Unsupported request for user input from PAM\n", stderr); On 2016/09/09 21:24:31, rkjnsn wrote: > On 2016/09/09 18:27:11, Jamie wrote: > > LOG(ERROR) would be more appropriate here, unless you're specifically trying > to > > avoid pulling in more libraries than we need. > > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:138: void SetVariable(const char* variable, const char* value, bool overwrite) { On 2016/09/09 21:24:31, rkjnsn wrote: > On 2016/09/09 18:27:11, Jamie wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2323153002/diff/20001/remoting/host/linux/rem... remoting/host/linux/remoting_pam_session.cc:167: env_.insert(env_.end() - 1, new_entry); // Insert before terminating null On 2016/09/09 21:24:31, rkjnsn wrote: > On 2016/09/09 18:27:11, Jamie wrote: > > push_back() would be clearer. > > push_back would insert the entry after the terminating NULL. We need to insert > it before the NULL (hence `- 1`). Ack.
Okay, after getting a change I needed to LaunchProcess committed, I have now migrated this CL to use it. It almost doubles the binary size of the wrapper, but considering how small the wrapper is compared to the rest of the host, that's probably okay. If this looks good, I'll go ahead and make the rest of the changes I mentioned in my initial comment.
Okay, after getting a change I needed to LaunchProcess committed, I have now migrated this CL to use it. It almost doubles the binary size of the wrapper, but considering how small the wrapper is compared to the rest of the host, that's probably okay. If this looks good, I'll go ahead and make the rest of the changes I mentioned in my initial comment.
This looks a lot nicer! Basically LG, though I haven't gone over it in detail.
Here's the integrated code for option A, where we only use the wrapper at boot and keep the on-demand logic the same. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:1247: parser.add_option("", "--keep-parent-env", dest="keep_env", default=False, Should this flag be public and documented? https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:307: fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid); If root manually runs the wrapper with the --foreground option and redirects stdout to a file, this will change the owner of that file to the target user, which may be unexpected. Alternatively, I could pass in whether we daemonized, and only call fchown if we did. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:366: int OpenLogFile() { The Python script provides two ways to customize the log file. First, the log file can be set explicitly by setting CHROME_REMOTE_DESKTOP_LOG_FILE to the full path that should be used. Second, if CHROME_REMOTE_DESKTOP_LOG_FILE isn't specified, Python's temporary file logic will check the TMPDIR, TEMP, and TMP environment variables (in that order) to determine what directory to use. Because we daemonize before opening the user session, any user-specified values for these variables wouldn't have been set, yet. Currently, I just hardcode to using /tmp with the same template that the Python script uses by default. I could still check TMPDIR, TEMP, and TMP in case they are set system-wide. Alternatively, we could defer creation of a log file until we start the python script in the user session, but any error messages that occur before then would be lost (or have to be stored somewhere else). https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:431: // Done! I could print the name of the log file here like the Python script does, but it would probably only be seen if the wrapper were being run manually.
https://codereview.chromium.org/2323153002/diff/60001/remoting/host/pam_autho... File remoting/host/pam_authorization_factory_posix.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/pam_autho... remoting/host/pam_authorization_factory_posix.cc:125: HOST_LOG << "pam_start: " << pam_strerror(handle, result); Oops. These were for debugging. They'll be removed in future revisions.
LGTM with nits, but given the size of this CL, how critical it is to the correct functioning of the Linux host, and how long it's been since I did anything PAM, please wait for a second LGTM from either Sergey or Lambros. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:383: # PAM-authenticated session. 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. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:1247: parser.add_option("", "--keep-parent-env", dest="keep_env", default=False, On 2016/11/15 22:52:57, rkjnsn wrote: > Should this flag be public and documented? TBH, I don't see the point of having separate public and secret arguments. The whole script is not intended to be used directly. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:122: LOG(WARNING) << "[PAM] " << (message->msg ? message->msg : ""); Add a comment about why we're not using LOG(ERROR) here? https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:212: for (char ** variable = environment; *variable != nullptr; ++ variable) { Nit: No space before ** https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:220: std::free(environment); I'm surprised to see a std:: prefix on these and other standard functions. Certainly for strchr, it is not commonly used (free is harder to search for given how generic the name is). If this is the recommended style then it's fine, but if not I would remove it. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:231: void CheckReturnCode(int return_code) { Add an error string to this so it's clear from the logs what failed. You can just pass in the method name--no need for anything very verbose. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:250: // because it should take place after setuid. Is this to ensure that the correct permissions are enforced? If so then I think it's worth adding that. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:269: No newline here, please. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:307: fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid); On 2016/11/15 22:52:57, rkjnsn wrote: > If root manually runs the wrapper with the --foreground option and redirects > stdout to a file, this will change the owner of that file to the target user, > which may be unexpected. Alternatively, I could pass in whether we daemonized, > and only call fchown if we did. I think this is fine as-is. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:384: void Daemonize() { It's a bit odd to have the method declaration in the middle of this comment. I would add a blank comment line between the short summary and the long description and move it all above the method name.
lgtm https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:414: if "LD_LIBRARY_PATH" in self.child_env: Appreciate the bug-fix :) I agree this should normally be a separate CL, but no need to change it now. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:1247: parser.add_option("", "--keep-parent-env", dest="keep_env", default=False, On 2016/11/16 01:27:27, Jamie wrote: > On 2016/11/15 22:52:57, rkjnsn wrote: > > Should this flag be public and documented? > > TBH, I don't see the point of having separate public and secret arguments. The > whole script is not intended to be used directly. 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. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:137: if (response_list == nullptr) I think, if you call base::EnableTerminationOnOutOfMemory(), this will override malloc/calloc and make them crash the process if they can't allocate memory. So you won't need these checks. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:212: for (char ** variable = environment; *variable != nullptr; ++ variable) { On 2016/11/16 01:27:27, Jamie wrote: > Nit: No space before ** .. or after ++ https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:220: std::free(environment); On 2016/11/16 01:27:27, Jamie wrote: > I'm surprised to see a std:: prefix on these and other standard functions. > Certainly for strchr, it is not commonly used (free is harder to search for > given how generic the name is). If this is the recommended style then it's fine, > but if not I would remove it. If you keep std:: prefix, apply it to 'calloc' (line 135) as well. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:366: int OpenLogFile() { On 2016/11/15 22:52:57, rkjnsn wrote: > The Python script provides two ways to customize the log file. First, the log > file can be set explicitly by setting CHROME_REMOTE_DESKTOP_LOG_FILE to the full > path that should be used. Second, if CHROME_REMOTE_DESKTOP_LOG_FILE isn't > specified, Python's temporary file logic will check the TMPDIR, TEMP, and TMP > environment variables (in that order) to determine what directory to use. > > Because we daemonize before opening the user session, any user-specified values > for these variables wouldn't have been set, yet. Currently, I just hardcode to > using /tmp with the same template that the Python script uses by default. I > could still check TMPDIR, TEMP, and TMP in case they are set system-wide. > > Alternatively, we could defer creation of a log file until we start the python > script in the user session, but any error messages that occur before then would > be lost (or have to be stored somewhere else). We could let the script manage the log file (and any log-rotation) itself. Any logs generated in this wrapper should really go into syslog() instead. syslog() is not suited to the script because of the massive output, but it can be used here. Does Chrome have a way to direct LOG()s to syslog() ? Doesn't matter for now, can be a separate CL. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:431: // Done! On 2016/11/15 22:52:57, rkjnsn wrote: > I could print the name of the log file here like the Python script does, but it > would probably only be seen if the wrapper were being run manually. We shouldn't pollute the init.d system output. Logging to syslog() or /var/log means we don't need to print anything.
See various replies below. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:383: # PAM-authenticated session. On 2016/11/16 01:27:27, Jamie wrote: > 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. 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. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:122: LOG(WARNING) << "[PAM] " << (message->msg ? message->msg : ""); On 2016/11/16 01:27:27, Jamie wrote: > Add a comment about why we're not using LOG(ERROR) here? So, my thought was that such messages from PAM don't necessarily mean that authentication will fail. For example, the module might be optional, or there might be a fallback option in the PAM configuration. We won't know if it's actually a problem until we get the final return code from PAM (where we consider everything other than PAM_SUCCESS to be a fatal error). That said, it's probably a but subjective. I usually consider ERROR to mean the current operation can't continue, which isn't necessarily the case here. On the other hand, pam_authorization_factory_posix.cc uses LOG(ERROR). I'll add a comment explaining my reasoning, but let me know if you'd prefer me to change it to ERROR. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:137: if (response_list == nullptr) On 2016/11/17 21:19:35, Lambros wrote: > I think, if you call base::EnableTerminationOnOutOfMemory(), this will override > malloc/calloc and make them crash the process if they can't allocate memory. So > you won't need these checks. Since this is the only manual allocation we do and failure is not necessarily fatal, I'd personally prefer to leave it as is. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:212: for (char ** variable = environment; *variable != nullptr; ++ variable) { On 2016/11/17 21:19:35, Lambros wrote: > On 2016/11/16 01:27:27, Jamie wrote: > > Nit: No space before ** > > .. or after ++ Acknowledged. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:220: std::free(environment); On 2016/11/16 01:27:27, Jamie wrote: > I'm surprised to see a std:: prefix on these and other standard functions. > Certainly for strchr, it is not commonly used (free is harder to search for > given how generic the name is). If this is the recommended style then it's fine, > but if not I would remove it. On C++ projects I've worked on in the past, using the C++ version of standard headers was preferred to the C version (that is `#include <cstdlib>` was preferred to `#include <stdlib.h>`). These C++ headers put the functions in the `std` namespace, and so the prefix is needed. I don't see a stated preference between equivalent C++ and C headers in the style guide, so I can switch to the C version if you'd prefer. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:231: void CheckReturnCode(int return_code) { On 2016/11/16 01:27:27, Jamie wrote: > Add an error string to this so it's clear from the logs what failed. You can > just pass in the method name--no need for anything very verbose. Acknowledged. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:250: // because it should take place after setuid. On 2016/11/16 01:27:27, Jamie wrote: > Is this to ensure that the correct permissions are enforced? If so then I think > it's worth adding that. Partially for permissions, and partially because chdir can apparently fail for some configurations using, e.g., NFS home directories if the uid isn't correct. I'll add a note. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:269: On 2016/11/16 01:27:27, Jamie wrote: > No newline here, please. I added a newline to try to indicate that this comment is for the whole region of code (lines 268-304), not just the following line. What's the preferred way to express that? https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:366: int OpenLogFile() { On 2016/11/17 21:19:35, Lambros wrote: > On 2016/11/15 22:52:57, rkjnsn wrote: > > The Python script provides two ways to customize the log file. First, the log > > file can be set explicitly by setting CHROME_REMOTE_DESKTOP_LOG_FILE to the > full > > path that should be used. Second, if CHROME_REMOTE_DESKTOP_LOG_FILE isn't > > specified, Python's temporary file logic will check the TMPDIR, TEMP, and TMP > > environment variables (in that order) to determine what directory to use. > > > > Because we daemonize before opening the user session, any user-specified > values > > for these variables wouldn't have been set, yet. Currently, I just hardcode to > > using /tmp with the same template that the Python script uses by default. I > > could still check TMPDIR, TEMP, and TMP in case they are set system-wide. > > > > Alternatively, we could defer creation of a log file until we start the python > > script in the user session, but any error messages that occur before then > would > > be lost (or have to be stored somewhere else). > > We could let the script manage the log file (and any log-rotation) itself. Any > logs generated in this wrapper should really go into syslog() instead. syslog() > is not suited to the script because of the massive output, but it can be used > here. Does Chrome have a way to direct LOG()s to syslog() ? > Doesn't matter for now, can be a separate CL. We would need some kind of "don't daemonize but still redirect stdio" option for the Python script, but it's certainly doable. https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:384: void Daemonize() { On 2016/11/16 01:27:27, Jamie wrote: > It's a bit odd to have the method declaration in the middle of this comment. I > would add a blank comment line between the short summary and the long > description and move it all above the method name. In my head, these were two distinct comments. The comment above was the "this is what this function does comment", while the comment below is describing the implementation details (similarly to the docstring/comment distinction in the Python file. That said, I should probably at least add some more details to the description above.
https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/60001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:269: On 2016/11/18 22:46:49, rkjnsn wrote: > On 2016/11/16 01:27:27, Jamie wrote: > > No newline here, please. > > I added a newline to try to indicate that this comment is for the whole region > of code (lines 268-304), not just the following line. What's the preferred way > to express that? In some cases we've used a multi-line comment. If it's possible, you could also consider refactoring it into a separate method, but the interdependencies between the blocks of code might make that infeasible.
Addressed most of the feedback and make a couple tweaks. https://codereview.chromium.org/2323153002/diff/80001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/80001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:21: #include <ctime> I left these as the C++ headers with namespacing. While it looks like C style headers are a bit more common, both are used in different places in remoting/, and I like the C++ style a bit better. I can still change them if anyone has a strong opinion. https://codereview.chromium.org/2323153002/diff/80001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:319: if (chown_log) { I ended up adding the guard, because it turns out if this isn't guarded it will change the owner of the terminal device when run with --foreground. Oops!
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks like it needs a rebase.
On 2016/11/22 20:09:42, rkjnsn wrote: > Looks like it needs a rebase. And done, but see comments on Patch Set 4.
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rkjnsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org, jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2323153002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479915428133750,
"parent_rev": "523a5a2004cbe109913f9a36e69d9e528656cbbf", "commit_rev":
"80f18782444ee2c22e180148b7d4f91368fb4307"}
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 ========== to ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 Committed: https://crrev.com/ac6cd2ac8ea912ee4dc0c229f79b04874aee9cc3 Cr-Commit-Position: refs/heads/master@{#434172} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac6cd2ac8ea912ee4dc0c229f79b04874aee9cc3 Cr-Commit-Position: refs/heads/master@{#434172}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2527713002/ by lukasza@chromium.org. The reason for reverting is: This CL seems to have broken the build - see https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/p... : [4004/46998] CXX obj/remoting/host/linux/remoting_user_session/remoting_user_session.o FAILED: obj/remoting/host/linux/remoting_user_session/remoting_user_session.o ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/remoting/host/linux/remoting_user_session/remoting_user_session.o.d -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PDF=1 -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DUSE_PROPRIETARY_CODECS -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DOFFICIAL_BUILD -DGOOGLE_CHROME_BUILD -DENABLE_MEDIA_ROUTER=1 -DCR_CLANG_REVISION=287685-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNO_UNWIND_TABLES -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DCFI_ENFORCEMENT -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -I../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0 -I../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fno-unwind-tables -fno-asynchronous-unwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -fdebug-prefix-map=/b/build/slave/precise64_trunk/build/src=. -flto -fwhole-program-vtables -pthread -m64 -march=x86-64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -O2 -fno-ident -fdata-sections -ffunction-sections -g2 --sysroot=../../build/linux/debian_wheezy_amd64-sysroot -fsanitize=cfi-vcall -fsanitize-blacklist=../../tools/cfi/blacklist.txt -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti -fno-exceptions -c ../../remoting/host/linux/remoting_user_session.cc -o obj/remoting/host/linux/remoting_user_session/remoting_user_session.o ../../remoting/host/linux/remoting_user_session.cc:320:5: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result] fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid); ^~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated..
Message was sent while issue was closed.
Description was changed from ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 Committed: https://crrev.com/ac6cd2ac8ea912ee4dc0c229f79b04874aee9cc3 Cr-Commit-Position: refs/heads/master@{#434172} ========== to ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 ==========
(Hopefully) fix unused result error. https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... remoting/host/linux/remoting_user_session.cc:379: ignore_result(pam_handle.SetCredentials(PAM_DELETE_CRED)); ignore_result isn't needed here, but I thought it might make sense to add it for code documentation purposes. Thoughts?
LGTM when my comments are addressed. https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... remoting/host/linux/remoting_user_session.cc:319: PLOG_IF(WARNING, fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid) != 0) LOG statements shouldn't have side-effects. This is essential for debug logs, and is generally good practise anyway. Assign the result to a variable and test that instead. https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... remoting/host/linux/remoting_user_session.cc:379: ignore_result(pam_handle.SetCredentials(PAM_DELETE_CRED)); On 2016/11/23 20:40:36, rkjnsn wrote: > ignore_result isn't needed here, but I thought it might make sense to add it for > code documentation purposes. Thoughts? Why is it not needed? Is fchown annotated as having an important return value and this not? I think it's fine to be explicit about the fact that we don't care, although either a comment or using ignore_result is fine.
https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... remoting/host/linux/remoting_user_session.cc:319: PLOG_IF(WARNING, fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid) != 0) On 2016/11/23 21:00:34, Jamie wrote: > LOG statements shouldn't have side-effects. This is essential for debug logs, > and is generally good practise anyway. Assign the result to a variable and test > that instead. Acknowledged. https://codereview.chromium.org/2323153002/diff/120001/remoting/host/linux/re... remoting/host/linux/remoting_user_session.cc:379: ignore_result(pam_handle.SetCredentials(PAM_DELETE_CRED)); On 2016/11/23 21:00:34, Jamie wrote: > On 2016/11/23 20:40:36, rkjnsn wrote: > > ignore_result isn't needed here, but I thought it might make sense to add it > for > > code documentation purposes. Thoughts? > > Why is it not needed? Is fchown annotated as having an important return value > and this not? I think it's fine to be explicit about the fact that we don't > care, although either a comment or using ignore_result is fine. Right. On the buildbot in question, the return value of fchown is apparently explicitly marked as must-use.
lgtm
The CQ bit was checked by rkjnsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2323153002/#ps140001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rkjnsn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rkjnsn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480441630638290,
"parent_rev": "0cb9adfcaa8d0980a94e5bb2a9a855c1e2aaa483", "commit_rev":
"02c487eec17b1750106048c698fc88f8323005ef"}
Message was sent while issue was closed.
Description was changed from ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 ========== to ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 ========== to ========== Add PAM session wrapper Adds a wrapper to launch me2me script in a proper PAM session. BUG=616962 Committed: https://crrev.com/cbef66778a13c926d0518b269928a288777cbbfc Cr-Commit-Position: refs/heads/master@{#435044} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cbef66778a13c926d0518b269928a288777cbbfc Cr-Commit-Position: refs/heads/master@{#435044} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
