Implemented local login check for PAM (only enabled on Linux for now).

remoting/host/pam_authorization_factory_posix.cc

+// 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.
+
+#include "remoting/host/pam_authorization_factory_posix.h"
+
+#include <security/pam_appl.h>
+
+#include "base/environment.h"
+#include "base/logging.h"
+#include "remoting/protocol/channel_authenticator.h"

[Wez, 2012/10/26 03:25:43]

nit: Can't you just forward-define ChannelAuthenticator?

[Jamie, 2012/10/26 17:53:30]

No, it needs to be a complete type for scoped_ptr.

+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+namespace remoting {
+
+class PamAuthorizer : public protocol::Authenticator {

[Wez, 2012/10/26 03:25:43]

nit: Wrap this in an anonymous namespace.

[Wez, 2012/10/26 03:25:43]

nit: Consider moving this class' method implementations out-of-line; tends to
make classes easier to understand at a glance.

[Jamie, 2012/10/26 17:53:30]

Done.

[Jamie, 2012/10/26 17:53:30]

Done.

+ public:
+  PamAuthorizer(scoped_ptr<protocol::Authenticator> underlying)
+      : underlying_(underlying.Pass()),
+        local_login_status_(NOT_CHECKED) {
+  }
+
+  virtual State state() const OVERRIDE {

[Wez, 2012/10/26 03:25:43]

nit: Precede this with a comment "protocol::Authenticator implementation"

[Jamie, 2012/10/26 17:53:30]

s/implementation/interface done.

+    return local_login_status_ == DISALLOWED ?
+        REJECTED : underlying_->state();

[Wez, 2012/10/26 03:25:43]

nit: This would be more readable as an early-exit if and return.

[Jamie, 2012/10/26 17:53:30]

Done.

+  }
+
+  virtual RejectionReason rejection_reason() const OVERRIDE {
+    return local_login_status_ == DISALLOWED ?
+        INVALID_CREDENTIALS : underlying_->rejection_reason();

[Jamie, 2012/10/25 23:16:03]

This results in "invalid PIN" at the client, which is not very helpful. We might
need a new error code to support this check.

[Wez, 2012/10/26 03:25:43]

Agreed, but this is good enough for starters.

[Wez, 2012/10/26 03:25:43]

nit: See comment on state().

[Jamie, 2012/10/26 17:53:30]

Done.

+  }
+
+  virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE {
+    underlying_->ProcessMessage(message);
+    MaybeCheckLocalLogin();
+  }
+
+  virtual scoped_ptr<buzz::XmlElement> GetNextMessage() {
+    scoped_ptr<buzz::XmlElement> result (underlying_->GetNextMessage());
+    MaybeCheckLocalLogin();
+    return result.Pass();
+  }
+
+  virtual scoped_ptr<protocol::ChannelAuthenticator>
+      CreateChannelAuthenticator() const OVERRIDE {
+    return underlying_->CreateChannelAuthenticator();
+  }
+
+ private:
+  void MaybeCheckLocalLogin() {
+    if (local_login_status_ == NOT_CHECKED && state() == ACCEPTED) {
+      local_login_status_ = IsLocalLoginAllowed() ? ALLOWED : DISALLOWED;
+    }
+  }
+
+  bool IsLocalLoginAllowed() {
+    std::string username;
+    if (!base::Environment::Create()->GetVar("USER", &username)) {
+      return false;
+    }
+    struct pam_conv conv = { PamConversation, NULL };

[Wez, 2012/10/26 03:25:43]

nit: Blank line above this, to aid readability, plz.

[Jamie, 2012/10/26 17:53:30]

Done.

+    pam_handle_t* handle = NULL;
+    int result = pam_start("chrome-remote-desktop", username.c_str(),
+                           &conv, &handle);
+    if (result == PAM_SUCCESS) {
+      result = pam_acct_mgmt(handle, 0);
+    }
+    pam_end(handle, result);
+    LOG(INFO) << "Local login check for " << username

[Wez, 2012/10/26 03:25:43]

nit: Blank lines above & below LOG line, for readability.

[Jamie, 2012/10/26 17:53:30]

Done.

+              << (result == PAM_SUCCESS ? " succeeded." : " failed.");
+    return result == PAM_SUCCESS;
+  }
+
+  static int PamConversation(int num_messages,
+                             const struct pam_message** messages,
+                             struct pam_response** responses,
+                             void* context) {
+    // We don't expect this function to be called. Since we have no easy way

[Wez, 2012/10/26 03:25:43]

Is it likely that this be called nonetheless, or could we just CHECK() here?

[Jamie, 2012/10/26 17:53:30]

I've changed it to a LOG(FATAL) in the case that we're asked for input, since it
simplifies the code a bit, but if we're just being asked to log something, I
think it's better to do so.

+    // of returning a response, we consider it to be an error if we're asked
+    // for one. Informational and error messages are logged.
+    int result = PAM_SUCCESS;
+    for (int i = 0; i < num_messages; ++i) {
+      const struct pam_message* message = messages[i];
+      switch (message->msg_style) {
+        case PAM_ERROR_MSG:
+          LOG(ERROR) << "PAM conversation error message: " << message->msg;
+          break;
+        case PAM_TEXT_INFO:
+          LOG(INFO) << "PAM conversation message: " << message->msg;
+          break;
+        default:
+          LOG(ERROR) << "Unexpected PAM conversation response required: "
+                     << message->msg << "; msg_style = " << message->msg_style;
+          result = PAM_CONV_ERR;
+      }
+    }
+    // Except in case of error, we need to allocate space for the responses.
+    *responses = static_cast<struct pam_response*>(
+        calloc(num_messages, sizeof(struct pam_response)));
+    return result;
+  }
+
+  scoped_ptr<protocol::Authenticator> underlying_;
+  enum { NOT_CHECKED, ALLOWED, DISALLOWED } local_login_status_;
+};
+
+PamAuthorizationFactory::PamAuthorizationFactory(
+    scoped_ptr<protocol::AuthenticatorFactory> underlying)
+    : underlying_(underlying.Pass()) {
+}
+
+scoped_ptr<protocol::Authenticator>
+PamAuthorizationFactory::CreateAuthenticator(
+    const std::string& local_jid,
+    const std::string& remote_jid,
+    const buzz::XmlElement* first_message) {
+  scoped_ptr<protocol::Authenticator> authenticator(
+      underlying_->CreateAuthenticator(local_jid, remote_jid, first_message));

[Wez, 2012/10/26 03:25:43]

nit: Can you pass the output of CreateAuthenticator() directly to the
PamAuthorizer() ctor and save a couple of lines?

[Jamie, 2012/10/26 17:53:30]

I think that would be less readable.

[Wez, 2012/10/26 21:52:23]

Even if you fold the new PamAuthorizer on to the return line itself?  Actually,
I think that'd be one character too long...

+  return scoped_ptr<protocol::Authenticator>(
+      new PamAuthorizer(authenticator.Pass()));
+}
+
+
+}  // namespace remoting