Preference Pane for chromoting on Mac

remoting/host/me2me_preference_pane.mm

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import <Cocoa/Cocoa.h>
+#include <launch.h>
+#import <PreferencePanes/PreferencePanes.h>
+#import <SecurityInterface/SFAuthorizationView.h>
+
+#include "base/eintr_wrapper.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/mac/authorization_util.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/launchd.h"
+#include "base/mac/mac_logging.h"
+#include "base/mac/scoped_launch_data.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/stringprintf.h"
+#include "base/sys_string_conversions.h"
+#include "remoting/host/host_config.h"
+#include "remoting/host/json_host_config.h"
+#include "remoting/protocol/me2me_host_authenticator_factory.h"
+
+namespace {
+// The name of the Remoting Host service that is registered with launchd.
+#define kServiceName "org.chromium.chromoting"
+#define kConfigDir "/Library/PrivilegedHelperTools/"
+
+// This helper script is executed as root.  It is passed a command-line option
+// (--enable or --disable), which causes it to create or remove a trigger file.
+// The trigger file (defined in the service's plist file) informs launchd
+// whether the Host service should be running.  Creating the trigger file causes
+// launchd to immediately start the service.  Deleting the trigger file has no
+// immediate effect, but it prevents the service from being restarted if it
+// becomes stopped.
+const char kHelperTool[] = kConfigDir kServiceName ".me2me.sh";

[garykac, 2012/04/27 17:14:09]

At some point, it would be nice to have all these filenames/paths defined in one
place for all of the host tools to use.

[Lambros, 2012/04/28 01:54:19]

Agreed, though I'd prefer to wait until this PrefPane is well-established first
:)

+
+bool GetTemporaryConfigFilePath(FilePath* path) {
+  if (!file_util::GetTempDir(path))
+    return false;
+  *path = path->Append(kServiceName ".json");
+  return true;
+}

[Jamie, 2012/04/27 18:21:04]

I don't think this is a problem, because we present the contents to the user to
confirm before writing the config, but make sure you've considered the
implications of using a predictable file-name for this.

[Lambros, 2012/04/28 01:54:19]

Mac OS temp directories are private per-user, so we should be OK.

+
+bool IsPinValid(const std::string& pin, const std::string& host_id,
+                const std::string& host_secret_hash) {
+  remoting::protocol::SharedSecretHash hash;
+  if (!hash.Parse(host_secret_hash)) {
+    NSLog(@"Invalid host_secret_hash.");
+    return false;
+  }
+  std::string result =
+      remoting::protocol::AuthenticationMethod::ApplyHashFunction(
+          hash.hash_function, host_id, pin);
+  return result == hash.value;
+}
+
+}  // namespace
+
+@interface Me2MePreferencePane : NSPreferencePane {
+  IBOutlet NSTextField* status_message_;
+  IBOutlet NSButton* disable_button_;
+  IBOutlet NSTextField* pin_instruction_message_;
+  IBOutlet NSTextField* email_;
+  IBOutlet NSTextField* pin_;
+  IBOutlet NSButton* apply_button_;
+  IBOutlet SFAuthorizationView* authorization_view_;

[Jamie, 2012/04/27 18:21:04]

This is probably my ObjC ignorance, but why no @synthesize for these?

[dcaiafa, 2012/04/27 19:25:00]

Because these are instance variables, not properties. You could create
properties (properties are just syntactic sugar for accessors) for these fields,
but there is no reason to for these. 

On 2012/04/27 18:21:04, Jamie wrote:

+
+  // Holds the new proposed configuration if a temporary config file is
+  // present.
+  scoped_ptr<remoting::JsonHostConfig> config_;
+
+  NSTimer* timer_;
+
+  // Flags to control the UI state.  These are computed in the update...Status
+  // methods.  The updateUI method updates all the controls according to the
+  // values of these flags and the data stored in config_.
+  BOOL is_service_running_;
+  BOOL is_pane_unlocked_;
+  // True if a new proposed config file has been created.
+  BOOL have_new_config_;
+}
+
+- (void)mainViewDidLoad;
+- (void)willSelect;
+- (void)willUnselect;
+- (IBAction)onDisable:(id)sender;
+- (IBAction)onApply:(id)sender;
+- (void)onNewConfigFile:(NSNotification *)notification;

[Jamie, 2012/04/27 18:21:04]

Nit: No space before *, here and elsewhere.

[Lambros, 2012/04/28 01:54:19]

Done.

+- (void)onTimer:(NSTimer*)timer;

[Jamie, 2012/04/27 18:21:04]

onTimer is a rather generic name. Can you name it after what it does, rather
than how it's triggered?

[Lambros, 2012/04/28 01:54:19]

Done.

+- (void)authorizationViewDidAuthorize:(SFAuthorizationView *)view;
+- (void)authorizationViewDidDeauthorize:(SFAuthorizationView *)view;
+- (void)updateServiceStatus;
+- (void)updateAuthorizationStatus;
+- (void)updateConfigStatus;
+- (void)updateUI;
+
+// Save the new config to the system, and either start the service or inform
+// the currently-running service of the new config.
+- (void)applyNewServiceConfig;
+
+- (BOOL)runHelperAsRootWithCommand:(const char*)command
+                         InputData:(const std::string&)input_data;

[Jamie, 2012/04/27 18:21:04]

Nit: s/InputData/inputData/

[Lambros, 2012/04/28 01:54:19]

Done.

+@end
+
+@implementation Me2MePreferencePane
+
+- (void)mainViewDidLoad {
+  [authorization_view_ setDelegate:self];
+  [authorization_view_ setString:kAuthorizationRightExecute];
+  [authorization_view_ setAutoupdate:YES];
+}
+
+- (void)willSelect {
+  have_new_config_ = NO;
+
+  NSDistributedNotificationCenter* center =
+      [NSDistributedNotificationCenter defaultCenter];
+  [center addObserver:self

[dcaiafa, 2012/04/27 17:06:00]

You have to release this observer.

[Lambros, 2012/04/28 01:54:19]

Done.

+             selector:@selector(onNewConfigFile:)
+                 name:@kServiceName
+               object:nil];
+
+  timer_ = [[NSTimer scheduledTimerWithTimeInterval:2.0
+                                             target:self
+                                           selector:@selector(onTimer:)
+                                           userInfo:nil
+                                            repeats:YES] retain];
+  [self updateServiceStatus];
+  [self updateAuthorizationStatus];
+  [self updateConfigStatus];

[Jamie, 2012/04/27 18:21:04]

Why do this here? Don't you only expect a new config when the notification is
received, or is this to cope with the not-already-running case?

[Lambros, 2012/04/28 01:54:19]

Yes, it's to cope with the not-already-running case.  The NPAPI plugin launches
the pref-pane and immediately broadcasts an NSNotification.  But the launch API
is async - the plugin has no way of knowing when the pref-pane has finished
launching.

With this scheme, the pref-pane never misses a notification.  Either it starts
running (in which case, willSelect is triggered), or it is already running (in
which case, it will get the broadcast NSNotification from the plugin).

+  [self updateUI];
+}
+
+- (void)willUnselect {
+  [timer_ invalidate];
+  [timer_ release];

[aharper, 2012/04/30 21:23:32]

nil timer_

[Lambros, 2012/05/02 19:05:28]

Done.

+}
+
+- (void)onApply:(id)sender {
+  if (!have_new_config_) {
+    // It shouldn't be possible to hit the button if there is no config to
+    // apply, but check anyway just in case it happens somehow.
+    return;
+  }
+
+  // Ensure the authorization token is up-to-date before using it.
+  [self updateAuthorizationStatus];
+  [self updateUI];
+
+  std::string pin = base::SysNSStringToUTF8([pin_ stringValue]);
+  std::string host_id, host_secret_hash;
+  if (!config_->GetString(remoting::kHostIdConfigPath, &host_id) ||
+      !config_->GetString(remoting::kHostSecretHashConfigPath,
+                          &host_secret_hash)) {
+    LOG(ERROR) << "Failed to get config data for PIN verification";
+    return;

[aharper, 2012/04/30 21:23:32]

Should this be an error alert?

[Lambros, 2012/05/02 19:05:28]

Done.

+  }
+  if (!IsPinValid(pin, host_id, host_secret_hash)) {

[aharper, 2012/04/30 21:23:32]

I'm not seeing how this validation ties the email address in the prefpane to the
host information in the hash. Is the only link that they came from the same
config you read originally?

[Lambros, 2012/05/02 19:05:28]

Almost.  The host registration was done using the OAuth credentials in the
config file, which are tied to the e-mail address shown in the prefpane.

[Wez, 2012/05/03 00:13:44]

Alex's point is that we're displaying the email address and prompting for the
PIN, while the email address is tied to the OAuth credentials - so there is
nothing tying the email address, and the rest of the configuration to the PIN,
as such.  It's a fair point.

We should take a hash of the stringified configuration dictionary, combined with
the PIN, and verify that rather than the host secret hash, so that the entire
configuration, including the email address, is protected by the PIN.

+    NSAlert* alert = [[NSAlert alloc] init];
+    [alert setMessageText:@"Incorrect PIN entered"];
+    [alert setAlertStyle:NSWarningAlertStyle];
+    [alert runModal];

[aharper, 2012/04/30 21:23:32]

This runs application modal, which is probably inappropirate. Use
beginSheetModalForWindow:modalDelegate:didEndSelector:contextInfo: so you are a
sheet on system preferences

[Lambros, 2012/05/02 19:05:28]

Done.

+    [alert release];
+    return;
+  }
+
+  [self applyNewServiceConfig];
+  [self updateUI];
+}
+
+- (void)onDisable:(id)sender {
+  // Ensure the authorization token is up-to-date before using it.
+  [self updateAuthorizationStatus];
+  [self updateUI];
+
+  if (![self runHelperAsRootWithCommand:"--disable"
+                              InputData:""]) {
+    NSLog(@"Failed to run the helper tool");

[aharper, 2012/04/30 21:23:32]

Should this be an error alert?

[Lambros, 2012/05/02 19:05:28]

Done.

+  }
+
+  // Stop the launchd job.  This cannot easily be done by the helper tool,
+  // since the launchd job runs in the current user's context.
+  base::mac::ScopedLaunchData response(
+      base::mac::MessageForJob(kServiceName, LAUNCH_KEY_STOPJOB));
+  if (!response) {
+    LOG(ERROR) << "Failed to send message to launchd";

[Jamie, 2012/04/27 18:21:04]

Is logging an error sufficient here (and similarly elsewhere)? Shouldn't we
notify the user?

[Lambros, 2012/04/28 01:54:19]

I expect this particular error would hardly ever happen, but it's a fair point -
maybe we should have some generic way of telling the user something went wrong
(without having to localize a zillion different messages)?

Added a simple "showError" method that currently pops up an alert box.

+    return;
+  }
+
+  // Got a response, so check if launchd sent a non-zero error code, otherwise
+  // assume the command was successful.

[Jamie, 2012/04/27 18:21:04]

The word "assume" makes me nervous. Why do we need to assume anything if we have
a successful return code? What else could go wrong, and is there no way we could
detect it and return an error code?

[Lambros, 2012/04/28 01:54:19]

The thing that could go wrong (theoretically) is, getting a response that isn't
even an error-code.  AFAIK, there is no documented API contract saying: "launchd
will always send, on success, a response of type DATA_ERRNO with errno == 0".

But I take the point that anything unexpected should be treated as a potential
error, so I've updated the logic here.

+  if (launch_data_get_type(response.get()) == LAUNCH_DATA_ERRNO) {
+    int error = launch_data_get_errno(response.get());
+    if (error) {
+      LOG(ERROR) << "launchd returned error " << error;
+    }
+  }
+}
+
+- (void)onNewConfigFile:(NSNotification *)notification {
+  [self updateConfigStatus];
+  [self updateUI];
+}
+
+- (void)onTimer:(NSTimer*)timer {
+  BOOL was_running = is_service_running_;
+  [self updateServiceStatus];
+  if (was_running != is_service_running_)
+    [self updateUI];
+}
+
+- (void)authorizationViewDidAuthorize:(SFAuthorizationView *)view {
+  [self updateAuthorizationStatus];
+  [self updateUI];
+}
+
+- (void)authorizationViewDidDeauthorize:(SFAuthorizationView *)view {
+  [self updateAuthorizationStatus];
+  [self updateUI];
+}
+
+- (void)updateServiceStatus {
+  pid_t job_pid = base::mac::PIDForJob(kServiceName);
+  is_service_running_ = (job_pid > 0);
+}
+
+- (void)updateAuthorizationStatus {
+  is_pane_unlocked_ = [authorization_view_ updateStatus:authorization_view_];
+}
+
+- (void)updateConfigStatus {
+  FilePath file;
+  if (!GetTemporaryConfigFilePath(&file)) {
+    NSLog(@"Failed to get path of configuration data.");

[Jamie, 2012/04/27 18:21:04]

You're mixing LOG with NSLog; please stick to one or the other.

[Lambros, 2012/04/28 01:54:19]

Done.

+    return;
+  }
+  if (!file_util::PathExists(file)) {
+    return;

[Jamie, 2012/04/27 18:21:04]

No log here? If this is called even when there's no reason to believe there's a
new config, then that's probably correct; otherwise we should log an error.

[Lambros, 2012/04/28 01:54:19]

This gets called whenever the pref-pane is opened.  This could be from the
standard Mac OS "System Preferences" GUI, so there wouldn't be a config file in
that case.

+  }
+  config_.reset(new remoting::JsonHostConfig(file));
+  if (config_->Read()) {
+    have_new_config_ = YES;
+    file_util::Delete(file, false);
+  } else {
+    NSLog(@"Error reading configuration data from %s", file.value().c_str());
+    have_new_config_ = NO;

[Jamie, 2012/04/27 18:21:04]

This should be set at the top of the function, since this isn't the only path
that results in not having a new file.

[Lambros, 2012/04/28 01:54:19]

I've documented the intent of this function, clarifying that it's meant to read
any new config file, but not to forget any config data it previously read (I've
explained the reasoning in my comment).

I've also tweaked the implementation so that it never destroys a loaded config
(that is: I create a temporary new config object, try to read the file, then
"swap" it in only if successful).

+  }
+}
+
+- (void)updateUI {
+  if (is_service_running_) {
+    [status_message_ setStringValue:@"Me2Me is enabled"];
+  } else {
+    [status_message_ setStringValue:@"Me2Me is disabled"];
+  }
+
+  std::string email;
+  if (config_.get()) {
+    if (!config_->GetString(remoting::kXmppLoginConfigPath, &email)) {
+      NSLog(@"Failed to get config item '%s'", remoting::kXmppLoginConfigPath);
+    }
+  }
+  [email_ setStringValue:base::SysUTF8ToNSString(email)];
+
+  [disable_button_ setEnabled:(is_pane_unlocked_ && is_service_running_)];
+  [pin_instruction_message_ setEnabled:have_new_config_];
+  [email_ setEnabled:have_new_config_];
+  [pin_ setEnabled:have_new_config_];
+  [apply_button_ setEnabled:(is_pane_unlocked_ && have_new_config_)];
+}
+
+- (void)applyNewServiceConfig {
+  [self updateServiceStatus];
+  std::string serialized_config = config_->GetSerializedData();
+  const char* command = is_service_running_ ? "--save-config" : "--enable";

[Jamie, 2012/04/27 18:21:04]

It doesn't need to be part of this CL, but I think it would be better if we
always use --enable and make the script do the right thing if the service is
already running. That avoids the race condition.

[Lambros, 2012/04/28 01:54:19]

This would be nicer.  Unfortunately, I don't think the helper script can easily
get the PID of the running launchd agent, because the helper runs as root, and
the agent runs as the currently-logged-in user (so the agent PID would not
appear when the helper runs "launchctl list" or its C-API-equivalent).  I guess
I'd have to try it to know if this would work.

+  if (![self runHelperAsRootWithCommand:command
+                              InputData:serialized_config]) {
+    NSLog(@"Failed to run the helper tool");
+    return;
+  }
+  have_new_config_ = NO;

[Jamie, 2012/04/27 18:21:04]

Shouldn't we clear this earlier? If we get an error writing the config, we're
not going to try again, so we effectively don't have a new config.

[Lambros, 2012/04/28 01:54:19]

Well, the user could push the Apply button again :)  Are we really sure we don't
want the user to be able to retry this operation?

+
+  // If the service was previously running, send a signal to cause it to reload
+  // its configuration.

[Jamie, 2012/04/27 18:21:04]

This also feels like it would be better done by the script, but perhaps not in
this CL.

[Lambros, 2012/04/28 01:54:19]

Same problem as before (although if we could somehow get the PID, I'm pretty
sure we can manage to send a SIGHUP to it :)

+  if (is_service_running_) {
+    pid_t job_pid = base::mac::PIDForJob(kServiceName);
+    if (job_pid > 0) {
+      kill(job_pid, SIGHUP);
+    } else {
+      NSLog(@"Failed to obtain PID of service %s", kServiceName);
+    }
+  }
+}
+
+- (BOOL)runHelperAsRootWithCommand:(const char*)command
+                             InputData:(const std::string&)input_data {

[Jamie, 2012/04/27 18:21:04]

Nit: s/InputData/inputData/ and align colons.

[Lambros, 2012/04/28 01:54:19]

Done.

+  if (!file_util::VerifyPathControlledByAdmin(FilePath(kHelperTool))) {

[aharper, 2012/04/30 21:23:32]

This method name made me nervous, because the task it presents is basically
impossible to do properly in general. To confirm I've talked to the Chrome
folks, and this method isn’t actually intended for security use.

In your usage this is only used for /Library/PrivilegedHelperTools so I'd drop
this check.

[Lambros, 2012/05/02 19:05:28]

This check was added in response to a comment made by another reviewer:
http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon...

I'm happy to drop it if you still think it's more harm than good.  We aim to
replace this with a launchd helper tool as we discussed, so we won't need this
check anyway.

[Lambros, 2012/05/03 01:47:04]

Dropped, and added TODO comment.

+    LOG(ERROR) << "Security check failed for: " << kHelperTool;
+    return NO;
+  }
+
+  AuthorizationRef authorization =
+      [[authorization_view_ authorization] authorizationRef];
+  if (!authorization) {
+    LOG(ERROR) << "Failed to obtain authorizationRef";
+    return NO;
+  }
+
+  const char* arguments[] = { command, NULL };
+  FILE* pipe = NULL;
+  pid_t pid;
+  OSStatus status = base::mac::ExecuteWithPrivilegesAndGetPID(
+      authorization,
+      kHelperTool,
+      kAuthorizationFlagDefaults,
+      arguments,
+      &pipe,
+      &pid);
+  if (status != errAuthorizationSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "AuthorizationExecuteWithPrivileges";

[Jamie, 2012/04/27 18:21:04]

Yet another type of log? Any reason this needs to be different?

[Lambros, 2012/04/28 01:54:19]

I've ditched NSLog, so I think we're OK now.

+    return NO;
+  }
+  if (pid == -1) {
+    LOG(ERROR) << "Failed to get child PID";
+    return NO;
+  }
+
+  DCHECK(pipe);
+  if (!input_data.empty()) {
+    size_t bytes_written = fwrite(input_data.data(), sizeof(char),
+                                  input_data.size(), pipe);
+    // According to the fwrite manpage, a partial count is returned only if a
+    // write error has occurred.
+    if (bytes_written != input_data.size()) {
+      LOG(ERROR) << "Failed to write data to child process";

[Jamie, 2012/04/27 18:21:04]

Close and return NO?

[Lambros, 2012/04/28 01:54:19]

Sadly, no, because we still want to call waitpid to avoid a zombie.  I've
improved this, PTAL.

+    }
+    // Need to close, since the child waits for EOF on its stdin.
+    if (fclose(pipe) != 0) {

[dcaiafa, 2012/04/27 17:06:00]

Shouldn't you close the pipe even if input_data.empty()?

[Jamie, 2012/04/27 18:21:04]

+1

[Lambros, 2012/04/28 01:54:19]

Ooh, well spotted! That would leak the FILE* if not the file-descriptor as well.

Done, and expanded the comment.

+      PLOG(ERROR) << "fclose";

[Jamie, 2012/04/27 18:21:04]

Return NO?

[Lambros, 2012/04/28 01:54:19]

Done.

+    }
+  }
+
+  int exit_status;
+  pid_t wait_result = HANDLE_EINTR(waitpid(pid, &exit_status, 0));
+  if (wait_result != pid) {
+    PLOG(ERROR) << "waitpid";
+    return NO;
+  }
+  if (WIFEXITED(exit_status) && WEXITSTATUS(exit_status) == 0) {
+    return YES;
+  } else {
+    LOG(ERROR) << kHelperTool << " failed with exit status " << exit_status;
+    return NO;
+  }
+}
+
+@end